From 12442239ba8fa861568e8cf47765ccf53032e42a Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 7 Jun 2024 15:32:46 +0100 Subject: [PATCH 1/7] Require the 'from' parameter for `/notifications` be an integer. While this is classed as a 'string' in the spec, the provided value must be an integer in order for pagination to work. Thus any non-integer value must be rejected. --- synapse/rest/client/notifications.py | 5 ++++- synapse/storage/databases/main/event_push_actions.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/notifications.py b/synapse/rest/client/notifications.py index be9b584748a..7206d22dcb6 100644 --- a/synapse/rest/client/notifications.py +++ b/synapse/rest/client/notifications.py @@ -56,7 +56,10 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) user_id = requester.user.to_string() - from_token = parse_string(request, "from", required=False) + # While this is intended to be "string" to clients, the 'from' token + # is actually based on a numeric ID. So it must parse to as an int. + from_token = parse_integer(request, "from", required=False) + limit = parse_integer(request, "limit", default=50) only = parse_string(request, "only", required=False) diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index bdd0781c480..0ebf5b53d5e 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -1829,7 +1829,7 @@ def _clear_old_push_actions_staging_txn(txn: LoggingTransaction) -> bool: async def get_push_actions_for_user( self, user_id: str, - before: Optional[str] = None, + before: Optional[int] = None, limit: int = 50, only_highlight: bool = False, ) -> List[UserPushAction]: From 4e31809c7b4f31fd881c9a9da5e1ec2bdb3a8047 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 7 Jun 2024 15:35:08 +0100 Subject: [PATCH 2/7] Add a general and regression test Adds a test for pagination for /notifications. We also add a regression test for the 'from' parameter not being an integer. I wanted to break this up into two commits, but PyCharm made it hard. --- tests/rest/client/test_notifications.py | 171 +++++++++++++++++++++--- 1 file changed, 153 insertions(+), 18 deletions(-) diff --git a/tests/rest/client/test_notifications.py b/tests/rest/client/test_notifications.py index e9aa2e450e6..e4b0455ce8a 100644 --- a/tests/rest/client/test_notifications.py +++ b/tests/rest/client/test_notifications.py @@ -18,6 +18,7 @@ # [This file includes modifications made by New Vector Limited] # # +from typing import List, Optional, Tuple from unittest.mock import AsyncMock, Mock from twisted.test.proto_helpers import MemoryReactor @@ -48,6 +49,14 @@ def prepare( self.sync_handler = homeserver.get_sync_handler() self.auth_handler = homeserver.get_auth_handler() + self.user_id = self.register_user("user", "pass") + self.access_token = self.login("user", "pass") + self.other_user_id = self.register_user("otheruser", "pass") + self.other_access_token = self.login("otheruser", "pass") + + # Create a room + self.room_id = self.helper.create_room_as(self.user_id, tok=self.access_token) + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: # Mock out the calls over federation. fed_transport_client = Mock(spec=["send_transaction"]) @@ -61,32 +70,22 @@ def test_notify_for_local_invites(self) -> None: """ Local users will get notified for invites """ - - user_id = self.register_user("user", "pass") - access_token = self.login("user", "pass") - other_user_id = self.register_user("otheruser", "pass") - other_access_token = self.login("otheruser", "pass") - - # Create a room - room = self.helper.create_room_as(user_id, tok=access_token) - # Check we start with no pushes - channel = self.make_request( - "GET", - "/notifications", - access_token=other_access_token, - ) - self.assertEqual(channel.code, 200, channel.result) - self.assertEqual(len(channel.json_body["notifications"]), 0, channel.json_body) + self._request_notifications(from_token=None, limit=1, expected_count=0) # Send an invite - self.helper.invite(room=room, src=user_id, targ=other_user_id, tok=access_token) + self.helper.invite( + room=self.room_id, + src=self.user_id, + targ=self.other_user_id, + tok=self.access_token, + ) # We should have a notification now channel = self.make_request( "GET", "/notifications", - access_token=other_access_token, + access_token=self.other_access_token, ) self.assertEqual(channel.code, 200) self.assertEqual(len(channel.json_body["notifications"]), 1, channel.json_body) @@ -95,3 +94,139 @@ def test_notify_for_local_invites(self) -> None: "invite", channel.json_body, ) + + def test_pagination_of_notifications(self) -> None: + """ + Check that pagination of notifications works. + """ + # Check we start with no pushes + self._request_notifications(from_token=None, limit=1, expected_count=0) + + # Send an invite and have the other user join the room. + self.helper.invite( + room=self.room_id, + src=self.user_id, + targ=self.other_user_id, + tok=self.access_token, + ) + self.helper.join(self.room_id, self.other_user_id, tok=self.other_access_token) + + # Send 5 messages in the room and note down their event IDs. + sent_event_ids = [] + for _ in range(5): + resp = self.helper.send_event( + self.room_id, + "m.room.message", + {"body": "honk", "msgtype": "m.text"}, + tok=self.access_token, + ) + sent_event_ids.append(resp["event_id"]) + + # We expect to get notifications for messages in reverse order. + # So reverse this list of event IDs to make it easier to compare + # against later. + sent_event_ids.reverse() + + # We should have a few notifications now. Let's try and fetch the first 2. + notification_event_ids, _ = self._request_notifications( + from_token=None, limit=2, expected_count=2 + ) + + # Check we got the expected event IDs back. + self.assertEqual(notification_event_ids, sent_event_ids[:2]) + + # Try requesting again without a 'from' query parameter. We should get the + # same two notifications back. + notification_event_ids, next_token = self._request_notifications( + from_token=None, limit=2, expected_count=2 + ) + self.assertEqual(notification_event_ids, sent_event_ids[:2]) + + # Ask for the next 5 notifications, though there should only be + # 4 remaining; the next 3 messages and the invite. + # + # We need to use the "next_token" from the response as the "from" + # query parameter in the next request in order to paginate. + notification_event_ids, next_token = self._request_notifications( + from_token=next_token, limit=5, expected_count=4 + ) + # Ensure we chop off the invite on the end. + notification_event_ids = notification_event_ids[:-1] + self.assertEqual(notification_event_ids, sent_event_ids[2:]) + + def _request_notifications( + self, from_token: Optional[str], limit: int, expected_count: int + ) -> Tuple[List[str], str]: + """ + Make a request to /notifications to get the latest events to be notified about. + + Only the event IDs are returned. The request is made by the "other user". + + Args: + from_token: An optional starting parameter. + limit: The maximum number of results to return. + expected_count: The number of events to expect in the response. + + Returns: + A list of event IDs that the client should be notified about. + Events are returned newest-first. + """ + # Construct the request path. + path = f"/notifications?limit={limit}" + if from_token is not None: + path += f"&from={from_token}" + + channel = self.make_request( + "GET", + path, + access_token=self.other_access_token, + ) + + self.assertEqual(channel.code, 200) + self.assertEqual( + len(channel.json_body["notifications"]), expected_count, channel.json_body + ) + + # Extract the necessary data from the response. + next_token = channel.json_body["next_token"] + event_ids = [ + event["event"]["event_id"] for event in channel.json_body["notifications"] + ] + + return event_ids, next_token + + def test_parameters(self) -> None: + """ + Test that appropriate errors are returned when query parameters are malformed. + """ + # Test that no parameters are required. + channel = self.make_request( + "GET", + "/notifications", + access_token=self.other_access_token, + ) + self.assertEqual(channel.code, 200) + + # Test that limit cannot be negative + channel = self.make_request( + "GET", + "/notifications?limit=-1", + access_token=self.other_access_token, + ) + self.assertEqual(channel.code, 400) + + # Test that the 'limit' parameter must be an integer. + channel = self.make_request( + "GET", + "/notifications?limit=foobar", + access_token=self.other_access_token, + ) + self.assertEqual(channel.code, 400) + + # Test that the 'from' parameter must be an integer. + channel = self.make_request( + "GET", + "/notifications?from=osborne", + access_token=self.other_access_token, + ) + self.assertEqual(channel.code, 400) From 8b2592c7e7490d88760b731c58f32bc8e0befbc2 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 7 Jun 2024 15:40:22 +0100 Subject: [PATCH 3/7] changelog --- changelog.d/17283.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17283.bugfix diff --git a/changelog.d/17283.bugfix b/changelog.d/17283.bugfix new file mode 100644 index 00000000000..b12bcec863f --- /dev/null +++ b/changelog.d/17283.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug where a non-integer 'from' parameter to [`/notifications`](https://spec.matrix.org/v1.10/client-server-api/#get_matrixclientv3notifications) would result in an Internal Server Error. \ No newline at end of file From df4dd8aef249c9580afee491f035499fed3baa1c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 11 Jun 2024 10:00:25 +0100 Subject: [PATCH 4/7] Remove empty query parameter from test I'm not sure why it was there. --- tests/module_api/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py index 5eb1406a066..b6ba472d7d4 100644 --- a/tests/module_api/test_api.py +++ b/tests/module_api/test_api.py @@ -688,7 +688,7 @@ def test_set_push_rules_action(self) -> None: channel = self.make_request( "GET", - "/notifications?from=", + "/notifications", access_token=tok, ) self.assertEqual(channel.code, 200, channel.result) From 59693b7f393315db14e847c6b0bb5f68ad19ad28 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 11 Jun 2024 10:11:29 +0100 Subject: [PATCH 5/7] Provide a less obtuse error message Instead of telling the client that they should have used an integer - which could be confusing as the spec says that the query parameter must be a string - we simply tell the client that their "from" token is unknown to us. --- synapse/rest/client/notifications.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/notifications.py b/synapse/rest/client/notifications.py index 7206d22dcb6..d6ba7115c31 100644 --- a/synapse/rest/client/notifications.py +++ b/synapse/rest/client/notifications.py @@ -33,6 +33,7 @@ from synapse.types import JsonDict from ._base import client_patterns +from ...api.errors import SynapseError if TYPE_CHECKING: from synapse.server import HomeServer @@ -57,8 +58,20 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: user_id = requester.user.to_string() # While this is intended to be "string" to clients, the 'from' token - # is actually based on a numeric ID. So it must parse to as an int. - from_token = parse_integer(request, "from", required=False) + # is actually based on a numeric ID. So it must parse to an int. + from_token_str = parse_string(request, "from", required=False) + if from_token_str is not None: + # Parse to an integer. + try: + from_token = int(from_token_str) + except ValueError: + # If it doesn't parse to an integer, then this cannot possibly be a valid + # pagination token, as we only hand out integers. + raise SynapseError( + 400, "Query parameter \"from\" contains unrecognised token" + ) + else: + from_token = None limit = parse_integer(request, "limit", default=50) only = parse_string(request, "only", required=False) From 5f1b3589c58898b48ce1abb08bc0e58a6f0e2cf0 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 11 Jun 2024 10:13:52 +0100 Subject: [PATCH 6/7] lint --- synapse/rest/client/notifications.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/notifications.py b/synapse/rest/client/notifications.py index d6ba7115c31..168ce50d3ff 100644 --- a/synapse/rest/client/notifications.py +++ b/synapse/rest/client/notifications.py @@ -32,8 +32,8 @@ from synapse.http.site import SynapseRequest from synapse.types import JsonDict -from ._base import client_patterns from ...api.errors import SynapseError +from ._base import client_patterns if TYPE_CHECKING: from synapse.server import HomeServer @@ -68,7 +68,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: # If it doesn't parse to an integer, then this cannot possibly be a valid # pagination token, as we only hand out integers. raise SynapseError( - 400, "Query parameter \"from\" contains unrecognised token" + 400, 'Query parameter "from" contains unrecognised token' ) else: from_token = None From 12fb7d027481a2e12fb80047b1def712cdfb175e Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Wed, 19 Jun 2024 10:05:23 +0100 Subject: [PATCH 7/7] Update changelog.d/17283.bugfix Co-authored-by: Erik Johnston --- changelog.d/17283.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/17283.bugfix b/changelog.d/17283.bugfix index b12bcec863f..98c1f05cc24 100644 --- a/changelog.d/17283.bugfix +++ b/changelog.d/17283.bugfix @@ -1 +1 @@ -Fix a long-standing bug where a non-integer 'from' parameter to [`/notifications`](https://spec.matrix.org/v1.10/client-server-api/#get_matrixclientv3notifications) would result in an Internal Server Error. \ No newline at end of file +Fix a long-standing bug where an invalid 'from' parameter to [`/notifications`](https://spec.matrix.org/v1.10/client-server-api/#get_matrixclientv3notifications) would result in an Internal Server Error. \ No newline at end of file