From b0e6db743a9816057e3873896935de68120f1fc5 Mon Sep 17 00:00:00 2001 From: allen0099 Date: Sat, 26 Sep 2026 21:28:10 +0000 Subject: [PATCH] fix(state): bind OAuth states to the browser that started the flow Any stored state completed the flow in any browser, so an attacker could start a flow and send the victim to the callback with the attacker's state and code (login CSRF), although STATE.md described the states as CSRF protection. create_state(binding=...) stores the SHA-256 of a client secret, such as a nonce also set as a cookie, and consume_state(state, binding=...) raises InvalidStateError unless the same value is given. A bound state consumed without a binding, or an unbound one consumed with a binding, is rejected too, and the state is consumed either way. The STATE.md quick start now sets and checks a binding cookie. Closes #226 --- CHANGELOG.md | 8 ++++ CLAUDE.md | 1 + docs/STATE.md | 60 ++++++++++++++++++++++------ fastapi_cachex/state/manager.py | 50 +++++++++++++++++++++-- fastapi_cachex/state/models.py | 7 ++++ i18n/zh-TW/docs/STATE.md | 43 +++++++++++++++----- tests/state/test_login_csrf.py | 71 +++++++++++++++++++++++++++++++++ tests/state/test_manager.py | 64 +++++++++++++++++++++++++++++ 8 files changed, 278 insertions(+), 26 deletions(-) create mode 100644 tests/state/test_login_csrf.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8efc7d6..54b84fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -194,6 +194,14 @@ Note that 0.3.3 was never released; 0.3.4 follows 0.3.2. answered `401` to new visitors; it now uses the helper, and the migration section warns about the difference from Starlette. ([#225](https://github.com/allen0099/FastAPI-CacheX/issues/225)) +- **OAuth states can be bound to the browser that started the flow.** + `create_state(binding=...)` stores the SHA-256 of a client secret, such as a + nonce also set as a cookie, and `consume_state(state, binding=...)` rejects + the state with `InvalidStateError` unless the same value is given. Without a + binding any stored state completes the flow in any browser, which allowed + login CSRF although STATE.md described the states as CSRF protection. The + quick start now sets and checks a binding cookie. + ([#226](https://github.com/allen0099/FastAPI-CacheX/issues/226)) ## [0.3.7] - 2026-09-25 diff --git a/CLAUDE.md b/CLAUDE.md index 21557f6..cadd4e3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -75,6 +75,7 @@ The library has four independent subsystems: **4. State Management (`fastapi_cachex/state/`)** - `StateManager` provides one-time-use state tokens for OAuth flows. States are consumed (deleted) on first successful `consume_state()` call. - Uses the same cache backends, with key prefix `oauth_state:` by default. +- `create_state(binding=...)` / `consume_state(state, binding=...)` bind a state to the client that started the flow (SHA-256 stored, `hmac.compare_digest`); a mismatch in either direction raises `InvalidStateError`, and the state is consumed either way. ### Backends (`fastapi_cachex/backends/`) diff --git a/docs/STATE.md b/docs/STATE.md index 98dc770..fa8ac79 100644 --- a/docs/STATE.md +++ b/docs/STATE.md @@ -1,9 +1,16 @@ # State Management Extension -`fastapi_cachex.state` provides **one-time state tokens** that protect OAuth / OIDC -authorization flows against CSRF. Before starting the authorization, generate a random -state and store it in the cache backend. When the callback comes back, **consume** it. -A consumed state cannot be used a second time. +`fastapi_cachex.state` provides **one-time state tokens** for OAuth / OIDC authorization +flows. Before starting the authorization, generate a random state and store it in the cache +backend. When the callback comes back, **consume** it. A consumed state cannot be used a +second time. + +A state protects the flow against CSRF (RFC 6749 §10.12) only when it is **bound to the +browser that started the flow**. Storage alone is not enough: an attacker can start a flow +in their own browser and send the victim to the callback with the attacker's state and +code, which logs the victim in to the attacker's account. Pass a `binding` (a random nonce +you also set as a cookie) when creating the state and the same value when consuming it, as +in the quick start below. States live on the same backend as the HTTP cache but under their own key prefix (`oauth_state:` by default), so namespaced operations such as `CacheManager.clear_prefix()` @@ -15,7 +22,9 @@ Everything in this guide can also be imported from the top-level `fastapi_cachex ## Quick start ```python -from fastapi import FastAPI, HTTPException +import secrets + +from fastapi import FastAPI, HTTPException, Request from fastapi.responses import RedirectResponse from fastapi_cachex import BackendProxy @@ -25,26 +34,44 @@ from fastapi_cachex.state import StateError, StateManagerDep app = FastAPI() BackendProxy.set(MemoryBackend()) +BINDING_COOKIE = "oauth_binding" + @app.get("/login") async def login(states: StateManagerDep): - state = await states.create_state(metadata={"next": "/dashboard"}) - return RedirectResponse( + nonce = secrets.token_urlsafe(32) + state = await states.create_state(binding=nonce, metadata={"next": "/dashboard"}) + response = RedirectResponse( f"https://provider.example.com/authorize?state={state}&client_id=..." ) + # Lax, not Strict: the callback is a cross-site navigation from the provider. + response.set_cookie( + BINDING_COOKIE, nonce, max_age=600, httponly=True, secure=True, samesite="lax" + ) + return response @app.get("/callback") -async def callback(state: str, code: str, states: StateManagerDep): +async def callback(request: Request, state: str, code: str, states: StateManagerDep): try: - data = await states.consume_state(state) # one-time: deleted on retrieval - except StateError as e: # unknown, expired or malformed state + # One-time: deleted on retrieval. Rejected unless this browser started the flow. + data = await states.consume_state( + state, binding=request.cookies.get(BINDING_COOKIE) + ) + except StateError as e: # unknown, expired, malformed or issued to another browser raise HTTPException(status_code=400, detail="Invalid state") from e # Exchange the code for tokens, create a session ... - return {"next": data.metadata.get("next", "/")} + response = RedirectResponse(data.metadata.get("next", "/")) + response.delete_cookie(BINDING_COOKIE) + return response ``` +If the provider posts the callback (`response_mode=form_post`), a `SameSite=Lax` cookie is +not sent with that cross-site POST; use `samesite="none"` (which requires `secure=True`) +for the binding cookie. Starting a second login in another tab overwrites the cookie, so +the first tab's callback is then rejected; the user simply logs in again. + ## StateManager ```python @@ -61,7 +88,7 @@ With `backend=None` the backend is resolved **when the `StateManager` is constru not on each call. If `BackendProxy.set(...)` has not been called yet, the constructor raises `BackendNotFoundError`. Configure the backend first. -### `create_state(ttl=None, metadata=None) -> str` +### `create_state(ttl=None, metadata=None, *, binding=None) -> str` Generates a state string with `secrets.token_urlsafe(32)` (256 bits of entropy), stores it in the backend and returns it. `metadata` is an arbitrary JSON-serializable dict stored @@ -69,7 +96,12 @@ alongside the state (for example, the path to redirect to after authorization). is omitted, `default_ttl` is used. The same TTL is applied both as the backend TTL and as the state's `expires_at`. -### `consume_state(state) -> StateData` +`binding` ties the state to the client that starts the flow: a random nonce you also set +as a cookie, or any other secret only that client presents on the callback. Only its +SHA-256 is stored. An empty string raises `ValueError`, since a missing cookie read as `""` +would bind every such client to the same value. + +### `consume_state(state, *, binding=None) -> StateData` **One-time consumption.** The entry is retrieved and removed with the backend's atomic `get_and_delete()`, so when several concurrent calls present the same state **only one** @@ -78,6 +110,7 @@ gets it. A replayed callback cannot pass a second time. | Situation | Behavior | |------|------| | Missing, already consumed, or already evicted by the backend TTL | `InvalidStateError` | +| Created with a binding and consumed with a different one or none, or created without a binding and consumed with one | `InvalidStateError` (the entry has been deleted too) | | Retrieved but past its `expires_at` | `StateExpiredError` (the entry has been deleted too, nothing is left behind) | | Retrieved but the content is not valid `StateData` JSON | `StateDataError` (the entry has been deleted too) | | Otherwise | Returns `StateData` | @@ -116,6 +149,7 @@ class StateData(BaseModel): created_at: datetime # creation time (UTC) expires_at: datetime # expiry time (UTC) metadata: dict[str, Any] # metadata attached at creation + binding_hash: str | None # SHA-256 of the binding, None for an unbound state ``` `expires_at` is a logical expiry stored inside the data, independent of the backend TTL. diff --git a/fastapi_cachex/state/manager.py b/fastapi_cachex/state/manager.py index 278dd6c..cd0a380 100644 --- a/fastapi_cachex/state/manager.py +++ b/fastapi_cachex/state/manager.py @@ -1,6 +1,7 @@ """State manager for OAuth and session state handling.""" import hashlib +import hmac import json import logging import secrets @@ -38,6 +39,21 @@ def _state_ref(state: str) -> str: return hashlib.sha256(state.encode("utf-8", "surrogatepass")).hexdigest()[:12] +def _binding_hash(binding: str) -> str: + return hashlib.sha256(binding.encode("utf-8", "surrogatepass")).hexdigest() + + +def _binding_matches(stored: str | None, binding: str | None) -> bool: + """Whether ``binding`` is the one a state was created with. + + An unbound state matches only an absent binding and a bound one only its own, + so neither side can drop the check by leaving its binding out. + """ + if stored is None or binding is None: + return stored is None and binding is None + return hmac.compare_digest(stored, _binding_hash(binding)) + + def _log_decode_failure(state: str) -> None: # No traceback: pydantic validation errors echo the stored input, which # includes the state itself. @@ -131,18 +147,25 @@ async def create_state( self, ttl: int | None = None, metadata: dict[str, Any] | None = None, + *, + binding: str | None = None, ) -> str: """Create a new random OAuth state and store it with metadata. Args: ttl: Time-to-live in seconds (uses default_ttl if not provided) metadata: Additional metadata to store with the state (e.g., callback_url, user_info) + binding: A secret tied to the client that starts the flow, such as a + random nonce also set as a cookie. ``consume_state()`` then + accepts the state only with the same binding, so a state issued + to one browser cannot complete the flow in another (login + CSRF). Only its SHA-256 is stored. Returns: The generated state string Raises: - ValueError: If ``ttl`` is zero or negative. + ValueError: If ``ttl`` is zero or negative, or ``binding`` is empty. Backend errors (for example a Redis connection error) propagate unchanged; they are not wrapped in ``StateDataError``. @@ -153,12 +176,18 @@ async def create_state( # Use provided TTL or default effective_ttl = ttl if ttl is not None else self.default_ttl validate_ttl(effective_ttl) + if binding == "": + # A missing cookie read as "" would otherwise bind every such + # client to the same value. + msg = "binding must not be empty" + raise ValueError(msg) # Create state data model state_data = StateData( state=state, expires_at=datetime.now(timezone.utc) + timedelta(seconds=effective_ttl), metadata=metadata or {}, + binding_hash=None if binding is None else _binding_hash(binding), ) content = state_data.model_dump_json().encode("utf-8") @@ -172,17 +201,24 @@ async def create_state( ) return state - async def consume_state(self, state: str) -> StateData: + async def consume_state( + self, state: str, *, binding: str | None = None + ) -> StateData: """Consume and validate an OAuth state, removing it from storage. Args: state: The state string to validate and consume + binding: The binding the state was created with, if any. A state + created with a binding is accepted only with that binding, and + one created without is rejected when a binding is given. The + state is consumed either way. Returns: StateData object containing state data and metadata Raises: - InvalidStateError: If state is invalid or not found + InvalidStateError: If state is invalid or not found, or the binding + does not match StateExpiredError: If state has expired StateDataError: If state data format is invalid """ @@ -209,6 +245,14 @@ async def consume_state(self, state: str) -> StateData: msg = "State has expired" raise StateExpiredError(msg) + if not _binding_matches(state_data.binding_hash, binding): + logger.info( + "OAuth state presented with a different binding; state_ref=%s", + _state_ref(state), + ) + msg = "State was issued to a different client" + raise InvalidStateError(msg) + logger.debug( "OAuth state consumed and deleted; state_ref=%s", _state_ref(state) ) diff --git a/fastapi_cachex/state/models.py b/fastapi_cachex/state/models.py index d260051..0d5e5ec 100644 --- a/fastapi_cachex/state/models.py +++ b/fastapi_cachex/state/models.py @@ -24,6 +24,13 @@ class StateData(BaseModel): metadata: dict[str, Any] = Field( default_factory=dict, description="Additional metadata associated with state" ) + binding_hash: str | None = Field( + default=None, + description=( + "SHA-256 of the binding the state was created with, or None for an " + "unbound state" + ), + ) @field_serializer("created_at", "expires_at", when_used="json") def serialize_datetime(self, value: datetime) -> str: diff --git a/i18n/zh-TW/docs/STATE.md b/i18n/zh-TW/docs/STATE.md index 91b93e6..23be6f5 100644 --- a/i18n/zh-TW/docs/STATE.md +++ b/i18n/zh-TW/docs/STATE.md @@ -1,6 +1,8 @@ # State 管理擴充 {#state-management-extension} -`fastapi_cachex.state` 提供**一次性 state 權杖**，保護 OAuth / OIDC 授權流程免於 CSRF 攻擊。開始授權之前，先產生一個隨機 state 並存入快取後端；回呼（callback）回來時，再將它**消耗**掉。已消耗的 state 無法再使用第二次。 +`fastapi_cachex.state` 為 OAuth / OIDC 授權流程提供**一次性 state 權杖**。開始授權之前，先產生一個隨機 state 並存入快取後端；回呼（callback）回來時，再將它**消耗**掉。已消耗的 state 無法再使用第二次。 + +只有當 state **綁定到發起流程的瀏覽器**時，它才能保護流程免於 CSRF 攻擊（RFC 6749 §10.12）。光是儲存並不夠：攻擊者可以在自己的瀏覽器中發起流程，再把受害者導向帶有攻擊者 state 與 code 的回呼，使受害者登入攻擊者的帳號。請在建立 state 時傳入 `binding`（一個同時設為 Cookie 的隨機 nonce），並在消耗時傳入相同的值，如下方快速開始所示。 State 與 HTTP 快取存放在同一個後端，但使用自己的鍵前綴（預設為 `oauth_state:`），因此像 `CacheManager.clear_prefix()` 這類依命名空間的操作不會動到它們。不過後端層級的 `clear()`（例如 `BackendProxy.get().clear()`）會移除它們，因為它會清除後端命名空間底下的所有內容。 @@ -9,7 +11,9 @@ State 與 HTTP 快取存放在同一個後端，但使用自己的鍵前綴（ ## 快速開始 {#quick-start} ```python -from fastapi import FastAPI, HTTPException +import secrets + +from fastapi import FastAPI, HTTPException, Request from fastapi.responses import RedirectResponse from fastapi_cachex import BackendProxy @@ -19,26 +23,41 @@ from fastapi_cachex.state import StateError, StateManagerDep app = FastAPI() BackendProxy.set(MemoryBackend()) +BINDING_COOKIE = "oauth_binding" + @app.get("/login") async def login(states: StateManagerDep): - state = await states.create_state(metadata={"next": "/dashboard"}) - return RedirectResponse( + nonce = secrets.token_urlsafe(32) + state = await states.create_state(binding=nonce, metadata={"next": "/dashboard"}) + response = RedirectResponse( f"https://provider.example.com/authorize?state={state}&client_id=..." ) + # 使用 Lax 而非 Strict：回呼是從提供者發起的跨站導覽。 + response.set_cookie( + BINDING_COOKIE, nonce, max_age=600, httponly=True, secure=True, samesite="lax" + ) + return response @app.get("/callback") -async def callback(state: str, code: str, states: StateManagerDep): +async def callback(request: Request, state: str, code: str, states: StateManagerDep): try: - data = await states.consume_state(state) # 一次性：取出時即刪除 - except StateError as e: # 未知、已過期或格式錯誤的 state + # 一次性：取出時即刪除。除非是這個瀏覽器發起的流程，否則會被拒絕。 + data = await states.consume_state( + state, binding=request.cookies.get(BINDING_COOKIE) + ) + except StateError as e: # 未知、已過期、格式錯誤或發給其他瀏覽器的 state raise HTTPException(status_code=400, detail="Invalid state") from e # 以 code 換取權杖、建立 Session…… - return {"next": data.metadata.get("next", "/")} + response = RedirectResponse(data.metadata.get("next", "/")) + response.delete_cookie(BINDING_COOKIE) + return response ``` +若提供者以 POST 送出回呼（`response_mode=form_post`），`SameSite=Lax` 的 Cookie 不會隨這個跨站 POST 送出；此時綁定用的 Cookie 請改用 `samesite="none"`（需要 `secure=True`）。在另一個分頁再次開始登入會覆寫這個 Cookie，因此第一個分頁的回呼會被拒絕；使用者只要重新登入即可。 + ## StateManager {#statemanager} ```python @@ -53,17 +72,20 @@ states = StateManager( 當 `backend=None` 時，後端是在**建構 `StateManager` 時**解析，而不是每次呼叫時才解析。若尚未呼叫 `BackendProxy.set(...)`，建構子會拋出 `BackendNotFoundError`。請先設定後端。 -### `create_state(ttl=None, metadata=None) -> str` {#create_statettlnone-metadatanone-str} +### `create_state(ttl=None, metadata=None, *, binding=None) -> str` {#create_statettlnone-metadatanone-bindingnone-str} 以 `secrets.token_urlsafe(32)`（256 位元熵）產生 state 字串，存入後端並回傳。`metadata` 是與 state 一併儲存的任意可 JSON 序列化 dict（例如授權完成後要重新導向的路徑）。省略 `ttl` 時使用 `default_ttl`。同一個 TTL 會同時作為後端 TTL 與 state 的 `expires_at`。 -### `consume_state(state) -> StateData` {#consume_statestate-statedata} +`binding` 將 state 綁定到發起流程的用戶端：一個同時設為 Cookie 的隨機 nonce，或任何只有該用戶端會在回呼時提交的秘密值。後端只會儲存它的 SHA-256。空字串會拋出 `ValueError`，因為把缺少的 Cookie 讀成 `""` 時，所有這類用戶端都會綁定到同一個值。 + +### `consume_state(state, *, binding=None) -> StateData` {#consume_statestate-bindingnone-statedata} **一次性消耗。** 項目透過後端的原子操作 `get_and_delete()` 取出並移除，因此當多個並行呼叫提交同一個 state 時，**只有一個**會取得它。重送的回呼無法通過第二次。 | 情況 | 行為 | |------|------| | 不存在、已被消耗，或已因後端 TTL 而被淘汰 | `InvalidStateError` | +| 建立時有綁定、消耗時綁定不同或未提供；或建立時沒有綁定、消耗時卻提供了綁定 | `InvalidStateError`（項目也已被刪除） | | 已取出但超過其 `expires_at` | `StateExpiredError`（項目也已被刪除，不會殘留） | | 已取出但內容不是有效的 `StateData` JSON | `StateDataError`（項目也已被刪除） | | 其他情況 | 回傳 `StateData` | @@ -93,6 +115,7 @@ class StateData(BaseModel): created_at: datetime # 建立時間（UTC） expires_at: datetime # 過期時間（UTC） metadata: dict[str, Any] # 建立時附加的 metadata + binding_hash: str | None # binding 的 SHA-256；未綁定的 state 為 None ``` `expires_at` 是儲存在資料內的邏輯過期時間，與後端 TTL 無關。後端 TTL 到期時，項目就會消失；`expires_at` 則確保後端仍保留、但在邏輯上已過期的項目同樣會被拒絕。 diff --git a/tests/state/test_login_csrf.py b/tests/state/test_login_csrf.py new file mode 100644 index 0000000..f5252f0 --- /dev/null +++ b/tests/state/test_login_csrf.py @@ -0,0 +1,71 @@ +"""The STATE.md quick start: a state bound to a nonce cookie (#226).""" + +import secrets +from urllib.parse import parse_qs +from urllib.parse import urlparse + +from fastapi import FastAPI +from fastapi import HTTPException +from fastapi import Request +from fastapi.responses import RedirectResponse +from fastapi.testclient import TestClient + +from fastapi_cachex.backends.memory import MemoryBackend +from fastapi_cachex.state import StateError +from fastapi_cachex.state import StateManager + +COOKIE = "oauth_binding" + + +def _app() -> FastAPI: + app = FastAPI() + states = StateManager(backend=MemoryBackend()) + + @app.get("/login") + async def login(): + nonce = secrets.token_urlsafe(32) + state = await states.create_state(binding=nonce) + response = RedirectResponse( + f"https://provider.example.com/authorize?state={state}" + ) + response.set_cookie(COOKIE, nonce, max_age=600, httponly=True, samesite="lax") + return response + + @app.get("/callback") + async def callback(request: Request, state: str, code: str): + try: + await states.consume_state(state, binding=request.cookies.get(COOKIE)) + except StateError as e: + raise HTTPException(status_code=400, detail="Invalid state") from e + return {"logged_in_with_code": code} + + return app + + +def _start(client: TestClient) -> str: + response = client.get("/login", follow_redirects=False) + location: str = response.headers["location"] + return parse_qs(urlparse(location).query)["state"][0] + + +def test_the_browser_that_started_the_flow_completes_it() -> None: + client = TestClient(_app()) + state = _start(client) + + response = client.get("/callback", params={"state": state, "code": "c"}) + + assert response.status_code == 200 + + +def test_a_state_started_by_an_attacker_is_rejected_in_the_victims_browser() -> None: + """The login CSRF from #226: before binding, the victim was logged in as the attacker.""" + app = _app() + attacker, victim = TestClient(app), TestClient(app) + state = _start(attacker) + _start(victim) # the victim has a nonce cookie of their own + + response = victim.get( + "/callback", params={"state": state, "code": "ATTACKERS_CODE"} + ) + + assert response.status_code == 400 diff --git a/tests/state/test_manager.py b/tests/state/test_manager.py index 6a375fe..c9e9d5a 100644 --- a/tests/state/test_manager.py +++ b/tests/state/test_manager.py @@ -914,3 +914,67 @@ async def test_malformed_state_data_is_logged_once_without_the_state( assert records[0].levelno == logging.WARNING assert records[0].exc_info is None assert state not in caplog.text + + +@pytest.mark.asyncio +async def test_bound_state_is_accepted_with_its_binding( + state_manager: StateManager, +) -> None: + """The client that started the flow completes it (#226).""" + state = await state_manager.create_state(binding="nonce-a") + + data = await state_manager.consume_state(state, binding="nonce-a") + + assert data.state == state + assert data.binding_hash == hashlib.sha256(b"nonce-a").hexdigest() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("binding", ["nonce-b", None]) +async def test_bound_state_is_rejected_for_another_client( + state_manager: StateManager, binding: str | None +) -> None: + """Login CSRF: a state issued to the attacker must not complete in the victim's browser (#226). + + The victim's callback carries the victim's binding (or none), not the + attacker's, and the state is consumed so it cannot be retried. + """ + state = await state_manager.create_state(binding="nonce-a") + + with pytest.raises(InvalidStateError, match="different client"): + await state_manager.consume_state(state, binding=binding) + + with pytest.raises(InvalidStateError): + await state_manager.consume_state(state, binding="nonce-a") + + +@pytest.mark.asyncio +async def test_unbound_state_is_rejected_with_a_binding( + state_manager: StateManager, +) -> None: + """A caller that checks bindings must not accept a state issued without one.""" + state = await state_manager.create_state() + + with pytest.raises(InvalidStateError, match="different client"): + await state_manager.consume_state(state, binding="nonce-a") + + +@pytest.mark.asyncio +async def test_empty_binding_is_rejected(state_manager: StateManager) -> None: + """An empty binding (a missing cookie read as "") would bind everyone alike.""" + with pytest.raises(ValueError, match="binding must not be empty"): + await state_manager.create_state(binding="") + + +@pytest.mark.asyncio +async def test_binding_is_not_stored_in_plain_text( + memory_backend: MemoryBackend, +) -> None: + """Only the SHA-256 of the binding reaches the backend.""" + manager = StateManager(backend=memory_backend) + state = await manager.create_state(binding="nonce-secret") + + entry = await memory_backend.get(f"{manager.key_prefix}{state}") + + assert entry is not None + assert b"nonce-secret" not in entry.content