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

<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->

* [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 <erik@matrix.org>

changelog.d/20218.feature

@@ -0,0 +1 @@
+Add a rate limit on the client profile lookup endpoints, configurable via `rc_profile`.

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"

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

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.

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

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,

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.

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},
+        )

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<user_id>[^/]*)$", 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(

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)

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",

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)