Reject boolean power levels

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.

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,

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)
 

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:

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)

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,86 @@ 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),
+            # 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 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(
+        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;
+        # - 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"
-        ):
-            self.helper.send_state(
+        ), patch("synapse.handlers.event_auth.check_state_dependent_auth_rules"):
+            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,17 +141,33 @@ 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],
             )
         )
 
         bulk_evaluator = BulkPushRuleEvaluator(self.hs)
         # 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"""

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,38 @@ 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:
+        """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(
+                {
+                    "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": {}},
+            {"ban": False},
+            {"users": {"@george:boole.me.uk": True}},
+        ]
+        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"