diff --git a/changelog.d/20439.bugfix b/changelog.d/20439.bugfix new file mode 100644 index 0000000000..d9b27c3eba --- /dev/null +++ b/changelog.d/20439.bugfix @@ -0,0 +1 @@ +Fix a bug where `/sync` could fail with a 500 error (`Missing state for event that is not user's own membership`) shortly after an invite or invite rejection received over federation entered a room's state. diff --git a/synapse/events/utils.py b/synapse/events/utils.py index c3bc6fa9da..9e8815aa5b 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -120,12 +120,14 @@ class FilteredEvent: """The event to be serialized.""" membership: str | None - """The user whose requesting the event's membership at the time of the - event was sent. + """The requesting user's membership at the time the event was sent. This is None if we didn't compute the membership. In Synapse this happens a) - when returning state events to state endpoints, or b) when the event is - returned to an admin. + when returning state events to state endpoints, b) when the event is + returned to an admin, or c) when we have no state at the event, which + happens for an event let through by `always_include_ids` whose copy still + carries the outlier flag (see + `synapse.visibility.filter_and_transform_events_for_client`). According to the spec we don't have to include the membership for any events if we don't want to, especially if its expensive to compute. In practice diff --git a/synapse/visibility.py b/synapse/visibility.py index d6b3d4d452..b7aeeffddd 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -197,13 +197,10 @@ async def filter_and_transform_events_for_client( # Filter out call invites in public rooms, as this would potentially # ring a lot of users. if event.type == EventTypes.CallInvite and not event.is_state(): - # `state_after_event` should only be None if the event is an outlier, - # and earlier code should filter out outliers entirely. - # - # In addition, we only create outliers locally for out-of-band - # invite rejections, invites received over federation, or state - # events needed to authorise other events. None of this applies to - # call invites. + # `state_after_event` is None only when our copy of the event carries + # the outlier flag (see below). Synapse only flags state events as + # outliers (out-of-band memberships, auth events and MSC4242 state DAG + # events), so a non-state call invite always has state here. assert state_after_event is not None room_join_rules = state_after_event.get((EventTypes.JoinRules, "")) @@ -216,8 +213,9 @@ async def filter_and_transform_events_for_client( # Annotate the event with the user's membership after the event. # # Normally we just look in `state_after_event`, but if the event is an outlier - # we won't have such a state. The only outliers that are returned here are the - # user's own membership event, so we can just inspect that. + # we won't have such a state. An outlier here is either the user's own + # membership event, which we inspect directly, or an event in + # `always_include_ids`, which we return without a membership. user_membership_event: EventBase | None if event.type == EventTypes.Member and event.state_key == user_id: @@ -225,8 +223,23 @@ async def filter_and_transform_events_for_client( elif state_after_event is not None: user_membership_event = state_after_event.get((EventTypes.Member, user_id)) else: - # unreachable! - raise Exception("Missing state for event that is not user's own membership") + # We have no state for this event: our copy carries the outlier flag, + # so the state fetch above skipped it, but `always_include_ids` let it + # through anyway. Sync builds `always_include_ids` from an uncached read + # of `current_state_events`, so it sees an out-of-band membership event + # that has been de-outliered into the current state as soon as the + # persister commits, while the event object we were handed still comes + # from a cache that has not been invalidated yet. We don't know the + # user's membership at this point in the DAG; the spec makes + # `unsigned.membership` optional, so return the event unannotated rather + # than dropping it or failing the whole request. + logger.info( + "filter_and_transform_events_for_client: no state for always-included" + " event %s in room %s; returning it without a membership annotation", + event.event_id, + event.room_id, + ) + return FilteredEvent(event=filtered, membership=None) user_membership = ( user_membership_event.membership diff --git a/tests/test_visibility.py b/tests/test_visibility.py index 0654f84351..23a4609899 100644 --- a/tests/test_visibility.py +++ b/tests/test_visibility.py @@ -589,9 +589,12 @@ class FilterEventsForClientTestCase(HomeserverTestCase): class FilterEventsOutOfBandEventsForClientTestCase( unittest.FederatingHomeserverTestCase ): - def test_out_of_band_invite_rejection(self) -> None: - # this is where we have received an invite event over federation, and then - # rejected it. + def _receive_and_reject_invite(self) -> tuple[EventBase, EventBase]: + """Receive an invite over federation and reject it locally. + + Returns the invite event and the rejection event, both of which are + out-of-band membership events. + """ invite_pdu = { "room_id": "!room:id", "depth": 1, @@ -637,6 +640,10 @@ class FilterEventsOutOfBandEventsForClientTestCase( [invite_event_id, reject_event_id] ) ) + return invite_event, reject_event + + def test_out_of_band_invite_rejection(self) -> None: + invite_event, reject_event = self._receive_and_reject_invite() # the invited user should be able to see both the invite and the rejection filtered_events = self.get_success( @@ -667,6 +674,28 @@ class FilterEventsOutOfBandEventsForClientTestCase( [], ) + def test_always_included_event_without_state_has_no_membership(self) -> None: + """An event let through by `always_include_ids` that has no state is + returned without a membership annotation rather than failing the request. + + Regression test for https://github.com/element-hq/synapse/issues/19858. + """ + invite_event, reject_event = self._receive_and_reject_invite() + + filtered_events = self.get_success( + filter_and_transform_events_for_client( + self.hs.get_storage_controllers(), + "@other:test", + [invite_event, reject_event], + always_include_ids=frozenset({invite_event.event_id}), + ) + ) + self.assertEqual( + [e.event.event_id for e in filtered_events], + [invite_event.event_id], + ) + self.assertEqual([e.membership for e in filtered_events], [None]) + async def inject_visibility_event( hs: HomeServer,