Treat "\u0000" as "\u0020" for the purposes of message search (message indexing)

changelog.d/10820.misc

@@ -0,0 +1 @@
+Fix a bug where sending an `m.room.text` event containing a null byte would cause an internal server error.

synapse/storage/databases/main/search.py

@@ -15,12 +15,12 @@
 import logging
 import re
 from collections import namedtuple
-from typing import Collection, List, Optional, Set
+from typing import Collection, Iterable, List, Optional, Set
 
 from synapse.api.errors import SynapseError
 from synapse.events import EventBase
 from synapse.storage._base import SQLBaseStore, db_to_json, make_in_list_sql_clause
-from synapse.storage.database import DatabasePool
+from synapse.storage.database import DatabasePool, LoggingTransaction
 from synapse.storage.databases.main.events_worker import EventRedactBehaviour
 from synapse.storage.engines import PostgresEngine, Sqlite3Engine
 
@@ -33,13 +33,19 @@
 
 
 class SearchWorkerStore(SQLBaseStore):
-    def store_search_entries_txn(self, txn, entries):
+    def _clean_value_for_search(self, value: str) -> str:
+        """Replaces null code points in string as postgres/sql do not
+        like the insertion of strings with null code points"""
+        return value.replace("\u0000", " ")
+
+    def store_search_entries_txn(
+        self, txn: LoggingTransaction, entries: Iterable[SearchEntry]
+    ) -> None:
         """Add entries to the search table
 
         Args:
-            txn (cursor):
-            entries (iterable[SearchEntry]):
-                entries to be added to the table
+            txn:
+            entries: entries to be added to the table
         """
         if not self.hs.config.enable_search:
             return
@@ -55,7 +61,7 @@ def store_search_entries_txn(self, txn, entries):
                     entry.event_id,
                     entry.room_id,
                     entry.key,
-                    entry.value,
+                    self._clean_value_for_search(entry.value),
                     entry.stream_ordering,
                     entry.origin_server_ts,
                 )
@@ -70,11 +76,16 @@ def store_search_entries_txn(self, txn, entries):
                 " VALUES (?,?,?,?)"
             )
             args = (
-                (entry.event_id, entry.room_id, entry.key, entry.value)
+                (
+                    entry.event_id,
+                    entry.room_id,
+                    entry.key,
+                    self._clean_value_for_search(entry.value),
+                )
                 for entry in entries
             )
-
             txn.execute_batch(sql, args)
+
         else:
             # This should be unreachable.
             raise Exception("Unrecognized database engine")
@@ -646,6 +657,7 @@ def f(txn):
                 for key in ("body", "name", "topic"):
                     v = event.content.get(key, None)
                     if v:
+                        v = self._clean_value_for_search(v)
                         values.append(v)
 
                 if not values:

tests/storage/test_room_search.py

@@ -0,0 +1,74 @@
+# Copyright 2021 The Matrix.org Foundation C.I.C.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import synapse.rest.admin
+from synapse.rest.client import login, room
+from synapse.storage.engines import PostgresEngine
+
+from tests.unittest import HomeserverTestCase
+
+
+class NullByteInsertionTest(HomeserverTestCase):
+    servlets = [
+        synapse.rest.admin.register_servlets_for_client_rest_resource,
+        login.register_servlets,
+        room.register_servlets,
+    ]
+
+    def test_null_byte(self):
+        """
+        Postgres/SQLite don't like null bytes going into the search tables. Internally
+        we replace those with a space.
+
+        Ensure this doesn't break anything.
+        """
+
+        # register a user and create a room, create some messages
+        self.register_user("alice", "password")
+        access_token = self.login("alice", "password")
+        room_id = self.helper.create_room_as("alice", True, "1", access_token)
+
+        # send messages and ensure they don't cause an internal server
+        # error
+        for body in ["hi\u0000bob", "another message", "hi alice"]:
+            response = self.helper.send(room_id, body, tok=access_token)
+            self.assertIn("event_id", response)
+
+        # check that search still works with the message where the null byte was replaced
+        store = self.hs.get_datastore()
+        result = self.get_success(
+            store.search_msgs([room_id], "hi bob", ["content.body"])
+        )
+        self.assertEquals(result.get("count"), 1)
+        if isinstance(store.database_engine, PostgresEngine):
+            self.assertIn("hi", result.get("highlights"))
+            self.assertIn("bob", result.get("highlights"))
+
+        # check that search still works with another unrelated message
+        result = self.get_success(
+            store.search_msgs([room_id], "another", ["content.body"])
+        )
+        self.assertEquals(result.get("count"), 1)
+        if isinstance(store.database_engine, PostgresEngine):
+            self.assertIn("another", result.get("highlights"))
+
+        # check that search still works when given a search term that overlaps
+        # with the message that we replaced the null byte in and an unrelated one
+        result = self.get_success(store.search_msgs([room_id], "hi", ["content.body"]))
+        self.assertEquals(result.get("count"), 2)
+        result = self.get_success(
+            store.search_msgs([room_id], "hi alice", ["content.body"])
+        )
+        if isinstance(store.database_engine, PostgresEngine):
+            self.assertIn("alice", result.get("highlights"))