From 419812f3618f2d746ce9e887777e73121bde53e6 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 14 Jun 2022 00:39:20 +0100 Subject: [PATCH 1/8] Rename test_fedclient to match its source file --- tests/http/{test_fedclient.py => test_matrixfederationclient.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/http/{test_fedclient.py => test_matrixfederationclient.py} (100%) diff --git a/tests/http/test_fedclient.py b/tests/http/test_matrixfederationclient.py similarity index 100% rename from tests/http/test_fedclient.py rename to tests/http/test_matrixfederationclient.py From 422a27b13644b393b4ac7826eba4b0fc0cbfdb11 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 14 Jun 2022 00:40:03 +0100 Subject: [PATCH 2/8] Require at least one destination to be truthy --- synapse/http/matrixfederationclient.py | 7 +++++-- tests/http/test_matrixfederationclient.py | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 776ed43f0359..c63d068f74c6 100644 --- a/synapse/http/matrixfederationclient.py +++ b/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"), diff --git a/tests/http/test_matrixfederationclient.py b/tests/http/test_matrixfederationclient.py index 006dbab093d5..be9eaf34e8e7 100644 --- a/tests/http/test_matrixfederationclient.py +++ b/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"" + ) From 47dfbf865940e2bdff1b73ec54c55ab236d2a2f5 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 14 Jun 2022 01:00:46 +0100 Subject: [PATCH 3/8] Raise when constructing a user ID with no domain --- synapse/types.py | 8 ++++++++ tests/test_types.py | 14 +++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/synapse/types.py b/synapse/types.py index 0586d2cbb95a..8cd2b5ad76e4 100644 --- a/synapse/types.py +++ b/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 diff --git a/tests/test_types.py b/tests/test_types.py index 0b10dae84839..783c2a9feed5 100644 --- a/tests/test_types.py +++ b/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") From a9ca64124bccc859646a852861dfe4b387a565bb Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 14 Jun 2022 01:36:06 +0100 Subject: [PATCH 4/8] Changelog --- changelog.d/13041.bugfix | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelog.d/13041.bugfix diff --git a/changelog.d/13041.bugfix b/changelog.d/13041.bugfix new file mode 100644 index 000000000000..edb1635eb984 --- /dev/null +++ b/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. + From ce3ad2ef3fc1e9e9c9fd3f5c4e58c29d934c3718 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 14 Jun 2022 01:41:06 +0100 Subject: [PATCH 5/8] Test the endpoint does actually return 400 --- tests/rest/client/test_profile.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 77c3ced42ec1..9920e3260bff 100644 --- a/tests/rest/client/test_profile.py +++ 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", From db34e0b47ec99618a8a401d419bf83573d65afb4 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 14 Jun 2022 13:09:17 +0100 Subject: [PATCH 6/8] urlencode `@` Co-authored-by: Patrick Cloke --- tests/rest/client/test_profile.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 9920e3260bff..9540f96dac2f 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -51,8 +51,7 @@ def test_get_displayname(self) -> None: 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") + channel = self.make_request("GET", "/profile/notanmxid%40example.com/displayname") self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result) def test_set_displayname(self) -> None: From 51ae97291685749206885033e9e617a27ebb278d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 14 Jun 2022 15:24:38 +0100 Subject: [PATCH 7/8] linter --- tests/rest/client/test_profile.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 9540f96dac2f..b7b575686a32 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -51,7 +51,9 @@ def test_get_displayname(self) -> None: self.assertEqual(res, "owner") def test_get_displayname_rejects_bad_username(self) -> None: - channel = self.make_request("GET", "/profile/notanmxid%40example.com/displayname") + channel = self.make_request( + "GET", "/profile/notanmxid%40example.com/displayname" + ) self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result) def test_set_displayname(self) -> None: From c7f90f3ab1c87a3c7563a61d291e884201064d27 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 14 Jun 2022 15:47:35 +0100 Subject: [PATCH 8/8] Don't verify domain is nonempty in from_string Use is_valid instead --- synapse/rest/client/profile.py | 20 ++++++++++++++++---- synapse/types.py | 11 ++--------- tests/rest/client/test_profile.py | 3 ++- tests/test_types.py | 5 ++--- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index c684636c0a01..c16d70790916 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -13,7 +13,7 @@ # limitations under the License. """ This module contains REST servlets to do with profile: /profile/ """ - +from http import HTTPStatus from typing import TYPE_CHECKING, Tuple from synapse.api.errors import Codes, SynapseError @@ -45,8 +45,12 @@ async def on_GET( requester = await self.auth.get_user_by_req(request) requester_user = requester.user - user = UserID.from_string(user_id) + if not UserID.is_valid(user_id): + raise SynapseError( + HTTPStatus.BAD_REQUEST, "Invalid user id", Codes.INVALID_PARAM + ) + user = UserID.from_string(user_id) await self.profile_handler.check_profile_query_allowed(user, requester_user) displayname = await self.profile_handler.get_displayname(user) @@ -98,8 +102,12 @@ async def on_GET( requester = await self.auth.get_user_by_req(request) requester_user = requester.user - user = UserID.from_string(user_id) + if not UserID.is_valid(user_id): + raise SynapseError( + HTTPStatus.BAD_REQUEST, "Invalid user id", Codes.INVALID_PARAM + ) + user = UserID.from_string(user_id) await self.profile_handler.check_profile_query_allowed(user, requester_user) avatar_url = await self.profile_handler.get_avatar_url(user) @@ -150,8 +158,12 @@ async def on_GET( requester = await self.auth.get_user_by_req(request) requester_user = requester.user - user = UserID.from_string(user_id) + if not UserID.is_valid(user_id): + raise SynapseError( + HTTPStatus.BAD_REQUEST, "Invalid user id", Codes.INVALID_PARAM + ) + user = UserID.from_string(user_id) await self.profile_handler.check_profile_query_allowed(user, requester_user) displayname = await self.profile_handler.get_displayname(user) diff --git a/synapse/types.py b/synapse/types.py index 8cd2b5ad76e4..668d48d646ae 100644 --- a/synapse/types.py +++ b/synapse/types.py @@ -267,15 +267,6 @@ 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 return cls(localpart=parts[0], domain=domain) @@ -287,6 +278,8 @@ def to_string(self) -> str: @classmethod def is_valid(cls: Type[DS], s: str) -> bool: """Parses the input string and attempts to ensure it is valid.""" + # TODO: this does not reject an empty localpart or an overly-long string. + # See https://spec.matrix.org/v1.2/appendices/#identifier-grammar try: obj = cls.from_string(s) # Apply additional validation to the domain. This is only done diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index b7b575686a32..29bed0e87225 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -13,6 +13,7 @@ # limitations under the License. """Tests REST events for /profile paths.""" +import urllib.parse from http import HTTPStatus from typing import Any, Dict, Optional @@ -52,7 +53,7 @@ def test_get_displayname(self) -> None: def test_get_displayname_rejects_bad_username(self) -> None: channel = self.make_request( - "GET", "/profile/notanmxid%40example.com/displayname" + "GET", f"/profile/{urllib.parse.quote('@alice:')}/displayname" ) self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result) diff --git a/tests/test_types.py b/tests/test_types.py index 783c2a9feed5..d8d82a517ea9 100644 --- a/tests/test_types.py +++ b/tests/test_types.py @@ -38,9 +38,8 @@ 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_validation_rejects_missing_domain(self): + self.assertFalse(UserID.is_valid("@alice:")) def test_build(self): user = UserID("5678efgh", "my.domain")