From 44216bf2b68b3099020a13d58cbcf8a420b0c0dc Mon Sep 17 00:00:00 2001 From: Olivier 'reivilibre Date: Fri, 24 Jul 2026 13:25:26 +0100 Subject: [PATCH] Limit the size of push rules Fixes: https://github.com/element-hq/synapse/security/advisories/GHSA-fp53-rw9v-hcf9 Fixes: https://github.com/matrix-org/internal-config/issues/1071 Because this is an out-of-spec limit and could break someone's workflow on release day, I've opted to make it configurable. ----- Reviewed-on: https://github.com/element-hq/synapse-private/pull/149 --- .../configuration/config_documentation.md | 21 +- schema/synapse-config.schema.yaml | 58 +++++- synapse/config/_base.pyi | 2 + synapse/config/_util.py | 15 +- synapse/config/homeserver.py | 3 + synapse/config/push_rules.py | 63 ++++++ synapse/storage/databases/main/push_rule.py | 101 ++++++++- tests/rest/client/test_push_rule_attrs.py | 193 ++++++++++++++++++ 8 files changed, 437 insertions(+), 19 deletions(-) create mode 100644 synapse/config/push_rules.py diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 62a4676adb..8c3a35ae4e 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -3808,7 +3808,7 @@ This setting has the following sub-options: Defaults to `null`. -* `update_profile_information` (boolean): Use this setting to keep a user's profile fields in sync with information from the identity provider. Fields are checked on every SSO login, and are updated if necessary. Note that enabling this option will override user profile information, regardless of whether users have opted-out of syncing that information when first signing in. Fields that will be synced: +* `update_profile_information` (boolean): Use this setting to keep a user's profile fields in sync with information from the identity provider. Fields are checked on every SSO login, and are updated if necessary. Note that enabling this option will override user profile information, regardless of whether users have opted-out of syncing that information when first signing in. Fields that will be synced: * displayname * picture - only if Synapse media repository is running in the main process (i.e. not workerized) and media is stored locally Defaults to `false`. @@ -3938,6 +3938,25 @@ push: jitter_delay: 10s ``` --- +### `push_rules` + +*(object)* Options for push rules + +This setting has the following sub-options: + +* `limits` (object): Limits on the size of push rules that users can have + + This setting has the following sub-options: + + * `rule_count` (integer): This is the total number of push rules that each user can have. Power users may expect to have one push rule per room. Defaults to `10000`. + + * `rule_id_length` (integer): This is the maximum length of a push rule ID, in bytes. Push rule IDs need to be allowed to be at least as long as a room ID (which are [limited to 255 bytes per specification](https://spec.matrix.org/v1.19/appendices/#room-ids)) + It's recommended to leave this option as it is. We expect to remove this option if/when the specification standardises on a limit. Defaults to `300`. + + * `rule_size` (integer): This is the maximum size of a push rule's body, in bytes. + The exact mechanism for calculating this size is currently an implementation detail, subject to change. This limit should be treated as a coarse sanity limit rather than something to fine-tune. + It's recommended to leave this option as it is. We expect to remove this option if/when the specification standardises on a limit and a mechanism for calculating it. Defaults to `1024`. +--- ## Rooms Config options relating to rooms. diff --git a/schema/synapse-config.schema.yaml b/schema/synapse-config.schema.yaml index ea5bd7c7af..691335f119 100644 --- a/schema/synapse-config.schema.yaml +++ b/schema/synapse-config.schema.yaml @@ -2408,7 +2408,7 @@ properties: Enable the local on-disk media storage provider. When disabled, media is stored only in configured `media_storage_providers` and temporary files are used for processing. - + **Warning:** If this option is set to `false` and no `media_storage_providers` are configured, all media requests will return 404 errors as there will be no storage backend available. @@ -2962,7 +2962,7 @@ properties: examples: - false matrix_rtc: - type: object + type: object description: >- Options related to MatrixRTC. properties: @@ -2982,14 +2982,13 @@ properties: description: >- The base URL of the LiveKit service. Should only be used with LiveKit-based transports. example: https://matrix-rtc.example.com/livekit/jwt - description: - A list of transport types and arguments to use for MatrixRTC connections. + description: A list of transport types and arguments to use for MatrixRTC connections. default: [] default: {} examples: - transports: - - type: livekit - livekit_service_url: https://matrix-rtc.example.com/livekit/jwt + - type: livekit + livekit_service_url: https://matrix-rtc.example.com/livekit/jwt enable_registration: type: boolean description: >- @@ -4664,10 +4663,10 @@ properties: type: boolean description: >- Use this setting to keep a user's profile fields in sync with - information from the identity provider. Fields are checked on every - SSO login, and are updated if necessary. Note that enabling this - option will override user profile information, regardless of whether - users have opted-out of syncing that information when first signing + information from the identity provider. Fields are checked on every + SSO login, and are updated if necessary. Note that enabling this + option will override user profile information, regardless of whether + users have opted-out of syncing that information when first signing in. Fields that will be synced: * displayname @@ -4879,6 +4878,45 @@ properties: include_content: false group_unread_count_by_room: false jitter_delay: 10s + push_rules: + type: object + description: Options for push rules + properties: + limits: + type: object + description: Limits on the size of push rules that users can have + properties: + rule_count: + type: integer + default: 10000 + description: >- + This is the total number of push rules that each user can have. + Power users may expect to have one push rule per room. + rule_id_length: + type: integer + default: 300 + description: >- + This is the maximum length of a push rule ID, in bytes. + Push rule IDs need to be allowed to be at least as long + as a room ID (which are [limited to 255 bytes per specification](https://spec.matrix.org/v1.19/appendices/#room-ids)) + + It's recommended to leave this option as it is. + We expect to remove this option if/when the specification standardises on + a limit. + rule_size: + type: integer + default: 1024 + description: >- + This is the maximum size of a push rule's body, in bytes. + + The exact mechanism for calculating this size is currently an implementation + detail, subject to change. + This limit should be treated as a coarse sanity limit rather than something + to fine-tune. + + It's recommended to leave this option as it is. + We expect to remove this option if/when the specification standardises on + a limit and a mechanism for calculating it. encryption_enabled_by_default_for_room_type: type: string description: >- diff --git a/synapse/config/_base.pyi b/synapse/config/_base.pyi index 7c371d161c..f226afc2b4 100644 --- a/synapse/config/_base.pyi +++ b/synapse/config/_base.pyi @@ -38,6 +38,7 @@ from synapse.config import ( # noqa: F401 oidc, password_auth_providers, push, + push_rules, ratelimiting, redis, registration, @@ -103,6 +104,7 @@ class RootConfig: worker: workers.WorkerConfig authproviders: password_auth_providers.PasswordAuthProviderConfig push: push.PushConfig + push_rules: push_rules.PushRulesConfig spamchecker: spam_checker.SpamCheckerConfig room: room.RoomConfig userdirectory: user_directory.UserDirectoryConfig diff --git a/synapse/config/_util.py b/synapse/config/_util.py index e09c68ebd4..8e1dd52c29 100644 --- a/synapse/config/_util.py +++ b/synapse/config/_util.py @@ -18,12 +18,13 @@ # [This file includes modifications made by New Vector Limited] # # -from typing import Any, TypeVar +from typing import Annotated, Any, TypeVar import jsonschema -from pydantic import BaseModel, TypeAdapter, ValidationError +from pydantic import BaseModel, BeforeValidator, StrictInt, TypeAdapter, ValidationError +from pydantic_core.core_schema import int_schema -from synapse.config._base import ConfigError +from synapse.config._base import Config, ConfigError from synapse.types import JsonDict, StrSequence @@ -97,3 +98,11 @@ def parse_and_validate_mapping( except ValidationError as e: raise ConfigError(str(e)) from e return instances + + +ConfigByteSize = Annotated[ + StrictInt, BeforeValidator(Config.parse_size), int_schema(ge=0) +] +""" +A size in bytes. Pydantic-compatible wrapper for `Config.parse_size` +""" diff --git a/synapse/config/homeserver.py b/synapse/config/homeserver.py index 94ebe583a4..e2ae182fc3 100644 --- a/synapse/config/homeserver.py +++ b/synapse/config/homeserver.py @@ -19,6 +19,8 @@ # # +from synapse.config.push_rules import PushRulesConfig + from ._base import ConfigError, RootConfig from .account_validity import AccountValidityConfig from .api import ApiConfig @@ -102,6 +104,7 @@ class HomeServerConfig(RootConfig): EmailConfig, PasswordAuthProviderConfig, PushConfig, + PushRulesConfig, SpamCheckerConfig, RoomConfig, UserDirectoryConfig, diff --git a/synapse/config/push_rules.py b/synapse/config/push_rules.py new file mode 100644 index 0000000000..deb7e8dde1 --- /dev/null +++ b/synapse/config/push_rules.py @@ -0,0 +1,63 @@ +# +# This file is licensed under the Affero General Public License (AGPL) version 3. +# +# Copyright (C) 2026 Element Creations Ltd +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# See the GNU Affero General Public License for more details: +# . + +from typing import Annotated, Any + +from pydantic import ( + Field, + StrictInt, + ValidationError, +) + +from synapse.config._util import ConfigByteSize +from synapse.types import JsonDict +from synapse.util.pydantic_models import ParseModel + +from ._base import Config, ConfigError + + +class PushRulesLimitsConfig(ParseModel): + # Chosen arbitrarily, but with the rough rationale that a user + # might have on the order of 10k rooms and want to set a push rule override for each one. + rule_count: Annotated[StrictInt, Field(ge=0)] = 10_000 + + # Chosen arbitrarily, but with the rationale that room IDs are allowed to be up to 255 bytes + # and they are often used in rule IDs. + rule_id_length: Annotated[StrictInt, Field(ge=1)] = 300 + + # Chosen arbitrarily, but with the rationale that real-world push rules don't get + # nearly this big in practice. + # Even 512 bytes would probably have been fine, but we should leave space for the use cases + # of push rules to grow in the future. + rule_size: Annotated[ConfigByteSize, Field(ge=1)] = 1024 + + +class PushRulesConfigModel(ParseModel): + limits: PushRulesLimitsConfig = Field(default_factory=PushRulesLimitsConfig) + + +class PushRulesConfig(Config): + section = "push_rules" + + def read_config(self, config: JsonDict, **kwargs: Any) -> None: + raw_config = config.get("push_rules", {}) + + try: + parsed = PushRulesConfigModel(**raw_config) + except ValidationError as e: + raise ConfigError( + f"Could not validate configuration: {e}", + path=("push_rules",), + ) from e + + self.limits = parsed.limits diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index d361166cec..5086f4639b 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -19,6 +19,7 @@ # # import logging +from http import HTTPStatus from typing import ( TYPE_CHECKING, Any, @@ -31,7 +32,7 @@ from typing import ( from twisted.internet import defer -from synapse.api.errors import StoreError +from synapse.api.errors import Codes, StoreError, SynapseError from synapse.config.homeserver import ExperimentalConfig from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.replication.tcp.streams import PushRulesStream @@ -112,6 +113,22 @@ def _load_rules( return filtered_rules +def _push_rule_size_for_limits(*, conditions_json: str, actions_json: str) -> int: + """ + Returns the size of a push rule, as used for applying the size limit. + + We aren't tied to any particular definition, but currently this is + simply the size in bytes of the conditions and actions JSON added together, + so not rocket science, but this function provides a 'label' for it. + """ + # This is not a very predictable way of calculating the size from the + # point of view of the client, but since it's an out-of-spec limit + # entirely at our discretion, we don't really have to worry about + # the exact calculation. + # FIXME: Spec a predictable push rule size limit + return len(conditions_json.encode("utf-8")) + len(actions_json.encode("utf-8")) + + class PushRulesWorkerStore( ApplicationServiceWorkerStore, PusherWorkerStore, @@ -170,6 +187,8 @@ class PushRulesWorkerStore( self._push_rule_id_gen = IdGenerator(db_conn, "push_rules", "id") self._push_rules_enable_id_gen = IdGenerator(db_conn, "push_rules_enable", "id") + self._config = hs.config.push_rules + def get_max_push_rules_stream_id(self) -> int: """Get the position of the push rules stream. @@ -409,6 +428,29 @@ class PushRulesWorkerStore( conditions_json = json_encoder.encode(conditions) actions_json = json_encoder.encode(actions) + + rule_id_len = len(rule_id.encode("utf-8")) + if rule_id_len > self._config.limits.rule_id_length: + raise SynapseError( + HTTPStatus.REQUEST_ENTITY_TOO_LARGE, + f"Push rule ID length exceeds server limit ({rule_id_len} bytes > {self._config.limits.rule_id_length} bytes).", + # FIXME: Provide a better error code. + # None of the existing options seem entirely correct, though. + Codes.UNKNOWN, + ) + + rule_body_size = _push_rule_size_for_limits( + conditions_json=conditions_json, actions_json=actions_json + ) + if rule_body_size > self._config.limits.rule_size: + raise SynapseError( + HTTPStatus.REQUEST_ENTITY_TOO_LARGE, + f"Push rule size exceeds server limit ({rule_body_size} bytes > {self._config.limits.rule_size} bytes).", + # FIXME: Provide a better error code. + # None of the existing options seem entirely correct, though. + Codes.UNKNOWN, + ) + async with self._push_rules_stream_id_gen.get_next() as stream_id: event_stream_ordering = self._stream_id_gen.get_current_token() @@ -578,13 +620,19 @@ class PushRulesWorkerStore( actions_json: str, update_stream: bool = True, ) -> None: + """Specialised version of simple_upsert_txn that picks a push_rule_id + using the _push_rule_id_gen if it needs to insert the rule. + + Preconditions: + - this worker is a push writer + - the "push_rules" table is locked + - the push rule has already been validated, + including for rule ID length and rule body size. + """ + if not self._is_push_writer: raise Exception("Not a push writer") - """Specialised version of simple_upsert_txn that picks a push_rule_id - using the _push_rule_id_gen if it needs to insert the rule. It assumes - that the "push_rules" table is locked""" - sql = ( "UPDATE push_rules" " SET priority_class = ?, priority = ?, conditions = ?, actions = ?" @@ -597,6 +645,27 @@ class PushRulesWorkerStore( ) if txn.rowcount == 0: + # About to add a new rule, so check our limits first. + txn.execute( + """ + SELECT COUNT(*) FROM push_rules + WHERE user_name = ? + """, + (user_id,), + ) + (num_push_rules,) = cast(tuple[int], txn.fetchone()) + if num_push_rules >= self._config.limits.rule_count: + raise SynapseError( + HTTPStatus.BAD_REQUEST, + f"Creating a push rule would exceed the limit on the number of push rules associated with your account ({num_push_rules + 1} rules > {self._config.limits.rule_count} rules)", + # FIXME: Provide a better error code, especially for this case. + # None of the existing options seem entirely correct, though. + # `M_USER_LIMIT_EXCEEDED` comes closest but needs an `info_uri`. + # Should follow up and add a built-in error page template, similarly to + # https://github.com/element-hq/synapse/pull/18876 ? + Codes.UNKNOWN, + ) + # We didn't update a row with the given rule_id so insert one push_rule_id = self._push_rule_id_gen.get_next() @@ -839,6 +908,28 @@ class PushRulesWorkerStore( ) else: try: + # Before updating the push rule, we need to check that we won't exceed + # the size limit on push rules. + # For that, we need to fetch the `conditions` JSON. + conditions_json = self.db_pool.simple_select_one_onecol_txn( + txn, + "push_rules", + {"user_name": user_id, "rule_id": rule_id}, + "conditions", + ) + + rule_body_size = _push_rule_size_for_limits( + conditions_json=conditions_json, actions_json=actions_json + ) + if rule_body_size > self._config.limits.rule_size: + raise SynapseError( + HTTPStatus.REQUEST_ENTITY_TOO_LARGE, + f"Push rule size exceeds server limit ({rule_body_size} bytes > {self._config.limits.rule_size} bytes).", + # FIXME: Provide a better error code. + # None of the existing options seem entirely correct, though. + Codes.UNKNOWN, + ) + self.db_pool.simple_update_one_txn( txn, "push_rules", diff --git a/tests/rest/client/test_push_rule_attrs.py b/tests/rest/client/test_push_rule_attrs.py index 53c36b7a9c..f6bbc7496e 100644 --- a/tests/rest/client/test_push_rule_attrs.py +++ b/tests/rest/client/test_push_rule_attrs.py @@ -3,6 +3,7 @@ # # Copyright 2020 The Matrix.org Foundation C.I.C. # Copyright (C) 2023 New Vector, Ltd +# Copyright (C) 2026 Element Creations Ltd # # This program is free software: you can redistribute it and/or modify # it under the terms of the GNU Affero General Public License as @@ -20,9 +21,13 @@ # from http import HTTPStatus +import canonicaljson +from parameterized.parameterized import parameterized + import synapse from synapse.api.errors import Codes from synapse.rest.client import login, push_rule, room +from synapse.types import JsonDict from tests.unittest import HomeserverTestCase @@ -508,3 +513,191 @@ class PushRuleAttributesTestCase(HomeserverTestCase): Codes.INVALID_PARAM, channel.json_body["errcode"], ) + + +class PushRuleLimitTestCase(HomeserverTestCase): + """ + Tests for server-configured limits on push rule size. + + See: https://github.com/element-hq/synapse/security/advisories/GHSA-fp53-rw9v-hcf9 + """ + + servlets = [ + synapse.rest.admin.register_servlets_for_client_rest_resource, + room.register_servlets, + login.register_servlets, + push_rule.register_servlets, + ] + hijack_auth = False + + def default_config(self) -> JsonDict: + config = super().default_config() + # Set some small limits for push rule sizes so that + # we can easily test them. + config["push_rules"] = { + "limits": { + # Size limit of each rule in bytes (canonical JSON) + "rule_size": 64, + # Size limit of the rule ID (in bytes) + "rule_id_length": 24, + # Limit on how many rules you can have + "rule_count": 2, + } + } + return config + + @parameterized.expand( + ( + ( + { + "actions": ["notify"], + "conditions": [{"kind": "event_match", "key": "a", "pattern": "a"}], + }, + 58, + True, + ), + ( + { + "actions": ["notify"], + "conditions": [ + {"kind": "event_match", "key": "a", "pattern": "abcdefg"} + ], + }, + 64, + True, + ), + ( + { + "actions": ["notify"], + "conditions": [ + {"kind": "event_match", "key": "a", "pattern": "abcdefgH"} + ], + }, + 65, + False, + ), + ) + ) + def test_limit_on_push_rule_size( + self, body: JsonDict, expected_body_num_bytes: int, expected_allowed: bool + ) -> None: + """ + Tests that the `push_rules.limits.rule_size` applies to the size of the push rule. + """ + self.register_user("alice", "pass") + token = self.login("alice", "pass") + + # Sanity check the test data: the canonical JSON size of the push rule body + # should be exactly as we expect, otherwise our test is void. + body_bytes = canonicaljson.encode_canonical_json(body) + # We exclude the size of the wrapper, as our implementation currently only + # counts the size of the actions and conditions fragments themselves. + self.assertEqual( + len(body_bytes) - len('{"actions":,"conditions":}'.encode("utf-8")), + expected_body_num_bytes, + ) + + channel = self.make_request( + "PUT", + "/pushrules/global/underride/rule1", + body, + access_token=token, + ) + if expected_allowed: + self.assertEqual( + channel.code, + HTTPStatus.OK, + f"Push rule ({body_bytes!r}) within size limit should be accepted: {channel.json_body}", + ) + else: + self.assertEqual( + channel.code, + HTTPStatus.REQUEST_ENTITY_TOO_LARGE, + f"Push rule ({body_bytes!r}) exceeding size limit should be rejected", + ) + self.assertEqual(channel.json_body["errcode"], Codes.UNKNOWN) + + @parameterized.expand( + ( + ( + 18, + True, + ), + ( + 24, + True, + ), + ( + 25, + False, + ), + ) + ) + def test_limit_on_rule_id_length( + self, rule_id_length: int, expected_allowed: bool + ) -> None: + """ + Tests that the `push_rules.limits.rule_id_length` applies to the byte length + of the push rule ID. + """ + self.register_user("alice", "pass") + token = self.login("alice", "pass") + + PREFIX = "global/underride/" + rule_suffix_length = rule_id_length - len(PREFIX) + assert rule_suffix_length >= 1, "can't construct a rule ID that short" + channel = self.make_request( + "PUT", + f"/pushrules/{PREFIX}{rule_suffix_length * 'a'}", + {"conditions": [], "actions": ["notify"]}, + access_token=token, + ) + if expected_allowed: + self.assertEqual( + channel.code, + HTTPStatus.OK, + f"Push rule ID ({rule_id_length} B) within size limit should be accepted: {channel.json_body}", + ) + else: + self.assertEqual( + channel.code, + HTTPStatus.REQUEST_ENTITY_TOO_LARGE, + "Push rule ID ({rule_id_length} B) exceeding size limit should be rejected", + ) + self.assertEqual(channel.json_body["errcode"], Codes.UNKNOWN) + + def test_limit_on_push_rule_count(self) -> None: + """ + Tests that we are allowed to create exactly the number of push rules + specified by `push_rules.limits.rule_count`, but not a single one more. + """ + self.register_user("bob", "pass") + token = self.login("bob", "pass") + + # First 2 rules are allowable + for i in range(2): + channel = self.make_request( + "PUT", + f"/pushrules/global/underride/rule{i}", + {"actions": ["notify"], "conditions": []}, + access_token=token, + ) + self.assertEqual( + channel.code, + HTTPStatus.OK, + f"Push rule within count limit should be allowed: {channel.json_body}", + ) + + # 3rd rule gets denied as it goes over the limit + channel = self.make_request( + "PUT", + "/pushrules/global/underride/rule3", + {"actions": ["notify"], "conditions": []}, + access_token=token, + ) + self.assertEqual( + channel.code, + HTTPStatus.BAD_REQUEST, + "Push rule exceeding count limit should be rejected", + ) + self.assertEqual(channel.json_body["errcode"], Codes.UNKNOWN)