mirror of
https://github.com/element-hq/synapse.git
synced 2026-09-14 02:16:29 +00:00
Paul ChobertandQuentin Gliech
e35c8f0eff
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>