Merge branch 'main' into devon/package-name

matrix-org · Jun 26, 2024 · 8885f41 · 8885f41
2 parents 48a3b9e + cf37677
commit 8885f41
Show file tree

Hide file tree

Showing 6 changed files with 333 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,15 @@
+# Sygnal 0.14.4 (2024-06-26)
+
+### Bugfixes
+
+- Truncate large values inside of Firebase notification content fields. ([\#386](https://github.com/matrix-org/sygnal/issues/386))
+- Fixes an issue where retry attempts using the Firebase v1 API would fail due to nested `messages`. ([\#387](https://github.com/matrix-org/sygnal/issues/387))
+
+### Internal Changes
+
+- Bump `tornado` from 6.4 to 6.4.1. ([\#382](https://github.com/matrix-org/sygnal/issues/382))
+
+
 # Sygnal 0.14.3 (2024-05-31)
 
 ### Bugfixes

diff --git a/changelog.d/382.misc b/changelog.d/382.misc
diff --git a/pyproject.toml b/pyproject.toml
@@ -56,7 +56,7 @@ build-backend = "poetry.core.masonry.api"
 
 [tool.poetry]
 name = "matrix-sygnal"
-version = "0.14.3"
+version = "0.14.4"
 description = "Reference Push Gateway for Matrix Notifications"
 authors = ["Matrix.org Team and Contributors <[email protected]>"]
 readme = "README.md"

diff --git a/sygnal/gcmpushkin.py b/sygnal/gcmpushkin.py
@@ -85,6 +85,11 @@
 RETRY_DELAY_BASE = 10
 RETRY_DELAY_BASE_QUOTA_EXCEEDED = 60
 MAX_BYTES_PER_FIELD = 1024
+MAX_FIREBASE_MESSAGE_SIZE = 4096
+
+# Subtract 1 since the combined size of the other non-overflowing fields will push it over the
+# edge otherwise.
+MAX_NOTIFICATION_OVERFLOW_FIELDS = MAX_FIREBASE_MESSAGE_SIZE / MAX_BYTES_PER_FIELD - 1
 
 AUTH_SCOPES = ["https://www.googleapis.com/auth/firebase.messaging"]
 
@@ -579,17 +584,20 @@ async def _dispatch_notification_unlimited(
                 else:
                     body["android"] = priority
 
+            if self.api_version is APIVersion.V1:
+                body["token"] = device.pushkey
+                new_body = body
+                body = {}
+                body["message"] = new_body
+
             for retry_number in range(0, MAX_TRIES):
+                # This has to happen inside the retry loop since `pushkeys` can be modified in the
+                # event of a failure that warrants a retry.
                 if self.api_version is APIVersion.Legacy:
                     if len(pushkeys) == 1:
                         body["to"] = pushkeys[0]
                     else:
                         body["registration_ids"] = pushkeys
-                elif self.api_version is APIVersion.V1:
-                    body["token"] = device.pushkey
-                    new_body = body
-                    body = {}
-                    body["message"] = new_body
 
                 log.info(
                     "Sending (attempt %i) => %r room:%s, event:%s",
@@ -673,6 +681,7 @@ def _build_data(
             JSON-compatible dict or None if the default_payload is misconfigured
         """
         data = {}
+        overflow_fields = 0
 
         if device.data:
             default_payload = device.data.get("default_payload", {})
@@ -699,14 +708,23 @@ def _build_data(
                 data[attr] = getattr(n, attr)
                 # Truncate fields to a sensible maximum length. If the whole
                 # body is too long, GCM will reject it.
-                if data[attr] is not None and len(data[attr]) > MAX_BYTES_PER_FIELD:
-                    data[attr] = data[attr][0:MAX_BYTES_PER_FIELD]
+                if data[attr] is not None and isinstance(data[attr], str):
+                    # The only `attr` that shouldn't be of type `str` is `content`,
+                    # which is handled explicitly later on.
+                    data[attr], truncated = truncate_str(
+                        data[attr], MAX_BYTES_PER_FIELD
+                    )
+                    if truncated:
+                        overflow_fields += 1
 
         if api_version is APIVersion.V1:
             if isinstance(data.get("content"), dict):
                 for attr, value in data["content"].items():
                     if not isinstance(value, str):
                         continue
+                    value, truncated = truncate_str(value, MAX_BYTES_PER_FIELD)
+                    if truncated:
+                        overflow_fields += 1
                     data["content_" + attr] = value
                 del data["content"]
 
@@ -722,4 +740,33 @@ def _build_data(
                 data["unread"] = str(n.counts.unread)
                 data["missed_calls"] = str(n.counts.missed_calls)
 
+        if overflow_fields > MAX_NOTIFICATION_OVERFLOW_FIELDS:
+            logger.warning(
+                "Payload contains too many overflowing fields. Notification likely to be rejected by Firebase."
+            )
+
         return data
+
+
+def truncate_str(input: str, max_bytes: int) -> Tuple[str, bool]:
+    """
+    Truncate the given string. If the truncation would occur in the middle of a unicode
+    character, that character will be removed entirely instead.
+    Appends a `…` character to the resulting string when truncation occurs.
+
+    Args:
+        `input`: the string to be truncated
+        `max_bytes`: maximum length, in bytes, that the payload should occupy when truncated
+
+    Returns:
+        Tuple of (truncated string, whether truncation took place)
+    """
+
+    str_bytes = input.encode("utf-8")
+    if len(str_bytes) <= max_bytes:
+        return (input, False)
+
+    try:
+        return (str_bytes[: max_bytes - 3].decode("utf-8") + "…", True)
+    except UnicodeDecodeError as err:
+        return (str_bytes[: err.start].decode("utf-8") + "…", True)
diff --git a/tests/test_gcm.py b/tests/test_gcm.py
@@ -17,6 +17,7 @@
 from typing import TYPE_CHECKING, Any, AnyStr, Dict, List, Tuple
 from unittest.mock import MagicMock
 
+from sygnal.exceptions import TemporaryNotificationDispatchException
 from sygnal.gcmpushkin import APIVersion, GcmPushkin
 
 from tests import testutils
@@ -259,9 +260,6 @@ def test_expected_api_v1(self) -> None:
         """
         self.apns_pushkin_snotif = MagicMock()
         gcm = self.get_test_pushkin("com.example.gcm.apiv1")
-        gcm.preload_with_response(
-            200, {"results": [{"message_id": "msg42", "registration_id": "spqr"}]}
-        )
 
         # type safety: using ignore here due to mypy not handling monkeypatching,
         # see https://github.com/python/mypy/issues/2427
@@ -464,6 +462,74 @@ def test_batching_individual_failure(self) -> None:
         self.assertEqual(gcm.last_request_body["registration_ids"], ["spqr", "spqr2"])
         self.assertEqual(gcm.num_requests, 1)
 
+    def test_api_v1_retry(self) -> None:
+        """
+        Tests that a Firebase response of 502 results in Sygnal retrying.
+        Also checks the notification message to ensure it is sane after retrying
+        multiple times.
+        """
+        self.gcm_pushkin_snotif = MagicMock()
+
+        gcm = self.get_test_pushkin("com.example.gcm.apiv1")
+
+        # type safety: using ignore here due to mypy not handling monkeypatching,
+        # see https://github.com/python/mypy/issues/2427
+        gcm._request_dispatch = self.gcm_pushkin_snotif  # type: ignore[assignment] # noqa: E501
+
+        async def side_effect(*_args: Any, **_kwargs: Any) -> None:
+            raise TemporaryNotificationDispatchException(
+                "GCM server error, hopefully temporary.", custom_retry_delay=None
+            )
+
+        method = self.gcm_pushkin_snotif
+        method.side_effect = side_effect
+
+        _resp = self._request(self._make_dummy_notification([DEVICE_EXAMPLE_APIV1]))
+
+        self.assertEqual(3, method.call_count)
+        notification_req = method.call_args.args
+
+        self.assertEqual(
+            {
+                "message": {
+                    "data": {
+                        "event_id": "$qTOWWTEL48yPm3uT-gdNhFcoHxfKbZuqRVnnWWSkGBs",
+                        "type": "m.room.message",
+                        "sender": "@exampleuser:matrix.org",
+                        "room_name": "Mission Control",
+                        "room_alias": "#exampleroom:matrix.org",
+                        "membership": None,
+                        "sender_display_name": "Major Tom",
+                        "content_msgtype": "m.text",
+                        "content_body": "I'm floating in a most peculiar way.",
+                        "room_id": "!slw48wfj34rtnrf:example.com",
+                        "prio": "high",
+                        "unread": "2",
+                        "missed_calls": "1",
+                    },
+                    "android": {
+                        "notification": {
+                            "body": {
+                                "test body",
+                            },
+                        },
+                        "priority": "high",
+                    },
+                    "apns": {
+                        "payload": {
+                            "aps": {
+                                "content-available": 1,
+                                "mutable-content": 1,
+                                "alert": "",
+                            },
+                        },
+                    },
+                    "token": "spqr",
+                }
+            },
+            notification_req[2],
+        )
+
     def test_fcm_options(self) -> None:
         """
         Tests that the config option `fcm_options` allows setting a base layer
@@ -480,3 +546,105 @@ def test_fcm_options(self) -> None:
         assert gcm.last_request_body is not None
         self.assertEqual(gcm.last_request_body["mutable_content"], True)
         self.assertEqual(gcm.last_request_body["content_available"], True)
+
+    def test_api_v1_large_fields(self) -> None:
+        """
+        Tests the gcm pushkin truncates unusually large fields. Includes large
+        fields both at the top level of `data`, and nested within `content`.
+        """
+        self.apns_pushkin_snotif = MagicMock()
+        gcm = self.get_test_pushkin("com.example.gcm.apiv1")
+        gcm.preload_with_response(
+            200, {"results": [{"message_id": "msg42", "registration_id": "spqr"}]}
+        )
+
+        # type safety: using ignore here due to mypy not handling monkeypatching,
+        # see https://github.com/python/mypy/issues/2427
+        gcm._request_dispatch = self.apns_pushkin_snotif  # type: ignore[assignment] # noqa: E501
+
+        method = self.apns_pushkin_snotif
+        method.side_effect = testutils.make_async_magic_mock(([], []))
+
+        resp = self._request(
+            self._make_dummy_notification_large_fields([DEVICE_EXAMPLE_APIV1])
+        )
+
+        self.assertEqual(1, method.call_count)
+        notification_req = method.call_args.args
+
+        # The values for `room_name` & `content_other` should be truncated from the original.
+        self.assertEqual(
+            {
+                "message": {
+                    "data": {
+                        "event_id": "$qTOWWTEL48yPm3uT-gdNhFcoHxfKbZuqRVnnWWSkGBs",
+                        "type": "m.room.message",
+                        "sender": "@exampleuser:matrix.org",
+                        "room_name": "xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooox…",
+                        "room_alias": "#exampleroom:matrix.org",
+                        "membership": None,
+                        "sender_display_name": "Major Tom",
+                        "content_msgtype": "m.text",
+                        "content_body": "I'm floating in a most peculiar way.",
+                        "content_other": "xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxoooooooooo\
+xxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxxooooooooooxxxxxxxxxx\
+ooooooooooxxxxxxxxxx🦉oooooo£xxxxxxxx☻oo🦉…",
+                        "room_id": "!slw48wfj34rtnrf:example.com",
+                        "prio": "high",
+                        "unread": "2",
+                        "missed_calls": "1",
+                    },
+                    "android": {
+                        "notification": {
+                            "body": {
+                                "test body",
+                            },
+                        },
+                        "priority": "high",
+                    },
+                    "apns": {
+                        "payload": {
+                            "aps": {
+                                "content-available": 1,
+                                "mutable-content": 1,
+                                "alert": "",
+                            },
+                        },
+                    },
+                    "token": "spqr",
+                }
+            },
+            notification_req[2],
+        )
+
+        self.assertEqual(resp, {"rejected": []})
+        assert notification_req[3] is not None
+        self.assertEqual(
+            notification_req[3].get("Authorization"), ["Bearer myaccesstoken"]
+        )