From 2452122f221aa6c7e233ff6ef34ea6e7fab9ca99 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 30 Jan 2023 14:03:07 +0000 Subject: [PATCH 01/10] Better test for bad values in power levels events The previous test only checked that Synapse didn't raise an exception, but didn't check that we had correctly interpreted the value of the dodgy power level. It also conflated two things: bad room notification levels, and bad user levels. There _is_ logic for converting the latter to integers, but we should test it separately. --- tests/push/test_bulk_push_rule_evaluator.py | 77 +++++++++++++++++---- 1 file changed, 65 insertions(+), 12 deletions(-) diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index aba62b5dc8fe..7d294b6597ac 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -15,6 +15,8 @@ from typing import Any from unittest.mock import patch +from parameterized import parameterized + from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import EventContentFields @@ -48,35 +50,70 @@ def prepare( self.requester = create_requester(self.alice) self.room_id = self.helper.create_room_as( - self.alice, room_version=RoomVersions.V9.identifier, tok=self.token + # This is deliberately set to V9, because we want to test the logic which + # handles stringy power levels. Stringy power levels were outlawed in V10. + self.alice, + room_version=RoomVersions.V9.identifier, + tok=self.token, ) self.event_creation_handler = self.hs.get_event_creation_handler() - def test_action_for_event_by_user_handles_noninteger_power_levels(self) -> None: - """We should convert floats and strings to integers before passing to Rust. + @parameterized.expand( + [ + # The historically-permitted bad values. Alice's notification should be + # allowed if this threshold is at or below her power level (60) + ("100", False), + ("0", True), + (12.34, True), + (60.0, True), + (67.89, False), + ] + ) + def test_action_for_event_by_user_handles_noninteger_room_power_levels( + self, bad_room_level: object, should_permit: bool + ) -> None: + """We should convert strings in `room` to integers before passing to Rust. + + Test this as follows: + - Create a room as Alice and invite two other users Bob and Charlie. + - Set PLs so that Alice has PL 60 and `notifications.room` is set to a bad value. + - Have Alice create a message notifying @room. + - Evaluate notification actions for that message. This should not raise. + - Look in the DB to see if that message triggered a highlight for Bob. + + The test is parameterised with two arguments: + - the bad power level value for "room", before JSON serisalistion + - whether Bob should expect the message to be highlighted Reproduces #14060. A lack of validation: the gift that keeps on giving. """ - - # Alter the power levels in that room to include stringy and floaty levels. - # We need to suppress the validation logic or else it will reject these dodgy - # values. (Presumably this validation was not always present.) + # Join another user to the room, so that there is someone to see Alice's + # @room notification. + bob = self.register_user("bob", "pass") + bob_token = self.login(bob, "pass") + self.helper.join(self.room_id, bob, tok=bob_token) + + # Alter the power levels in that room to include the bad @room notification + # level. We need to suppress + # - canonicaljson validation, because canonicaljson forbids floats, and + # - the event jsonschema validation, because it will forbid bad values. + # (Presumably this validation was not always present.) with patch("synapse.events.validator.validate_canonicaljson"), patch( "synapse.events.validator.jsonschema.validate" ): - self.helper.send_state( + pl_event_id = self.helper.send_state( self.room_id, "m.room.power_levels", { - "users": {self.alice: "100"}, # stringy - "notifications": {"room": 100.0}, # float + "users": {self.alice: 60}, + "notifications": {"room": bad_room_level}, }, self.token, state_key="", - ) + )["event_id"] # Create a new message event, and try to evaluate it under the dodgy # power level event. @@ -88,10 +125,11 @@ def test_action_for_event_by_user_handles_noninteger_power_levels(self) -> None: "room_id": self.room_id, "content": { "msgtype": "m.text", - "body": "helo", + "body": "helo @room", }, "sender": self.alice, }, + prev_event_ids=[pl_event_id], ) ) @@ -99,6 +137,21 @@ def test_action_for_event_by_user_handles_noninteger_power_levels(self) -> None: # should not raise self.get_success(bulk_evaluator.action_for_events_by_user([(event, context)])) + # Did Bob see Alice's @room notification? + highlighted_actions = self.get_success( + self.hs.get_datastores().main.db_pool.simple_select_list( + table="event_push_actions_staging", + keyvalues={ + "event_id": event.event_id, + "user_id": bob, + "highlight": 1, + }, + retcols=("*",), + desc="get_event_push_actions_staging", + ) + ) + self.assertEqual(len(highlighted_actions), int(should_permit)) + @override_config({"push": {"enabled": False}}) def test_action_for_event_by_user_disabled_by_config(self) -> None: """Ensure that push rules are not calculated when disabled in the config""" From 37971fafe26c62212e0e145fbd2c82964a6435b9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 30 Jan 2023 16:54:46 +0000 Subject: [PATCH 02/10] Check we ignore types that don't convert to int --- tests/push/test_bulk_push_rule_evaluator.py | 22 +++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index 7d294b6597ac..fda48d9f61db 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -68,6 +68,14 @@ def prepare( (12.34, True), (60.0, True), (67.89, False), + # Values that int(...) would not successfully cast should be ignored. + # The room notification level should then default to 50, per the spec, so + # Alice's notification is allowed. + (None, True), + # We haven't seen `"room": []` or `"room": {}` in the wild (yet), but + # let's check them for paranoia's sake. + ([], True), + ({}, True), ] ) def test_action_for_event_by_user_handles_noninteger_room_power_levels( @@ -98,12 +106,18 @@ def test_action_for_event_by_user_handles_noninteger_room_power_levels( # Alter the power levels in that room to include the bad @room notification # level. We need to suppress - # - canonicaljson validation, because canonicaljson forbids floats, and - # - the event jsonschema validation, because it will forbid bad values. - # (Presumably this validation was not always present.) + # + # - canonicaljson validation, because canonicaljson forbids floats; + # - the event jsonschema validation, because it will forbid bad values; and + # - the auth rules checks, because they stop us from creating power levels + # with `"room": null`. (We want to test this case, because we have seen it + # in the wild.) + # + # We have seen stringy and null values for "room" in the wild, so presumably + # some of this validation was missing in the past. with patch("synapse.events.validator.validate_canonicaljson"), patch( "synapse.events.validator.jsonschema.validate" - ): + ), patch("synapse.handlers.event_auth.check_state_dependent_auth_rules"): pl_event_id = self.helper.send_state( self.room_id, "m.room.power_levels", From e194241a10d42be3c6b5f8c52da8c91776ad540c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 30 Jan 2023 13:32:24 +0000 Subject: [PATCH 03/10] Handle `None` values in `notifications.room` --- synapse/push/bulk_push_rule_evaluator.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index deaec1956472..88cfc05d0552 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -69,6 +69,9 @@ } +SENTINEL = object() + + def _should_count_as_unread(event: EventBase, context: EventContext) -> bool: # Exclude rejected and soft-failed events. if context.rejected or event.internal_metadata.is_soft_failed(): @@ -343,11 +346,21 @@ async def _action_for_event_by_user( related_events = await self._related_events(event) # It's possible that old room versions have non-integer power levels (floats or - # strings). Workaround this by explicitly converting to int. + # strings; even the occasional `null`). For old rooms, we interpret these as if + # they were integers. Do this here for the `@room` power level threshold. + # Note that this is done automatically for the sender's power level by + # _get_power_levels_and_sender_level in its call to get_user_power_level + # (even for room V10.) notification_levels = power_levels.get("notifications", {}) if not event.room_version.msc3667_int_only_power_levels: - for user_id, level in notification_levels.items(): - notification_levels[user_id] = int(level) + keys = list(notification_levels.keys()) + for key in keys: + level = notification_levels.get(key, SENTINEL) + if level is not SENTINEL and type(level) is not int: + try: + notification_levels[key] = int(level) + except (TypeError, ValueError): + del notification_levels[key] # Pull out any user and room mentions. mentions = event.content.get(EventContentFields.MSC3952_MENTIONS) From 289983c7637d3347dd08c524b10e0c6e3b025567 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 30 Jan 2023 17:42:12 +0000 Subject: [PATCH 04/10] Changelog --- changelog.d/14942.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/14942.bugfix diff --git a/changelog.d/14942.bugfix b/changelog.d/14942.bugfix new file mode 100644 index 000000000000..a3ca3eb7e9d9 --- /dev/null +++ b/changelog.d/14942.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse 1.68.0 where we were unable to service remote joins in rooms with `@room` notification levels set to `null` in their (malformed) power levels. From 698c288aea5a33abbc29b13d44facdd1f21100ad Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 30 Jan 2023 19:19:59 +0000 Subject: [PATCH 05/10] Also test that bad values are rejected by event auth --- tests/test_event_auth.py | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py index f4d9fba0a14c..564879b42e3e 100644 --- a/tests/test_event_auth.py +++ b/tests/test_event_auth.py @@ -13,7 +13,7 @@ # limitations under the License. import unittest -from typing import Collection, Dict, Iterable, List, Optional +from typing import Any, Collection, Dict, Iterable, List, Optional from parameterized import parameterized @@ -728,6 +728,31 @@ def test_room_v10_rejects_string_power_levels(self) -> None: pl_event.room_version, pl_event2, {("fake_type", "fake_key"): pl_event} ) + def test_room_v10_rejects_other_non_integer_power_levels(self) -> None: + def create_event(pl_event_content: Dict[str, Any]) -> EventBase: + return make_event_from_dict( + { + "room_id": TEST_ROOM_ID, + **_maybe_get_event_id_dict_for_room_version(RoomVersions.V10), + "type": "m.room.power_levels", + "sender": "@test:test.com", + "state_key": "", + "content": pl_event_content, + "signatures": {"test.com": {"ed25519:0": "some9signature"}}, + }, + room_version=RoomVersions.V10, + ) + + contents: Iterable[Dict[str, Any]] = [ + {"notifications": {"room": None}}, + {"users": {"@alice:wonderland": []}}, + {"users_default": {}}, + ] + for content in contents: + event = create_event(content) + with self.assertRaises(SynapseError): + event_auth._check_power_levels(event.room_version, event, {}) + # helpers for making events TEST_DOMAIN = "example.com" From 6d8208478d86ee8b54c68ce7e6cef894bcf5980d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 30 Jan 2023 20:49:04 +0000 Subject: [PATCH 06/10] Docstring --- tests/test_event_auth.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py index 564879b42e3e..57c9473b4a63 100644 --- a/tests/test_event_auth.py +++ b/tests/test_event_auth.py @@ -729,6 +729,10 @@ def test_room_v10_rejects_string_power_levels(self) -> None: ) def test_room_v10_rejects_other_non_integer_power_levels(self) -> None: + """We should reject PLs that are non-integer, non-string JSON values. + + test_room_v10_rejects_string_power_levels above handles the string case. + """ def create_event(pl_event_content: Dict[str, Any]) -> EventBase: return make_event_from_dict( { From 7b9faea42af2b2743573905a447ef12f007943bc Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 30 Jan 2023 20:52:17 +0000 Subject: [PATCH 07/10] linter scripttttttttt --- tests/test_event_auth.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py index 57c9473b4a63..0a7937f1cc72 100644 --- a/tests/test_event_auth.py +++ b/tests/test_event_auth.py @@ -733,6 +733,7 @@ def test_room_v10_rejects_other_non_integer_power_levels(self) -> None: test_room_v10_rejects_string_power_levels above handles the string case. """ + def create_event(pl_event_content: Dict[str, Any]) -> EventBase: return make_event_from_dict( { From 18cec34dcad05b96e6ba00ece3d599ef4ddf7186 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 30 Jan 2023 19:41:59 +0000 Subject: [PATCH 08/10] Test boolean values in PL content --- tests/push/test_bulk_push_rule_evaluator.py | 4 +++- tests/test_event_auth.py | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/push/test_bulk_push_rule_evaluator.py b/tests/push/test_bulk_push_rule_evaluator.py index fda48d9f61db..0bd3add2669b 100644 --- a/tests/push/test_bulk_push_rule_evaluator.py +++ b/tests/push/test_bulk_push_rule_evaluator.py @@ -72,10 +72,12 @@ def prepare( # The room notification level should then default to 50, per the spec, so # Alice's notification is allowed. (None, True), - # We haven't seen `"room": []` or `"room": {}` in the wild (yet), but + # We haven't seen array, object or boolean values in the wild (yet), but # let's check them for paranoia's sake. ([], True), ({}, True), + (True, True), + (False, True), ] ) def test_action_for_event_by_user_handles_noninteger_room_power_levels( diff --git a/tests/test_event_auth.py b/tests/test_event_auth.py index 0a7937f1cc72..cac77bca70b9 100644 --- a/tests/test_event_auth.py +++ b/tests/test_event_auth.py @@ -752,6 +752,8 @@ def create_event(pl_event_content: Dict[str, Any]) -> EventBase: {"notifications": {"room": None}}, {"users": {"@alice:wonderland": []}}, {"users_default": {}}, + {"ban": False}, + {"users": {"@george:boole.me.uk": True}}, ] for content in contents: event = create_event(content) From 5d59891de267b498761ec1b673720ab2f5d95b52 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 30 Jan 2023 19:48:22 +0000 Subject: [PATCH 09/10] Reject boolean power levels --- synapse/event_auth.py | 4 ++-- synapse/events/utils.py | 6 +++--- synapse/federation/federation_base.py | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/event_auth.py b/synapse/event_auth.py index c4a7b16413c5..e0be9f88cc9d 100644 --- a/synapse/event_auth.py +++ b/synapse/event_auth.py @@ -875,11 +875,11 @@ def _check_power_levels( "kick", "invite", }: - if not isinstance(v, int): + if type(v) is not int: raise SynapseError(400, f"{v!r} must be an integer.") if k in {"events", "notifications", "users"}: if not isinstance(v, collections.abc.Mapping) or not all( - isinstance(v, int) for v in v.values() + type(v) is int for v in v.values() ): raise SynapseError( 400, diff --git a/synapse/events/utils.py b/synapse/events/utils.py index 52e4b467e876..ebf8c7ed83ba 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -648,10 +648,10 @@ def _copy_power_level_value_as_integer( ) -> None: """Set `power_levels[key]` to the integer represented by `old_value`. - :raises TypeError: if `old_value` is not an integer, nor a base-10 string + :raises TypeError: if `old_value` is neither an integer nor a base-10 string representation of an integer. """ - if isinstance(old_value, int): + if type(old_value) is int: power_levels[key] = old_value return @@ -679,7 +679,7 @@ def validate_canonicaljson(value: Any) -> None: * Floats * NaN, Infinity, -Infinity """ - if isinstance(value, int): + if type(value) is int: if value < CANONICALJSON_MIN_INT or CANONICALJSON_MAX_INT < value: raise SynapseError(400, "JSON integer out of range", Codes.BAD_JSON) diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index 6bd4742140c4..29fae716f589 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -280,7 +280,7 @@ def event_from_pdu_json(pdu_json: JsonDict, room_version: RoomVersion) -> EventB _strip_unsigned_values(pdu_json) depth = pdu_json["depth"] - if not isinstance(depth, int): + if type(depth) is not int: raise SynapseError(400, "Depth %r not an intger" % (depth,), Codes.BAD_JSON) if depth < 0: From f4c481bf41520edb2c3a7a8ce4b8868cfefacc13 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 30 Jan 2023 21:01:05 +0000 Subject: [PATCH 10/10] Changelog --- changelog.d/14944.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/14944.bugfix diff --git a/changelog.d/14944.bugfix b/changelog.d/14944.bugfix new file mode 100644 index 000000000000..5fe1fb322b40 --- /dev/null +++ b/changelog.d/14944.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse v1.64 where boolean power levels were erroneously permitted in [v10 rooms](https://spec.matrix.org/v1.5/rooms/v10/).