fix(mcp): close obo auth-column leak, capture gate, and cooldown classification

Round-5 review follow-up — three CONFIRMED (one security) plus two
correctness issues, all traceable to earlier fixes in this branch.

SECURITY: the round-2 redesign gated the "scrub OAuth columns this
auth_type doesn't use" on is_flip, replacing the old unconditional
scrub. A same-type static/none/obo edit could then inject an
oauth_authorization_server_url that survived a later flip to oauth_user
(which uses that column) and redirected every consenting user's OAuth
traffic to an attacker AS. The scrub is now applied on EVERY write, and a
flip into oauth_user recomputes the oauth_user-only columns from the
request so a stale value can't carry in — the persisted OAuth columns
are once again a pure function of the target auth_type.

- The oauth_obo write gate now also requires capture_user_credential to
  be enabled: without it, login persists no credential and every dispatch
  returns "missing" with a remedy that can never succeed — the permanent
  misconfig the gate exists to reject.

- A permanent obo mint failure arms the cooldown (its shared credential
  survives the per-server revoke), but the in-cooldown short-circuit
  reported it as a retryable transient for the whole window, flapping
  against the honest re-login/admin affordance. The backoff state now
  records whether the arming failure was permanent, and the short-circuit
  surfaces the matching classification.

- The ambiguous-escalation revoke cleared the cooldown without re-arming;
  for obo (surviving credential) that let the next dispatch immediately
  re-mint against the still-failing IdP. It now re-arms the same terminal
  backstop the permanent branch has.

- The force-refresh reuse gate keyed on the cache row's 1-second `created`
  time, which couldn't tell a concurrent peer's fresh mint from the
  caller's own just-rejected token minted in the same second — so a retry
  could re-serve the rejected bearer. It now decides by token identity
  (the under-lock row differs from the pre-lock one), preserving the
  single-flight reuse while never re-serving a rejected token.

Also: guard _pool_error_detail's str.format so placeholder-free copy
can't raise inside the error renderer, and note why the connections-list
classifies obo rows by authoritative auth_type on that cold path.
This commit is contained in:
Patrick Buckley
2026-07-12 07:12:48 -07:00
parent c53bd464d0
commit 09aa50b7a1
5 changed files with 281 additions and 52 deletions
+82 -1
View File
@@ -201,7 +201,10 @@ def _enabled_oidc(profile: str = "entra") -> SimpleNamespace:
bad-profile config instead to assert the rejection.
"""
return SimpleNamespace(
enabled=True, issuer="https://idp.example.com", obo_grant_profile=profile
enabled=True,
issuer="https://idp.example.com",
obo_grant_profile=profile,
capture_user_credential=True,
)
@@ -764,6 +767,84 @@ class TestUpdateMcpServer:
assert data["auth_type"] == "oauth_obo"
assert data["oauth_audience"] == "api://mcp-a"
def test_same_type_static_edit_cannot_inject_oauth_columns(self, client, storage):
"""Review finding (SECURITY): the OAuth columns must be a pure function
of the target auth_type on EVERY write, not just a flip. A same-type
static edit that injects oauth_authorization_server_url must be scrubbed
to NULL — otherwise a later flip to oauth_user (which legitimately uses
that column) would inherit the attacker AS URL and redirect every
consenting user's OAuth traffic."""
r = client.post(
"/v1/api/admin/mcp-servers",
json={
"name": "static-inject",
"transport": "streamable-http",
"url": "https://mcp.example.com/sse",
"auth_type": "static",
},
)
sid = r.json()["server_id"]
# Same-type static edit trying to smuggle an oauth_user-only column.
r2 = client.put(
f"/v1/api/admin/mcp-servers/{sid}",
json={
"auth_type": "static",
"oauth_authorization_server_url": "https://attacker.example",
},
)
assert r2.status_code == 200, r2.text
row = storage.get_mcp_server(sid)
assert (row.get("oauth_authorization_server_url") or None) is None
def test_flip_to_oauth_user_does_not_inherit_stale_as_url(self, client, storage):
"""Review finding (SECURITY): flipping a non-oauth_user row to oauth_user
must recompute the oauth_user-only columns from the request, never
inherit a stale/injected authorization_server_url left on the pre-flip
row (defence-in-depth for a value that predates the unconditional
scrub)."""
# Plant a static row that already carries a stale AS URL directly in DB.
storage.create_mcp_server(
server_id="stale-asurl-id",
name="stale-asurl",
transport="streamable-http",
url="https://mcp.example.com/sse",
auth_type="static",
oauth_authorization_server_url="https://attacker.example",
)
# Flip to oauth_user WITHOUT supplying an AS URL in the body.
r = client.put(
"/v1/api/admin/mcp-servers/stale-asurl-id",
json={"auth_type": "oauth_user", "oauth_client_id": "cli_x"},
)
assert r.status_code == 200, r.text
row = storage.get_mcp_server("stale-asurl-id")
assert (row.get("oauth_authorization_server_url") or None) is None
assert row.get("oauth_client_id") == "cli_x"
def test_create_obo_rejected_when_capture_disabled(self, client):
"""Review finding: oauth_obo mints from the user's CAPTURED sign-in
credential, so with capture_user_credential off, login persists nothing
and every dispatch returns kind='missing' with an unsatisfiable remedy.
Reject at write time."""
client.app.state.oidc_config = SimpleNamespace(
enabled=True,
issuer="https://idp.example.com",
obo_grant_profile="entra",
capture_user_credential=False,
)
r = client.post(
"/v1/api/admin/mcp-servers",
json={
"name": "obo-no-capture",
"transport": "streamable-http",
"url": "https://mcp.example.com/sse",
"auth_type": "oauth_obo",
"oauth_audience": "api://mcp-a",
},
)
assert r.status_code == 400, r.text
assert "capture_user_credential" in r.json()["error"]
def test_create_obo_rejected_when_oidc_disabled(self, client):
"""Review finding: oauth_obo mints from the user's OIDC sign-in, so an
install with OIDC disabled can NEVER mint. Reject at write time (a
+89 -21
View File
@@ -762,7 +762,9 @@ class TestFailureHandling:
common missing-tenant-grant case — the user never had a token for this
server) must NOT emit a token_revoked audit for a row that never
existed. The cooldown is still armed as the terminal backstop (the
credential survives), so the second dispatch short-circuits."""
credential survives), and — because the failure was PERMANENT — the
second dispatch surfaces the honest permanent classification during the
cooldown window (not a misleading retryable transient)."""
_seed_obo_server(storage)
client = MagicMock(spec=httpx.AsyncClient)
client.post = AsyncMock(
@@ -783,8 +785,9 @@ class TestFailureHandling:
first, second = asyncio.run(_run())
assert first.kind == "refresh_failed"
# Terminal: the cooldown short-circuits the second dispatch entirely.
assert second.kind == "refresh_failed_transient"
# Terminal: the cooldown short-circuits the second dispatch — and reports
# the PERMANENT classification, not a retryable transient.
assert second.kind == "refresh_failed"
assert client.post.call_count == 1 # NOT re-minted
# No row was ever deleted → no bogus revoke audit.
events = storage.list_audit_events(action="mcp_server.oauth.token_revoked")
@@ -867,35 +870,100 @@ class TestFailureHandling:
assert result.token == "at-reminted"
assert client.post.call_count == 1
def test_force_refresh_reuses_row_minted_while_waiting_for_lock(
def test_force_refresh_reuses_concurrently_minted_token_without_reminting(
self, storage: SQLiteBackend
) -> None:
"""Review finding: the under-lock "another caller already minted" reuse
gate keyed on ``last_refreshed``, which obo cache rows NEVER set
(delete+create hardcodes NULL) — so every serialized force_refresh
waiter re-ran a full IdP redemption. The gate now keys on ``created``
(the mint time under delete+create): a fresh row created at/after the
caller's lock-request time is served without a new mint."""
"""Serialized force_refresh waiters must single-flight the re-mint: a
waiter that acquires the lock AFTER a peer already re-minted reuses the
peer's fresh token instead of running its own redundant IdP redemption.
The reuse is decided by token IDENTITY (the under-lock row holds a
DIFFERENT token than the rejected one this caller came in with), not by
mint time — so a same-second concurrent mint is still reused."""
from unittest.mock import patch
_seed_obo_server(storage)
client = MagicMock(spec=httpx.AsyncClient)
client.post = AsyncMock() # any IdP call would be a gate failure
state = _make_app_state(storage, http_client=client, oidc_config=_make_oidc_config())
_seed_credential(state)
# Seeded moments before the call: at second granularity its ``created``
# is >= the caller's lock-request time, exactly what a concurrent
# waiter observes after the winner persisted its mint.
_seed_cache_row(state, expires_in_seconds=3600, access_token="winner-minted-at")
async def _run() -> Any:
return await get_obo_access_token_classified(
app_state=state, user_id=USER, server_name=SERVER, force_refresh=True
)
def _fresh_row(access_token: str) -> Any:
return {
"user_id": USER,
"server_name": SERVER,
"access_token": access_token,
"refresh_token": None,
"expires_at": (datetime.now(UTC) + timedelta(seconds=3600)).strftime(_ISO),
"scopes": None,
"as_issuer": ISSUER,
"audience": AUDIENCE,
"created": datetime.now(UTC).strftime(_ISO),
"last_refreshed": None,
}
result = asyncio.run(_run())
# Pre-lock read returns the rejected token; the under-lock re-read returns
# a DIFFERENT token (a concurrent waiter re-minted while we held-waited).
reads = [_fresh_row("rejected-at"), _fresh_row("peer-reminted-at")]
with patch.object(state.mcp_token_store, "get_user_token", side_effect=reads):
async def _run() -> Any:
return await get_obo_access_token_classified(
app_state=state, user_id=USER, server_name=SERVER, force_refresh=True
)
result = asyncio.run(_run())
assert result.kind == "token"
assert result.token == "winner-minted-at"
assert client.post.call_count == 0
assert result.token == "peer-reminted-at" # reused the peer's fresh token
assert client.post.call_count == 0 # no redundant redemption
def test_force_refresh_remints_when_cache_still_holds_rejected_token(
self, storage: SQLiteBackend
) -> None:
"""The other half of the identity gate: when the under-lock row still
holds the SAME token the caller came in with (no peer re-minted), a
force_refresh must RE-MINT — never re-serve the just-rejected bearer.
A mint-time gate at 1-second ``created`` granularity would wrongly
re-serve a token minted in the same second as the retry."""
from unittest.mock import patch
_seed_obo_server(storage)
client = MagicMock(spec=httpx.AsyncClient)
client.post = AsyncMock(
return_value=_mk_response(
200, {"access_token": "genuinely-reminted", "expires_in": 3600}
)
)
state = _make_app_state(storage, http_client=client, oidc_config=_make_oidc_config())
_seed_credential(state)
rejected = {
"user_id": USER,
"server_name": SERVER,
"access_token": "rejected-at",
"refresh_token": None,
"expires_at": (datetime.now(UTC) + timedelta(seconds=3600)).strftime(_ISO),
"scopes": None,
"as_issuer": ISSUER,
"audience": AUDIENCE,
"created": datetime.now(UTC).strftime(_ISO),
"last_refreshed": None,
}
# Both the pre-lock and under-lock reads return the SAME (rejected) token.
with patch.object(
state.mcp_token_store, "get_user_token", side_effect=[rejected, rejected]
):
async def _run() -> Any:
return await get_obo_access_token_classified(
app_state=state, user_id=USER, server_name=SERVER, force_refresh=True
)
result = asyncio.run(_run())
assert result.kind == "token"
assert result.token == "genuinely-reminted" # re-minted, NOT re-served
assert client.post.call_count == 1
def test_rotation_persist_failure_does_not_break_the_mint(self, storage: SQLiteBackend) -> None:
"""Review finding: a storage error inside the rotation-persist callback
+46 -16
View File
@@ -9889,6 +9889,22 @@ def _enforce_oauth_obo_requirements(
},
status_code=400,
)
if not getattr(oidc_config, "capture_user_credential", False):
# The mint redeems the user's CAPTURED IdP refresh token; with capture
# off, login persists nothing, so every dispatch returns kind="missing"
# and the "sign in again" remedy can never succeed — a permanent
# misconfig this choke point exists to reject. (Enabling capture also
# requires the encryption key, checked at boot.)
return JSONResponse(
{
"error": (
"auth_type=oauth_obo requires [oidc] capture_user_credential=true "
"so each user's sign-in credential is captured for minting; it is "
"currently disabled, so this server could never obtain a token."
)
},
status_code=400,
)
profile = _obo_profile(request)
from turnstone.core.mcp_oauth import OBO_GRANT_PROFILES
@@ -10551,22 +10567,36 @@ async def admin_update_mcp_server(request: Request) -> JSONResponse:
):
del updates[_noop_key]
# Clear the OAuth columns the target auth_type does NOT use (shared
# ``_oauth_columns_to_clear`` policy). ``oauth_client_secret_ct`` is owned by
# a dedicated write path (see below).
if is_flip:
target_auth = new_auth # non-None: is_flip requires new_auth is not None
if is_user_scoped_auth(target_auth):
# FLIP into a user-scoped type: recompute the semantic columns from
# the body — present → that value, absent → NULL — so neither the
# old audience nor the old scopes can leak across the semantic
# boundary. The top-of-handler loop already copied any body value
# into ``updates``; forcing the key present here NULLs it when the
# body omitted it (e.g. an API PUT of just {"auth_type": ...}, or the
# console form clearing the field on the auth-type switch).
updates["oauth_audience"] = updates.get("oauth_audience")
updates["oauth_scopes"] = updates.get("oauth_scopes")
updates.update(_oauth_columns_to_clear(target_auth))
# Scrub the OAuth columns the target auth_type does NOT use — on EVERY write,
# not just a flip. This is a SECURITY invariant, not a flip nicety: a
# same-type edit can still inject a column the type doesn't use (e.g. an API
# PUT of {"auth_type":"static","oauth_authorization_server_url":"…attacker…"}
# onto a static row), which — because oauth_user legitimately USES that
# column — would then survive a later flip to oauth_user and redirect every
# consenting user's OAuth traffic. The persisted OAuth columns must be a pure
# function of the target auth_type (the create handler enforces the same via
# ``_oauth_columns_to_clear``). ``oauth_client_secret_ct`` is owned by a
# dedicated write path (see below).
target_auth = new_auth if new_auth is not None else old_auth
if is_flip and new_auth is not None and is_user_scoped_auth(new_auth):
# FLIP into a user-scoped type: the columns whose MEANING differs across
# oauth_user↔oauth_obo must be recomputed from the body — present → that
# value, absent → NULL — never inherited from the old model's row. For
# oauth_audience/oauth_scopes this is the semantic-boundary rule; for a
# flip INTO oauth_user the oauth_user-only columns (client_id /
# registration / AS-URL) are also recomputed, so a stale/injected value
# left on the pre-flip static/none/obo row cannot carry in (those models
# don't use these columns, so the operator supplies them fresh here).
updates["oauth_audience"] = updates.get("oauth_audience")
updates["oauth_scopes"] = updates.get("oauth_scopes")
if new_auth == "oauth_user":
for _user_col in (
"oauth_client_id",
"oauth_registration_mode",
"oauth_authorization_server_url",
):
updates[_user_col] = updates.get(_user_col)
updates.update(_oauth_columns_to_clear(target_auth))
# Renaming a pool-backed row needs the same per-user-token purge as
# delete: the tokens are keyed on the OLD ``server_name`` and a row
# later created with that old name (with attacker-controlled URL)
+8 -1
View File
@@ -7279,7 +7279,14 @@ def _pool_error_detail(server_row: dict[str, Any], situation: str, *, kind: str
card; everything else is treated as ``oauth_user``.
"""
model = "oauth_obo" if server_row.get("auth_type") == "oauth_obo" else "oauth_user"
return _POOL_ERROR_DETAIL[(model, situation)].format(kind=kind, kind_cap=kind.capitalize())
template = _POOL_ERROR_DETAIL[(model, situation)]
# Only the insufficient_scope rows carry {kind}/{kind_cap}; the rest are
# plain strings. Format ONLY when a kind is supplied so a future message
# containing a literal brace (a scope/JSON example) can't raise inside this
# error-rendering path and turn a clean structured error into a 500.
if kind:
return template.format(kind=kind, kind_cap=kind.capitalize())
return template
def _consent_missing_detail(server_row: dict[str, Any]) -> str:
+56 -13
View File
@@ -1419,6 +1419,13 @@ class _RefreshBackoffState:
last_failure_monotonic: float = 0.0
ambiguous_streak: int = 0
# True when the failure that armed the current cooldown was a PERMANENT
# dead-grant (only the oauth_obo path arms a cooldown on permanent, because
# its shared credential survives the per-server revoke). The in-cooldown
# short-circuit reads this so it surfaces the honest permanent classification
# (re-login / admin remedy) instead of a misleading "retry" transient for the
# whole window. Reset to False whenever a transient/ambiguous failure arms.
last_failure_permanent: bool = False
def _refresh_backoff_state(app_state: Any, user_id: str, server_name: str) -> _RefreshBackoffState:
@@ -1435,7 +1442,9 @@ def _refresh_backoff_state(app_state: Any, user_id: str, server_name: str) -> _R
return state
def _arm_cooldown(app_state: Any, user_id: str, server_name: str) -> _RefreshBackoffState:
def _arm_cooldown(
app_state: Any, user_id: str, server_name: str, *, permanent: bool = False
) -> _RefreshBackoffState:
"""Stamp the per-(user, server) transient-failure cooldown clock to now.
Single definition of the "back off this pair" operation (previously written
@@ -1444,9 +1453,16 @@ def _arm_cooldown(app_state: Any, user_id: str, server_name: str) -> _RefreshBac
missed site can't silently keep hammering the AS/IdP on that path. Returns
the backoff state so a caller that also mutates the ambiguous streak reuses
the same object instead of re-fetching it.
``permanent`` records whether the failure that armed the cooldown was a
dead-grant (obo only): the in-cooldown short-circuit reads it to surface the
honest permanent vs. transient classification. A transient/ambiguous arm
resets it to False so a later transient window can't inherit a stale
permanent flag.
"""
state = _refresh_backoff_state(app_state, user_id, server_name)
state.last_failure_monotonic = time.monotonic()
state.last_failure_permanent = permanent
return state
@@ -1701,8 +1717,10 @@ async def _handle_refresh_failure(
)
if arm_cooldown_on_permanent:
# _revoke_after_refresh_failure cleared the backoff; re-arm the
# cooldown as the terminal backstop for the surviving credential.
_arm_cooldown(app_state, user_id, server_name)
# cooldown as the terminal backstop for the surviving credential, and
# mark it permanent so the in-cooldown short-circuit surfaces the
# honest dead-grant classification (not a misleading "retry").
_arm_cooldown(app_state, user_id, server_name, permanent=True)
return result
# Transient or ambiguous: keep the token (a blip must never revoke a
# user's consent) and arm the cooldown so a down AS isn't hit on
@@ -1724,7 +1742,7 @@ async def _handle_refresh_failure(
streak=backoff.ambiguous_streak,
error=str(exc),
)
return await _revoke_after_refresh_failure(
escalation_result = await _revoke_after_refresh_failure(
app_state,
token_store,
user_id,
@@ -1732,6 +1750,16 @@ async def _handle_refresh_failure(
server_id_for_audit,
reason=escalation_reason,
)
if arm_cooldown_on_permanent:
# Same shared-credential backstop as the PERMANENT branch: an
# escalation is a treated-as-dead grant, but the revoke
# cleared the cooldown, and for obo the credential survives —
# so without re-arming, the very next dispatch immediately
# re-mints against the still-failing IdP and re-escalates
# each cycle. Re-arm (marked permanent for the honest
# in-cooldown classification).
_arm_cooldown(app_state, user_id, server_name, permanent=True)
return escalation_result
# Background priming: an UNCLASSIFIABLE sustained rejection is
# exactly where a bulk prime of servers the user may not be
# using must not revoke consent. Defer the escalation-revoke to
@@ -2319,8 +2347,6 @@ async def get_obo_access_token_classified(
log.debug("mcp_server.oauth.token_store_unconfigured")
return TokenLookupResult(kind="missing")
t_lock_request_started = datetime.now(UTC).replace(microsecond=0)
# Resolve the server row + audience FIRST (callers on the dispatch/priming
# path pass server_row, so this is normally no SQL) — the fast-path cache
# serve must validate the row's audience against the CURRENT one, so it can't
@@ -2363,6 +2389,14 @@ async def get_obo_access_token_classified(
# still-fresh cache must fall through to the locked re-read so it can pick up
# a token a cluster-mate just minted, rather than fail transient in-cooldown.
if not fresh and _refresh_in_cooldown(app_state, user_id, server_name):
# Surface the classification that armed the cooldown: obo arms it on a
# PERMANENT dead-grant too (its credential survives the per-server
# revoke), and reporting that as a retryable "transient" for the whole
# window would tell the user to retry a permanently-broken server and
# flap against the honest re-login/admin affordance the mint returned.
backoff = _refresh_backoff_state(app_state, user_id, server_name)
if backoff.last_failure_permanent:
return TokenLookupResult(kind="refresh_failed")
return TokenLookupResult(kind="refresh_failed_transient")
if oidc_config is not None and not getattr(oidc_config, "enabled", False):
# A node that booted during a transient IdP outage carries
@@ -2435,13 +2469,18 @@ async def get_obo_access_token_classified(
if _is_fresh_obo_cache_row(plain2, audience, scopes) and plain2 is not None:
if not force_refresh:
return _token_result(app_state, user_id, server_name, plain2["access_token"])
# Obo cache rows are delete+create'd per mint and never touched by
# update_user_token_after_refresh, so ``created`` IS the mint time
# (``last_refreshed`` stays NULL on this path — the oauth_user
# gate's column would never fire here, and every serialized
# force_refresh waiter would run its own redundant IdP redemption).
minted_at = _parse_iso_to_utc(plain2.get("created") or "")
if minted_at is not None and minted_at >= t_lock_request_started:
# force_refresh means the caller's bearer was rejected; serialized
# waiters must single-flight the re-mint (avoid N redundant IdP
# redemptions) WITHOUT re-serving the very token that was just
# rejected. Distinguish by token IDENTITY, not mint time: the pre-lock
# ``plain`` is the token this caller came in with (the rejected one);
# if the under-lock row now holds a DIFFERENT token, a concurrent
# waiter re-minted while we waited — reuse it. If it is the SAME
# token, nothing has changed, so fall through and re-mint. (Mint time
# can't distinguish these at the cache row's 1-second ``created``
# granularity — a same-second own-mint would read as "fresh".)
pre_lock_token = plain["access_token"] if plain is not None else None
if plain2["access_token"] != pre_lock_token:
return _token_result(app_state, user_id, server_name, plain2["access_token"])
# Re-read the credential under the lock — a concurrent mint for a
@@ -3577,6 +3616,10 @@ async def _handle_mcp_oauth_list_connections_inner(request: Request) -> Response
# "Disconnect" that silently undid itself — the row deletes, then
# session-start priming re-mints from the surviving captured credential —
# so the connections list shows only rows the user can actually revoke.
# Classify by the authoritative server ``auth_type`` (one read on this cold
# settings-page path) rather than inferring obo from a NULL refresh token:
# the auth_type is the source of truth, and a token-shape heuristic would
# silently hide any oauth_user row that ever lacked a refresh token.
# Fail open on a server-list read error: worst case an obo row renders
# and the revoke endpoint below still refuses it honestly.
storage = _get_storage(request.app.state)