From 15624be2799e1cb83e4b600b658cd1ea9a29bab4 Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Fri, 18 Sep 2026 13:00:45 +0200 Subject: [PATCH] Profile endpoint rate limit (#20218) This provides configurable (via `rc_profile`) rate limits for profile endpoints: - `GET /profile/{username}` - `GET /profile/{username}/{keyName}` including (`displayname` & `avatar_url`) ### Pull Request Checklist * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: Erik Johnston --- changelog.d/20218.feature | 1 + demo/start.sh | 3 + .../conf/workers-shared-extra.yaml.j2 | 4 ++ .../configuration/config_documentation.md | 21 +++++++ schema/synapse-config.schema.yaml | 14 +++++ synapse/api/auth/__init__.py | 14 +++++ synapse/api/auth/base.py | 38 +++++++++++- synapse/config/ratelimiting.py | 6 ++ synapse/rest/client/profile.py | 42 +++++++++---- synapse/server.py | 8 +++ tests/api/test_auth.py | 35 +++++++++++ tests/rest/client/test_profile.py | 61 +++++++++++++++++++ 12 files changed, 235 insertions(+), 12 deletions(-) create mode 100644 changelog.d/20218.feature diff --git a/changelog.d/20218.feature b/changelog.d/20218.feature new file mode 100644 index 0000000000..f8e85a88f8 --- /dev/null +++ b/changelog.d/20218.feature @@ -0,0 +1 @@ +Add a rate limit on the client profile lookup endpoints, configurable via `rc_profile`. diff --git a/demo/start.sh b/demo/start.sh index 471e881f46..5e4e1c10b9 100755 --- a/demo/start.sh +++ b/demo/start.sh @@ -151,6 +151,9 @@ for port in 8080 8081 8082; do rc_user_directory: per_second: 1000 burst_count: 1000 + rc_profile: + per_second: 1000 + burst_count: 1000 RC ) echo "${ratelimiting}" >> "$port.config" diff --git a/docker/complement/conf/workers-shared-extra.yaml.j2 b/docker/complement/conf/workers-shared-extra.yaml.j2 index 4dc4eb932b..251018e1d2 100644 --- a/docker/complement/conf/workers-shared-extra.yaml.j2 +++ b/docker/complement/conf/workers-shared-extra.yaml.j2 @@ -108,6 +108,10 @@ rc_user_directory: per_second: 9999 burst_count: 9999 +rc_profile: + per_second: 9999 + burst_count: 9999 + federation_rr_transactions_per_room_per_second: 9999 allow_device_name_lookup_over_federation: true diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index c3bc8ebcb2..566596dc8a 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -2116,6 +2116,27 @@ rc_user_directory: burst_count: 200.0 ``` --- +### `rc_profile` + +*(object)* This option allows admins to ratelimit profile lookups by clients. + +Requests are limited per user when the request is authenticated, otherwise per client IP address. + +_Added in Synapse 1.162.0._ + +This setting has the following sub-options: + +* `per_second` (number): Maximum number of requests a client can send per second. + +* `burst_count` (number): Maximum number of requests a client can send before being throttled. + +Default configuration: +```yaml +rc_profile: + per_second: 1.0 + burst_count: 500.0 +``` +--- ### `federation_rr_transactions_per_room_per_second` *(integer)* Sets outgoing federation transaction frequency for sending read-receipts, per-room. diff --git a/schema/synapse-config.schema.yaml b/schema/synapse-config.schema.yaml index ad5e87ef18..4550ad3e06 100644 --- a/schema/synapse-config.schema.yaml +++ b/schema/synapse-config.schema.yaml @@ -2391,6 +2391,20 @@ properties: default: per_second: 0.016 burst_count: 200.0 + rc_profile: + $ref: "#/$defs/rc" + description: >- + This option allows admins to ratelimit profile lookups by clients. + + + Requests are limited per user when the request is authenticated, + otherwise per client IP address. + + + _Added in Synapse 1.162.0._ + default: + per_second: 1.0 + burst_count: 500.0 federation_rr_transactions_per_room_per_second: type: integer description: >- diff --git a/synapse/api/auth/__init__.py b/synapse/api/auth/__init__.py index d8d3b31b9d..dd0f05b1d3 100644 --- a/synapse/api/auth/__init__.py +++ b/synapse/api/auth/__init__.py @@ -98,6 +98,20 @@ class Auth(Protocol): AuthError if access is denied for the user in the access token """ + async def get_optional_user_by_req( + self, + request: SynapseRequest, + allow_guest: bool = False, + allow_expired: bool = False, + allow_locked: bool = False, + ) -> Requester | None: + """Like `get_user_by_req`, except returns None when the request carries + no access token at all. A token that is present but invalid still + raises, as with `get_user_by_req`. + + For endpoints where authentication is optional. + """ + async def get_user_by_req_experimental_feature( self, request: SynapseRequest, diff --git a/synapse/api/auth/base.py b/synapse/api/auth/base.py index 7b8214fabb..93f579c69a 100644 --- a/synapse/api/auth/base.py +++ b/synapse/api/auth/base.py @@ -19,6 +19,7 @@ # # import logging +from abc import ABC, abstractmethod from http import HTTPStatus from typing import TYPE_CHECKING @@ -49,7 +50,7 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) -class BaseAuth: +class BaseAuth(ABC): """Common base class for all auth implementations.""" def __init__(self, hs: "HomeServer"): @@ -187,6 +188,7 @@ class BaseAuth: 403, "Application service has not registered this user (%s)" % user_id ) + @abstractmethod async def is_server_admin(self, requester: Requester) -> bool: """Check if the given user is a local server admin. @@ -240,6 +242,40 @@ class BaseAuth: return user_level >= send_level + @abstractmethod + async def get_user_by_req( + self, + request: SynapseRequest, + allow_guest: bool = False, + allow_expired: bool = False, + allow_locked: bool = False, + ) -> Requester: + """Get a registered user's ID. See `Auth.get_user_by_req`.""" + raise NotImplementedError() + + async def get_optional_user_by_req( + self, + request: SynapseRequest, + allow_guest: bool = False, + allow_expired: bool = False, + allow_locked: bool = False, + ) -> Requester | None: + """Like `get_user_by_req`, except returns None when the request carries + no access token at all. A token that is present but invalid still + raises, as with `get_user_by_req`. + + For endpoints where authentication is optional. + """ + if not self.has_access_token(request): + return None + + return await self.get_user_by_req( + request, + allow_guest=allow_guest, + allow_expired=allow_expired, + allow_locked=allow_locked, + ) + @staticmethod def has_access_token(request: Request) -> bool: """Checks if the request has an access_token. diff --git a/synapse/config/ratelimiting.py b/synapse/config/ratelimiting.py index 13c9c4dba0..e5f29b6dc1 100644 --- a/synapse/config/ratelimiting.py +++ b/synapse/config/ratelimiting.py @@ -258,3 +258,9 @@ class RatelimitConfig(Config): "rc_user_directory", defaults={"per_second": 0.016, "burst_count": 200}, ) + + self.rc_profile = RatelimitSettings.parse( + config, + "rc_profile", + defaults={"per_second": 1, "burst_count": 500}, + ) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index 58a6c7c4e2..aadda870fd 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -36,7 +36,7 @@ from synapse.http.servlet import ( ) from synapse.http.site import SynapseRequest from synapse.rest.client._base import client_patterns -from synapse.types import JsonDict, UserID +from synapse.types import JsonDict, Requester, UserID from synapse.util.stringutils import is_namedspaced_grammar if TYPE_CHECKING: @@ -57,6 +57,32 @@ def _read_propagate(hs: "HomeServer", request: SynapseRequest) -> bool: return propagate +async def _auth_and_ratelimit_profile_lookup( + hs: "HomeServer", request: SynapseRequest +) -> Requester | None: + """Authenticate a profile lookup request and apply the `rc_profile` rate + limit to it. + + Authentication is optional unless `require_auth_for_profile_requests` is + set. The rate limit is applied per user if credentials were supplied, else + per client IP address. + + Returns: + The requester if the request was authenticated, else None. + """ + auth = hs.get_auth() + requester: Requester | None + if hs.config.server.require_auth_for_profile_requests: + requester = await auth.get_user_by_req(request) + else: + requester = await auth.get_optional_user_by_req(request, allow_guest=True) + + await hs.get_profile_lookup_ratelimiter().ratelimit( + requester, key=None if requester else request.getClientAddress().host + ) + return requester + + class ProfileRestServlet(RestServlet): PATTERNS = client_patterns("/profile/(?P[^/]*)$", v1=True) CATEGORY = "Event sending requests" @@ -70,11 +96,8 @@ class ProfileRestServlet(RestServlet): async def on_GET( self, request: SynapseRequest, user_id: str ) -> tuple[int, JsonDict]: - requester_user = None - - if self.hs.config.server.require_auth_for_profile_requests: - requester = await self.auth.get_user_by_req(request) - requester_user = requester.user + requester = await _auth_and_ratelimit_profile_lookup(self.hs, request) + requester_user = requester.user if requester else None if not UserID.is_valid(user_id): raise SynapseError( @@ -119,11 +142,8 @@ class ProfileFieldRestServlet(RestServlet): async def on_GET( self, request: SynapseRequest, user_id: str, field_name: str ) -> tuple[int, JsonDict]: - requester_user = None - - if self.hs.config.server.require_auth_for_profile_requests: - requester = await self.auth.get_user_by_req(request) - requester_user = requester.user + requester = await _auth_and_ratelimit_profile_lookup(self.hs, request) + requester_user = requester.user if requester else None if not UserID.is_valid(user_id): raise SynapseError( diff --git a/synapse/server.py b/synapse/server.py index 9422882167..9bea208974 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -722,6 +722,14 @@ class HomeServer(metaclass=abc.ABCMeta): cfg=self.config.ratelimiting.rc_registration, ) + @cache_in_self + def get_profile_lookup_ratelimiter(self) -> Ratelimiter: + return Ratelimiter( + store=self.get_datastores().main, + clock=self.get_clock(), + cfg=self.config.ratelimiting.rc_profile, + ) + @cache_in_self def get_federation_client(self) -> FederationClient: return FederationClient(self) diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index e7ee7eec91..5604f41437 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -106,6 +106,41 @@ class AuthTestCase(unittest.HomeserverTestCase): self.assertEqual(f.code, 401) self.assertEqual(f.errcode, "M_MISSING_TOKEN") + def test_get_optional_user_by_req_valid_token(self) -> None: + user_info = TokenLookupResult( + user_id=self.test_user_id.to_string(), token_id=5, device_id="device" + ) + self.store.get_user_by_access_token = AsyncMock(return_value=user_info) + self.store.mark_access_token_as_used = AsyncMock(return_value=None) + self.store.get_user_locked_status = AsyncMock(return_value=False) + + request = Mock(args={}) + request.args[b"access_token"] = [self.test_token] + request.requestHeaders.getRawHeaders = mock_getRawHeaders() + requester = self.get_success(self.auth.get_optional_user_by_req(request)) + assert requester is not None + self.assertEqual(requester.user, self.test_user_id) + + def test_get_optional_user_by_req_bad_token(self) -> None: + """A token that is present but invalid is still rejected.""" + self.store.get_user_by_access_token = AsyncMock(return_value=None) + + request = Mock(args={}) + request.args[b"access_token"] = [self.test_token] + request.requestHeaders.getRawHeaders = mock_getRawHeaders() + f = self.get_failure( + self.auth.get_optional_user_by_req(request), InvalidClientTokenError + ).value + self.assertEqual(f.code, 401) + self.assertEqual(f.errcode, Codes.UNKNOWN_TOKEN) + + def test_get_optional_user_by_req_missing_token(self) -> None: + """A request without any token yields no requester rather than an error.""" + request = Mock(args={}) + request.requestHeaders.getRawHeaders = mock_getRawHeaders() + requester = self.get_success(self.auth.get_optional_user_by_req(request)) + self.assertIsNone(requester) + def test_get_user_by_req_appservice_valid_token(self) -> None: app_service = Mock( id="as_id", diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 30f58f1cc3..b3a719d851 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -1037,3 +1037,64 @@ class OwnProfileUnrestrictedTestCase(unittest.HomeserverTestCase): access_token=self.requester_tok, ) self.assertEqual(channel.code, 200, channel.result) + + +class ProfileRatelimitTestCase(unittest.HomeserverTestCase): + servlets = [ + admin.register_servlets_for_client_rest_resource, + login.register_servlets, + profile.register_servlets, + ] + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.owner = self.register_user("owner", "pass") + self.owner_tok = self.login("owner", "pass") + self.other = self.register_user("other", "pass", displayname="Bob") + self.other_tok = self.login("other", "pass") + + @unittest.override_config({"rc_profile": {"per_second": 0.1, "burst_count": 3}}) + def test_ratelimit_authenticated(self) -> None: + """Profile lookups from an authenticated user are rate limited per user, + with the limit shared across the profile endpoints. + """ + channel = self.make_request( + "GET", f"/profile/{self.other}", access_token=self.owner_tok + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", f"/profile/{self.other}/displayname", access_token=self.owner_tok + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", f"/profile/{self.other}/avatar_url", access_token=self.owner_tok + ) + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request( + "GET", f"/profile/{self.other}", access_token=self.owner_tok + ) + self.assertEqual(channel.code, 429, channel.result) + + # Another user is not affected by the first user's limit. + channel = self.make_request( + "GET", f"/profile/{self.owner}", access_token=self.other_tok + ) + self.assertEqual(channel.code, 200, channel.result) + + @unittest.override_config({"rc_profile": {"per_second": 0.1, "burst_count": 3}}) + def test_ratelimit_unauthenticated(self) -> None: + """Unauthenticated profile lookups are rate limited per IP address.""" + for _ in range(3): + channel = self.make_request("GET", f"/profile/{self.other}") + self.assertEqual(channel.code, 200, channel.result) + + channel = self.make_request("GET", f"/profile/{self.other}") + self.assertEqual(channel.code, 429, channel.result) + + # An authenticated user is not affected by the per-IP limit. + channel = self.make_request( + "GET", f"/profile/{self.other}", access_token=self.owner_tok + ) + self.assertEqual(channel.code, 200, channel.result)