From e4138c1c3fe73aca5f2a9baec82f5286bd2fd366 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 25 Feb 2022 12:32:16 +0000 Subject: [PATCH 1/8] Replace `get_state_for_pdu` This is fairly easy to write in terms of `get_state_ids_for_pdu` and `get_events_as_list`, so let's do that. --- synapse/federation/federation_server.py | 7 +++---- synapse/handlers/federation.py | 26 ------------------------- 2 files changed, 3 insertions(+), 30 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 482bbdd86744..af2d0f7d7932 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -22,7 +22,6 @@ Callable, Collection, Dict, - Iterable, List, Optional, Tuple, @@ -577,10 +576,10 @@ async def _on_state_ids_request_compute( async def _on_context_state_request_compute( self, room_id: str, event_id: Optional[str] ) -> Dict[str, list]: + pdus: Collection[EventBase] if event_id: - pdus: Iterable[EventBase] = await self.handler.get_state_for_pdu( - room_id, event_id - ) + event_ids = await self.handler.get_state_ids_for_pdu(room_id, event_id) + pdus = await self.store.get_events_as_list(event_ids) else: pdus = (await self.state.get_current_state(room_id)).values() diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index db39aeabded6..e977904de080 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -950,32 +950,6 @@ async def on_make_knock_request( return event - async def get_state_for_pdu(self, room_id: str, event_id: str) -> List[EventBase]: - """Returns the state at the event. i.e. not including said event.""" - - event = await self.store.get_event(event_id, check_room_id=room_id) - - state_groups = await self.state_store.get_state_groups(room_id, [event_id]) - - if state_groups: - _, state = list(state_groups.items()).pop() - results = {(e.type, e.state_key): e for e in state} - - if event.is_state(): - # Get previous state - if "replaces_state" in event.unsigned: - prev_id = event.unsigned["replaces_state"] - if prev_id != event.event_id: - prev_event = await self.store.get_event(prev_id) - results[(event.type, event.state_key)] = prev_event - else: - del results[(event.type, event.state_key)] - - res = list(results.values()) - return res - else: - return [] - async def get_state_ids_for_pdu(self, room_id: str, event_id: str) -> List[str]: """Returns the state at the event. i.e. not including said event.""" event = await self.store.get_event(event_id, check_room_id=room_id) From 662c6da1c4ace582bc031756ca060f539db3e13a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 25 Feb 2022 12:11:44 +0000 Subject: [PATCH 2/8] Return a 404 from `/state` and `/state_ids` for an outlier --- synapse/handlers/federation.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index e977904de080..1a78fe132196 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -953,6 +953,8 @@ async def on_make_knock_request( async def get_state_ids_for_pdu(self, room_id: str, event_id: str) -> List[str]: """Returns the state at the event. i.e. not including said event.""" event = await self.store.get_event(event_id, check_room_id=room_id) + if event.internal_metadata.outlier: + raise NotFoundError("State not known at event %s" % (event_id,)) state_groups = await self.state_store.get_state_groups_ids(room_id, [event_id]) From c09f032f98d2292bdba32ee1f21e5786138b16ae Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 25 Feb 2022 12:37:41 +0000 Subject: [PATCH 3/8] Remove redundant condition `get_state_groups_ids` now cannot return an empty result, so replace condition with an assertion --- synapse/handlers/federation.py | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 1a78fe132196..84b0159ff493 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -958,22 +958,22 @@ async def get_state_ids_for_pdu(self, room_id: str, event_id: str) -> List[str]: state_groups = await self.state_store.get_state_groups_ids(room_id, [event_id]) - if state_groups: - _, state = list(state_groups.items()).pop() - results = state - - if event.is_state(): - # Get previous state - if "replaces_state" in event.unsigned: - prev_id = event.unsigned["replaces_state"] - if prev_id != event.event_id: - results[(event.type, event.state_key)] = prev_id - else: - results.pop((event.type, event.state_key), None) - - return list(results.values()) - else: - return [] + # get_state_groups_ids should return exactly one result + assert len(state_groups) == 1 + + _, state = list(state_groups.items()).pop() + results = state + + if event.is_state(): + # Get previous state + if "replaces_state" in event.unsigned: + prev_id = event.unsigned["replaces_state"] + if prev_id != event.event_id: + results[(event.type, event.state_key)] = prev_id + else: + results.pop((event.type, event.state_key), None) + + return list(results.values()) async def on_backfill_request( self, origin: str, room_id: str, pdu_list: List[str], limit: int From 962da90b27add8796e3414a60df25221e920c944 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 25 Feb 2022 12:39:39 +0000 Subject: [PATCH 4/8] Rename variable --- synapse/handlers/federation.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 84b0159ff493..ef9a5d15c4c0 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -961,19 +961,18 @@ async def get_state_ids_for_pdu(self, room_id: str, event_id: str) -> List[str]: # get_state_groups_ids should return exactly one result assert len(state_groups) == 1 - _, state = list(state_groups.items()).pop() - results = state + _, state_map = list(state_groups.items()).pop() if event.is_state(): # Get previous state if "replaces_state" in event.unsigned: prev_id = event.unsigned["replaces_state"] if prev_id != event.event_id: - results[(event.type, event.state_key)] = prev_id + state_map[(event.type, event.state_key)] = prev_id else: - results.pop((event.type, event.state_key), None) + state_map.pop((event.type, event.state_key), None) - return list(results.values()) + return list(state_map.values()) async def on_backfill_request( self, origin: str, room_id: str, pdu_list: List[str], limit: int From ba17bae60bd171cdac9798047706838912bb4b64 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 25 Feb 2022 12:42:55 +0000 Subject: [PATCH 5/8] Skip conversion to list --- synapse/handlers/federation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index ef9a5d15c4c0..0ef3f70c9465 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -961,7 +961,7 @@ async def get_state_ids_for_pdu(self, room_id: str, event_id: str) -> List[str]: # get_state_groups_ids should return exactly one result assert len(state_groups) == 1 - _, state_map = list(state_groups.items()).pop() + state_map = next(iter(state_groups.values())) if event.is_state(): # Get previous state From 2daf131a18a3a5bd94021107f651b235be4f757d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 25 Feb 2022 12:49:48 +0000 Subject: [PATCH 6/8] Only remove `event` if it was in the result in the first place. --- synapse/handlers/federation.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 0ef3f70c9465..2c2fdddd6598 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -963,14 +963,17 @@ async def get_state_ids_for_pdu(self, room_id: str, event_id: str) -> List[str]: state_map = next(iter(state_groups.values())) - if event.is_state(): - # Get previous state + # we need the state *before* event. + state_key = event.get_state_key() + if ( + state_key is not None + and state_map.get((event.type, state_key)) == event.event_id + ): if "replaces_state" in event.unsigned: prev_id = event.unsigned["replaces_state"] - if prev_id != event.event_id: - state_map[(event.type, event.state_key)] = prev_id + state_map[(event.type, state_key)] = prev_id else: - state_map.pop((event.type, event.state_key), None) + del state_map[(event.type, state_key)] return list(state_map.values()) From 961dfc7c13bcfbf9b49c4b55cd6e267782d33132 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 3 Mar 2022 18:50:38 +0000 Subject: [PATCH 7/8] newsfile --- changelog.d/12087.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12087.bugfix diff --git a/changelog.d/12087.bugfix b/changelog.d/12087.bugfix new file mode 100644 index 000000000000..6dacdddd0dcf --- /dev/null +++ b/changelog.d/12087.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug which caused the `/_matrix/federation/v1/state` and `.../state_ids` endpoints to return incorrect or invalid data when called for an event which we have stored as an "outlier". From 59273a7cb12b41521fa923eb3f9f1168a9a3055f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 11 Mar 2022 14:30:17 +0000 Subject: [PATCH 8/8] Assume that the state at an event includes that event --- synapse/handlers/federation.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 2c2fdddd6598..350ec9c03af1 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -963,12 +963,15 @@ async def get_state_ids_for_pdu(self, room_id: str, event_id: str) -> List[str]: state_map = next(iter(state_groups.values())) - # we need the state *before* event. state_key = event.get_state_key() - if ( - state_key is not None - and state_map.get((event.type, state_key)) == event.event_id - ): + if state_key is not None: + # the event was not rejected (get_event raises a NotFoundError for rejected + # events) so the state at the event should include the event itself. + assert ( + state_map.get((event.type, state_key)) == event.event_id + ), "State at event did not include event itself" + + # ... but we need the state *before* that event if "replaces_state" in event.unsigned: prev_id = event.unsigned["replaces_state"] state_map[(event.type, state_key)] = prev_id