mirror of
https://github.com/element-hq/synapse.git
synced 2026-09-14 02:16:29 +00:00
Fix client_secret param validator (#20104)
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
<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->
* [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 <quenting@element.io>
This commit is contained in:
co-authored by
Quentin Gliech
parent
470b941895
commit
e35c8f0eff
@@ -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).
|
||||
@@ -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,
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user