Skip to content

fix(client/auth): discard stored client registrations whose secret has expired #3264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
claude wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix(client/auth): discard stored client registrations whose secret has expired #3264

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4fac3c7
fix(client/auth): discard stored client registrations with an expired…
claude Aug 7, 2026
50d6b0b
fix(client/auth): re-check secret expiry in the 401 flow; document th…
claude Aug 7, 2026
6b7ab0d
chore: retrigger CI after PyPI download timeout
claude Aug 7, 2026
5a246e6
fix(client/auth): run the expiry discard after the SEP-2352 issuer ch…
claude Aug 10, 2026
e53e526
fix(client/auth): drop the orphaned refresh token on expiry discard; …
claude Aug 10, 2026
32a056d
test(client/auth): make the expiry-discard test's storage assertions …
claude Aug 10, 2026
43d9599
fix(client/auth): run discovery before the 403 step-up re-registers w…
claude Aug 10, 2026
aedc88c
fix(client/auth): finalize the discovery sub-generator when the flow …
claude Aug 10, 2026
928b7bf
test(client/auth): cover the 403 step-up's exception relay; drop its …
claude Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix(client/auth): run the expiry discard after the SEP-2352 issuer ch… 
…ecks; cover the refresh and 403 step-up paths

The expired-registration discard previously nulled client_info before the
SEP-2352 issuer-binding checks, so a record that was both expired and
issuer-mismatched skipped the cross-issuer cleanup (old tokens and cached AS
metadata were kept). The discard now runs just before Step 4, after both
issuer checks; _initialize loads the record as-is and defers the discard to
the flow.

The two other paths that present the minted secret are covered as well: the
refresh branch skips a refresh whose registration secret has lapsed (falling
through to the 401 flow's re-registration instead of failing invalid_client),
and the 403 insufficient_scope step-up re-registers — mirroring Step 4,
reusing any discovered AS metadata, extracted into
_prepare_client_registration/_complete_client_registration — before running
the interactive authorization, instead of burning a consent doomed to fail
invalid_client while the live access token keeps the 401 discard from running.
  • Loading branch information
@claude
claude committed Aug 10, 2026
commit 5a246e67730261e92fa4adf2f1a348df6e2b5d8f
181 changes: 113 additions & 68 deletions src/mcp/client/auth/oauth2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,21 +566,16 @@
async def _initialize(self) -> None:
"""Load stored tokens and client info.

Stored client information whose minted secret has expired (RFC 7591
`client_secret_expires_at`) is treated as absent: reusing it can only produce
`invalid_client` at the token endpoint — even interactive re-authorization ends in
the same failure, permanently — so it is discarded here and the next 401 flow
re-registers (or resolves CIMD), overwriting the dead record in storage. Any still
stored tokens are kept: a live access token keeps working without client
authentication, and with no client info the refresh path (which would present the
lapsed secret) is skipped.
A stored registration whose minted secret has expired (RFC 7591
`client_secret_expires_at`) is loaded as-is rather than discarded here: the auth
flow discards it right before re-registering, *after* the SEP-2352 issuer checks,
which need the record's issuer stamp — an expired record that is also bound to a
different issuer must still get its cross-issuer cleanup (dropping the old
issuer's tokens and cached metadata). Until then the dead secret is never
presented: the refresh branch and the 403 step-up skip it explicitly.
"""
self.context.current_tokens = await self.context.storage.get_tokens()
client_info = await self.context.storage.get_client_info()
if client_info is not None and stored_registration_expired(client_info):
logger.debug("Stored client registration secret has expired; discarding so the next flow re-registers")
client_info = None
self.context.client_info = client_info
self.context.client_info = await self.context.storage.get_client_info()
self._initialized = True

def _add_auth_header(self, request: httpx2.Request) -> None:
Expand All @@ -607,6 +602,63 @@
if not check_resource_allowed(requested_resource=default_resource, configured_resource=prm_resource):
raise OAuthFlowError(f"Protected resource {prm_resource} does not match expected {default_resource}")

def _registration_issuer(self) -> str | None:
"""SEP-2352: the issuer to bind newly minted credentials to, when known."""
if self.context.oauth_metadata is not None:
return self.context.auth_server_url or str(self.context.oauth_metadata.issuer)
return None

async def _prepare_client_registration(self) -> httpx2.Request | None:
"""Resolve a URL-based client ID (CIMD) or build a Dynamic Client Registration request.

When the server supports CIMD the client information is created (and persisted)
immediately and ``None`` is returned — no network round trip is needed. Otherwise
the returned registration request must be sent and its response passed to
`_complete_client_registration`.
"""
if should_use_client_metadata_url(self.context.oauth_metadata, self.context.client_metadata_url):
# Use URL-based client ID (CIMD). CIMD records are portable across
# authorization servers, so the issuer stamp is informational.
logger.debug(f"Using URL-based client ID (CIMD): {self.context.client_metadata_url}")
client_information = create_client_info_from_metadata_url(
self.context.client_metadata_url, # type: ignore[arg-type]
redirect_uris=self.context.client_metadata.redirect_uris,
)
client_information.issuer = self._registration_issuer()
self.context.client_info = client_information
await self.context.storage.set_client_info(client_information)
return None

# Fallback to Dynamic Client Registration
fallback_base = self.context.get_authorization_base_url(self.context.server_url)
return create_client_registration_request(
self.context.oauth_metadata, self.context.client_metadata, fallback_base
)

async def _complete_client_registration(self, response: httpx2.Response) -> None:
"""Handle a Dynamic Client Registration response and persist the minted record."""
client_information = await handle_registration_response(response)
check_registration_usable(client_information)
discovered_issuer = self._registration_issuer()
fallback_base = self.context.get_authorization_base_url(self.context.server_url)
# Only record the issuer when the registration actually targeted the discovered
# AS — either via its published registration_endpoint, or because the
# resource-origin /register fallback is on the issuer's own host (legacy
# same-origin embedded AS). Otherwise the fallback hit a different server and
# recording a binding to the PRM-advertised AS would persist a binding that was
# never established.
if (
self.context.oauth_metadata is not None
and discovered_issuer is not None
and (
self.context.oauth_metadata.registration_endpoint is not None
or self.context.get_authorization_base_url(discovered_issuer) == fallback_base
)
):
client_information.issuer = discovered_issuer
self.context.client_info = client_information
await self.context.storage.set_client_info(client_information)

async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx2.Request, httpx2.Response]:
"""httpx2 auth flow integration."""
async with self.context.lock:
Expand All @@ -616,7 +668,16 @@
# Capture protocol version from request headers
self.context.protocol_version = request.headers.get(MCP_PROTOCOL_VERSION_HEADER)

if not self.context.is_token_valid() and self.context.can_refresh_token():
# A refresh request authenticates with the minted secret, so a registration
# whose secret has lapsed (RFC 7591 `client_secret_expires_at`) can only fail
# `invalid_client`. Skip the doomed refresh and fall through to the 401 flow,
# which re-registers; the record itself is kept for now so the flow's SEP-2352
# issuer checks can still read its issuer stamp before the expiry discard runs.
registration_expired = self.context.client_info is not None and stored_registration_expired(
self.context.client_info
)

Check warning on line 678 in src/mcp/client/auth/oauth2.py

View check run for this annotation

Claude / Claude Code Review

[quality] Expiry guard repeated verbatim at three flow sites 

The compound guard `self.context.client_info is not None and stored_registration_expired(self.context.client_info)` is written out verbatim at three sites in `async_auth_flow` (the refresh gate, the 401 flow's pre-Step-4 discard, and the 403 step-up). Since `OAuthContext` already hosts the sibling flow-gating predicates `is_token_valid()` and `can_refresh_token()`, a small `OAuthContext.registration_secret_expired()` method (or widening `stored_registration_expired` to accept `OAuthClientInforma
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

if not self.context.is_token_valid() and self.context.can_refresh_token() and not registration_expired:
# Try to refresh token
refresh_request = await self._refresh_token()
refresh_response = yield refresh_request
Expand All @@ -635,17 +696,6 @@
try:
# OAuth flow must be inline due to generator constraints

# A registration whose minted secret lapsed mid-session (after
# _initialize already loaded it) can no longer authenticate either —
# discard it here too, so Step 4 re-registers instead of running an
# interactive authorization doomed to fail `invalid_client` at the
# token endpoint.
if self.context.client_info is not None and stored_registration_expired(self.context.client_info):
logger.debug(
"Stored client registration secret has expired; discarding so this flow re-registers"
)
self.context.client_info = None

www_auth_resource_metadata_url = extract_resource_metadata_from_www_auth(response)

# Step 1: Discover protected resource metadata (SEP-985 with fallback support)
Expand Down Expand Up @@ -736,53 +786,28 @@
self.context.oauth_metadata,
self.context.client_metadata.grant_types,
)

# A registration whose minted secret lapsed (RFC 7591
# `client_secret_expires_at`) — whether loaded from storage or expired
# mid-session — can no longer authenticate: reusing it would burn an
# interactive authorization doomed to fail `invalid_client` at the
# token endpoint. Discard it only now, after the SEP-2352 issuer
# checks above, so an expired record bound to a different issuer
# still got its cross-issuer cleanup; Step 4 then re-registers,
# overwriting the dead record in storage. Any stored tokens are kept:
# a live access token keeps working without client authentication.
if self.context.client_info is not None and stored_registration_expired(self.context.client_info):
logger.debug(
"Stored client registration secret has expired; discarding so this flow re-registers"
)
self.context.client_info = None

Check warning on line 803 in src/mcp/client/auth/oauth2.py

View check run for this annotation

Claude / Claude Code Review

Expiry discard orphans a refresh token bound to the discarded client_id 

The expiry discard nulls `client_info` but keeps `current_tokens` including a `refresh_token` that was issued to the discarded `client_id` — if the flow then fails between `_complete_client_registration`'s `set_client_info` and the token exchange (abandoned consent, state mismatch, token-endpoint 5xx, crash/restart), the next request pairs fresh `client_info` with the old refresh token, `can_refresh_token()` passes, and `_refresh_token()` presents the old client's refresh token authenticated as
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

# Step 4: Register client or use URL-based client ID (CIMD)
if not self.context.client_info:
# SEP-2352: the issuer to bind these credentials to, when known.
discovered_issuer: str | None = None
if self.context.oauth_metadata is not None:
discovered_issuer = self.context.auth_server_url or str(self.context.oauth_metadata.issuer)

if should_use_client_metadata_url(
self.context.oauth_metadata, self.context.client_metadata_url
):
# Use URL-based client ID (CIMD). CIMD records are portable across
# authorization servers, so the issuer stamp is informational.
logger.debug(f"Using URL-based client ID (CIMD): {self.context.client_metadata_url}")
client_information = create_client_info_from_metadata_url(
self.context.client_metadata_url, # type: ignore[arg-type]
redirect_uris=self.context.client_metadata.redirect_uris,
)
client_information.issuer = discovered_issuer
self.context.client_info = client_information
await self.context.storage.set_client_info(client_information)
else:
# Fallback to Dynamic Client Registration
fallback_base = self.context.get_authorization_base_url(self.context.server_url)
registration_request = create_client_registration_request(
self.context.oauth_metadata, self.context.client_metadata, fallback_base
)
registration_request = await self._prepare_client_registration()
if registration_request is not None:
registration_response = yield registration_request
client_information = await handle_registration_response(registration_response)
check_registration_usable(client_information)
# Only record the issuer when the registration above actually targeted
# the discovered AS — either via its published registration_endpoint,
# or because the resource-origin /register fallback is on the issuer's
# own host (legacy same-origin embedded AS). Otherwise the fallback hit
# a different server and recording a binding to the PRM-advertised AS
# would persist a binding that was never established.
if (
self.context.oauth_metadata is not None
and discovered_issuer is not None
and (
self.context.oauth_metadata.registration_endpoint is not None
or self.context.get_authorization_base_url(discovered_issuer) == fallback_base
)
):
client_information.issuer = discovered_issuer
self.context.client_info = client_information
await self.context.storage.set_client_info(client_information)
await self._complete_client_registration(registration_response)

# Step 5: Perform authorization and complete token exchange
token_response = yield await self._perform_authorization()
Expand Down Expand Up @@ -815,6 +840,26 @@
prior_scope = union_scopes(self.context.client_metadata.scope, granted_scope)
self.context.client_metadata.scope = union_scopes(prior_scope, challenged_scope)

# A registration whose minted secret lapsed (RFC 7591
# `client_secret_expires_at`) cannot complete the step-up: the
# token exchange would fail `invalid_client` after burning a full
# interactive consent — and the still-live access token keeps the
# 401 flow's discard from ever running. Discard it and mint fresh
# credentials first (mirroring the 401 flow's Step 4, reusing any
# AS metadata already discovered).
if self.context.client_info is not None and stored_registration_expired(
self.context.client_info
):
logger.debug(
"Stored client registration secret has expired; re-registering before the step-up"
)
self.context.client_info = None
if not self.context.client_info:
registration_request = await self._prepare_client_registration()
if registration_request is not None:
registration_response = yield registration_request
await self._complete_client_registration(registration_response)

# Step 2b: Perform (re-)authorization and token exchange
token_response = yield await self._perform_authorization()
await self._handle_token_response(token_response)
Comment thread
claude[bot] marked this conversation as resolved.
Expand Down
Loading
Loading