From b5cb8b1c1e5f69f71b0566c5d3337aaa6f65cfe4 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 1 Sep 2022 11:28:30 +0100 Subject: [PATCH 1/7] Pydantic for MsisdnThreepidRequestTokenRestServlet --- synapse/rest/client/account.py | 39 ++++++++++++++++------------------ synapse/rest/client/models.py | 14 ++++++++++-- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 1f9a8ccc2349..8e858eb1da4b 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -41,7 +41,11 @@ from synapse.http.site import SynapseRequest from synapse.metrics import threepid_send_requests from synapse.push.mailer import Mailer -from synapse.rest.client.models import AuthenticationData, EmailRequestTokenBody +from synapse.rest.client.models import ( + AuthenticationData, + EmailRequestTokenBody, + MsisdnRequestTokenBody, +) from synapse.rest.models import RequestBodyModel from synapse.types import JsonDict from synapse.util.msisdn import phone_number_to_msisdn @@ -400,23 +404,16 @@ def __init__(self, hs: "HomeServer"): self.identity_handler = hs.get_identity_handler() async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: - body = parse_json_object_from_request(request) - assert_params_in_dict( - body, ["client_secret", "country", "phone_number", "send_attempt"] + body = parse_and_validate_json_object_from_request( + request, MsisdnRequestTokenBody ) - client_secret = body["client_secret"] - assert_valid_client_secret(client_secret) - - country = body["country"] - phone_number = body["phone_number"] - send_attempt = body["send_attempt"] - next_link = body.get("next_link") # Optional param - - msisdn = phone_number_to_msisdn(country, phone_number) + msisdn = phone_number_to_msisdn(body.country, body.phone_number) if not await check_3pid_allowed(self.hs, "msisdn", msisdn): raise SynapseError( 403, + # TODO: is this error message accurate? Looks like we've only rejected + # this phone number, not necessarily all phone numbers "Account phone numbers are not authorized on this server", Codes.THREEPID_DENIED, ) @@ -425,9 +422,9 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: request, "msisdn", msisdn ) - if next_link: + if body.next_link: # Raise if the provided next_link value isn't valid - assert_valid_next_link(self.hs, next_link) + assert_valid_next_link(self.hs, body.next_link) existing_user_id = await self.store.get_user_id_by_threepid("msisdn", msisdn) @@ -454,15 +451,15 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: ret = await self.identity_handler.requestMsisdnToken( self.hs.config.registration.account_threepid_delegate_msisdn, - country, - phone_number, - client_secret, - send_attempt, - next_link, + body.country, + body.phone_number, + body.client_secret, + body.send_attempt, + body.next_link, ) threepid_send_requests.labels(type="msisdn", reason="add_threepid").observe( - send_attempt + body.send_attempt ) return 200, ret diff --git a/synapse/rest/client/models.py b/synapse/rest/client/models.py index 31506029973a..45ef6a1ef97e 100644 --- a/synapse/rest/client/models.py +++ b/synapse/rest/client/models.py @@ -36,7 +36,7 @@ class Config: type: Optional[StrictStr] = None -class EmailRequestTokenBody(RequestBodyModel): +class ThreePidRequestTokenBody(RequestBodyModel): if TYPE_CHECKING: client_secret: StrictStr else: @@ -47,7 +47,7 @@ class EmailRequestTokenBody(RequestBodyModel): max_length=255, strict=True, ) - email: StrictStr + id_server: Optional[StrictStr] id_access_token: Optional[StrictStr] next_link: Optional[StrictStr] @@ -61,9 +61,19 @@ def token_required_for_identity_server( raise ValueError("id_access_token is required if an id_server is supplied.") return token + +class EmailRequestTokenBody(ThreePidRequestTokenBody): + email: StrictStr + # Canonicalise the email address. The addresses are all stored canonicalised # in the database. This allows the user to reset his password without having to # 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. _email_validator = validator("email", allow_reuse=True)(validate_email) + + +class MsisdnRequestTokenBody(ThreePidRequestTokenBody): + # Two-letter uppercase ISO-3166-1-alpha-2 + country: StrictStr + phone_number: StrictStr From 98f534bbc62d0d2b78d2bf0a7019dce79256569b Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 1 Sep 2022 12:40:52 +0100 Subject: [PATCH 2/7] Pydantic for experimental `account_status` endpoint --- synapse/http/servlet.py | 19 +++++++++++++++++-- synapse/rest/client/account.py | 20 ++++++++++++-------- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py index 26aaabfb34fa..80acbdcf3ce7 100644 --- a/synapse/http/servlet.py +++ b/synapse/http/servlet.py @@ -28,7 +28,8 @@ overload, ) -from pydantic import BaseModel, ValidationError +from pydantic import BaseModel, MissingError, PydanticValueError, ValidationError +from pydantic.error_wrappers import ErrorWrapper from typing_extensions import Literal from twisted.web.server import Request @@ -714,7 +715,21 @@ def parse_and_validate_json_object_from_request( try: instance = model_type.parse_obj(content) except ValidationError as e: - raise SynapseError(HTTPStatus.BAD_REQUEST, str(e), errcode=Codes.BAD_JSON) + # Choose a matrix error code. The catch-all is BAD_JSON, but we try to find a + # more specific error if possible (which occasionally helps us to be spec- + # compliant) This is a bit awkward because the spec's error codes aren't very + # clear-cut: BAD_JSON arguably overlaps with MISSING_PARAM and INVALID_PARAM. + errcode = Codes.BAD_JSON + + raw_errors = e.raw_errors + if len(raw_errors) == 1 and isinstance(raw_errors[0], ErrorWrapper): + raw_error = raw_errors[0].exc + if isinstance(raw_error, MissingError): + errcode = Codes.MISSING_PARAM + elif isinstance(raw_error, PydanticValueError): + errcode = Codes.INVALID_PARAM + + raise SynapseError(HTTPStatus.BAD_REQUEST, str(e), errcode=errcode) return instance diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 8e858eb1da4b..931376c1d38a 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -15,10 +15,10 @@ # limitations under the License. import logging import random -from typing import TYPE_CHECKING, Optional, Tuple +from typing import TYPE_CHECKING, List, Optional, Tuple from urllib.parse import urlparse -from pydantic import StrictBool, StrictStr, constr +from pydantic import StrictBool, StrictStr, conlist, constr from twisted.web.server import Request @@ -842,17 +842,21 @@ def __init__(self, hs: "HomeServer"): self._auth = hs.get_auth() self._account_handler = hs.get_account_handler() + class PostBody(RequestBodyModel): + # TODO: we could validate that each user id is an mxid here, and/or parse it + # as a UserID + if TYPE_CHECKING: + user_ids: List[StrictStr] + else: + user_ids: conlist(item_type=StrictStr, min_items=1) + async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: await self._auth.get_user_by_req(request) - body = parse_json_object_from_request(request) - if "user_ids" not in body: - raise SynapseError( - 400, "Required parameter 'user_ids' is missing", Codes.MISSING_PARAM - ) + body = parse_and_validate_json_object_from_request(request, self.PostBody) statuses, failures = await self._account_handler.get_account_statuses( - body["user_ids"], + body.user_ids, allow_remote=True, ) From 6180af63cf4f8b074f176224b44e009a3fdb7d6a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 1 Sep 2022 11:58:35 +0100 Subject: [PATCH 3/7] Fixup comment in the light of #13563 --- synapse/rest/client/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/models.py b/synapse/rest/client/models.py index 45ef6a1ef97e..56530882a404 100644 --- a/synapse/rest/client/models.py +++ b/synapse/rest/client/models.py @@ -25,8 +25,8 @@ class AuthenticationData(RequestBodyModel): (The name "Authentication Data" is taken directly from the spec.) - Additional keys will be present, depending on the `type` field. Use `.dict()` to - access them. + Additional keys will be present, depending on the `type` field. Use + `.dict(exclude_unset=True)` to access them. """ class Config: From 714ee54895ea978e21f700af01bb2ea01e668cc5 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 1 Sep 2022 11:32:14 +0100 Subject: [PATCH 4/7] Changelog --- changelog.d/13687.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13687.feature diff --git a/changelog.d/13687.feature b/changelog.d/13687.feature new file mode 100644 index 000000000000..dac53ec122c4 --- /dev/null +++ b/changelog.d/13687.feature @@ -0,0 +1 @@ +Improve validation of request bodies for the following client-server API endpoints: [`/account/3pid/msisdn/requestToken`](https://spec.matrix.org/v1.3/client-server-api/#post_matrixclientv3account3pidmsisdnrequesttoken) and [`/org.matrix.msc3720/account_status`](https://github.com/matrix-org/matrix-spec-proposals/blob/babolivier/user_status/proposals/3720-account-status.md#post-_matrixclientv1account_status). \ No newline at end of file From b861950ea0bf514aa4045bd75000a69fd51761ae Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 5 Sep 2022 12:38:10 +0100 Subject: [PATCH 5/7] Relax length constraint on user_ids per https://github.com/matrix-org/matrix-spec-proposals/commit/bda488084ec0c7d762d6760e7442f54082a77cc9 --- synapse/rest/client/account.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 931376c1d38a..85512c2843e2 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -845,10 +845,7 @@ def __init__(self, hs: "HomeServer"): class PostBody(RequestBodyModel): # TODO: we could validate that each user id is an mxid here, and/or parse it # as a UserID - if TYPE_CHECKING: - user_ids: List[StrictStr] - else: - user_ids: conlist(item_type=StrictStr, min_items=1) + user_ids: List[StrictStr] async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: await self._auth.get_user_by_req(request) From eff0ffa33680dedf52e14223f2473155438d01c5 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 5 Sep 2022 12:44:21 +0100 Subject: [PATCH 6/7] Sneak in extra validation for `country` --- synapse/rest/client/models.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/synapse/rest/client/models.py b/synapse/rest/client/models.py index 56530882a404..6278450c7047 100644 --- a/synapse/rest/client/models.py +++ b/synapse/rest/client/models.py @@ -73,7 +73,13 @@ class EmailRequestTokenBody(ThreePidRequestTokenBody): _email_validator = validator("email", allow_reuse=True)(validate_email) +if TYPE_CHECKING: + ISO3116_1_Alpha_2 = StrictStr +else: + # Per spec: two-letter uppercase ISO-3166-1-alpha-2 + ISO3116_1_Alpha_2 = constr(regex="[A-Z]{2}", strict=True) + + class MsisdnRequestTokenBody(ThreePidRequestTokenBody): - # Two-letter uppercase ISO-3166-1-alpha-2 - country: StrictStr + country: ISO3116_1_Alpha_2 phone_number: StrictStr From 1ed55449464a2e0303ad733869a89bdd4c34217d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 5 Sep 2022 13:40:47 +0100 Subject: [PATCH 7/7] Fix lint --- synapse/rest/client/account.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 85512c2843e2..a09aaf3448df 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -18,7 +18,7 @@ from typing import TYPE_CHECKING, List, Optional, Tuple from urllib.parse import urlparse -from pydantic import StrictBool, StrictStr, conlist, constr +from pydantic import StrictBool, StrictStr, constr from twisted.web.server import Request