Fix destination_is errors seen in sentry.

changelog.d/13041.bugfix

@@ -0,0 +1,2 @@
+Fix a bug introduced in Synapse 1.58 where profile requests for a malformed user ID would ccause an internal error. Synapse now returns 400 Bad Request in this situation.
+

synapse/http/matrixfederationclient.py

@@ -731,8 +731,11 @@ def build_auth_headers(
         Returns:
             A list of headers to be added as "Authorization:" headers
         """
-        if destination is None and destination_is is None:
-            raise ValueError("destination and destination_is cannot both be None!")
+        if not destination and not destination_is:
+            raise ValueError(
+                "At least one of the arguments destination and destination_is "
+                "must be a nonempty bytestring."
+            )
 
         request: JsonDict = {
             "method": method.decode("ascii"),

synapse/types.py

@@ -267,6 +267,14 @@ def from_string(cls: Type[DS], s: str) -> DS:
             )
 
         domain = parts[1]
+        if not domain:
+            raise SynapseError(
+                400,
+                f"{cls.__name__} must have a domain after the colon",
+                Codes.INVALID_PARAM,
+            )
+        # TODO: this does not reject an empty localpart or an overly-long string.
+        # See https://spec.matrix.org/v1.2/appendices/#identifier-grammar
 
         # This code will need changing if we want to support multiple domain
         # names on one HS

tests/http/test_fedclient.py → tests/http/test_matrixfederationclient.py

@@ -617,3 +617,17 @@ def test_too_big(self):
         self.assertIsInstance(f.value, RequestSendFailed)
 
         self.assertTrue(transport.disconnecting)
+
+    def test_build_auth_headers_rejects_falsey_destinations(self) -> None:
+        with self.assertRaises(ValueError):
+            self.cl.build_auth_headers(None, b"GET", b"https://example.com")
+        with self.assertRaises(ValueError):
+            self.cl.build_auth_headers(b"", b"GET", b"https://example.com")
+        with self.assertRaises(ValueError):
+            self.cl.build_auth_headers(
+                None, b"GET", b"https://example.com", destination_is=b""
+            )
+        with self.assertRaises(ValueError):
+            self.cl.build_auth_headers(
+                b"", b"GET", b"https://example.com", destination_is=b""
+            )

tests/rest/client/test_profile.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 """Tests REST events for /profile paths."""
+from http import HTTPStatus
 from typing import Any, Dict, Optional
 
 from twisted.test.proto_helpers import MemoryReactor
@@ -49,6 +50,11 @@ def test_get_displayname(self) -> None:
         res = self._get_displayname()
         self.assertEqual(res, "owner")
 
+    def test_get_displayname_rejects_bad_username(self) -> None:
+        # Note: probably ought to urlencode the userid here, since it contains an `@`
+        channel = self.make_request("GET", "/profile/notanmxid@example.com/displayname")
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
+
     def test_set_displayname(self) -> None:
         channel = self.make_request(
             "PUT",

tests/test_types.py

@@ -26,10 +26,22 @@ def test_parse(self):
         self.assertEqual("test", user.domain)
         self.assertEqual(True, self.hs.is_mine(user))
 
-    def test_pase_empty(self):
+    def test_parse_rejects_empty_id(self):
         with self.assertRaises(SynapseError):
             UserID.from_string("")
 
+    def test_parse_rejects_missing_sigil(self):
+        with self.assertRaises(SynapseError):
+            UserID.from_string("alice:example.com")
+
+    def test_parse_rejects_missing_separator(self):
+        with self.assertRaises(SynapseError):
+            UserID.from_string("@alice.example.com")
+
+    def test_parse_rejects_missing_domain(self):
+        with self.assertRaises(SynapseError):
+            UserID.from_string("@alice:")
+
     def test_build(self):
         user = UserID("5678efgh", "my.domain")