From f39bfc54cea2a81d50ddcbba892b69e4c4d2deff Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 7 Sep 2022 13:52:48 +0100 Subject: [PATCH 01/11] Use consistent capitalisation of `Threepid` --- synapse/rest/client/models.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/rest/client/models.py b/synapse/rest/client/models.py index 6278450c7047..b55af0df39f3 100644 --- a/synapse/rest/client/models.py +++ b/synapse/rest/client/models.py @@ -36,7 +36,7 @@ class Config: type: Optional[StrictStr] = None -class ThreePidRequestTokenBody(RequestBodyModel): +class ThreepidRequestTokenBody(RequestBodyModel): if TYPE_CHECKING: client_secret: StrictStr else: @@ -62,7 +62,7 @@ def token_required_for_identity_server( return token -class EmailRequestTokenBody(ThreePidRequestTokenBody): +class EmailRequestTokenBody(ThreepidRequestTokenBody): email: StrictStr # Canonicalise the email address. The addresses are all stored canonicalised @@ -80,6 +80,6 @@ class EmailRequestTokenBody(ThreePidRequestTokenBody): ISO3116_1_Alpha_2 = constr(regex="[A-Z]{2}", strict=True) -class MsisdnRequestTokenBody(ThreePidRequestTokenBody): +class MsisdnRequestTokenBody(ThreepidRequestTokenBody): country: ISO3116_1_Alpha_2 phone_number: StrictStr From b854c09eb75b3b4ad2a293ca85200be1fcf52c73 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 7 Sep 2022 13:54:39 +0100 Subject: [PATCH 02/11] Pull out ClientSecretType for upcoming reuse --- synapse/rest/client/models.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/synapse/rest/client/models.py b/synapse/rest/client/models.py index b55af0df39f3..eb5368766d45 100644 --- a/synapse/rest/client/models.py +++ b/synapse/rest/client/models.py @@ -36,18 +36,20 @@ class Config: type: Optional[StrictStr] = None -class ThreepidRequestTokenBody(RequestBodyModel): - if TYPE_CHECKING: - client_secret: StrictStr - else: - # See also assert_valid_client_secret() - client_secret: constr( - regex="[0-9a-zA-Z.=_-]", # noqa: F722 - min_length=0, - max_length=255, - strict=True, - ) +if TYPE_CHECKING: + ClientSecretType = StrictStr +else: + # See also assert_valid_client_secret() + ClientSecretType = constr( + regex="[0-9a-zA-Z.=_-]", # noqa: F722 + min_length=0, + max_length=255, + strict=True, + ) + +class ThreepidRequestTokenBody(RequestBodyModel): + client_secret: ClientSecretType id_server: Optional[StrictStr] id_access_token: Optional[StrictStr] next_link: Optional[StrictStr] From cdeb0a3827e4be135c280ef89a38557fdff49b82 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 7 Sep 2022 13:55:49 +0100 Subject: [PATCH 03/11] PostBody for /account/3pid/add --- synapse/rest/client/account.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index a09aaf3448df..7f41a4bcd5a0 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -43,6 +43,7 @@ from synapse.push.mailer import Mailer from synapse.rest.client.models import ( AuthenticationData, + ClientSecretType, EmailRequestTokenBody, MsisdnRequestTokenBody, ) @@ -627,6 +628,11 @@ def __init__(self, hs: "HomeServer"): self.auth = hs.get_auth() self.auth_handler = hs.get_auth_handler() + class PostBody(RequestBodyModel): + auth: Optional[AuthenticationData] = None + client_secret: ClientSecretType + sid: StrictStr + @interactive_auth_handler async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if not self.hs.config.registration.enable_3pid_changes: @@ -636,22 +642,17 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) user_id = requester.user.to_string() - body = parse_json_object_from_request(request) - - assert_params_in_dict(body, ["client_secret", "sid"]) - sid = body["sid"] - client_secret = body["client_secret"] - assert_valid_client_secret(client_secret) + body = parse_and_validate_json_object_from_request(request, self.PostBody) await self.auth_handler.validate_user_via_ui_auth( requester, request, - body, + body.dict(exclude_unset=True), "add a third-party identifier to your account", ) validation_session = await self.identity_handler.validate_threepid_session( - client_secret, sid + body.client_secret, body.sid ) if validation_session: await self.auth_handler.add_threepid( From 92f7f01a91b736fa35c2b87406ad04d4b106a054 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 7 Sep 2022 13:56:28 +0100 Subject: [PATCH 04/11] PostBody for /account/3pid/bind --- synapse/rest/client/account.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 7f41a4bcd5a0..1425be7f1bc6 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -677,23 +677,20 @@ def __init__(self, hs: "HomeServer"): self.identity_handler = hs.get_identity_handler() self.auth = hs.get_auth() - async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: - body = parse_json_object_from_request(request) + class PostBody(RequestBodyModel): + client_secret: ClientSecretType + id_access_token: StrictStr + id_server: StrictStr + sid: StrictStr - assert_params_in_dict( - body, ["id_server", "sid", "id_access_token", "client_secret"] - ) - id_server = body["id_server"] - sid = body["sid"] - id_access_token = body["id_access_token"] - client_secret = body["client_secret"] - assert_valid_client_secret(client_secret) + async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: + body = parse_and_validate_json_object_from_request(request, self.PostBody) requester = await self.auth.get_user_by_req(request) user_id = requester.user.to_string() await self.identity_handler.bind_threepid( - client_secret, sid, user_id, id_server, id_access_token + body.client_secret, body.sid, user_id, body.id_server, body.id_access_token ) return 200, {} From e584fa0c286415645ae438b56e90e12905112f99 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 7 Sep 2022 13:56:53 +0100 Subject: [PATCH 05/11] Define `ThreepidMedium` enum This one looks a bit magic, so I've added some extra tests for sanity's sake. --- synapse/rest/client/models.py | 7 ++++++ tests/rest/client/test_models.py | 41 ++++++++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/synapse/rest/client/models.py b/synapse/rest/client/models.py index eb5368766d45..8b863dccbe48 100644 --- a/synapse/rest/client/models.py +++ b/synapse/rest/client/models.py @@ -11,6 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from enum import Enum from typing import TYPE_CHECKING, Dict, Optional from pydantic import Extra, StrictInt, StrictStr, constr, validator @@ -19,6 +20,12 @@ from synapse.util.threepids import validate_email +class ThreepidMedium(str, Enum): + # Per advice at https://pydantic-docs.helpmanual.io/usage/types/#enums-and-choices + email = "email" + msisdn = "msisdn" + + class AuthenticationData(RequestBodyModel): """ Data used during user-interactive authentication. diff --git a/tests/rest/client/test_models.py b/tests/rest/client/test_models.py index a9da00665e19..4f5232ea7dbf 100644 --- a/tests/rest/client/test_models.py +++ b/tests/rest/client/test_models.py @@ -11,14 +11,47 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -import unittest +import unittest as stdlib_unittest -from pydantic import ValidationError +from pydantic import BaseModel, ValidationError -from synapse.rest.client.models import EmailRequestTokenBody +from synapse.rest.client.models import EmailRequestTokenBody, ThreepidMedium -class EmailRequestTokenBodyTestCase(unittest.TestCase): +class ThreepidMediumEnumTestCase(stdlib_unittest.TestCase): + class Model(BaseModel): + medium: ThreepidMedium + + def test_accepts_valid_medium_string(self) -> None: + """Sanity check that Pydantic behaves sensibly with an enum-of-str + + This is arguably more of a test of a class that inherits from str and Enum + simultaneously. + """ + model = self.Model.parse_obj({"medium": "email"}) + self.assertIsInstance(model.medium, str) + self.assertEqual(model.medium, "email") + self.assertEqual(model.medium, ThreepidMedium.email) + self.assertIs(model.medium, ThreepidMedium.email) + + self.assertNotEqual(model.medium, "msisdn") + self.assertNotEqual(model.medium, ThreepidMedium.msisdn) + self.assertIsNot(model.medium, ThreepidMedium.msisdn) + + def test_accepts_valid_medium_enum(self) -> None: + model = self.Model.parse_obj({"medium": ThreepidMedium.email}) + self.assertIs(model.medium, ThreepidMedium.email) + + def test_rejects_invalid_medium_value(self) -> None: + with self.assertRaises(ValidationError): + self.Model.parse_obj({"medium": "interpretive_dance"}) + + def test_rejects_invalid_medium_type(self) -> None: + with self.assertRaises(ValidationError): + self.Model.parse_obj({"medium": 123}) + + +class EmailRequestTokenBodyTestCase(stdlib_unittest.TestCase): base_request = { "client_secret": "hunter2", "email": "alice@wonderland.com", From 76d615e222dec31d845b525e19570b06d1fe2e5a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 7 Sep 2022 13:58:36 +0100 Subject: [PATCH 06/11] PostBody for /account/3pid/unbind --- synapse/rest/client/account.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index 1425be7f1bc6..b01e3cbdc7c8 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -46,6 +46,7 @@ ClientSecretType, EmailRequestTokenBody, MsisdnRequestTokenBody, + ThreepidMedium, ) from synapse.rest.models import RequestBodyModel from synapse.types import JsonDict @@ -706,23 +707,27 @@ def __init__(self, hs: "HomeServer"): self.auth = hs.get_auth() self.datastore = self.hs.get_datastores().main + class PostBody(RequestBodyModel): + address: StrictStr + id_server: Optional[StrictStr] = None + medium: ThreepidMedium + async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: """Unbind the given 3pid from a specific identity server, or identity servers that are known to have this 3pid bound """ requester = await self.auth.get_user_by_req(request) - body = parse_json_object_from_request(request) - assert_params_in_dict(body, ["medium", "address"]) - - medium = body.get("medium") - address = body.get("address") - id_server = body.get("id_server") + body = parse_and_validate_json_object_from_request(request, self.PostBody) # Attempt to unbind the threepid from an identity server. If id_server is None, try to # unbind from all identity servers this threepid has been added to in the past result = await self.identity_handler.try_unbind_threepid( requester.user.to_string(), - {"address": address, "medium": medium, "id_server": id_server}, + { + "address": body.address, + "medium": body.medium, + "id_server": body.id_server, + }, ) return 200, {"id_server_unbind_result": "success" if result else "no-support"} From 3defe6fdd2ecad4e64ed79307f88566b01bf025e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 7 Sep 2022 13:58:46 +0100 Subject: [PATCH 07/11] PostBody for /account/3pid/delete --- synapse/rest/client/account.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index b01e3cbdc7c8..c77d02199302 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -741,21 +741,25 @@ def __init__(self, hs: "HomeServer"): self.auth = hs.get_auth() self.auth_handler = hs.get_auth_handler() + class PostBody(RequestBodyModel): + address: StrictStr + id_server: Optional[StrictStr] = None + medium: ThreepidMedium + async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if not self.hs.config.registration.enable_3pid_changes: raise SynapseError( 400, "3PID changes are disabled on this server", Codes.FORBIDDEN ) - body = parse_json_object_from_request(request) - assert_params_in_dict(body, ["medium", "address"]) + body = parse_and_validate_json_object_from_request(request, self.PostBody) requester = await self.auth.get_user_by_req(request) user_id = requester.user.to_string() try: ret = await self.auth_handler.delete_threepid( - user_id, body["medium"], body["address"], body.get("id_server") + user_id, body.medium, body.address, body.id_server ) except Exception: # NB. This endpoint should succeed if there is nothing to From 35d664a74d1a1643b56013166948fc3138ea67a9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 7 Sep 2022 14:54:26 +0100 Subject: [PATCH 08/11] Changelog --- changelog.d/13736.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13736.feature diff --git a/changelog.d/13736.feature b/changelog.d/13736.feature new file mode 100644 index 000000000000..60a63c100929 --- /dev/null +++ b/changelog.d/13736.feature @@ -0,0 +1 @@ +Improve validation of request bodies for the following client-server API endpoints: [`/account/3pid/add`](https://spec.matrix.org/v1.3/client-server-api/#post_matrixclientv3account3pidadd), [`/account/3pid/bind`](https://spec.matrix.org/v1.3/client-server-api/#post_matrixclientv3account3pidbind), [`/account/3pid/delete`](https://spec.matrix.org/v1.3/client-server-api/#post_matrixclientv3account3piddelete) and [`/account/3pid/unbind`](https://spec.matrix.org/v1.3/client-server-api/#post_matrixclientv3account3pidunbind). From 8143310f90331f59a775c8b0d9406dc93223047d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 8 Sep 2022 17:34:21 +0100 Subject: [PATCH 09/11] Fix min length of `client_secret` --- synapse/rest/client/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/models.py b/synapse/rest/client/models.py index 8b863dccbe48..a4c25e1d3a70 100644 --- a/synapse/rest/client/models.py +++ b/synapse/rest/client/models.py @@ -49,7 +49,7 @@ class Config: # See also assert_valid_client_secret() ClientSecretType = constr( regex="[0-9a-zA-Z.=_-]", # noqa: F722 - min_length=0, + min_length=1, max_length=255, strict=True, ) From 38ae8c7d234f8b1b3fb7e4591519f163a4c9d940 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 13 Sep 2022 15:29:42 +0100 Subject: [PATCH 10/11] Opt for simpler `Literal[...]` type I would like to see a StrEnum but I am somewhat anxious of making that change right now. I think we should get validation in place first and then look at propagating better types into the application. --- synapse/rest/client/account.py | 6 +++--- synapse/rest/client/models.py | 7 ------- tests/rest/client/test_models.py | 16 +++------------- 3 files changed, 6 insertions(+), 23 deletions(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index c77d02199302..a2c15b8e38e8 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -19,6 +19,7 @@ from urllib.parse import urlparse from pydantic import StrictBool, StrictStr, constr +from typing_extensions import Literal from twisted.web.server import Request @@ -46,7 +47,6 @@ ClientSecretType, EmailRequestTokenBody, MsisdnRequestTokenBody, - ThreepidMedium, ) from synapse.rest.models import RequestBodyModel from synapse.types import JsonDict @@ -710,7 +710,7 @@ def __init__(self, hs: "HomeServer"): class PostBody(RequestBodyModel): address: StrictStr id_server: Optional[StrictStr] = None - medium: ThreepidMedium + medium: Literal["email", "msisdn"] async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: """Unbind the given 3pid from a specific identity server, or identity servers that are @@ -744,7 +744,7 @@ def __init__(self, hs: "HomeServer"): class PostBody(RequestBodyModel): address: StrictStr id_server: Optional[StrictStr] = None - medium: ThreepidMedium + medium: Literal["email", "msisdn"] async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: if not self.hs.config.registration.enable_3pid_changes: diff --git a/synapse/rest/client/models.py b/synapse/rest/client/models.py index a4c25e1d3a70..cfb477f2a098 100644 --- a/synapse/rest/client/models.py +++ b/synapse/rest/client/models.py @@ -11,7 +11,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from enum import Enum from typing import TYPE_CHECKING, Dict, Optional from pydantic import Extra, StrictInt, StrictStr, constr, validator @@ -20,12 +19,6 @@ from synapse.util.threepids import validate_email -class ThreepidMedium(str, Enum): - # Per advice at https://pydantic-docs.helpmanual.io/usage/types/#enums-and-choices - email = "email" - msisdn = "msisdn" - - class AuthenticationData(RequestBodyModel): """ Data used during user-interactive authentication. diff --git a/tests/rest/client/test_models.py b/tests/rest/client/test_models.py index 4f5232ea7dbf..0b8fcb0c47f4 100644 --- a/tests/rest/client/test_models.py +++ b/tests/rest/client/test_models.py @@ -14,13 +14,14 @@ import unittest as stdlib_unittest from pydantic import BaseModel, ValidationError +from typing_extensions import Literal -from synapse.rest.client.models import EmailRequestTokenBody, ThreepidMedium +from synapse.rest.client.models import EmailRequestTokenBody class ThreepidMediumEnumTestCase(stdlib_unittest.TestCase): class Model(BaseModel): - medium: ThreepidMedium + medium: Literal["email", "msisdn"] def test_accepts_valid_medium_string(self) -> None: """Sanity check that Pydantic behaves sensibly with an enum-of-str @@ -29,18 +30,7 @@ def test_accepts_valid_medium_string(self) -> None: simultaneously. """ model = self.Model.parse_obj({"medium": "email"}) - self.assertIsInstance(model.medium, str) self.assertEqual(model.medium, "email") - self.assertEqual(model.medium, ThreepidMedium.email) - self.assertIs(model.medium, ThreepidMedium.email) - - self.assertNotEqual(model.medium, "msisdn") - self.assertNotEqual(model.medium, ThreepidMedium.msisdn) - self.assertIsNot(model.medium, ThreepidMedium.msisdn) - - def test_accepts_valid_medium_enum(self) -> None: - model = self.Model.parse_obj({"medium": ThreepidMedium.email}) - self.assertIs(model.medium, ThreepidMedium.email) def test_rejects_invalid_medium_value(self) -> None: with self.assertRaises(ValidationError): From a840e8ccdb693cad914c09f6e2d69ee34627d6b3 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 15 Sep 2022 17:46:42 +0100 Subject: [PATCH 11/11] ClientSecretType -> ClientSecretStr --- synapse/rest/client/account.py | 6 +++--- synapse/rest/client/models.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py index a2c15b8e38e8..2db2a04f95df 100644 --- a/synapse/rest/client/account.py +++ b/synapse/rest/client/account.py @@ -44,7 +44,7 @@ from synapse.push.mailer import Mailer from synapse.rest.client.models import ( AuthenticationData, - ClientSecretType, + ClientSecretStr, EmailRequestTokenBody, MsisdnRequestTokenBody, ) @@ -631,7 +631,7 @@ def __init__(self, hs: "HomeServer"): class PostBody(RequestBodyModel): auth: Optional[AuthenticationData] = None - client_secret: ClientSecretType + client_secret: ClientSecretStr sid: StrictStr @interactive_auth_handler @@ -679,7 +679,7 @@ def __init__(self, hs: "HomeServer"): self.auth = hs.get_auth() class PostBody(RequestBodyModel): - client_secret: ClientSecretType + client_secret: ClientSecretStr id_access_token: StrictStr id_server: StrictStr sid: StrictStr diff --git a/synapse/rest/client/models.py b/synapse/rest/client/models.py index cfb477f2a098..3d7940b0fc50 100644 --- a/synapse/rest/client/models.py +++ b/synapse/rest/client/models.py @@ -37,10 +37,10 @@ class Config: if TYPE_CHECKING: - ClientSecretType = StrictStr + ClientSecretStr = StrictStr else: # See also assert_valid_client_secret() - ClientSecretType = constr( + ClientSecretStr = constr( regex="[0-9a-zA-Z.=_-]", # noqa: F722 min_length=1, max_length=255, @@ -49,7 +49,7 @@ class Config: class ThreepidRequestTokenBody(RequestBodyModel): - client_secret: ClientSecretType + client_secret: ClientSecretStr id_server: Optional[StrictStr] id_access_token: Optional[StrictStr] next_link: Optional[StrictStr]