Discard null-containing strings before updating the user directory (#12762)

Author: David Robertson

changelog.d/12762.misc

@@ -0,0 +1 @@
+Fix a long-standing bug where the user directory background process would fail to make forward progress if a user included a null codepoint in their display name or avatar.

synapse/rest/client/room.py

@@ -109,10 +109,10 @@ def __init__(self, hs: "HomeServer"):
         self.auth = hs.get_auth()
 
     def register(self, http_server: HttpServer) -> None:
-        # /room/$roomid/state/$eventtype
+        # /rooms/$roomid/state/$eventtype
         no_state_key = "/rooms/(?P<room_id>[^/]*)/state/(?P<event_type>[^/]*)$"
 
-        # /room/$roomid/state/$eventtype/$statekey
+        # /rooms/$roomid/state/$eventtype/$statekey
         state_key = (
             "/rooms/(?P<room_id>[^/]*)/state/"
             "(?P<event_type>[^/]*)/(?P<state_key>[^/]*)$"

synapse/storage/databases/main/events.py

@@ -53,6 +53,7 @@
 from synapse.types import StateMap, get_domain_from_id
 from synapse.util import json_encoder
 from synapse.util.iterutils import batch_iter, sorted_topologically
+from synapse.util.stringutils import non_null_str_or_none
 
 if TYPE_CHECKING:
     from synapse.server import HomeServer
@@ -1737,9 +1738,6 @@ def _store_room_members_txn(
                 not affect the current local state.
         """
 
-        def non_null_str_or_none(val: Any) -> Optional[str]:
-            return val if isinstance(val, str) and "\u0000" not in val else None
-
         self.db_pool.simple_insert_many_txn(
             txn,
             table="room_memberships",

synapse/storage/databases/main/user_directory.py

@@ -29,6 +29,7 @@
 from typing_extensions import TypedDict
 
 from synapse.api.errors import StoreError
+from synapse.util.stringutils import non_null_str_or_none
 
 if TYPE_CHECKING:
     from synapse.server import HomeServer
@@ -469,11 +470,9 @@ async def update_profile_in_user_dir(
         """
         Update or add a user's profile in the user directory.
         """
-        # If the display name or avatar URL are unexpected types, overwrite them.
-        if not isinstance(display_name, str):
-            display_name = None
-        if not isinstance(avatar_url, str):
-            avatar_url = None
+        # If the display name or avatar URL are unexpected types, replace with None.
+        display_name = non_null_str_or_none(display_name)
+        avatar_url = non_null_str_or_none(avatar_url)
 
         def _update_profile_in_user_dir_txn(txn: LoggingTransaction) -> None:
             self.db_pool.simple_upsert_txn(

synapse/util/stringutils.py

@@ -16,7 +16,7 @@
 import re
 import secrets
 import string
-from typing import Iterable, Optional, Tuple
+from typing import Any, Iterable, Optional, Tuple
 
 from netaddr import valid_ipv6
 
@@ -247,3 +247,11 @@ def base62_encode(num: int, minwidth: int = 1) -> str:
     # pad to minimum width
     pad = "0" * (minwidth - len(res))
     return pad + res
+
+
+def non_null_str_or_none(val: Any) -> Optional[str]:
+    """Check that the arg is a string containing no null (U+0000) codepoints.
+
+    If so, returns the given string unmodified; otherwise, returns None.
+    """
+    return val if isinstance(val, str) and "\u0000" not in val else None

tests/handlers/test_user_directory.py

@@ -1007,6 +1007,34 @@ def test_local_user_leaving_room_remains_in_user_directory(self) -> None:
         self.assertEqual(in_public, {(bob, room1), (bob, room2)})
         self.assertEqual(in_private, set())
 
+    def test_ignore_display_names_with_null_codepoints(self) -> None:
+        MXC_DUMMY = "mxc://dummy"
+
+        # Alice creates a public room.
+        alice = self.register_user("alice", "pass")
+
+        # Alice has a user directory entry to start with.
+        self.assertIn(
+            alice,
+            self.get_success(self.user_dir_helper.get_profiles_in_user_directory()),
+        )
+
+        # Alice changes her name to include a null codepoint.
+        self.get_success(
+            self.hs.get_user_directory_handler().handle_local_profile_change(
+                alice,
+                ProfileInfo(
+                    display_name="abcd\u0000efgh",
+                    avatar_url=MXC_DUMMY,
+                ),
+            )
+        )
+        # Alice's profile should be updated with the new avatar, but no display name.
+        self.assertEqual(
+            self.get_success(self.user_dir_helper.get_profiles_in_user_directory()),
+            {alice: ProfileInfo(display_name=None, avatar_url=MXC_DUMMY)},
+        )
+
 
 class TestUserDirSearchDisabled(unittest.HomeserverTestCase):
     servlets = [