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)
|
