diff --git a/changelog.d/19637.bugfix b/changelog.d/19637.bugfix new file mode 100644 index 0000000000..94cef387c8 --- /dev/null +++ b/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. diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 89b68331f2..2c65a55ea1 100644 --- a/synapse/rest/client/keys.py +++ b/synapse/rest/client/keys.py @@ -159,6 +159,23 @@ class KeyUploadServlet(RestServlet): 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,7 +258,7 @@ class KeyUploadServlet(RestServlet): 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 @@ -249,13 +266,13 @@ class KeyUploadServlet(RestServlet): # # 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, diff --git a/tests/rest/client/test_keys.py b/tests/rest/client/test_keys.py index 9c83a284d7..fc21d83ef2 100644 --- a/tests/rest/client/test_keys.py +++ b/tests/rest/client/test_keys.py @@ -160,9 +160,12 @@ class KeyUploadTestCase(unittest.HomeserverTestCase): 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 @@ class KeyUploadTestCase(unittest.HomeserverTestCase): "/_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):