Require the 'from' parameter for `/notifications` be an integer (#17283)

Co-authored-by: Erik Johnston <erikj@element.io>

5 files changed, 173 insertions, 21 deletions

diff --git a/changelog.d/17283.bugfix b/changelog.d/17283.bugfix
new file mode 100644
index 0000000000..98c1f05cc2
--- /dev/null
+++ b/changelog.d/17283.bugfix
@@ -0,0 +1 @@
+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
diff --git a/synapse/rest/client/notifications.py b/synapse/rest/client/notifications.py
index be9b584748..168ce50d3f 100644
--- a/synapse/rest/client/notifications.py
+++ b/synapse/rest/client/notifications.py
@@ -32,6 +32,7 @@ from synapse.http.servlet import RestServlet, parse_integer, parse_string
 from synapse.http.site import SynapseRequest
 from synapse.types import JsonDict
 
+from ...api.errors import SynapseError
 from ._base import client_patterns
 
 if TYPE_CHECKING:
@@ -56,7 +57,22 @@ class NotificationsServlet(RestServlet):
         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 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)
 
diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py
index bdd0781c48..0ebf5b53d5 100644
--- a/synapse/storage/databases/main/event_push_actions.py
+++ b/synapse/storage/databases/main/event_push_actions.py
@@ -1829,7 +1829,7 @@ class EventPushActionsWorkerStore(ReceiptsWorkerStore, StreamWorkerStore, SQLBas
     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]:
diff --git a/tests/module_api/test_api.py b/tests/module_api/test_api.py
index 5eb1406a06..b6ba472d7d 100644
--- a/tests/module_api/test_api.py
+++ b/tests/module_api/test_api.py
@@ -688,7 +688,7 @@ class ModuleApiTestCase(BaseModuleApiTestCase):
 
         channel = self.make_request(
             "GET",
-            "/notifications?from=",
+            "/notifications",
             access_token=tok,
         )
         self.assertEqual(channel.code, 200, channel.result)
diff --git a/tests/rest/client/test_notifications.py b/tests/rest/client/test_notifications.py
index e9aa2e450e..e4b0455ce8 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 @@ class HTTPPusherTests(HomeserverTestCase):
         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 @@ class HTTPPusherTests(HomeserverTestCase):
         """
         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 @@ class HTTPPusherTests(HomeserverTestCase):
             "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)