diff --git a/synapse/handlers/pagination.py b/synapse/handlers/pagination.py index 8cbe4b63c8..2bc7efeb5e 100644 --- a/synapse/handlers/pagination.py +++ b/synapse/handlers/pagination.py @@ -566,7 +566,7 @@ class PaginationHandler: ( events, next_key, - _, + limited, ) = await self.store.paginate_room_events_by_topological_ordering( room_id=room_id, from_key=from_token.room_key, @@ -645,7 +645,7 @@ class PaginationHandler: ( events, next_key, - _, + limited, ) = await self.store.paginate_room_events_by_topological_ordering( room_id=room_id, from_key=from_token.room_key, @@ -668,11 +668,12 @@ class PaginationHandler: next_token = from_token.copy_and_replace(StreamKeyType.ROOM, next_key) - # if no events are returned from pagination, that implies - # we have reached the end of the available events. + # if no events are returned from pagination (this page is empty) + # and there aren't any more pages (not limited), + # that implies we have reached the end of the available events. # In that case we do not return end, to tell the client # there is no need for further queries. - if not events: + if not limited and not events: return GetMessagesResult( messages_chunk=[], bundled_aggregations={}, diff --git a/synapse/storage/databases/main/stream.py b/synapse/storage/databases/main/stream.py index 8fa1e2e5a9..7d14f9f4d8 100644 --- a/synapse/storage/databases/main/stream.py +++ b/synapse/storage/databases/main/stream.py @@ -2425,12 +2425,19 @@ class StreamWorkerStore(EventsWorkerStore, SQLBaseStore): event_filter: If provided filters the events to those that match the filter. Returns: - The results as a list of events, a token that points to the end of - the result set, and a boolean to indicate if there were more events - but we hit the limit. If no events are returned then the end of the + - The results as a list of events; + - a token that points to the end of the result set; and + - a boolean to indicate if there were more events + but we hit the limit (`limited`) + + If no events are returned and `limited` is false, then the end of the stream has been reached (i.e. there are no events between `from_key` and `to_key`). + When `limited` is true, that means that more pagination can be attempted. + Note that `limited` can be true even if no events are returned, + because rejected events are filtered out after the limit check. + When Direction.FORWARDS: from_key < x <= to_key, (ascending order) When Direction.BACKWARDS: from_key >= x > to_key, (descending order) """ diff --git a/tests/rest/client/test_rooms.py b/tests/rest/client/test_rooms.py index 221121007d..61e7e87f62 100644 --- a/tests/rest/client/test_rooms.py +++ b/tests/rest/client/test_rooms.py @@ -66,7 +66,10 @@ from synapse.util.stringutils import random_string from tests import unittest from tests.http.server._base import make_request_with_cancellation_test from tests.storage.test_stream import PaginationTestCase -from tests.test_utils.event_injection import create_event +from tests.test_utils.event_injection import ( + create_event, + inject_event, +) from tests.unittest import override_config from tests.utils import default_config @@ -2371,6 +2374,87 @@ class RoomMessageListTestCase(RoomBase): channel.json_body["errcode"], Codes.NOT_JSON, channel.json_body ) + def test_room_messages_paginate_through_rejected_events( + self, + ) -> None: + """Test that pagination continues past a batch of rejected events. + + Regression test for https://github.com/element-hq/synapse/security/advisories/GHSA-6qf2-7x63-mm6v + + Synapse before 1.152.1 had a bug meaning that a batch full of only + rejected events would cause `/messages` to not return any more + pagination tokens, falsely signalling the end of backpagination. + """ + # Send an early message that should not be filtered. + early_event_id = self.helper.send(self.room_id, "early message")["event_id"] + + # Inject a batch of events and mark them as rejected in the database. + # We create more events than a single pagination request would fetch, + # so that one page of backward pagination request would only see rejected events. + for _ in range(3): + event = self.get_success( + inject_event( + self.hs, + room_id=self.room_id, + sender=self.user_id, + type=EventTypes.Message, + content={"body": "filtered event", "msgtype": "m.text"}, + ) + ) + self.get_success( + self.hs.get_datastores().main.db_pool.runInteraction( + "mark_rejected", + self.hs.get_datastores().main.mark_event_rejected_txn, + event.event_id, + "testing", + ) + ) + + # Send a message after all the rejected events. + latest_event_id = self.helper.send(self.room_id, "latest message")["event_id"] + + # Start backpaginating. + channel = self.make_request( + "GET", f"/rooms/{self.room_id}/messages?dir=b&limit=2" + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + + events_in_page = [e["event_id"] for e in channel.json_body["chunk"]] + end_token: str | None = channel.json_body["end"] + + self.assertEqual( + events_in_page, + [latest_event_id], + "The latest event should be included in the first page we see whilst backpaginating", + ) + + event_ids_in_pages: list[list[str]] = [events_in_page] + + # Bound the number of backpagination attempts to 2 + for _ in range(2): + channel = self.make_request( + "GET", f"/rooms/{self.room_id}/messages?from={end_token}&dir=b&limit=2" + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) + events_in_page = [e["event_id"] for e in channel.json_body["chunk"]] + event_ids_in_pages.append(events_in_page) + + if early_event_id in events_in_page: + # We have found the event we were looking for + return + + self.assertIn( + "end", + channel.json_body, + f"No `end` token received. Did not find {early_event_id} whilst backpaginating ({latest_event_id = }, {event_ids_in_pages = })", + ) + # Use the end_token in the next iteration + end_token = channel.json_body["end"] + + self.fail( + f"Exhausted backpagination attempts. Did not find {early_event_id} whilst backpaginating ({latest_event_id = }, {event_ids_in_pages = })" + ) + class RoomMessageFilterTestCase(RoomBase): """Tests /rooms/$room_id/messages REST events."""