From b6359c28c1aece8b0879a51cccf2493f4fc73e96 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 21 Mar 2023 14:56:17 -0400 Subject: [PATCH 1/3] Reject mentions on the C-S API which are invalid. --- changelog.d/15311.misc | 1 + synapse/events/validator.py | 52 ++++++++++++++++++++++++++++++------- 2 files changed, 44 insertions(+), 9 deletions(-) create mode 100644 changelog.d/15311.misc diff --git a/changelog.d/15311.misc b/changelog.d/15311.misc new file mode 100644 index 000000000000..ce03cb95238f --- /dev/null +++ b/changelog.d/15311.misc @@ -0,0 +1 @@ +Reject events with an invalid "mentions" property pert [MSC3952](https://github.com/matrix-org/matrix-spec-proposals/pull/3952). diff --git a/synapse/events/validator.py b/synapse/events/validator.py index fb1737b910e9..463fff0c8ab7 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -16,7 +16,12 @@ import jsonschema -from synapse.api.constants import MAX_ALIAS_LENGTH, EventTypes, Membership +from synapse.api.constants import ( + MAX_ALIAS_LENGTH, + EventContentFields, + EventTypes, + Membership, +) from synapse.api.errors import Codes, SynapseError from synapse.api.room_versions import EventFormatVersions from synapse.config.homeserver import HomeServerConfig @@ -88,27 +93,27 @@ def validate_new(self, event: EventBase, config: HomeServerConfig) -> None: Codes.INVALID_PARAM, ) - if event.type == EventTypes.Retention: + elif event.type == EventTypes.Retention: self._validate_retention(event) - if event.type == EventTypes.ServerACL: + elif event.type == EventTypes.ServerACL: if not server_matches_acl_event(config.server.server_name, event): raise SynapseError( 400, "Can't create an ACL event that denies the local server" ) - if event.type == EventTypes.PowerLevels: + elif event.type == EventTypes.PowerLevels: try: jsonschema.validate( instance=event.content, schema=POWER_LEVELS_SCHEMA, - cls=plValidator, + cls=POWER_LEVELS_VALIDATOR, ) except jsonschema.ValidationError as e: if e.path: # example: "users_default": '0' is not of type 'integer' # cast safety: path entries can be integers, if we fail to validate - # items in an array. However the POWER_LEVELS_SCHEMA doesn't expect + # items in an array. However, the POWER_LEVELS_SCHEMA doesn't expect # to see any arrays. message = ( '"' + cast(str, e.path[-1]) + '": ' + e.message # noqa: B306 @@ -125,6 +130,19 @@ def validate_new(self, event: EventBase, config: HomeServerConfig) -> None: errcode=Codes.BAD_JSON, ) + # If the event contains a mentions key, validate it. + if ( + EventContentFields.MSC3952_MENTIONS in event.content + and config.experimental.msc3952_intentional_mentions + ): + mentions = event.content[EventContentFields.MSC3952_MENTIONS] + try: + jsonschema.validate( + instance=mentions, schema=MENTIONS_SCHEMA, cls=MENTIONS_VALIDATOR + ) + except jsonschema.ValidationError as e: + raise SynapseError(400, e.message) + def _validate_retention(self, event: EventBase) -> None: """Checks that an event that defines the retention policy for a room respects the format enforced by the spec. @@ -252,11 +270,26 @@ def _ensure_state_event(self, event: Union[EventBase, EventBuilder]) -> None: }, } +MENTIONS_SCHEMA = { + "type": "object", + "properties": { + "user_ids": { + "type": "array", + "items": { + "type": "string", + }, + }, + "room": { + "type": "boolean", + }, + }, +} + # This could return something newer than Draft 7, but that's the current "latest" # validator. -def _create_power_level_validator() -> Type[jsonschema.Draft7Validator]: - validator = jsonschema.validators.validator_for(POWER_LEVELS_SCHEMA) +def _create_validator(schema: JsonDict) -> Type[jsonschema.Draft7Validator]: + validator = jsonschema.validators.validator_for(schema) # by default jsonschema does not consider a frozendict to be an object so # we need to use a custom type checker @@ -268,4 +301,5 @@ def _create_power_level_validator() -> Type[jsonschema.Draft7Validator]: return jsonschema.validators.extend(validator, type_checker=type_checker) -plValidator = _create_power_level_validator() +POWER_LEVELS_VALIDATOR = _create_validator(POWER_LEVELS_SCHEMA) +MENTIONS_VALIDATOR = _create_validator(MENTIONS_SCHEMA) From caa764bd6806cb4d585176ee4c36c88bf20fffdf Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Mar 2023 14:02:47 -0400 Subject: [PATCH 2/3] Switch to pydantic. --- synapse/events/validator.py | 34 +++++++++++----------------------- synapse/http/servlet.py | 22 ++++++++++++++++------ 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/synapse/events/validator.py b/synapse/events/validator.py index 463fff0c8ab7..90df9fd4f9e7 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -12,9 +12,10 @@ # See the License for the specific language governing permissions and # limitations under the License. import collections.abc -from typing import Iterable, Type, Union, cast +from typing import Iterable, List, Type, Union, cast import jsonschema +from pydantic import Field, StrictBool, StrictStr from synapse.api.constants import ( MAX_ALIAS_LENGTH, @@ -33,6 +34,8 @@ validate_canonicaljson, ) from synapse.federation.federation_server import server_matches_acl_event +from synapse.http.servlet import validate_json_object +from synapse.rest.models import RequestBodyModel from synapse.types import EventID, JsonDict, RoomID, UserID @@ -135,13 +138,9 @@ def validate_new(self, event: EventBase, config: HomeServerConfig) -> None: EventContentFields.MSC3952_MENTIONS in event.content and config.experimental.msc3952_intentional_mentions ): - mentions = event.content[EventContentFields.MSC3952_MENTIONS] - try: - jsonschema.validate( - instance=mentions, schema=MENTIONS_SCHEMA, cls=MENTIONS_VALIDATOR - ) - except jsonschema.ValidationError as e: - raise SynapseError(400, e.message) + validate_json_object( + event.content[EventContentFields.MSC3952_MENTIONS], Mentions + ) def _validate_retention(self, event: EventBase) -> None: """Checks that an event that defines the retention policy for a room respects the @@ -270,20 +269,10 @@ def _ensure_state_event(self, event: Union[EventBase, EventBuilder]) -> None: }, } -MENTIONS_SCHEMA = { - "type": "object", - "properties": { - "user_ids": { - "type": "array", - "items": { - "type": "string", - }, - }, - "room": { - "type": "boolean", - }, - }, -} + +class Mentions(RequestBodyModel): + user_ids: List[StrictStr] = Field(default_factory=list) + room: StrictBool = False # This could return something newer than Draft 7, but that's the current "latest" @@ -302,4 +291,3 @@ def _create_validator(schema: JsonDict) -> Type[jsonschema.Draft7Validator]: POWER_LEVELS_VALIDATOR = _create_validator(POWER_LEVELS_SCHEMA) -MENTIONS_VALIDATOR = _create_validator(MENTIONS_SCHEMA) diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index 0070bd2940ea..fc6279362881 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -778,17 +778,13 @@ def parse_json_object_from_request( Model = TypeVar("Model", bound=BaseModel) -def parse_and_validate_json_object_from_request( - request: Request, model_type: Type[Model] -) -> Model: - """Parse a JSON object from the body of a twisted HTTP request, then deserialise and - validate using the given pydantic model. +def validate_json_object(content: JsonDict, model_type: Type[Model]) -> Model: + """Validate a deserialized JSON object using the given pydantic model. Raises: SynapseError if the request body couldn't be decoded as JSON or if it wasn't a JSON object. """ - content = parse_json_object_from_request(request, allow_empty_body=False) try: instance = model_type.parse_obj(content) except ValidationError as e: @@ -811,6 +807,20 @@ def parse_and_validate_json_object_from_request( return instance +def parse_and_validate_json_object_from_request( + request: Request, model_type: Type[Model] +) -> Model: + """Parse a JSON object from the body of a twisted HTTP request, then deserialise and + validate using the given pydantic model. + + Raises: + SynapseError if the request body couldn't be decoded as JSON or + if it wasn't a JSON object. + """ + content = parse_json_object_from_request(request, allow_empty_body=False) + return validate_json_object(content, model_type) + + def assert_params_in_dict(body: JsonDict, required: Iterable[str]) -> None: absent = [] for k in required: From 7ea1176a88a8e22c2cc9eaa129aa0b963c54ada2 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 22 Mar 2023 15:37:14 -0400 Subject: [PATCH 3/3] Fix-up tests. --- tests/push/test_bulk_push_rule_evaluator.py | 94 ++++++++++++--------- 1 file changed, 56 insertions(+), 38 deletions(-) diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index 46df0102f77a..9501096a7732 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -243,22 +243,28 @@ def test_user_mentions(self) -> None: ) # Non-dict mentions should be ignored. - mentions: Any - for mentions in (None, True, False, 1, "foo", []): - self.assertFalse( - self._create_and_process( - bulk_evaluator, {EventContentFields.MSC3952_MENTIONS: mentions} + # + # Avoid C-S validation as these aren't expected. + with patch( + "synapse.events.validator.EventValidator.validate_new", + new=lambda s, event, config: True, + ): + mentions: Any + for mentions in (None, True, False, 1, "foo", []): + self.assertFalse( + self._create_and_process( + bulk_evaluator, {EventContentFields.MSC3952_MENTIONS: mentions} + ) ) - ) - # A non-list should be ignored. - for mentions in (None, True, False, 1, "foo", {}): - self.assertFalse( - self._create_and_process( - bulk_evaluator, - {EventContentFields.MSC3952_MENTIONS: {"user_ids": mentions}}, + # A non-list should be ignored. + for mentions in (None, True, False, 1, "foo", {}): + self.assertFalse( + self._create_and_process( + bulk_evaluator, + {EventContentFields.MSC3952_MENTIONS: {"user_ids": mentions}}, + ) ) - ) # The Matrix ID appearing anywhere in the list should notify. self.assertTrue( @@ -291,26 +297,32 @@ def test_user_mentions(self) -> None: ) # Invalid entries in the list are ignored. - self.assertFalse( - self._create_and_process( - bulk_evaluator, - { - EventContentFields.MSC3952_MENTIONS: { - "user_ids": [None, True, False, {}, []] - } - }, + # + # Avoid C-S validation as these aren't expected. + with patch( + "synapse.events.validator.EventValidator.validate_new", + new=lambda s, event, config: True, + ): + self.assertFalse( + self._create_and_process( + bulk_evaluator, + { + EventContentFields.MSC3952_MENTIONS: { + "user_ids": [None, True, False, {}, []] + } + }, + ) ) - ) - self.assertTrue( - self._create_and_process( - bulk_evaluator, - { - EventContentFields.MSC3952_MENTIONS: { - "user_ids": [None, True, False, {}, [], self.alice] - } - }, + self.assertTrue( + self._create_and_process( + bulk_evaluator, + { + EventContentFields.MSC3952_MENTIONS: { + "user_ids": [None, True, False, {}, [], self.alice] + } + }, + ) ) - ) # The legacy push rule should not mention if the mentions field exists. self.assertFalse( @@ -351,14 +363,20 @@ def test_room_mentions(self) -> None: ) # Invalid data should not notify. - mentions: Any - for mentions in (None, False, 1, "foo", [], {}): - self.assertFalse( - self._create_and_process( - bulk_evaluator, - {EventContentFields.MSC3952_MENTIONS: {"room": mentions}}, + # + # Avoid C-S validation as these aren't expected. + with patch( + "synapse.events.validator.EventValidator.validate_new", + new=lambda s, event, config: True, + ): + mentions: Any + for mentions in (None, False, 1, "foo", [], {}): + self.assertFalse( + self._create_and_process( + bulk_evaluator, + {EventContentFields.MSC3952_MENTIONS: {"room": mentions}}, + ) ) - ) # The legacy push rule should not mention if the mentions field exists. self.assertFalse(