Keep earlier consent CSRF tokens valid within a transaction (#4818)

Co-authored-by: nate nowack <thrast36@gmail.com>
This commit is contained in:
Trevor Hudson 2026-08-13 12:24:02 -07:00 committed by GitHub
commit 19d9360cc1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 553 additions and 33 deletions

View file

@ -20,7 +20,11 @@ from pydantic import AnyUrl
from starlette.requests import Request
from starlette.responses import HTMLResponse, RedirectResponse
from fastmcp.server.auth.oauth_proxy.models import ProxyDCRClient
from fastmcp.server.auth.oauth_proxy.models import (
ConsentCSRFToken,
ProxyDCRClient,
_hash_token,
)
from fastmcp.server.auth.oauth_proxy.ui import create_consent_html
from fastmcp.server.auth.redirect_validation import (
build_client_redirect,
@ -36,9 +40,39 @@ if TYPE_CHECKING:
# Keeps the Cookie header bounded to avoid hitting reverse proxy header limits.
_MAX_REMEMBERED_CLIENTS = 25
# Maximum number of consent-state cookies the browser carries at once. Each
# render of a consent page adds one, and a handful of renders is normal (a
# reload, a preload, an extension re-fetching the URL); the bound keeps the
# Cookie header from growing without limit across many pending flows.
#
# The matching server-side state is bounded by its own 15-minute TTL rather
# than by a count, because counting entries would mean reading them back and
# rewriting them — the read-modify-write that concurrent renders race on.
_MAX_CSRF_TOKENS = 10
# Base name of the consent-state cookie. One cookie is set per issued CSRF
# token (`MCP_CONSENT_STATE_<digest>`) rather than one list shared by all of
# them: two renders in flight at once both build their Set-Cookie from the same
# inbound Cookie header, so a shared list silently drops whichever entry was
# written first. Separate names never collide.
#
# The unsuffixed name is the pre-upgrade flat list. It is read, never written,
# so a consent page rendered before an upgrade can still be submitted after it.
_CONSENT_STATE_COOKIE_BASE = "MCP_CONSENT_STATE"
logger = get_logger(__name__)
def _consent_state_base_name(csrf_token: str) -> str:
"""Base cookie name carrying a single issued CSRF token.
The token is hashed rather than used directly so the raw token does not end
up in a cookie name, which is far more likely to be logged than its value.
"""
digest = hashlib.sha256(csrf_token.encode()).hexdigest()[:32]
return f"{_CONSENT_STATE_COOKIE_BASE}_{digest}"
class ConsentMixin:
"""Mixin class providing consent management functionality for OAuthProxy.
@ -179,6 +213,115 @@ class ConsentMixin:
path="/",
)
def _read_consent_state_cookies(
self: OAuthProxy, request: Request
) -> dict[str, tuple[str, float]]:
"""Per-token consent-state cookies the browser sent, by cookie name.
Returns {cookie_name: (txn_id, issued_at)} for every cookie whose
signature verifies. Unsigned, tampered, or unparsable cookies are
skipped rather than raising, the same way the other cookie readers here
treat them.
"""
prefix = self._cookie_name(f"{_CONSENT_STATE_COOKIE_BASE}_")
found: dict[str, tuple[str, float]] = {}
for name, raw in request.cookies.items():
if not name.startswith(prefix):
continue
payload = self._verify_cookie(raw)
if not payload:
logger.debug("Cookie signature verification failed for %s", name)
continue
try:
data = json.loads(base64.b64decode(payload.encode()).decode())
except Exception:
logger.debug("Failed to decode cookie %s; ignoring", name)
continue
if not isinstance(data, dict):
continue
txn_id = data.get("txn")
issued_at = data.get("iat")
if isinstance(txn_id, str) and isinstance(issued_at, int | float):
found[name] = (txn_id, float(issued_at))
return found
def _set_consent_state_cookie(
self: OAuthProxy,
response: HTMLResponse | RedirectResponse,
csrf_token: str,
txn_id: str,
issued_at: float,
) -> None:
"""Record that this browser received `csrf_token`, under its own name.
The cookie is what makes the double-submit check meaningful: the token
in the form has to match one this browser was actually handed. Writing
it under a name derived from the token keeps that property while making
the write independent of every other render's.
"""
payload = base64.b64encode(
json.dumps(
{"txn": txn_id, "iat": issued_at}, separators=(",", ":")
).encode()
).decode()
response.set_cookie(
self._cookie_name(_consent_state_base_name(csrf_token)),
self._sign_cookie(payload),
max_age=15 * 60,
secure=self._is_https,
httponly=True,
samesite="lax",
path="/",
)
def _clear_consent_state_for_transaction(
self: OAuthProxy,
request: Request,
response: HTMLResponse | RedirectResponse,
txn_id: str,
*,
include_legacy: bool,
) -> None:
"""Expire the consent state belonging to one completed transaction.
Only this transaction's cookies are removed. Another consent flow the
same browser has open keeps its own state, which a single shared list
had no way to express completing either flow wiped both.
"""
for name, (cookie_txn, _issued_at) in self._read_consent_state_cookies(
request
).items():
if hmac.compare_digest(cookie_txn, txn_id):
self._expire_cookie(response, name)
if include_legacy:
# The pre-upgrade cookie is a flat list with no transaction
# attached, so it can only be cleared wholesale. Reached only when
# the submitted token came from it, which means every flow sharing
# it was rendered before the upgrade too.
self._set_list_cookie(
response,
_CONSENT_STATE_COOKIE_BASE,
self._encode_list_cookie([]),
max_age=60,
)
def _expire_cookie(
self: OAuthProxy,
response: HTMLResponse | RedirectResponse,
name: str,
) -> None:
"""Expire one cookie by name, matching the attributes it was set with."""
response.set_cookie(
name,
"",
max_age=0,
secure=self._is_https,
httponly=True,
samesite="lax",
path="/",
)
def _read_consent_bindings(self: OAuthProxy, request: Request) -> dict[str, str]:
"""Read the consent binding map from the signed cookie.
@ -369,20 +512,31 @@ class ConsentMixin:
sec_fetch_site,
)
# Need consent: issue CSRF token and show HTML
# Need consent: issue CSRF token and show HTML.
#
# A transaction can be rendered more than once before it is submitted —
# a reload, a browser preload, an extension re-fetching the URL. Every
# render issues its own token, so that a token stays unique to the
# browser that received it and the double-submit cookie check keeps its
# meaning, and every token issued for the transaction stays valid until
# it expires. Dropping the earlier one kills the form the user is
# already looking at: they click Approve and get "Invalid or expired
# consent token" on a flow that never expired, with no way to recover.
#
# The token is stored under its own key rather than appended to a list
# on the transaction. Two renders in flight at once would both read the
# same transaction, each append their token, and the second write would
# drop the first — `AsyncKeyValue` has no compare-and-swap to prevent
# it. Independent keys make the writes commute, including across
# processes sharing one storage backend.
csrf_token = secrets.token_urlsafe(32)
csrf_expires_at = time.time() + 15 * 60
# Update transaction with CSRF token
txn_model.csrf_token = csrf_token
txn_model.csrf_expires_at = csrf_expires_at
await self._transaction_store.put(
key=txn_id, value=txn_model, ttl=15 * 60
) # Auto-expire after 15 minutes
# Update dict for use in HTML generation
txn["csrf_token"] = csrf_token
txn["csrf_expires_at"] = csrf_expires_at
issued_at = time.time()
csrf_expires_at = issued_at + 15 * 60
await self._consent_csrf_store.put(
key=_hash_token(csrf_token),
value=ConsentCSRFToken(txn_id=txn_id, expires_at=csrf_expires_at),
ttl=15 * 60, # Auto-expire after 15 minutes
)
# Load client to get client_name and CIMD info if available
client = await self.get_client(txn["client_id"])
@ -423,15 +577,19 @@ class ConsentMixin:
cimd_domain=cimd_domain,
)
response = create_secure_html_response(html)
# Merge new CSRF token with any existing ones (supports concurrent flows)
existing_tokens = self._decode_list_cookie(request, "MCP_CONSENT_STATE")
existing_tokens.append(csrf_token)
self._set_list_cookie(
response,
"MCP_CONSENT_STATE",
self._encode_list_cookie(existing_tokens),
max_age=15 * 60,
)
self._set_consent_state_cookie(response, csrf_token, txn_id, issued_at)
# Keep the browser's consent state bounded. The cookie just set always
# survives; the oldest of the rest are expired to make room. Eviction
# is by issued-at from the cookie itself, so it does not depend on any
# server-side bookkeeping that renders would have to share.
others = self._read_consent_state_cookies(request)
others.pop(self._cookie_name(_consent_state_base_name(csrf_token)), None)
surplus = len(others) + 1 - _MAX_CSRF_TOKENS
if surplus > 0:
by_age = sorted(others.items(), key=lambda item: item[1][1])
for name, _entry in by_age[:surplus]:
self._expire_cookie(response, name)
return response
async def _submit_consent(
@ -455,10 +613,34 @@ class ConsentMixin:
)
txn = txn_model.model_dump()
expected_csrf = txn.get("csrf_token")
expires_at = float(txn.get("csrf_expires_at") or 0)
if not expected_csrf or csrf_token != expected_csrf or time.time() > expires_at:
# Look the token up by its own key. A record proves the token was
# issued by a render of THIS transaction; nothing else can have written
# it, and a concurrent render cannot have removed it.
csrf_record = (
await self._consent_csrf_store.get(key=_hash_token(csrf_token))
if csrf_token
else None
)
if csrf_record is not None:
legacy_csrf = False
csrf_valid = (
hmac.compare_digest(csrf_record.txn_id, txn_id)
and time.time() <= csrf_record.expires_at
)
else:
# No record: either the token is bogus, or the consent page was
# rendered by a version that kept the token on the transaction.
# Honouring the old location keeps a flow that was already open
# during an upgrade submittable instead of failing at Approve.
stored = txn_model.csrf_token
csrf_valid = bool(csrf_token and stored) and (
hmac.compare_digest(stored or "", csrf_token)
and time.time() <= (txn_model.csrf_expires_at or 0)
)
legacy_csrf = csrf_valid
if not csrf_valid:
return create_secure_html_response(
"<h1>Error</h1><p>Invalid or expired consent token</p>", status_code=400
)
@ -467,8 +649,16 @@ class ConsentMixin:
# Without this, an attacker who knows their own tx_id/csrf_token can
# CSRF the victim's browser into approving consent, bypassing the
# consent binding cookie protection.
cookie_csrf_tokens = self._decode_list_cookie(request, "MCP_CONSENT_STATE")
if csrf_token not in cookie_csrf_tokens:
if legacy_csrf:
cookie_ok = csrf_token in self._decode_list_cookie(
request, _CONSENT_STATE_COOKIE_BASE
)
else:
entry = self._read_consent_state_cookies(request).get(
self._cookie_name(_consent_state_base_name(csrf_token))
)
cookie_ok = entry is not None and hmac.compare_digest(entry[0], txn_id)
if not cookie_ok:
logger.warning(
"CSRF double-submit check failed for transaction %s "
"(possible cross-site consent forgery)",
@ -509,9 +699,12 @@ class ConsentMixin:
max_age=365 * 24 * 3600,
)
# Clear CSRF cookie by setting empty short-lived value
self._set_list_cookie(
response, "MCP_CONSENT_STATE", self._encode_list_cookie([]), max_age=60
# Retire this transaction's consent state, both halves of it: the
# stored token so it cannot be replayed, and the cookies that
# carried it. Other pending flows are left alone.
await self._consent_csrf_store.delete(key=_hash_token(csrf_token))
self._clear_consent_state_for_transaction(
request, response, txn_id, include_legacy=legacy_csrf
)
self._set_consent_binding_cookie(request, response, txn_id, consent_token)
return response
@ -549,8 +742,9 @@ class ConsentMixin:
max_age=365 * 24 * 3600,
)
self._set_list_cookie(
response, "MCP_CONSENT_STATE", self._encode_list_cookie([]), max_age=60
await self._consent_csrf_store.delete(key=_hash_token(csrf_token))
self._clear_consent_state_for_transaction(
request, response, txn_id, include_legacy=legacy_csrf
)
return response

View file

@ -58,11 +58,32 @@ class OAuthTransaction(BaseModel):
created_at: float
resource: str | None = None
proxy_code_verifier: str | None = None
# Deprecated: consent CSRF tokens are now stored under their own keys (see
# ConsentCSRFToken) so that concurrent renders cannot overwrite each other.
# These two fields are only read, never written, and only to keep a consent
# flow that started before the upgrade submittable after it.
csrf_token: str | None = None
csrf_expires_at: float | None = None
consent_token: str | None = None
class ConsentCSRFToken(BaseModel):
"""One CSRF token issued for one render of the consent page.
Stored under a key derived from the token itself rather than on the
transaction. Every render of a consent page issues its own token, and two
renders can be in flight at once (a reload, a browser preload, an extension
re-fetching the URL). Appending to a list on the transaction loses one of
them whenever that happens: `AsyncKeyValue` has no compare-and-swap, so two
handlers read the same transaction, each append their own token, and the
second write drops the first. Giving each token its own key makes the
writes independent, which holds across processes sharing one backend.
"""
txn_id: str
expires_at: float
class ClientCode(BaseModel):
"""Client authorization code with PKCE and upstream tokens.

View file

@ -100,6 +100,7 @@ from fastmcp.server.auth.oauth_proxy.models import (
DEFAULT_REFRESH_TOKEN_EXPIRY_SECONDS,
HTTP_TIMEOUT_SECONDS,
ClientCode,
ConsentCSRFToken,
JTIMapping,
OAuthTransaction,
ProxyDCRClient,
@ -649,6 +650,19 @@ class OAuthProxy(OAuthProvider, ConsentMixin):
raise_on_validation_error=True,
)
# Consent CSRF tokens, keyed by a hash of the token rather than by
# transaction. Each render of a consent page writes its own key, so
# renders that overlap cannot overwrite one another the way appending
# to a list on the transaction would.
self._consent_csrf_store: PydanticAdapter[ConsentCSRFToken] = PydanticAdapter[
ConsentCSRFToken
](
key_value=self._client_storage,
pydantic_model=ConsentCSRFToken,
default_collection="mcp-consent-csrf-tokens",
raise_on_validation_error=True,
)
self._code_store: PydanticAdapter[ClientCode] = PydanticAdapter[ClientCode](
key_value=self._client_storage,
pydantic_model=ClientCode,

View file

@ -12,11 +12,13 @@ This test suite verifies:
9. Consent binding cookie prevents confused deputy attacks (GHSA-rww4-4w9c-7733)
"""
import asyncio
import re
import secrets
import time
from urllib.parse import parse_qs, urlparse
import httpx
import pytest
from key_value.aio.stores.memory import MemoryStore
from mcp.server.auth.provider import AuthorizationParams
@ -27,6 +29,10 @@ from starlette.testclient import TestClient
from fastmcp.server.auth.auth import AccessToken, TokenVerifier
from fastmcp.server.auth.oauth_proxy import OAuthProxy
from fastmcp.server.auth.oauth_proxy.consent import (
_CONSENT_STATE_COOKIE_BASE,
_MAX_CSRF_TOKENS,
)
from fastmcp.server.auth.oauth_proxy.models import OAuthTransaction
@ -167,6 +173,49 @@ def _extract_csrf(html: str) -> str | None:
return m.group(1) if m else None
def _consent_state_cookie_names(client: httpx.AsyncClient | TestClient) -> list[str]:
"""Names of the per-token consent-state cookies currently held."""
prefix = f"__Host-{_CONSENT_STATE_COOKIE_BASE}_"
return [c.name for c in client.cookies.jar if c.name.startswith(prefix)]
class _RenderGate:
"""Holds arrivals until `parties` of them are waiting, then releases all.
asyncio.Barrier would do this, but it is 3.11+ and fastmcp supports 3.10.
Single-threaded event loop, so the counter needs no lock.
"""
def __init__(self, parties: int) -> None:
self._parties = parties
self._arrived = 0
self._opened = asyncio.Event()
async def wait(self) -> None:
self._arrived += 1
if self._arrived >= self._parties:
self._opened.set()
await self._opened.wait()
def _gate_transaction_reads(proxy: OAuthProxy, gate: _RenderGate) -> None:
"""Hold every transaction read open until the gate releases.
Parks concurrent consent renders in the window where each has read the
transaction and none has stored its token yet. That interleaving is what
loses a token when tokens are appended to the transaction: both handlers
read the same value, both append, and the second write wins.
"""
original_get = proxy._transaction_store.get
async def gated_get(*args, **kwargs):
result = await original_get(*args, **kwargs)
await gate.wait()
return result
proxy._transaction_store.get = gated_get # ty: ignore[invalid-assignment]
class TestServerSideStorage:
"""Tests verifying OAuth state is stored in AsyncKeyValue storage."""
@ -537,6 +586,248 @@ class TestCSRFDoubleSubmit:
assert response.status_code == 403
class TestConcurrentConsentRenders:
"""A transaction can be rendered more than once before it is submitted."""
async def test_earlier_render_still_submittable(self, oauth_proxy_with_storage):
"""A second render must not invalidate the form from the first one.
A consent URL gets loaded twice more often than you would think: a
reload, a browser preload, an extension re-fetching it. Each render
issues a new CSRF token, and if that replaces the previous one, the
page the user is actually looking at is already dead when they click
Approve. They get "Invalid or expired consent token" on a transaction
that has not expired, and retrying does not help.
"""
txn_id, _ = await _start_flow(
oauth_proxy_with_storage,
"concurrent-render-client",
"http://localhost:9090/callback",
)
app = Starlette(routes=oauth_proxy_with_storage.get_routes())
# https base_url so the Secure/__Host- consent cookie is retained
# between requests, the way it is in a browser.
with TestClient(app, base_url="https://myserver.com") as test_client:
first = _extract_csrf(test_client.get(f"/consent?txn_id={txn_id}").text)
second = _extract_csrf(test_client.get(f"/consent?txn_id={txn_id}").text)
assert first and second
# Each render gets its own token, so a token stays bound to the
# browser it was issued to and the double-submit check keeps working.
assert first != second
# The user submits the page they had open, which is the first one.
response = test_client.post(
"/consent",
data={"action": "approve", "txn_id": txn_id, "csrf_token": first},
follow_redirects=False,
)
assert response.status_code == 302
async def test_forged_token_still_rejected_without_cookie(
self, oauth_proxy_with_storage
):
"""Keeping older tokens valid must not weaken the double-submit check.
An attacker who renders the consent page for a transaction they started
learns a token that stays valid. It is still useless against a victim's
browser, because it never lands in the victim's cookie.
"""
txn_id, _ = await _start_flow(
oauth_proxy_with_storage,
"concurrent-render-attacker",
"http://localhost:9090/callback",
)
app = Starlette(routes=oauth_proxy_with_storage.get_routes())
with TestClient(app, base_url="https://myserver.com") as attacker_client:
attacker_token = _extract_csrf(
attacker_client.get(f"/consent?txn_id={txn_id}").text
)
assert attacker_token
with TestClient(app, base_url="https://myserver.com") as victim_client:
victim_client.get(f"/consent?txn_id={txn_id}")
response = victim_client.post(
"/consent",
data={
"action": "approve",
"txn_id": txn_id,
"csrf_token": attacker_token,
},
follow_redirects=False,
)
assert response.status_code == 403
@pytest.mark.parametrize("submitted", [0, 1])
async def test_overlapping_renders_both_submittable(
self, oauth_proxy_with_storage, submitted
):
"""Two renders in flight at once must both survive.
Sequential renders are the common case, but nothing serialises them.
Two handlers can read the same transaction before either has stored its
token, and if tokens live in a list on the transaction the second write
drops the first there is no compare-and-swap on `AsyncKeyValue` to
catch it, and it happens across processes sharing one backend too.
The gate forces exactly that interleaving. Both responses are applied to
one cookie jar, the way a browser applies them, and then whichever form
the user happened to be looking at is submitted.
"""
txn_id, _ = await _start_flow(
oauth_proxy_with_storage,
"overlapping-render-client",
"http://localhost:9090/callback",
)
gate = _RenderGate(2)
_gate_transaction_reads(oauth_proxy_with_storage, gate)
app = Starlette(routes=oauth_proxy_with_storage.get_routes())
async with httpx.AsyncClient(
transport=httpx.ASGITransport(app=app),
base_url="https://myserver.com",
) as client:
first, second = await asyncio.gather(
client.get(f"/consent?txn_id={txn_id}"),
client.get(f"/consent?txn_id={txn_id}"),
)
tokens = [_extract_csrf(first.text), _extract_csrf(second.text)]
assert all(tokens)
# Each render still issues its own token, so a token stays unique
# to the browser it was handed to.
assert tokens[0] != tokens[1]
# Both responses left their own cookie; neither overwrote the other.
assert len(_consent_state_cookie_names(client)) == 2
response = await client.post(
"/consent",
data={
"action": "approve",
"txn_id": txn_id,
"csrf_token": tokens[submitted],
},
)
assert response.status_code == 302
class TestConsentStateCookieScope:
"""Consent state is per transaction and bounded."""
async def test_completing_one_flow_leaves_another_submittable(
self, oauth_proxy_with_storage
):
"""Approving one transaction must not clear a different pending one.
A browser can have two consent flows open two clients connecting, or
one client retried in a second tab. Consent state held as a single flat
list cannot express that: completing either flow wipes both, and the
one still on screen fails the double-submit check on Approve.
"""
first_txn, _ = await _start_flow(
oauth_proxy_with_storage, "pending-flow-a", "http://localhost:9090/callback"
)
second_txn, _ = await _start_flow(
oauth_proxy_with_storage, "pending-flow-b", "http://localhost:9090/callback"
)
app = Starlette(routes=oauth_proxy_with_storage.get_routes())
with TestClient(app, base_url="https://myserver.com") as client:
first_token = _extract_csrf(client.get(f"/consent?txn_id={first_txn}").text)
second_token = _extract_csrf(
client.get(f"/consent?txn_id={second_txn}").text
)
assert first_token and second_token
completed = client.post(
"/consent",
data={
"action": "approve",
"txn_id": first_txn,
"csrf_token": first_token,
},
follow_redirects=False,
)
assert completed.status_code == 302
# The flow still on screen is unaffected.
still_open = client.post(
"/consent",
data={
"action": "approve",
"txn_id": second_txn,
"csrf_token": second_token,
},
follow_redirects=False,
)
assert still_open.status_code == 302
# Both flows are done, so nothing is left behind either.
assert _consent_state_cookie_names(client) == []
async def test_completing_a_flow_removes_only_its_own_state(
self, oauth_proxy_with_storage
):
"""Approve clears the transaction it completed and nothing else."""
first_txn, _ = await _start_flow(
oauth_proxy_with_storage, "scoped-flow-a", "http://localhost:9090/callback"
)
second_txn, _ = await _start_flow(
oauth_proxy_with_storage, "scoped-flow-b", "http://localhost:9090/callback"
)
app = Starlette(routes=oauth_proxy_with_storage.get_routes())
with TestClient(app, base_url="https://myserver.com") as client:
first_token = _extract_csrf(client.get(f"/consent?txn_id={first_txn}").text)
client.get(f"/consent?txn_id={second_txn}")
assert first_token
before = set(_consent_state_cookie_names(client))
assert len(before) == 2
client.post(
"/consent",
data={
"action": "approve",
"txn_id": first_txn,
"csrf_token": first_token,
},
follow_redirects=False,
)
after = set(_consent_state_cookie_names(client))
assert len(after) == 1
assert after < before
async def test_consent_state_cookies_are_bounded(self, oauth_proxy_with_storage):
"""Repeated renders must not grow the Cookie header without limit.
One cookie per issued token is what keeps concurrent renders from
overwriting each other, so the count has to be capped somewhere. The
newest render always survives the cull.
"""
txn_id, _ = await _start_flow(
oauth_proxy_with_storage,
"bounded-render-client",
"http://localhost:9090/callback",
)
app = Starlette(routes=oauth_proxy_with_storage.get_routes())
with TestClient(app, base_url="https://myserver.com") as client:
newest = None
for _ in range(_MAX_CSRF_TOKENS + 3):
newest = _extract_csrf(client.get(f"/consent?txn_id={txn_id}").text)
assert newest
assert len(_consent_state_cookie_names(client)) <= _MAX_CSRF_TOKENS
response = client.post(
"/consent",
data={"action": "approve", "txn_id": txn_id, "csrf_token": newest},
follow_redirects=False,
)
assert response.status_code == 302
class TestStoragePersistence:
"""Tests for state persistence across storage backends."""