Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Ensure that we correctly auth events returned by send_join #11012

Merged
merged 6 commits into from
Oct 25, 2021
Merged

Ensure that we correctly auth events returned by send_join #11012

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d37c283
Hoist `_check_for_soft_fail` and `_maybe_kick_guest_users` checks
richvdh Oct 19, 2021
44a0762
Stop papering over broken send_join responses
richvdh Oct 6, 2021
3701fee
process_remote_join: use auth_and_persist_outliers
richvdh Oct 7, 2021
50f58a0
Also auth the join event itself
richvdh Oct 7, 2021
32e9237
changelog
richvdh Oct 7, 2021
8c751f0
Drop events with missing auth events
richvdh Oct 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions synapse/handlers/federation_event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,7 @@ async def on_send_membership_event(
# need to.
await self._event_creation_handler.cache_joined_hosts_for_event(event, context)

await self._check_for_soft_fail(event, None, origin=origin)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the call to check_event_auth is a fair way up, so we're now only doing the soft_fail check if all the other checks pass.

await self._run_push_actions_and_persist_event(event, context)
return event, context

Expand Down Expand Up @@ -1013,11 +1014,19 @@ async def _process_received_pdu(
logger.exception("Unexpected AuthError from _check_event_auth")
raise FederationError("ERROR", e.code, e.msg, affected=event.event_id)

if not backfilled and not context.rejected:
# For new (non-backfilled and non-outlier) events we check if the event
# passes auth based on the current state. If it doesn't then we
# "soft-fail" the event.
await self._check_for_soft_fail(event, state, origin=origin)

await self._run_push_actions_and_persist_event(event, context, backfilled)

if backfilled:
if backfilled or context.rejected:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously, we were doing the later stuff even if the event was rejected, which I think was a bug.

return

await self._maybe_kick_guest_users(event)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously, we were doing this before persisting the event (which, at best, would create an unnecessary fork in the event dag), and also doing it for backfilled events, which I'm pretty sure was a bug.


# For encrypted messages we check that we know about the sending device,
# if we don't then we mark the device cache for that user as stale.
if event.type == EventTypes.Encrypted:
Expand Down Expand Up @@ -1445,10 +1454,6 @@ async def _check_event_auth(
except AuthError as e:
logger.warning("Failed auth resolution for %r because %s", event, e)
context.rejected = RejectedReason.AUTH_ERROR
return context

await self._check_for_soft_fail(event, state, backfilled, origin=origin)
await self._maybe_kick_guest_users(event)

return context

Expand All @@ -1468,7 +1473,6 @@ async def _check_for_soft_fail(
self,
event: EventBase,
state: Optional[Iterable[EventBase]],
backfilled: bool,
origin: str,
) -> None:
"""Checks if we should soft fail the event; if so, marks the event as
Expand All @@ -1477,15 +1481,8 @@ async def _check_for_soft_fail(
Args:
event
state: The state at the event if we don't have all the event's prev events
backfilled: Whether the event is from backfill
origin: The host the event originates from.
"""
# For new (non-backfilled and non-outlier) events we check if the event
# passes auth based on the current state. If it doesn't then we
# "soft-fail" the event.
if backfilled or event.internal_metadata.is_outlier():
return
Comment on lines -1483 to -1487
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the backfilled test has been pulled out to the call sites, where appropriate. The is_outlier test was redundant because this codepath is no longer reached for outliers.


extrem_ids_list = await self._store.get_latest_event_ids_in_room(event.room_id)
extrem_ids = set(extrem_ids_list)
prev_event_ids = set(event.prev_event_ids())
Expand Down