From e35c8f0eff7d62a1dd830d54d8676d8e961cff3a Mon Sep 17 00:00:00 2001 From: Paul Chobert Date: Mon, 7 Sep 2026 10:35:33 +0200 Subject: [PATCH] Fix `client_secret` param validator (#20104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The validation of `client_secret` params accepts invalid values: any value that includes a char in `[0-9a-zA-Z.=_-]` is accepted, for example `"café"` is accepted. Instead synapse should only accept if all chars are within `[0-9a-zA-Z.=_-]`, not just one. This is the current validator: ```python ClientSecretStr = Annotated[ str, StringConstraints( pattern="[0-9a-zA-Z.=_-]", min_length=1, max_length=255, strict=True, ), ] ``` Unfortunately, Pydantic only defines the `pattern` argument as: > ### pattern > A regex pattern that the string must match. Which is extremely imprecise. Here is a little script to verify the behavior: ```python from typing import Annotated from pydantic import BaseModel, StringConstraints, ValidationError def check(pattern: str) -> None: ClientSecretStr = Annotated[ str, StringConstraints(pattern=pattern, min_length=1, max_length=255, strict=True), ] class Body(BaseModel): client_secret: ClientSecretStr try: Body.model_validate({"client_secret": "café"}) print(f"pattern = {pattern!r}: 'café' ACCEPTED <-- should have been rejected") except ValidationError: print(f"pattern = {pattern!r}: 'café' rejected") check("[0-9a-zA-Z.=_-]") # before the fix (unanchored) check("^[0-9a-zA-Z.=_-]+$") # after the fix ``` which would output ``` pattern = '[0-9a-zA-Z.=_-]': 'café' ACCEPTED <-- should have been rejected pattern = '^[0-9a-zA-Z.=_-]+$': 'café' rejected ``` ## History This is a regression of a previously reported and fixed bug: - matrix-org/synapse#6766 (2020) reported that Synapse did not enforce the spec's `client_secret` regex at all — with real-world fallout: FluffyChat had started sending secrets containing `:` because nothing rejected them. Fixed by introducing `assert_valid_client_secret` (matrix-org/synapse#6767). - matrix-org/synapse#13188 (Synapse 1.66.0) ported the account endpoints to Pydantic and transcribed the regex without anchors/quantifier; Pydantic v1's `re.match` semantics meant only the *first* character was validated. - #19071 (Synapse 1.142.0) migrated to Pydantic v2, whose *search* semantics weakened it further to "any one character anywhere". --- ### Pull Request Checklist * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: Quentin Gliech --- changelog.d/20104.bugfix | 1 + synapse/types/rest/client/__init__.py | 2 +- tests/rest/client/test_models.py | 40 ++++++++++++++++++++++++++- 3 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 changelog.d/20104.bugfix diff --git a/changelog.d/20104.bugfix b/changelog.d/20104.bugfix new file mode 100644 index 0000000000..f04945660c --- /dev/null +++ b/changelog.d/20104.bugfix @@ -0,0 +1 @@ +Fix a regression where `client_secret` request parameters were not validated against the character set required by the spec on Pydantic-validated endpoints, regressed in Synapse 1.66.0 (originally fixed for https://github.com/matrix-org/synapse/issues/6766). diff --git a/synapse/types/rest/client/__init__.py b/synapse/types/rest/client/__init__.py index 17647e621d..dbd4455b82 100644 --- a/synapse/types/rest/client/__init__.py +++ b/synapse/types/rest/client/__init__.py @@ -62,7 +62,7 @@ class AuthenticationData(RequestBodyModel): ClientSecretStr = Annotated[ str, StringConstraints( - pattern="[0-9a-zA-Z.=_-]", + pattern="^[0-9a-zA-Z.=_-]+$", min_length=1, max_length=255, strict=True, diff --git a/tests/rest/client/test_models.py b/tests/rest/client/test_models.py index f297856830..e5dc922023 100644 --- a/tests/rest/client/test_models.py +++ b/tests/rest/client/test_models.py @@ -23,7 +23,7 @@ from typing import Literal from pydantic import BaseModel, ValidationError -from synapse.types.rest.client import EmailRequestTokenBody +from synapse.types.rest.client import ClientSecretStr, EmailRequestTokenBody class ThreepidMediumEnumTestCase(stdlib_unittest.TestCase): @@ -48,6 +48,44 @@ class ThreepidMediumEnumTestCase(stdlib_unittest.TestCase): self.Model.model_validate({"medium": 123}) +class ClientSecretStrTestCase(stdlib_unittest.TestCase): + class Model(BaseModel): + client_secret: ClientSecretStr + + def test_accepts_valid_client_secrets(self) -> None: + """Secrets consisting entirely of `[0-9a-zA-Z.=_-]` are accepted.""" + for client_secret in ( + "this.is-a_valid=secret", + "foobar", + "a", + "0123456789", + "a" * 255, + ): + with self.subTest(client_secret=client_secret): + model = self.Model.model_validate({"client_secret": client_secret}) + self.assertEqual(model.client_secret, client_secret) + + def test_rejects_client_secrets_with_invalid_characters(self) -> None: + for client_secret in ( + "foo bar", + "secret!", + "café", + # Little bobby tables + "Robert'; DROP TABLE students;--", + ): + with self.subTest(client_secret=client_secret): + with self.assertRaises(ValidationError): + self.Model.model_validate({"client_secret": client_secret}) + + def test_rejects_empty_client_secret(self) -> None: + with self.assertRaises(ValidationError): + self.Model.model_validate({"client_secret": ""}) + + def test_rejects_overlong_client_secret(self) -> None: + with self.assertRaises(ValidationError): + self.Model.model_validate({"client_secret": "a" * 256}) + + class EmailRequestTokenBodyTestCase(stdlib_unittest.TestCase): base_request = { "client_secret": "hunter2",