Skip to content

MSC4140: rate limit management against delay_id #19830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AndrewFerr wants to merge 19 commits into
base: develop
Choose a base branch
from
+58 −19
Open

MSC4140: rate limit management against delay_id #19830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a016de0
MSC4140: allow auth on management endpoints
AndrewFerr May 21, 2026
6526be9
Add changelog
AndrewFerr May 21, 2026
33eca5e
Lint
AndrewFerr May 21, 2026
f2ba5b3
Update ratelimit documentation
AndrewFerr May 21, 2026
9d9f542
Merge with 'develop'
AndrewFerr May 28, 2026
97a65a8
Merge with 'develop' again
AndrewFerr Jun 5, 2026
dc3772e
Merge with 'develop' again
AndrewFerr Jun 10, 2026
239c6f4
MSC4140: rate limit management against delay_id
AndrewFerr Jun 5, 2026
410f4fa
Add changelog
AndrewFerr Jun 5, 2026
0199f65
Fixup
AndrewFerr Jun 5, 2026
5e0f44a
Still use source IP address as ratelimit fallback
AndrewFerr Jun 5, 2026
f43bc62
Update tests
AndrewFerr Jun 5, 2026
efe144c
Satisfy mypy
AndrewFerr Jun 5, 2026
3745bd7
Revert always awaiting on _initialized_from_db
AndrewFerr Jun 10, 2026
0ad97ef
Fix plural in comment
anoadragon453 Jun 18, 2026
863e586
Minor wording changes
anoadragon453 Jun 18, 2026
c17cdf1
Update generated documentation
AndrewFerr Jun 18, 2026
c50e040
Merge with updates to authed management endpoints
AndrewFerr Jun 18, 2026
881e33e
Merge with 'develop' after #19794
AndrewFerr Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/19830.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[MSC4140: Cancellable delayed events](https://github.com/matrix-org/matrix-spec-proposals/pull/4140): Rate limit the delayed event management endpoints based on the `delay_id`.
23 changes: 16 additions & 7 deletions synapse/handlers/delayed_events.py
View file
Open in desktop
Comment thread
AndrewFerr marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ async def cancel(self, request: SynapseRequest, delay_id: str) -> None:
NotFoundError: if no matching delayed event could be found.
"""
assert self._is_master
await self._mgmt_ratelimit(request)
await self._mgmt_ratelimit(request, delay_id)

@anoadragon453 anoadragon453 Jun 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this weaken the rate-limiting too much? Now that a user can spam requests, as long as they cycle through (effectively unlimited) delay IDs?

I get the reasoning that an external service would end up getting rate-limited if it's managing lots of delay IDs for a bunch of users... but would it not make sense to just exclude such a service from rate-limiting?

And finally, I get that lots of logged-out users on the same corporate VPN would end up getting rate-limited due to sharing one IP... but at this point, what is the rate-limit protecting against? Seems it's only stopping a buggy client from spamming the same endpoint accidentally?

@AndrewFerr AndrewFerr Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this weaken the rate-limiting too much? Now that a user can spam requests, as long as they cycle through (effectively unlimited) delay IDs?

The worst that such spammy requests would do is make the server look up from a database index (to check for the existence of the target delayed event). IMO this is an acceptable cost to allow for the kind of rate-limiting this PR is after, but if that's still too costly, we can consider another approach.

would it not make sense to just exclude such a service from rate-limiting?

That's not a bad idea, but then the question is how Synapse would identify such an external service, given the "service" is currently just any arbitrary HTTP client that's been given some delay IDs.

what is the rate-limit protecting against? Seems it's only stopping a buggy client from spamming the same endpoint accidentally?

Yes, be it a buggy or malicious client. It takes non-trivial work to reset a delayed event (past the initial quick check for the existence of the delayed event), so it's worth something to protect it with a rate limit.

@AndrewFerr AndrewFerr Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It bears mentioning that the long-term solution to all of this is to have the external services be OAuth 2.0 clients, with a token that has scoped access to these management endpoints (detailed in MSC4140). Rate limits could then be applied per client, as the clients would be identifiable by their token. The idea of using delay_id grouping is just a stopgap measure to apply some kind of rate limit in the absence of a way to identify an unauthed external service.

await make_deferred_yieldable(self._initialized_from_db)

next_send_ts = await self._store.cancel_delayed_event(delay_id)
Expand All @@ -428,7 +428,7 @@ async def restart(self, request: SynapseRequest, delay_id: str) -> None:
Raises:
NotFoundError: if no matching delayed event could be found.
"""
await self._mgmt_ratelimit(request)
await self._mgmt_ratelimit(request, delay_id)

# Note: We don't need to wait on `self._initialized_from_db` here as the
# events that deals with are already marked as processed.
Expand All @@ -452,7 +452,7 @@ async def send(self, request: SynapseRequest, delay_id: str) -> None:
NotFoundError: if no matching delayed event could be found.
"""
assert self._is_master
await self._mgmt_ratelimit(request)
await self._mgmt_ratelimit(request, delay_id)
await make_deferred_yieldable(self._initialized_from_db)

event, next_send_ts = await self._store.process_target_delayed_event(delay_id)
Expand All @@ -462,18 +462,27 @@ async def send(self, request: SynapseRequest, delay_id: str) -> None:

await self._send_event(event)

async def _mgmt_ratelimit(self, request: SynapseRequest) -> None:
async def _mgmt_ratelimit(self, request: SynapseRequest, delay_id: str) -> None:
"""
Ratelimit requests with the `_delayed_event_mgmt_ratelimiter` keyed on the
ID of the delayed event to be managed, and either the
user making the request, or the request's IP address if unauthed.
"""
# NOTE: it would be more accurate to run this in a transaction that also contains
# the action to be done with the delay_id, but the worst that can happen without it
# is the possibility of the delay_id disappearing after having checked for it here,
# which isn't terrible since it's used here only for applying a ratelimit.
await self._store.assert_delayed_event(delay_id)
Comment thread
anoadragon453 marked this conversation as resolved.

if self._auth.has_access_token(request):
requester = await self._auth.get_user_by_req(request)
key = None
requester_key = requester.user.to_string()
else:
requester = None
key = request.getClientAddress().host
await self._delayed_event_mgmt_ratelimiter.ratelimit(requester, key)
requester_key = request.getClientAddress().host
await self._delayed_event_mgmt_ratelimiter.ratelimit(
requester, (requester_key, delay_id)
)

async def _send_on_timeout(self) -> None:
self._next_delayed_event_call = None
Expand Down
17 changes: 17 additions & 0 deletions synapse/storage/databases/main/delayed_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,23 @@ def restart_delayed_event_txn(
"restart_delayed_event", restart_delayed_event_txn
)

async def assert_delayed_event(self, delay_id: str) -> None:
"""Checks whether there is a delayed event in the DB with the given delay_id.

Raises:
NotFoundError: if there is no matching delayed event.
"""

res = await self.db_pool.simple_select_one_onecol(
table="delayed_events",
keyvalues={"delay_id": delay_id},
retcol="1",
allow_none=True,
desc="has_delayed_event",
)
if not res:
raise NotFoundError("Delayed event not found")

async def get_count_of_delayed_events(self) -> int:
"""Returns the number of pending delayed events in the DB."""

Expand Down
36 changes: 24 additions & 12 deletions tests/rest/client/test_delayed_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ def test_cancel_delayed_state_event(self, action_in_path: bool) -> None:
)
def test_cancel_delayed_event_ratelimit(self, action_in_path: bool) -> None:
delay_ids = []
for _ in range(3):
for _ in range(5):
channel = self.make_request(
"POST",
_get_path_for_delayed_send(self.room_id, _EVENT_TYPE, 100000),
Expand All @@ -329,11 +329,14 @@ def test_cancel_delayed_event_ratelimit(self, action_in_path: bool) -> None:
assert delay_id is not None
delay_ids.append(delay_id)

delay_id = delay_ids.pop(0)
channel = self._update_delayed_event(delay_id, "cancel", action_in_path)
self.assertEqual(HTTPStatus.OK, channel.code, channel.result)
for _ in range(2):
delay_id = delay_ids.pop(0)
channel = self._update_delayed_event(delay_id, "cancel", action_in_path)
self.assertEqual(HTTPStatus.OK, channel.code, channel.result)

delay_id = delay_ids.pop(0)
channel = self._update_delayed_event(delay_id, "restart", action_in_path)
self.assertEqual(HTTPStatus.OK, channel.code, channel.result)
channel = self._update_delayed_event(delay_id, "cancel", action_in_path)
self.assertEqual(HTTPStatus.TOO_MANY_REQUESTS, channel.code, channel.result)

Expand All @@ -347,6 +350,16 @@ def test_cancel_delayed_event_ratelimit(self, action_in_path: bool) -> None:
channel = self._update_delayed_event(
delay_id, "cancel", action_in_path, self.user1_access_token
)
self.assertEqual(HTTPStatus.OK, channel.code, channel.result)

delay_id = delay_ids.pop(0)
channel = self._update_delayed_event(
delay_id, "restart", action_in_path, self.user1_access_token
)
self.assertEqual(HTTPStatus.OK, channel.code, channel.result)
channel = self._update_delayed_event(
delay_id, "cancel", action_in_path, self.user1_access_token
)
self.assertEqual(HTTPStatus.TOO_MANY_REQUESTS, channel.code, channel.result)

# Add the current user to the ratelimit overrides, allowing them no ratelimiting.
Expand Down Expand Up @@ -502,23 +515,23 @@ def test_restart_delayed_state_event(self, action_in_path: bool) -> None:
)
def test_restart_delayed_event_ratelimit(self, action_in_path: bool) -> None:
delay_ids = []
for _ in range(3):
for _ in range(2):
channel = self.make_request(
"POST",
_get_path_for_delayed_send(self.room_id, _EVENT_TYPE, 100000),
{},
self.user1_access_token,
)
self.assertEqual(HTTPStatus.OK, channel.code, channel.result)
delay_id = channel.json_body.get("delay_id")
assert delay_id is not None
assert "delay_id" in channel.json_body
delay_id = channel.json_body["delay_id"]
delay_ids.append(delay_id)

delay_id = delay_ids.pop(0)
channel = self._update_delayed_event(delay_id, "restart", action_in_path)
self.assertEqual(HTTPStatus.OK, channel.code, channel.result)
for _ in range(2):
delay_id = delay_ids.pop(0)
channel = self._update_delayed_event(delay_id, "restart", action_in_path)
self.assertEqual(HTTPStatus.OK, channel.code, channel.result)

delay_id = delay_ids.pop(0)
channel = self._update_delayed_event(delay_id, "restart", action_in_path)
self.assertEqual(HTTPStatus.TOO_MANY_REQUESTS, channel.code, channel.result)

Expand All @@ -528,7 +541,6 @@ def test_restart_delayed_event_ratelimit(self, action_in_path: bool) -> None:
)
self.assertEqual(HTTPStatus.OK, channel.code, channel.result)

delay_id = delay_ids.pop(0)
channel = self._update_delayed_event(
delay_id, "restart", action_in_path, self.user1_access_token
)
Expand Down
Loading