Skip to content

Commit f47fd9a

Browse files
fix!: enforce OAuth state validation by default
`SSOBase.requires_state` defaulted to `False`, so the state validation added in 0.19.0 never ran unless an integrator opted in. A callback carrying only a `code` was accepted with nothing bound to the victim's session, allowing login CSRF (CWE-352, CWE-1188). `requires_state` now defaults to `True`, and a `state` present in the callback is always matched against the `sso_state` cookie regardless of the flag, so a state without a binding can no longer pass. The cookie is set `HttpOnly`, `SameSite=lax` and `Secure` unless `allow_insecure_http` is enabled, and the comparison is constant-time. Reported by mohammad adnan (cystack.ps redteam) in GHSA-wgrh-7h2j-rg46. BREAKING CHANGE: login flows that do not carry the `sso_state` cookie to the callback now fail with `401 State cookie not found`. Set `requires_state` to `False` on the instance to opt out, at the cost of CSRF protection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vc7GVDzhiJKacdr8cezuj
1 parent 60838d6 commit f47fd9a

8 files changed

Lines changed: 142 additions & 11 deletions

File tree

README.md

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,31 @@ Quick links for the eager ones:
3838

3939
## Security Notice
4040

41+
### Version `0.22.0` Update: OAuth `state` Validation Is Now Enabled By Default
42+
43+
The `state` validation added in `0.19.0` was gated behind `SSOBase.requires_state`, which defaulted to `False`.
44+
Applications that did not opt in performed no CSRF validation on the OAuth callback and remained vulnerable to
45+
login CSRF. This was reported by [@mohammedix88](https://github.com/mohammedix88) (cystack.ps redteam)
46+
in [GHSA-wgrh-7h2j-rg46](https://github.com/tomasvotava/fastapi-sso/security/advisories/GHSA-wgrh-7h2j-rg46).
47+
48+
Since `0.22.0`, `requires_state` defaults to `True` and a `state` received in the callback is always matched against
49+
the `sso_state` cookie set at login time.
50+
51+
**This is a breaking change.** A login flow that does not carry the `sso_state` cookie back to the callback now fails
52+
with `401 State cookie not found`. Two cases are affected:
53+
54+
- The SSO instance is not used as a context manager (`async with sso:`), so no state is generated. This usage already
55+
emitted a `SecurityWarning` and is now rejected at the callback.
56+
- The login and callback endpoints are served from different hosts, so the browser does not return the cookie.
57+
58+
If you cannot carry the cookie across your deployment, you can opt out per instance, at the cost of losing CSRF
59+
protection:
60+
61+
```python
62+
sso = GoogleSSO(client_id, client_secret, redirect_uri)
63+
sso.requires_state = False
64+
```
65+
4166
### Version `0.19.0` Update: OAuth `state` Validation Fix
4267

4368
A critical OAuth login CSRF vulnerability caused by missing `state` validation was

docs/how-to-guides/state-return-url.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@
99
cryptographically random, stored server-side, and verified when the provider
1010
redirects the user back.
1111

12-
If you do **not** pass a `state` explicitly, `fastapi-sso` will generate, store,
13-
and validate a secure random state for you.
12+
If you do **not** pass a `state` explicitly, `fastapi-sso` generates a secure random
13+
state for you, sets it as the `sso_state` cookie on the login redirect, and matches the
14+
two when the provider calls back. This requires the SSO instance to be used as a context
15+
manager (`async with sso:`) and the cookie to reach your callback endpoint.
1416

1517
Using `state` to carry arbitrary user-controlled data (such as return URLs)
1618
**without validation** is unsafe and can lead to critical vulnerabilities

fastapi_sso/sso/base.py

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import json
55
import logging
66
import os
7+
import secrets
78
import warnings
89
from collections.abc import Callable
910
from types import TracebackType
@@ -96,7 +97,7 @@ class SSOBase:
9697
scope: ClassVar[list[str]] = []
9798
additional_headers: ClassVar[dict[str, Any] | None] = None
9899
uses_pkce: bool = False
99-
requires_state: bool = False
100+
requires_state: bool = True
100101
use_id_token_for_user_info: ClassVar[bool] = False
101102
use_basic_auth: ClassVar[bool] = True
102103

@@ -336,7 +337,13 @@ async def get_login_redirect(
336337
if self.uses_pkce:
337338
response.set_cookie("pkce_code_verifier", str(self._pkce_code_verifier))
338339
if state is not None:
339-
response.set_cookie("sso_state", state)
340+
response.set_cookie(
341+
"sso_state",
342+
state,
343+
httponly=True,
344+
samesite="lax",
345+
secure=not self.allow_insecure_http,
346+
)
340347
return response
341348

342349
@overload
@@ -409,9 +416,9 @@ async def verify_and_process(
409416
raise SSOLoginError(400, "'state' parameter was not found in callback request")
410417
if self._state is not None:
411418
sso_state = request.cookies.get("sso_state")
412-
if sso_state is None and self.requires_state:
419+
if sso_state is None:
413420
raise SSOLoginError(401, "State cookie not found")
414-
if sso_state is not None and sso_state != self._state:
421+
if not secrets.compare_digest(sso_state.encode(), self._state.encode()):
415422
raise SSOLoginError(401, "Invalid state")
416423
pkce_code_verifier: str | None = None
417424
if self.uses_pkce:

fastapi_sso/sso/twitter.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ class TwitterSSO(SSOBase):
1414
provider = "twitter"
1515
scope: ClassVar = ["users.read", "tweet.read"]
1616
uses_pkce = True
17-
requires_state = True
1817

1918
async def get_discovery_document(self) -> DiscoveryDocument:
2019
return {

tests/test_base.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,9 @@ async def test_not_implemented_ssobase(self):
4848
with pytest.raises(NotImplementedError):
4949
await sso.openid_from_token({})
5050

51-
request = Request()
51+
request = Request(cookies={"sso_state": "state"})
5252
request.query_params["code"] = "code"
53+
request.query_params["state"] = "state"
5354
with pytest.raises(NotImplementedError), pytest.warns(
5455
SecurityWarning, match="Please make sure you are using SSO provider in an async context"
5556
):
@@ -76,7 +77,7 @@ class PostRequest:
7677
method = "POST"
7778
query_params = {}
7879
headers = {}
79-
cookies = {}
80+
cookies = {"sso_state": "state-from-form"}
8081
url = "http://localhost/auth/callback"
8182

8283
@staticmethod

tests/test_providers_individual.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ class PostRequest:
8282
method = "POST"
8383
query_params = {}
8484
headers = {}
85-
cookies = {}
85+
cookies = {"sso_state": "state"}
8686
url = URL("https://localhost/auth/callback")
8787

8888
@staticmethod

tests/test_state_validation.py

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
# type: ignore
2+
3+
import pytest
4+
from utils import Request
5+
6+
from fastapi_sso.sso.base import DiscoveryDocument, OpenID, SSOBase, SSOLoginError
7+
from fastapi_sso.sso.google import GoogleSSO
8+
9+
10+
class FakeSSO(SSOBase):
11+
provider = "fake"
12+
13+
async def get_discovery_document(self) -> DiscoveryDocument:
14+
return {
15+
"authorization_endpoint": "https://fake.com/authorize",
16+
"token_endpoint": "https://fake.com/token",
17+
"userinfo_endpoint": "https://fake.com/userinfo",
18+
}
19+
20+
async def openid_from_response(self, response: dict, session=None) -> OpenID:
21+
return OpenID(id="fake-id", provider=self.provider)
22+
23+
24+
@pytest.fixture()
25+
def sso(monkeypatch: pytest.MonkeyPatch) -> FakeSSO:
26+
async def fake_process_login(self, code, request, **kwargs):
27+
return "logged-in"
28+
29+
monkeypatch.setattr(SSOBase, "process_login", fake_process_login)
30+
return FakeSSO("client_id", "client_secret", redirect_uri="https://localhost/callback")
31+
32+
33+
def callback(state: str | None = None, cookie: str | None = None) -> Request:
34+
request = Request(cookies={"sso_state": cookie} if cookie is not None else None)
35+
request.query_params["code"] = "code"
36+
if state is not None:
37+
request.query_params["state"] = state
38+
return request
39+
40+
41+
@pytest.mark.parametrize("provider", [SSOBase, GoogleSSO])
42+
def test_state_is_required_by_default(provider: type[SSOBase]):
43+
assert provider("client_id", "client_secret").requires_state is True
44+
45+
46+
async def test_callback_without_state_is_rejected(sso: FakeSSO):
47+
async with sso:
48+
with pytest.raises(SSOLoginError, match="'state' parameter was not found"):
49+
await sso.verify_and_process(callback())
50+
51+
52+
async def test_callback_with_state_but_no_cookie_is_rejected(sso: FakeSSO):
53+
async with sso:
54+
with pytest.raises(SSOLoginError, match="State cookie not found"):
55+
await sso.verify_and_process(callback(state="attacker-state"))
56+
57+
58+
async def test_callback_with_mismatched_cookie_is_rejected(sso: FakeSSO):
59+
async with sso:
60+
with pytest.raises(SSOLoginError, match="Invalid state"):
61+
await sso.verify_and_process(callback(state="attacker-state", cookie="victim-state"))
62+
63+
64+
async def test_callback_with_non_ascii_state_is_rejected(sso: FakeSSO):
65+
async with sso:
66+
with pytest.raises(SSOLoginError, match="Invalid state"):
67+
await sso.verify_and_process(callback(state="státe", cookie="state"))
68+
69+
70+
async def test_callback_with_matching_cookie_is_accepted(sso: FakeSSO):
71+
async with sso:
72+
assert await sso.verify_and_process(callback(state="state", cookie="state")) == "logged-in"
73+
74+
75+
async def test_state_validation_can_be_opted_out_of(sso: FakeSSO):
76+
sso.requires_state = False
77+
async with sso:
78+
assert await sso.verify_and_process(callback()) == "logged-in"
79+
80+
81+
async def test_login_redirect_sets_hardened_state_cookie(sso: FakeSSO):
82+
async with sso:
83+
response = await sso.get_login_redirect()
84+
cookie = response.headers["set-cookie"]
85+
assert f"sso_state={sso._generated_state}" in cookie
86+
assert "HttpOnly" in cookie
87+
assert "Secure" in cookie
88+
assert "SameSite=lax" in cookie
89+
90+
91+
async def test_state_cookie_is_not_secure_over_insecure_http(monkeypatch: pytest.MonkeyPatch):
92+
monkeypatch.delenv("OAUTHLIB_INSECURE_TRANSPORT", raising=False)
93+
sso = FakeSSO("client_id", "client_secret", redirect_uri="http://localhost/callback", allow_insecure_http=True)
94+
async with sso:
95+
response = await sso.get_login_redirect()
96+
assert "Secure" not in response.headers["set-cookie"]

tests/utils.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,11 @@
22

33

44
class Request:
5-
def __init__(self, url="http://localhost", query_params=None):
5+
def __init__(self, url="http://localhost", query_params=None, cookies=None):
66
self.url = URL(url)
77
self.query_params = query_params or {}
88
self.headers = {}
9+
self.cookies = cookies or {}
910

1011

1112
class Response:

0 commit comments

Comments
 (0)