From 3772859fbcaac6ffa2baeea0025307fbdc192cbb Mon Sep 17 00:00:00 2001 From: Olivier 'reivilibre Date: Tue, 14 Jul 2026 12:14:08 +0100 Subject: [PATCH] Add missing same-room check on `/get_missing_events` handler Fixes: https://github.com/element-hq/synapse/security/advisories/GHSA-27p5-4f45-gx76 Fixes: https://github.com/matrix-org/internal-config/issues/1717 I've tried my best to make the tests a good narrative for the thought process here, but essentially the rationale is to make `/get_missing_events` not distinguish between 'event is in wrong room' and 'event is unknown to me'. ----- Reviewed-on: https://github.com/element-hq/synapse-private/pull/141 --- .../databases/main/event_federation.py | 50 ++- tests/federation/test_federation_server.py | 289 +++++++++++++++++- 2 files changed, 331 insertions(+), 8 deletions(-) diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py index d84c58dcf8..4cb55f8c46 100644 --- a/synapse/storage/databases/main/event_federation.py +++ b/synapse/storage/databases/main/event_federation.py @@ -1983,6 +1983,13 @@ class EventFederationWorkerStore( latest_events: list[str], limit: int, ) -> list[EventBase]: + """ + Walk backwards in the DAG of events, + starting at `latest_events` and stopping at `earliest_events` (or when having reached `limit` events). + + This function will check that `latest_events` and `earliest_events` are in the correct + room (`room_id`), appropriately ignoring any that aren't. + """ ids = await self.db_pool.runInteraction( "get_missing_events", self._get_missing_events, @@ -2001,20 +2008,49 @@ class EventFederationWorkerStore( latest_events: list[str], limit: int, ) -> list[str]: + # It's OK that this has not been filtered by correct-room, + # because we will only compare based on event ID from the events + # we happen to run into. seen_events = set(earliest_events) - front = set(latest_events) - seen_events - event_results: list[str] = [] - query = ( - "SELECT prev_event_id FROM event_edges " - "WHERE event_id = ? AND NOT is_state " - "LIMIT ?" + # Pre-filter the `latest_events` to only include those + # that are in this room (and that we know about) + # This makes events in the wrong room get treated the same as unknown events. + events_clause, events_args = make_in_list_sql_clause( + self.database_engine, + "event_id", + # Don't waste time looking at events that the requester told us + # they already know about. + # (They probably shouldn't send this in the first place) + set(latest_events) - seen_events, ) + txn.execute( + f""" + SELECT event_id + FROM events + WHERE {events_clause} AND room_id = ? + """, + (*events_args, room_id), + ) + # Start walking back from the legitimate and known `latest_events` + front = {latest_event_id for (latest_event_id,) in txn} + + event_results: list[str] = [] while front and len(event_results) < limit: new_front = set() for event_id in front: - txn.execute(query, (event_id, limit - len(event_results))) + txn.execute( + """ + SELECT ee.prev_event_id FROM event_edges AS ee + JOIN events ON events.event_id = ee.prev_event_id + WHERE ee.event_id = ? + AND events.room_id = ? + AND NOT ee.is_state + LIMIT ? + """, + (event_id, room_id, limit - len(event_results)), + ) new_results = {t[0] for t in txn} - seen_events new_front |= new_results diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index 75490d0f0f..cdb05a9b7a 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -38,7 +38,7 @@ from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer from synapse.storage.controllers.state import server_acl_evaluator_from_event -from synapse.types import JsonDict +from synapse.types import JsonDict, UserID from synapse.util.clock import Clock from tests import unittest @@ -94,6 +94,293 @@ class FederationServerTests(unittest.FederatingHomeserverTestCase): self.assertEqual(500, channel.code, channel.result) +class GetMissingEventsRoomCheckTests(unittest.FederatingHomeserverTestCase): + """ + Regression tests for room confusion in /get_missing_events + https://github.com/element-hq/synapse/security/advisories/GHSA-27p5-4f45-gx76 + """ + + servlets = [ + admin.register_servlets, + login.register_servlets, + room.register_servlets, + ] + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + super().prepare(reactor, clock, hs) + + # Local user + self.local_user_id = self.register_user("alice", "pass") + self.local_user_token = self.login("alice", "pass") + self.local_user = UserID.from_string(self.local_user_id) + + # Create 2 rooms (one with the remote server, one without). + # - The remote server will be in this room + self.room_allowed = self.helper.create_room_as( + self.local_user_id, tok=self.local_user_token + ) + self.inject_room_member( + self.room_allowed, f"@remote:{self.OTHER_SERVER_NAME}", "join" + ) + # - The remote server will _not_ be in this room + self.room_blocked = self.helper.create_room_as( + self.local_user_id, tok=self.local_user_token + ) + + # Insert a linear chain of events in both rooms + self.room_allowed_event_ids = self.helper.send_messages( + self.room_allowed, num_events=5, tok=self.local_user_token + ) + self.room_blocked_event_ids = self.helper.send_messages( + self.room_blocked, num_events=5, tok=self.local_user_token + ) + + def _extract_returned_event_ids(self, json_body: JsonDict) -> set[str]: + """ + Given the response body of `/get_missing_events`, return the event IDs + of the events that were returned in the response. + This only includes event IDs from `self.room_allowed_event_ids` and + `self.room_blocked_event_ids`; other events are ignored. + + As the federation PDU format doesn't include event IDs + (at least not for every room version), we match on the + `(room_id, content.body, prev_events)` triple against the events + we sent in the setup. + """ + store = self.hs.get_datastores().main + events = self.get_success( + store.get_events_as_list( + list(self.room_allowed_event_ids) + list(self.room_blocked_event_ids) + ) + ) + # (room_id, content.body, prev_events) -> event ID + event_lookup: dict[tuple[str, str, tuple[str, ...]], str] = {} + for event in events: + key = ( + event.room_id, + event.content["body"], + tuple(event.prev_event_ids()), + ) + event_lookup[key] = event.event_id + + returned_event_ids: set[str] = set() + for pdu in json_body["events"]: + key = ( + pdu.get("room_id"), + pdu.get("content", {}).get("body"), + tuple(pdu.get("prev_events", [])), + ) + event_id = event_lookup.get(key) + if event_id is None: + # Not one of the events we created; ignore it. + continue + returned_event_ids.add(event_id) + return returned_event_ids + + def test_get_missing_events_returns_events_from_correct_room(self) -> None: + """ + Tests the happy path when `latest_events` and `earliest_events` + are both in the correct room. + + returned + | + v + e1 <- e2 <- e3 <- e4 <- e5 + ^ ^ + | | + earliest latest + + Not a regression test; I'm just filling a gap in our (in-repo) testing + as far as I can tell. + """ + channel = self.make_signed_federation_request( + "POST", + f"/_matrix/federation/v1/get_missing_events/{self.room_allowed}", + content={ + "earliest_events": [self.room_allowed_event_ids[1]], + "latest_events": [self.room_allowed_event_ids[3]], + "limit": 10, + }, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + self.assertEqual( + self._extract_returned_event_ids(channel.json_body), + {self.room_allowed_event_ids[2]}, + ) + + def test_get_missing_events_with_empty_earliest_events(self) -> None: + """ + Tests that `/get_missing_events`, when given no `earliest_events`, + walks back to the start of the room, capped at `limit`. + + (Not a regression test; documents pre-existing behaviour) + """ + channel = self.make_signed_federation_request( + "POST", + f"/_matrix/federation/v1/get_missing_events/{self.room_allowed}", + content={ + "earliest_events": [], + "latest_events": [self.room_allowed_event_ids[-1]], + "limit": 10, + }, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + self.assertEqual( + self._extract_returned_event_ids(channel.json_body), + set(self.room_allowed_event_ids[:-1]), + ) + + def test_get_missing_events_with_unknown_earliest_event(self) -> None: + """ + Tests that `/get_missing_events` ignores unknown event IDs given in + `earliest_events`. + + This makes sense as the `earliest_events` are intuitively + 'events to stop at' when walking backwards. + Since we don't know about those events, we don't use them as stopping conditions. + (In other words, this falls back to the same behaviour as + `test_get_missing_events_with_empty_earliest_events`.) + + (Not a regression test; documents pre-existing behaviour) + """ + channel = self.make_signed_federation_request( + "POST", + f"/_matrix/federation/v1/get_missing_events/{self.room_allowed}", + content={ + "earliest_events": ["$someUnknownEventId"], + "latest_events": [self.room_allowed_event_ids[-1]], + "limit": 10, + }, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + self.assertEqual( + self._extract_returned_event_ids(channel.json_body), + set(self.room_allowed_event_ids[:-1]), + ) + + def test_get_missing_events_with_no_latest_event(self) -> None: + """ + Tests that when the `/get_missing_events` request references + no events in `latest_events`, the response is 200 OK + with an empty `events` list. + + (Not a regression test; documents pre-existing behaviour) + """ + channel = self.make_signed_federation_request( + "POST", + f"/_matrix/federation/v1/get_missing_events/{self.room_allowed}", + content={ + "earliest_events": ["$someOtherUnknownEventId"], + "latest_events": [], + "limit": 10, + }, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual(channel.json_body, {"events": []}) + + def test_get_missing_events_with_unknown_latest_event(self) -> None: + """ + Tests that when the `/get_missing_events` request references + unknown events in `latest_events`, the response is 200 OK + with an empty `events` list. + + I imagine this makes sense as you might request several events + in `latest_events` to start walking back from and we need to be + tolerant of the fact that servers don't always know about every event. + + (Not a regression test; documents pre-existing behaviour) + """ + channel = self.make_signed_federation_request( + "POST", + f"/_matrix/federation/v1/get_missing_events/{self.room_allowed}", + content={ + "earliest_events": ["$someOtherUnknownEventId"], + "latest_events": ["$someUnknownEventId"], + "limit": 10, + }, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual(channel.json_body, {"events": []}) + + def test_get_missing_events_ignores_events_from_other_room(self) -> None: + """ + Tests that providing `earliest_events` and `latest_events` from the wrong room + treats them the same as being unknown. + + From `test_get_missing_events_with_unknown_latest_event` we established that + unknown events in `latest_events` get skipped (to the point of returning an empty + `events: []` response) + + From `test_get_missing_events_with_unknown_earliest_event` we established that + unknown events in `earliest_events` get ignored as stopping conditions. + + This regression test previously failed. + """ + channel = self.make_signed_federation_request( + "POST", + f"/_matrix/federation/v1/get_missing_events/{self.room_allowed}", + content={ + "earliest_events": [self.room_blocked_event_ids[0]], + "latest_events": [self.room_blocked_event_ids[-1]], + "limit": 10, + }, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual(channel.json_body, {"events": []}) + + def test_get_missing_events_skips_latest_events_from_other_room(self) -> None: + """ + Tests that providing `latest_events` from the wrong room + treats it as being unknown, even if `earliest_events` are from the correct + room. + + From `test_get_missing_events_with_unknown_latest_event` we established that + unknown events in `latest_events` get skipped (to the point of returning an empty + `events: []` response) + + This regression test previously failed. + """ + channel = self.make_signed_federation_request( + "POST", + f"/_matrix/federation/v1/get_missing_events/{self.room_allowed}", + content={ + "earliest_events": [self.room_allowed_event_ids[0]], + "latest_events": [self.room_blocked_event_ids[-1]], + "limit": 10, + }, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + self.assertEqual(channel.json_body, {"events": []}) + + def test_get_missing_events_ignores_earliest_events_from_other_room(self) -> None: + """ + Tests that providing `earliest_events` from the wrong room causes those + events to be ignored as stopping conditions, + even though `latest_events` are from the correct room. + + From `test_get_missing_events_with_unknown_earliest_event` we established that + unknown events in `earliest_events` get ignored as stopping conditions. + + This test was previously fine, but is an obvious extra case. + """ + channel = self.make_signed_federation_request( + "POST", + f"/_matrix/federation/v1/get_missing_events/{self.room_allowed}", + content={ + # Use [-3] here as we want to see if the walk-back algorithm + # confuses depth (topological ordering) across the two rooms. + "earliest_events": [self.room_blocked_event_ids[-3]], + "latest_events": [self.room_allowed_event_ids[-1]], + "limit": 10, + }, + ) + self.assertEqual(HTTPStatus.OK, channel.code, channel.result) + self.assertEqual( + self._extract_returned_event_ids(channel.json_body), + set(self.room_allowed_event_ids[:-1]), + ) + + def _create_acl_event(content: JsonDict) -> EventBase: return make_test_event( {