Reject device_keys: null in POST /keys/upload (#19637)

The spec says `device_keys` may be omitted, but not set to `null`.
This was temporarily allowed as a workaround for misbehaving clients
(see #19023), which have since been fixed.

Fixes #19030

Author: sandhose

changelog.d/19637.bugfix

@@ -0,0 +1 @@
+Reject `device_keys: null` in the request to [`POST /_matrix/client/v3/keys/upload`](https://spec.matrix.org/v1.16/client-server-api/#post_matrixclientv3keysupload), as per the spec. This was temporarily allowed as a workaround for misbehaving clients.

synapse/rest/client/keys.py

@@ -159,6 +159,23 @@ class KeyObject(RequestBodyModel):
         device_keys: DeviceKeys | None = None
         """Identity keys for the device. May be absent if no new identity keys are required."""
 
+        @field_validator("device_keys", mode="before")
+        @classmethod
+        def validate_device_keys_not_null(cls, v: Any) -> Any:
+            """Reject explicit `null` for `device_keys` while still allowing
+            the field to be omitted (in which case the default `None` is used).
+
+            The spec says `device_keys` may be omitted, but when present it
+            must be a `DeviceKeys` object — not `null`.
+
+            Pydantic's experimental `Missing` sentinel would be a cleaner way
+            to express this, but it's not stable yet:
+            https://docs.pydantic.dev/latest/concepts/experimental/#missing-sentinel
+            """
+            if v is None:
+                raise ValueError("device_keys must not be null")
+            return v
+
         fallback_keys: Mapping[StrictStr, StrictStr | KeyObject] | None = None
         """
         The public key which should be used if the device's one-time keys are
@@ -241,21 +258,21 @@ async def on_POST(self, request: SynapseRequest) -> tuple[int, JsonDict]:
                 400, "To upload keys, you must pass device_id when authenticating"
             )
 
-        if "device_keys" in body and isinstance(body["device_keys"], dict):
+        if "device_keys" in body:
             # Validate the provided `user_id` and `device_id` fields in
             # `device_keys` match that of the requesting user. We can't do
             # this directly in the pydantic model as we don't have access
             # to the requester yet.
             #
             # TODO: We could use ValidationInfo when we switch to Pydantic v2.
             # https://docs.pydantic.dev/latest/concepts/validators/#validation-info
-            if body["device_keys"].get("user_id") != user_id:
+            if body["device_keys"]["user_id"] != user_id:
                 raise SynapseError(
                     code=HTTPStatus.BAD_REQUEST,
                     errcode=Codes.BAD_JSON,
                     msg="Provided `user_id` in `device_keys` does not match that of the authenticated user",
                 )
-            if body["device_keys"].get("device_id") != device_id:
+            if body["device_keys"]["device_id"] != device_id:
                 raise SynapseError(
                     code=HTTPStatus.BAD_REQUEST,
                     errcode=Codes.BAD_JSON,

tests/rest/client/test_keys.py

@@ -160,9 +160,12 @@ def test_upload_keys_fails_on_invalid_user_id_or_device_id(self) -> None:
             channel.result,
         )
 
-    def test_upload_keys_succeeds_when_fields_are_explicitly_set_to_null(self) -> None:
+    def test_upload_keys_rejects_device_keys_set_to_null(self) -> None:
         """
-        This is a regression test for https://github.com/element-hq/synapse/pull/19023.
+        Test that sending `device_keys: null` is rejected per spec.
+        The spec says `device_keys` may be omitted, but not set to `null`.
+
+        See https://github.com/element-hq/synapse/issues/19030.
         """
         device_id = "DEVICE_ID"
         self.register_user("alice", "wonderland")
@@ -173,12 +176,10 @@ def test_upload_keys_succeeds_when_fields_are_explicitly_set_to_null(self) -> No
             "/_matrix/client/v3/keys/upload",
             {
                 "device_keys": None,
-                "one_time_keys": None,
-                "fallback_keys": None,
             },
             alice_token,
         )
-        self.assertEqual(channel.code, HTTPStatus.OK, channel.result)
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
 
 
 class KeyQueryTestCase(unittest.HomeserverTestCase):