matrix-org · H-Shay · Jan 25, 2022 · Jan 5, 2022 · Jan 5, 2022 · Jan 5, 2022
@@ -0,0 +1 @@
+Fix a bug introduced in Sygnal 0.7.0 where a malformed `default_payload` could cause an internal server error.
@@ -283,10 +283,22 @@ async def _dispatch_notification_unlimited(
         with self.sygnal.tracer.start_span(
             "apns_dispatch", tags=span_tags, child_of=context.opentracing_span
         ) as span_parent:
+            # Before we build the payload, check that the default_payload is not
+            # malformed and reject the pushkey if it is
+
+            default_payload = {}
+
+            if device.data:
+                default_payload = device.data.get("default_payload", {})
+                if not isinstance(default_payload, dict):
+                    log.error(
+                        "default_payload is malformed, this value must be a dict."
+                    )
+                    return [device.pushkey]
 
             if n.event_id and not n.type:
                 payload: Optional[Dict[str, Any]] = self._get_payload_event_id_only(
-                    n, device
+                    n, default_payload
                 )
             else:
                 payload = self._get_payload_full(n, device, log)
@@ -336,7 +348,7 @@ async def _dispatch_notification_unlimited(
             raise NotificationDispatchException("Retried too many times.")
 
     def _get_payload_event_id_only(
-        self, n: Notification, device: Device
+        self, n: Notification, default_payload: Dict[str, Any]
     ) -> Dict[str, Any]:
         """
         Constructs a payload for a notification where we know only the event ID.
@@ -350,8 +362,7 @@ def _get_payload_event_id_only(
         """
         payload = {}
 
-        if device.data:
-            payload.update(device.data.get("default_payload", {}))
+        payload.update(default_payload)
 
         if n.room_id:
             payload["room_id"] = n.room_id

@@ -18,7 +18,7 @@
 import logging
 import time
 from io import BytesIO
-from typing import TYPE_CHECKING, Any, AnyStr, Dict, List, Tuple
+from typing import TYPE_CHECKING, Any, AnyStr, Dict, List, Optional, Tuple
 
 from opentracing import Span, logs, tags
 from prometheus_client import Counter, Gauge, Histogram
@@ -347,16 +347,21 @@ async def _dispatch_notification_unlimited(
         with self.sygnal.tracer.start_span(
             "gcm_dispatch", tags=span_tags, child_of=context.opentracing_span
         ) as span_parent:
+            # TODO: Implement collapse_key to queue only one message per room.
+            failed: List[str] = []
+
             data = GcmPushkin._build_data(n, device)
+
+            # Reject pushkey if default_payload is misconfigured
+            if data is None:
+                failed.append(device.pushkey)
+
             headers = {
                 "User-Agent": ["sygnal"],
                 "Content-Type": ["application/json"],
                 "Authorization": ["key=%s" % (self.api_key,)],
             }
 
-            # TODO: Implement collapse_key to queue only one message per room.
-            failed: List[str] = []
-
             body = self.base_request_body.copy()
             body["data"] = data
             body["priority"] = "normal" if n.prio == "low" else "high"
@@ -409,7 +414,7 @@ async def _dispatch_notification_unlimited(
             return failed
 
     @staticmethod
-    def _build_data(n: Notification, device: Device) -> Dict[str, Any]:
+    def _build_data(n: Notification, device: Device) -> Optional[Dict[str, Any]]:
         """
         Build the payload data to be sent.
         Args:
@@ -418,12 +423,19 @@ def _build_data(n: Notification, device: Device) -> Dict[str, Any]:
             will be sent.
 
         Returns:
-            JSON-compatible dict
+            JSON-compatible dict or None if the default_payload is misconfigured
         """
         data = {}
 
         if device.data:
-            data.update(device.data.get("default_payload", {}))
+            default_payload = device.data.get("default_payload", {})
+            if isinstance(default_payload, dict):
+                data.update(default_payload)
+            else:
+                logger.error(
+                    "default_payload was misconfigured, this value must be a dict."
+                )
+                return None
 
         for attr in [
             "event_id",

@@ -40,6 +40,13 @@
     },
 }
 
+DEVICE_EXAMPLE_WITH_BAD_DEFAULT_PAYLOAD = {
+    "app_id": "com.example.apns",
+    "pushkey": "badpayload",
+    "pushkey_ts": 42,
+    "data": {"default_payload": None},
+}
+
 
 class ApnsTestCase(testutils.TestCase):
     def setUp(self):
@@ -264,6 +271,15 @@ def test_expected_full_with_default_payload(self):
 
         self.assertEqual({"rejected": []}, resp)
 
+    def test_misconfigured_payload_is_rejected(self):
+        """Test that a malformed default_payload causes pushkey to be rejected"""
+
+        resp = self._request(
+            self._make_dummy_notification([DEVICE_EXAMPLE_WITH_BAD_DEFAULT_PAYLOAD])
+        )
+
+        self.assertEqual({"rejected": ["badpayload"]}, resp)
+
     def test_rejection(self):
         """
         Tests the rejection case: a rejection response from APNS leads to us

@@ -34,6 +34,16 @@
         }
     },
 }
+
+DEVICE_EXAMPLE_WITH_BAD_DEFAULT_PAYLOAD = {
+    "app_id": "com.example.gcm",
+    "pushkey": "badpayload",
+    "pushkey_ts": 42,
+    "data": {
+        "default_payload": None,
+    },
+}
+
 DEVICE_EXAMPLE_IOS = {
     "app_id": "com.example.gcm.ios",
     "pushkey": "spqr",
@@ -118,6 +128,22 @@ def test_expected_with_default_payload(self):
         self.assertEqual(resp, {"rejected": []})
         self.assertEqual(gcm.num_requests, 1)
 
+    def test_misformed_default_payload_rejected(self):
+        """
+        Tests that a non-dict default_payload is rejected.
+        """
+        gcm = self.get_test_pushkin("com.example.gcm")
+        gcm.preload_with_response(
+            200, {"results": [{"message_id": "msg42", "registration_id": "badpayload"}]}
+        )
+
+        resp = self._request(
+            self._make_dummy_notification([DEVICE_EXAMPLE_WITH_BAD_DEFAULT_PAYLOAD])
+        )
+
+        self.assertEqual(resp, {"rejected": ["badpayload"]})
+        self.assertEqual(gcm.num_requests, 1)
+
     def test_rejected(self):
         """
         Tests the rejected case: a pushkey rejected to GCM leads to Sygnal
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Fix a bug introduced in Sygnal 0.7.0 where a malformed `default_payload` could cause an internal server error.