From 366a020f79eaf7bca640347ec26fad29601c7092 Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Thu, 3 Sep 2026 13:05:08 +0200 Subject: [PATCH] Fix MSC4178 error codes on `/account/3pid/{email,msisdn}/requestToken` (#20101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of https://github.com/element-hq/synapse/issues/18118 ## What the spec says Since Matrix v1.13 (introduced by [MSC4178](https://github.com/matrix-org/matrix-spec-proposals/pull/4178)), the `400` response of [`POST /_matrix/client/v3/account/3pid/email/requestToken`](https://spec.matrix.org/v1.19/client-server-api/#post_matrixclientv3account3pidemailrequesttoken) lists, among the "Error codes that can be returned": > - `M_THREEPID_MEDIUM_NOT_SUPPORTED`: The homeserver does not support adding email addresses. > - `M_INVALID_PARAM`: The email address given was not valid. and [`POST /_matrix/client/v3/account/3pid/msisdn/requestToken`](https://spec.matrix.org/v1.19/client-server-api/#post_matrixclientv3account3pidmsisdnrequesttoken) likewise: > - `M_THREEPID_MEDIUM_NOT_SUPPORTED`: The homeserver does not support adding phone numbers. > - `M_INVALID_PARAM`: The phone number given was not valid. ## What was missing Synapse implemented the headline case (unsupported medium), but around it: - A malformed email address or country code was reported with the generic `M_BAD_JSON` instead of `M_INVALID_PARAM`. The email validator deliberately kept `M_BAD_JSON` "to ensure backward compatibility of HTTP error codes" (matrix-org/synapse#13687, 2022) — that predates Matrix v1.13, which now lists `M_INVALID_PARAM` for this case. - On the msisdn variant, the unsupported-medium check ran after the denied/in-use checks, so a request wrong in two ways reported the other fault; the email variant checks it first. ## What this PR changes - Malformed email addresses and country codes on `/account/3pid/{email,msisdn}/requestToken` are reported with `M_INVALID_PARAM`. Both flow through the existing errcode translation as `value_error`: the email validator raises a plain `ValueError`, and the country-code constraint (`ISO3166_1_Alpha_2`) declares its own error via pydantic-core's `custom_error_schema`. - On the msisdn variant the unsupported-medium check now runs before the denied/in-use checks, as on the email variant. - The country-code type is renamed from `ISO3116_1_Alpha_2` to `ISO3166_1_Alpha_2` (typo in the standard's number). [`/account/password/email/requestToken`](https://spec.matrix.org/v1.19/client-server-api/#post_matrixclientv3accountpasswordemailrequesttoken) (not covered by the v1.13 change) shares the email request body model, so a malformed email there is now also reported with `M_INVALID_PARAM` instead of `M_BAD_JSON`. Its `400` response is described as "the request was invalid" and only names `M_SERVER_NOT_TRUSTED` explicitly ("can be returned if…") rather than restricting the server to a fixed list, and `M_INVALID_PARAM` is the spec's generic code for "A parameter that was specified has the wrong value" ([other error codes](https://spec.matrix.org/v1.19/client-server-api/#other-error-codes)). --- changelog.d/20101.bugfix | 1 + synapse/rest/client/account.py | 22 +++---- synapse/types/rest/client/__init__.py | 33 ++++++---- tests/rest/client/test_account.py | 91 +++++++++++++++++++++++++-- 4 files changed, 120 insertions(+), 27 deletions(-) create mode 100644 changelog.d/20101.bugfix diff --git a/changelog.d/20101.bugfix b/changelog.d/20101.bugfix new file mode 100644 index 0000000000..60e1d3ada2 --- /dev/null +++ b/changelog.d/20101.bugfix @@ -0,0 +1 @@ +Return the `M_INVALID_PARAM` error code specified by Matrix v1.13 ([MSC4178](https://github.com/matrix-org/matrix-spec-proposals/pull/4178)) when a malformed email address or country code is submitted to `/account/3pid/{email,msisdn}/requestToken`, and report an unsupported medium ahead of the denied/in-use checks on the msisdn variant. diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 3b01e40121..13bc703628 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -423,6 +423,17 @@ class MsisdnThreepidRequestTokenRestServlet(RestServlet): self.identity_handler = hs.get_identity_handler() async def on_POST(self, request: SynapseRequest) -> tuple[int, JsonDict]: + if not self.hs.config.registration.account_threepid_delegate_msisdn: + logger.warning( + "No upstream msisdn account_threepid_delegate configured on the server to " + "handle this request" + ) + raise SynapseError( + 400, + "Adding phone numbers to user account is not supported by this homeserver", + Codes.THREEPID_MEDIUM_NOT_SUPPORTED, + ) + body = parse_and_validate_json_object_from_request( request, MsisdnRequestTokenBody ) @@ -462,17 +473,6 @@ class MsisdnThreepidRequestTokenRestServlet(RestServlet): logger.info("MSISDN %s is already in use by %s", msisdn, existing_user_id) raise SynapseError(400, "MSISDN is already in use", Codes.THREEPID_IN_USE) - if not self.hs.config.registration.account_threepid_delegate_msisdn: - logger.warning( - "No upstream msisdn account_threepid_delegate configured on the server to " - "handle this request" - ) - raise SynapseError( - 400, - "Adding phone numbers to user account is not supported by this homeserver", - Codes.THREEPID_MEDIUM_NOT_SUPPORTED, - ) - ret = await self.identity_handler.requestMsisdnToken( self.hs.config.registration.account_threepid_delegate_msisdn, body.country, diff --git a/synapse/types/rest/client/__init__.py b/synapse/types/rest/client/__init__.py index 353367c0d7..17647e621d 100644 --- a/synapse/types/rest/client/__init__.py +++ b/synapse/types/rest/client/__init__.py @@ -26,6 +26,7 @@ from pydantic import ( ConfigDict, Field, GetCoreSchemaHandler, + GetPydanticSchema, StrictBool, StrictInt, StrictStr, @@ -33,7 +34,7 @@ from pydantic import ( field_validator, model_validator, ) -from pydantic_core import CoreSchema, PydanticCustomError +from pydantic_core import CoreSchema from typing_extensions import Annotated, Self from synapse.types import Absent, AbsentType, NonNegativeStrictInt @@ -91,25 +92,33 @@ class EmailRequestTokenBody(ThreepidRequestTokenBody): # know the exact spelling (eg. upper and lower case) of address in the database. # Without this, an email stored in the database as "foo@bar.com" would cause # user requests for "FOO@bar.com" to raise a Not Found error. + # + # A ValueError produces a Pydantic error of type "value_error", which + # validate_json_object translates to M_INVALID_PARAM, the errcode the spec + # lists for an invalid address on /account/3pid/email/requestToken: + # https://spec.matrix.org/v1.19/client-server-api/#post_matrixclientv3account3pidemailrequesttoken @field_validator("email") @classmethod def _email_validator(cls, email: StrictStr) -> StrictStr: - try: - return validate_email(email) - except ValueError as e: - # To ensure backward compatibility of HTTP error codes, we return a - # Pydantic error with the custom, unrecognized error type - # "email_custom_err_type" instead of the default error type - # "value_error". This results in the more generic BAD_JSON HTTP - # error instead of the more specific INVALID_PARAM one. - raise PydanticCustomError("email_custom_err_type", str(e), None) from e + return validate_email(email) -ISO3116_1_Alpha_2 = Annotated[str, StringConstraints(pattern="[A-Z]{2}", strict=True)] +ISO3166_1_Alpha_2 = Annotated[ + str, + GetPydanticSchema( + lambda source, handler: pydantic_core.core_schema.custom_error_schema( + pydantic_core.core_schema.str_schema(pattern="[A-Z]{2}", strict=True), + custom_error_type="value_error", + custom_error_context={ + "error": "Not a valid ISO 3166-1 alpha-2 country code" + }, + ) + ), +] class MsisdnRequestTokenBody(ThreepidRequestTokenBody): - country: ISO3116_1_Alpha_2 + country: ISO3166_1_Alpha_2 phone_number: StrictStr diff --git a/tests/rest/client/test_account.py b/tests/rest/client/test_account.py index 8ffac644b6..074186b1e6 100644 --- a/tests/rest/client/test_account.py +++ b/tests/rest/client/test_account.py @@ -42,7 +42,7 @@ from synapse.types import JsonDict, UserID, create_requester from synapse.util.clock import Clock from tests import unittest -from tests.server import FakeSite, make_request +from tests.server import FakeChannel, FakeSite, make_request from tests.unittest import override_config @@ -359,6 +359,26 @@ class PasswordResetTestCase(unittest.HomeserverTestCase): self._validate_token(link, next_link) + def test_password_reset_invalid_email(self) -> None: + """A malformed email address is reported with M_INVALID_PARAM, as on + /account/3pid/email/requestToken (the two endpoints share the request + body model). + """ + channel = self.make_request( + "POST", + b"account/password/email/requestToken", + { + "client_secret": "foobar", + "email": "address-without-at.bar", + "send_attempt": 1, + }, + ) + self.assertEqual( + HTTPStatus.BAD_REQUEST, channel.code, msg=channel.result["body"] + ) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + self.assertIn("Unable to parse email address", channel.json_body["error"]) + def _request_token( self, email: str, @@ -1010,21 +1030,21 @@ class ThreepidEmailRestTestCase(unittest.HomeserverTestCase): def test_add_email_no_at(self) -> None: self._request_token_invalid_email( "address-without-at.bar", - expected_errcode=Codes.BAD_JSON, + expected_errcode=Codes.INVALID_PARAM, expected_error="Unable to parse email address", ) def test_add_email_two_at(self) -> None: self._request_token_invalid_email( "foo@foo@test.bar", - expected_errcode=Codes.BAD_JSON, + expected_errcode=Codes.INVALID_PARAM, expected_error="Unable to parse email address", ) def test_add_email_bad_format(self) -> None: self._request_token_invalid_email( "user@bad.example.net@good.example.com", - expected_errcode=Codes.BAD_JSON, + expected_errcode=Codes.INVALID_PARAM, expected_error="Unable to parse email address", ) @@ -1435,6 +1455,69 @@ class ThreepidEmailRestTestCase(unittest.HomeserverTestCase): self.assertIn(expected_email, threepids) +class ThreepidMsisdnRestTestCase(unittest.HomeserverTestCase): + """Tests the error codes of /account/3pid/msisdn/requestToken. + + See https://spec.matrix.org/v1.19/client-server-api/#post_matrixclientv3account3pidmsisdnrequesttoken + (error codes added in Matrix v1.13 by MSC4178). + """ + + servlets = [ + account.register_servlets, + login.register_servlets, + synapse.rest.admin.register_servlets_for_client_rest_resource, + ] + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.store = hs.get_datastores().main + self.user_id = self.register_user("kermit", "test") + + def _request_token(self, country: str, phone_number: str) -> FakeChannel: + return self.make_request( + "POST", + b"account/3pid/msisdn/requestToken", + { + "client_secret": "foobar", + "country": country, + "phone_number": phone_number, + "send_attempt": 1, + }, + ) + + @override_config({"account_threepid_delegates": {"msisdn": "https://id_server"}}) + def test_invalid_country_code(self) -> None: + """A malformed country code is reported with M_INVALID_PARAM.""" + channel = self._request_token("gb", "07700900001") + self.assertEqual( + HTTPStatus.BAD_REQUEST, channel.code, msg=channel.result["body"] + ) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + + @override_config({"account_threepid_delegates": {"msisdn": "https://id_server"}}) + def test_invalid_phone_number(self) -> None: + """An unparseable phone number is reported with M_INVALID_PARAM.""" + channel = self._request_token("GB", "not a phone number") + self.assertEqual( + HTTPStatus.BAD_REQUEST, channel.code, msg=channel.result["body"] + ) + self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"]) + + @override_config({"allowed_local_3pids": [{"medium": "email", "pattern": ".*"}]}) + def test_medium_not_supported_checked_before_denied(self) -> None: + """When the server cannot send validation SMSes, it reports + M_THREEPID_MEDIUM_NOT_SUPPORTED even if the phone number would be + denied: the unsupported-medium check comes first, as on the email + variant. + """ + channel = self._request_token("GB", "07700900001") + self.assertEqual( + HTTPStatus.BAD_REQUEST, channel.code, msg=channel.result["body"] + ) + self.assertEqual( + Codes.THREEPID_MEDIUM_NOT_SUPPORTED, channel.json_body["errcode"] + ) + + class AccountStatusTestCase(unittest.HomeserverTestCase): servlets = [ account.register_servlets,