Mirrors
/
synapse
mirror of https://github.com/element-hq/synapse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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

Co-authored-by: Erik Johnston <erikj@element.io>
This commit is contained in:
Andrew Morgan 2024-06-19 10:05:39 +01:00 committed by GitHub
parent 199223062a
commit afaf2d9388
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 173 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
changelog.d/17283.bugfix Normal file
View File

@ -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.

18
synapse/rest/client/notifications.py
View File

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

2
synapse/storage/databases/main/event_push_actions.py
View File

@ -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]:

2
tests/module_api/test_api.py
View File

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

171
tests/rest/client/test_notifications.py
View File

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