diff --git a/changelog.d/20153.bugfix b/changelog.d/20153.bugfix new file mode 100644 index 0000000000..c42d7af883 --- /dev/null +++ b/changelog.d/20153.bugfix @@ -0,0 +1,2 @@ +Reject setting `null` profile field values (following [an optional clause in the specification](https://spec.matrix.org/v1.19/client-server-api/#put_matrixclientv3profileuseridkeyname:~:text=Servers%20MAY%20reject%20null%20values%2E)), in order to avoid clients accidentally harming interoperability. + diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index aadda870fd..cb527a1bb5 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -203,13 +203,21 @@ class ProfileFieldRestServlet(RestServlet): is_admin = await self.auth.is_server_admin(requester) if not field_name: - raise SynapseError(400, "Field name too short", errcode=Codes.INVALID_PARAM) + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "Field name too short", + errcode=Codes.INVALID_PARAM, + ) if len(field_name.encode("utf-8")) > MAX_CUSTOM_FIELD_LEN: - raise SynapseError(400, "Field name too long", errcode=Codes.KEY_TOO_LARGE) + raise SynapseError( + HTTPStatus.BAD_REQUEST, + "Field name too long", + errcode=Codes.KEY_TOO_LARGE, + ) if not is_namedspaced_grammar(field_name): raise SynapseError( - 400, + HTTPStatus.BAD_REQUEST, "Field name does not follow Common Namespaced Identifier Grammar", errcode=Codes.INVALID_PARAM, ) @@ -219,7 +227,22 @@ class ProfileFieldRestServlet(RestServlet): new_value = content[field_name] except KeyError: raise SynapseError( - 400, f"Missing key '{field_name}'", errcode=Codes.MISSING_PARAM + HTTPStatus.BAD_REQUEST, + f"Missing key '{field_name}'", + errcode=Codes.MISSING_PARAM, + ) + + if new_value is None: + # > Servers MAY reject null values. + # — https://spec.matrix.org/v1.19/client-server-api/#put_matrixclientv3profileuseridkeyname + # + # Although we can safely handle nulls internally, for interoperability reasons it is preferable if we + # reject them. + # That way, clients don't accidentally rely on being able to send them. + raise SynapseError( + HTTPStatus.BAD_REQUEST, + f"'{field_name}' cannot be `null`.", + Codes.INVALID_PARAM, ) propagate = _read_propagate(self.hs, request) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index b3a719d851..7fb03fdb3f 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -27,6 +27,7 @@ from http import HTTPStatus from typing import Any from canonicaljson import encode_canonical_json +from parameterized import parameterized from twisted.internet.testing import MemoryReactor @@ -663,7 +664,6 @@ class ProfileTestCase(unittest.HomeserverTestCase): "array_field": ["test"], "object_field": {"test": "test"}, "numeric_field": 1, - "null_field": None, } for key, value in fields.items(): @@ -691,6 +691,37 @@ class ProfileTestCase(unittest.HomeserverTestCase): self.assertEqual(channel.code, HTTPStatus.OK, channel.result) self.assertEqual(channel.json_body, {key: value}) + @parameterized.expand( + [ + # These two fields are special as they are the original 2 fields + # and have their own storage implementation. + # They already rejected `null` before the introduction of this test. + ("displayname",), + ("avatar_url",), + # This is a 'custom field'. + ("org.example.custom_field",), + ] + ) + def test_null_field_rejected(self, field_name: str) -> None: + """ + Tests that setting a field to `null` is rejected. + + This is optional as per spec, but we prefer to apply this as it is best for interoperability + (prevents clients from relying on `null` values being accepted, which is essentially + Synapse-specific behaviour). + + > Servers MAY reject null values. + — https://spec.matrix.org/v1.19/client-server-api/#put_matrixclientv3profileuseridkeyname + """ + channel = self.make_request( + "PUT", + f"/_matrix/client/v3/profile/{self.owner}/{field_name}", + content={field_name: None}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result) + self.assertEqual(channel.json_body["errcode"], Codes.INVALID_PARAM) + def test_set_custom_field_noauth(self) -> None: channel = self.make_request( "PUT",