Fix MSC4178 error codes on /account/3pid/{email,msisdn}/requestToken (#20101)

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

@@ -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.

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,

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
 
 

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,