Reject null profile field values (following an optional clause in the specification), in order to avoid clients accidentally harming interoperability. (#20153)

Depends on: https://github.com/matrix-org/complement/pull/914

`displayname` and `avatar_url` already did not permit nulls, seemingly
from #8223.

The specification at v1.15 does not permit `null` for those fields
either, so this seems consistent with historical specification.

This only changes behaviour for 'custom' fields.

---------

Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>

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.
+

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)

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",