fix(client/auth): discard stored client registrations whose secret has expired - #3264
fix(client/auth): discard stored client registrations whose secret has expired#3264claude[bot] wants to merge 9 commits into
Conversation
… secret The client persists client_secret_expires_at (RFC 7591) through TokenStorage but never reads it back, and registration only happens when stored client info is absent. Once a dynamically registered secret lapses, every token-endpoint interaction fails with invalid_client - including the exchange after a fresh interactive authorization - so the client is permanently stuck until the application manually deletes the persisted client info (#3256). Treat a stored registration whose secret-authenticating record carries a non-zero, past client_secret_expires_at as absent when loading from storage. The next 401 flow then re-registers (or resolves CIMD) and overwrites the dead record via the existing set_client_info call - no change to the TokenStorage contract. Stored tokens are kept: a live access token continues to work without client authentication, and with no client info the refresh path that would present the lapsed secret is skipped. Fixes #3256 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjbXueCDdFNJK6imejCXgM
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether discarding the expired registration while keeping stored tokens could dead-end the 403 insufficient_scope step-up path (which needs client_info for _perform_authorization) — it raises OAuthFlowError there, but pre-PR the same scenario completed interactive authorization only to fail at token exchange with invalid_client, so the PR does not regress that path.
Extended reasoning...
Two nit-level findings were posted as inline comments (stale docs page and the mid-session secret-lapse gap), so the inline comments already signal the review outcome. This note records the one additional candidate examined and refuted this run: the 403 step-up dead-end with discarded client info. The step-up path with a lapsed secret was already a terminal failure before this PR (dead secret presented at token exchange → invalid_client), so treating the record as absent is not a regression on that path. Not approving: the change is in OAuth client auth code, which is security-sensitive per the approval criteria, even though the change itself is small, well-scoped, and well-tested.
…e discard Address review findings: _initialize runs once per provider instance, so a registration whose secret lapses mid-session (long-lived process) was still reused - Step 4 skipped re-registration and the interactive authorization burned a user consent only to fail invalid_client at the token exchange. Re-check stored_registration_expired at the start of the 401 handler and discard the dead record so Step 4 re-registers. Also update docs/client/oauth-clients.md: the storage tip and the 'stored client_info still wins' statement now mention the expired-secret discard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjbXueCDdFNJK6imejCXgM
The 3.13/locked/ubuntu job failed fetching opentelemetry-api from files.pythonhosted.org (operation timed out) during uv sync - an infrastructure flake unrelated to the change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjbXueCDdFNJK6imejCXgM
…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.
There was a problem hiding this comment.
Beyond the inline nits, this pass also examined whether the 403 step-up's new registration call could send a DCR POST to the resource-server origin when no AS metadata was ever discovered (restart with a live token, then a 403) — ruled out as a new issue: that resource-origin /register fallback is the same legacy no-metadata behavior the 401 flow's Step 4 and _perform_authorization (/authorize, /token) already use, not something this PR introduces.
Extended reasoning...
Bug hunting found two nits, posted inline. One additional candidate was investigated and refuted this run: the 403 step-up path with oauth_metadata is None does route _prepare_client_registration's DCR fallback to the resource-server origin, but this mirrors the pre-existing legacy fallback used throughout the flow when AS metadata is absent (the 401 flow's Step 4 falls back the same way, and _perform_authorization/_get_token_endpoint build resource-origin URLs on that path too), so it is consistent established behavior rather than a regression from this PR. Recording it so a later pass need not re-derive it.
…centralize the expiry predicate A refresh token is bound to the client it was issued to (RFC 6749 §6), so once the expiry discard replaces the registration, the kept refresh token could only be presented as the wrong client and fail invalid_grant — which happened whenever a flow failed between re-registration and token exchange. Both discard sites (401 pre-Step-4 and 403 step-up) now drop the refresh token with the record and persist the trimmed tokens, so an interrupted flow or a restart cannot resurrect the orphan; the live access token is a bearer credential and is kept. The null-guarded expiry check, previously inlined verbatim at three flow sites, moves into OAuthContext.registration_secret_expired() beside its sibling predicates, and the discard mechanics into OAuthContext.discard_expired_registration(), giving the rule a single home.
…non-vacuous MockTokenStorage stores tokens by reference and _initialize loads that same object into context.current_tokens, so the discard's in-place refresh-token trim reached the stored object even without the set_tokens persistence call - the storage-side assertions in test_expired_stored_registration_is_discarded_and_the_flow_re_registers passed vacuously. Re-seed storage with a distinct copy after the load so those assertions pass only if the discard actually persists the trimmed tokens (verified by mutation: deleting the set_tokens call now fails the test).
There was a problem hiding this comment.
Beyond the inline finding, this run also examined whether the SEP-2352 issuer-mismatch discard leaves the old issuer's tokens resurrectable from storage (its clear_tokens() is in-memory only) — ruled out: those call sites are pre-existing code unchanged by this PR, and after a restart the persisted record's issuer stamp is reloaded with the tokens, so the same issuer check re-fires before the old credentials could be presented.
Extended reasoning...
The candidate concern was that the SEP-2352 cross-issuer cleanup calls clear_tokens() without touching storage, so a restart could resurrect the old issuer's tokens. Verified this is not introduced by this PR: both clear_tokens() sites in the 401 flow's issuer checks (oauth2.py lines 756, 797) appear only as unchanged context in the diff, and the restart path reloads the old-issuer client_info alongside the tokens, so credentials_match_issuer fails again on the next 401 and the cleanup re-fires before any cross-issuer presentation. Recording it here so a later pass need not re-derive it.
…hen no AS metadata is cached After a restart _initialize() restores tokens and client info but never AS metadata, so a 403 insufficient_scope step-up that needs to re-register (lapsed secret, or no stored record) previously registered blind: it bypassed a configured CIMD URL (should_use_client_metadata_url is False without metadata) and POSTed the registration document to the resource origin's /register fallback — the wrong server in the standard separate-AS topology, failing every step-up until the access token expired. The 401 flow's discovery sequence (PRM + SEP-2352 issuer checks + ASM) now lives in a _discover_authorization_server_metadata sub-generator shared by both branches; the 403 step-up runs it before the expiry discard whenever oauth_metadata is None and re-registration is coming, so registration targets the discovered endpoint (or resolves CIMD). extract_resource_metadata_from_ www_auth now honors the resource_metadata parameter on 403 challenges too (RFC 9728 attaches it to any WWW-Authenticate challenge), so the step-up's discovery is seeded from the challenge header exactly like the 401 flow's. Covered by two restart-shaped step-up tests: DCR against the discovered registration endpoint, and CIMD resolution once discovery restores the advertised capability.
There was a problem hiding this comment.
Beyond the inline findings, this run also examined whether the SEP-2352 cross-issuer discard's in-memory-only clear_tokens() could, across a restart, pair the old issuer's still-persisted refresh token with a fresh registration — ruled out: the reloaded tokens carry no token_expiry_time, so is_token_valid() stays true and the refresh branch (the only path that would present them to the token endpoint) never runs; they surface only as a bearer header, and the ensuing 401 flow's token exchange overwrites the stored tokens.
Extended reasoning...
This run's two inline findings (the un-aclosed discovery sub-generator and the pre-existing 403 blind-authorize gap for valid registrations) already convey what needs attention, so no full review body is warranted. The one additional candidate examined — stale cross-issuer tokens surviving in storage because the SEP-2352 discard clears them only in memory — was refuted: after a restart, _initialize restores tokens without an expiry, the refresh branch is gated on not is_token_valid() and therefore never presents the orphaned refresh token, and the subsequent 401 flow either re-fires the issuer discard (old record still stamped) or completes an exchange whose set_tokens replaces the stale record. Recording it here so a later pass need not re-derive it.
| if self.context.oauth_metadata is None and ( | ||
| self.context.registration_secret_expired() or self.context.client_info is None | ||
| ): |
There was a problem hiding this comment.
🟣 Pre-existing issue (the 403 branch had no discovery before this PR either): the new step-up discovery guard runs only when re-registration is coming (registration_secret_expired() or client_info is None), but the restart state that motivated it — _initialize() restores tokens and client info but never oauth_metadata, and a reloaded live access token keeps the 401 flow's discovery from running — applies equally when the stored registration is still valid. In that case the step-up authorizes blind against urljoin(resource_origin, "/authorize") / "/token"), burning an interactive consent per 403 in the separate-AS topology; widening the guard to run discovery whenever oauth_metadata is None (dropping the second conjunct) would close it, since the SEP-2352 issuer checks no-op for a matching issuer.
Extended reasoning...
What the bug is. The 403 insufficient_scope step-up added in this PR runs the shared discovery sub-generator only under the guard at src/mcp/client/auth/oauth2.py:884-886:
if self.context.oauth_metadata is None and (
self.context.registration_secret_expired() or self.context.client_info is None
):i.e. only when re-registration is about to happen (lapsed secret, or no stored record). But the restart state that motivated commit 43d9599 — _initialize() restores only current_tokens and client_info, never oauth_metadata/protected_resource_metadata, and token_expiry_time is None after reload so is_token_valid() is True and the 401 flow's discovery never runs — applies just as much when the stored registration is valid. For that population the guard is False, discovery is skipped, and the step-up proceeds to _perform_authorization() with oauth_metadata still None.
The code path. With oauth_metadata=None, _perform_authorization_code_grant() falls back to urljoin(resource_origin, "/authorize") (the else branch around oauth2.py:417-420) and the exchange targets urljoin(resource_origin, "/token") via _get_token_endpoint(). In the standard separate-AS topology those endpoints do not exist on the resource server. Note the 403's WWW-Authenticate resource_metadata pointer is available on this path — extract_resource_metadata_from_www_auth now accepts 403s per this PR — but only the (skipped) discovery path reads it.
Step-by-step proof. (1) Storage holds a valid, unexpired DCR record and a live access token; the process restarts. First request: _initialize() loads both, token_expiry_time is None, so the Bearer header is attached and no 401 occurs — discovery never runs. (2) The request needs a broader scope: 403 insufficient_scope. The guard is False (record valid, client_info present), so no discovery runs. (3) Step 2b calls _perform_authorization(): the user's browser is sent to a dead https://resource-origin/authorize URL — a burned interactive consent — and the flow fails (the callback never completes, or the exchange 404s and _handle_token_response raises OAuthTokenError). (4) The except re-raises without resetting any state, so every subsequent 403 repeats identically until the access token itself expires and the 401 flow finally discovers for real.
Why existing safeguards miss it. The 401 flow always discovers before authorizing; the 403 branch relies on cached oauth_metadata. The guard added in 43d9599 restores discovery only for the re-registration populations (that fix targeted the blind registration POST), deliberately leaving the valid-record population out — which never registers, but still authorizes and exchanges against the fallback endpoints. No test covers oauth_metadata=None + valid record + 403: all the new restart-shaped 403 tests use a lapsed secret.
Why pre_existing, not normal. Before this PR the 403 branch performed no discovery at all, so the valid-record population's behavior is byte-for-byte unchanged — the PR neither introduced nor worsened it. All three verifiers agreed on this grading. It is still worth flagging here because the PR rewrote this exact branch and added the machinery (_discover_authorization_server_metadata) that would fix it.
How to fix. Widen the guard to run discovery whenever self.context.oauth_metadata is None — i.e. drop the second conjunct. Running discovery with a valid record is safe: the SEP-2352 issuer checks either pass (no-op for a matching issuer) or correctly discard cross-issuer credentials, and the step-up then authorizes at the discovered authorization_endpoint. Same one-line shape as the existing fix, plus a regression test for oauth_metadata=None + valid record + 403.
There was a problem hiding this comment.
Acknowledged — this is a real gap: after a restart with a still-valid registration, a 403 step-up authorizes against the {origin}/authorize fallback with no metadata cached. It's pre-existing behavior on main, though (the 403 branch never ran discovery before this PR), and this PR neither introduced nor worsens it — the guard added here deliberately covers only the re-registration path this PR touches. Widening it to any oauth_metadata is None case is better handled as a follow-up alongside the other eager-discovery work: #3263 covers the refresh path, and a follow-up can widen the 403/authorize path guard with its own regression test. Leaving code unchanged here.
Generated by Claude Code
…is closed mid-discovery httpx2's _send_handling_auth acloses the auth flow on any transport error or cancellation, throwing GeneratorExit at the relay's yield — caught by neither except StopAsyncIteration nor except Exception — so both hand-relay sites (the 401 branch and the 403 step-up) abandoned the suspended _discover_authorization_server_metadata sub-generator to GC (a ResourceWarning on trio, which filterwarnings=["error"] turns into a test failure for any future abort-mid-discovery test). Both relays now drive the sub-generator under contextlib.aclosing, matching the eager-refresh relay in #3263, so closing the outer flow finalizes it in the same unwind. Covered by two regression tests that close the flow mid-discovery and assert the captured sub-generator reports exhaustion instead of a live suspended frame.
There was a problem hiding this comment.
I reviewed this PR and found no new bugs in its final state (through aedc88c). Because it substantially reworks the OAuth client's credential lifecycle and the 401/403 flows — security-sensitive code — a human look is still worthwhile before merging.
What was reviewed:
- The expiry predicate (
stored_registration_expired) and its three call sites (refresh gate, 401 pre-Step-4 discard, 403 step-up), including its ordering after the SEP-2352 issuer checks — the cross-issuer cleanup test pins this. - The refresh-token drop in
discard_expired_registrationand its persistence, including the interrupted-flow and restart variants. - The discovery sub-generator extraction: both hand-relay sites are wrapped in
aclosing, and the widened 403resource_metadataextraction is scoped to 401/403 per RFC 9728. - All findings from earlier review rounds were fixed in follow-up commits; the one remaining item (403 step-up with a still-valid record and no cached AS metadata authorizes against the resource-origin fallback) is pre-existing on main and explicitly deferred to a follow-up in the thread.
Extended reasoning...
Overview
This PR fixes #3256: OAuthClientProvider persisted client_secret_expires_at but never read it back, so a lapsed dynamically-registered secret left the client permanently stuck on invalid_client. The fix adds an expiry predicate, a centralized OAuthContext.discard_expired_registration() (which also drops the refresh token issued to the discarded client_id and persists the trimmed tokens), gates the refresh path, discards just before Step 4 of the 401 flow, and adds a 403 step-up re-registration path with its own discovery. Along the way, the 401 flow's discovery sequence was extracted into a _discover_authorization_server_metadata sub-generator shared by both branches, driven under aclosing at both relay sites. Docs and ~860 lines of tests accompany the change.
Security risks
This is OAuth credential-lifecycle code: the risks are presenting a dead secret (information leak of the secret itself is moot — it is expired — but a doomed round trip), pairing an old client's refresh token with a fresh registration (RFC 6749 §6 invalid_grant), leaking the registration document to the wrong host via the resource-origin /register fallback, and skipping the SEP-2352 issuer-binding cleanup. Each of these was raised in earlier review rounds and fixed with a pinning regression test: the discard runs after the issuer checks, the refresh token goes with the discarded record (in memory and storage), and the 403 step-up now discovers AS metadata (honoring the 403's resource_metadata pointer) before re-registering. The widened extract_resource_metadata_from_www_auth (401 → 401/403) is consistent with RFC 9728, which attaches the parameter to any WWW-Authenticate challenge.
Level of scrutiny
High. This is production client auth code in a widely-used SDK, and the diff is large: a generator refactoring of the core async_auth_flow, new state-mutation paths that write to user-provided TokenStorage, and behavior changes documented as user-visible. That rules out shadow-approval regardless of how clean the final state looks — the guidelines reserve approval for simple, self-contained changes, and security-sensitive auth code is explicitly excluded. The multi-round review history (five substantive findings, each fixed and pinned by a test, verified across commits 50d6b0b → aedc88c) raises confidence but does not substitute for a maintainer weighing the design choices (e.g. keeping the live access token on discard, deferring reactive invalid_client invalidation, deferring the pre-existing 403 no-metadata authorize gap).
Other factors
Test coverage is strong: the predicate unit test covers the RFC 7591 edge cases (0 = never expires, absent field, non-secret auth methods), and the flow tests cover restart, mid-session lapse, cross-issuer + expiry interaction, interrupted-flow orphan prevention, CIMD resolution on both step-up shapes, and sub-generator finalization on mid-discovery close. One earlier finding about vacuous storage assertions was fixed by mutation-verified aliasing breakage (32a056d). The known remaining gap — a 403 step-up with a valid record and no cached metadata authorizing against the resource-origin fallback — is pre-existing on main, was explicitly acknowledged in the thread, and is scoped to a follow-up; it does not block this PR but is worth a maintainer's awareness when merging.
…stale no-cover pragma The mid-discovery close test traverses the 403 branch's except clause (GeneratorExit evaluates the match), so strict-no-cover now flags the 'pragma: no cover' on that line as wrongly marked. Cover the handler properly instead: a new test drives an OAuthFlowError (RFC 8707 resource mismatch during step-up discovery) through the branch, asserting it propagates to the caller like the 401 branch's error contract, and the pragma is removed.
There was a problem hiding this comment.
I reviewed this PR at its current revision (through 928b7bf) and found no new bugs — all findings from the earlier review rounds have been addressed. Because this is a large change to security-sensitive OAuth client code (registration, discovery, token handling), a human look is still worthwhile before merging.
What was reviewed this run:
- The expiry predicate and all three flow gates that consult it (refresh skip, 401 pre-Step-4 discard, 403 step-up), now centralized in
OAuthContext.registration_secret_expired()/discard_expired_registration(). - The extracted
_discover_authorization_server_metadatasub-generator: both hand-relay sites, theiraclosingfinalization, and the ordering of the SEP-2352 issuer checks before the expiry discard. - The orphaned-refresh-token drop and its persistence path (overwrite via
set_tokens, noTokenStoragecontract change), plus the widened 401/403resource_metadataextraction. - The eleven new tests, including the previously vacuous storage assertions fixed in 32a056d — no new gaps found.
Extended reasoning...
Overview
This PR fixes #3256: OAuthClientProvider persisted client_secret_expires_at but never read it back, so a dynamically registered client whose secret lapsed was permanently stuck failing invalid_client. The fix treats a secret-backed registration with a non-zero, past expiry as absent — checked at the refresh gate, in the 401 flow after the SEP-2352 issuer checks, and in the 403 insufficient_scope step-up — and re-registers (or resolves CIMD). Through five review rounds the PR also grew: centralized predicates on OAuthContext, a shared discovery sub-generator for the 401 and 403 branches (with aclosing finalization), an orphaned-refresh-token drop persisted through set_tokens, docs updates, and eleven new tests. It touches src/mcp/client/auth/oauth2.py (heavily), src/mcp/client/auth/utils.py (one guard widened to accept 403s), docs/client/oauth-clients.md, and tests/client/test_auth.py.
Security risks
This is authentication code, so the risk surface is real: when credentials are discarded, what re-registration targets, and which credentials are presented where. The scoping looks deliberate and correct — 0/absent expiry are kept per RFC 7591, none/private_key_jwt methods are exempt (they never present the minted secret), the refresh token is dropped with the record it was issued to (RFC 6749 §6) while the bearer access token is kept, and the 403 step-up now discovers AS metadata before registering rather than POSTing the registration document to the resource origin. The earlier rounds' findings (blind registration to the wrong host, orphaned refresh token presented under a new client identity) were all confirmed fixed in the current diff. One adjacent pre-existing gap (403 step-up with a valid record and no cached metadata authorizes against the resource-origin fallback) was examined and deliberately left as a follow-up, with a note already on the thread.
Level of scrutiny
Highest tier: this is production client-auth code in a widely used SDK, and the diff is large (~900 added test lines, a substantial refactor of async_auth_flow). Per the approval guidelines, security-sensitive auth code should not be shadow-approved regardless of how clean the review came back, and the discovery-sequence extraction plus the two-branch relay pattern is exactly the kind of structural change a human maintainer should sign off on.
Other factors
Test coverage is strong and was itself reviewed (a vacuous storage assertion was found and fixed by mutation-verified re-testing); the repo enforces 100% branch coverage and the author reports pyright/ruff clean. The PR is AI-authored via the maintainer triage workflow at a maintainer's request, and no human has reviewed it yet — another reason a human pass is the right next step. All prior bot findings are resolved on the thread, so this defer is the first and only review-body verdict, recording that the final revision was re-reviewed clean rather than restating any inline finding.
1 participant
Requested by Felix Weinberger · Slack thread
Note
AI disclosure: this PR was authored by Claude (an AI agent operated by the MCP maintainer team's triage workflow, at Felix Weinberger's request — see attribution above).
Fixes #3256.
Problem
OAuthClientProviderpersistsclient_secret_expires_at(RFC 7591) throughTokenStoragebut never reads it back, and dynamic registration only happens when stored client info is absent. Once a dynamically registered secret lapses — e.g. against an AS issuing 30-day secrets, or an SDK server withClientRegistrationOptions.client_secret_expiry_secondsset — every token-endpoint interaction fails withinvalid_client, including the exchange after a fresh interactive authorization, so the client is permanently stuck ("I re-authenticated and nothing changed") until the application manually deletes the persisted client info. With no RFC 7592 rotation endpoint, re-registration is the only standard recovery path, and only the SDK client can perform it.Fix
The minimal fix from the issue's "expected behavior" (1): a registration that authenticates with the minted secret (
client_secret_post/client_secret_basic) and carries a non-zero, pastclient_secret_expires_atis treated as absent —_initialize, when loading stored client info (restart case), andinvalid_client).The next 401 flow then re-registers (or resolves CIMD) and overwrites the dead record via the existing
set_client_infocall — no change to theTokenStoragecontract (unlike draft #3260, which makes the setters acceptNone).Deliberate scoping:
0means "never expires" and an absent field means no expiry was declared — both are kept (RFC 7591).token_endpoint_auth_method"none"/absent never present the secret, so a lapsed secret does not invalidate them.can_refresh_token()is false, so the refresh path that would present the lapsed secret is skipped._initializeand never load client info from storage.invalid_clienttoken response for servers that expire registrations without declaring it — is intentionally left out: it needs a way to delete (not just overwrite) persisted state, i.e. aTokenStoragecontract decision that deserves its own discussion.docs/client/oauth-clients.mdis updated in the same PR: theTokenStoragetip and the CIMD section's "storedclient_infostill wins" statement now mention the expired-secret discard.Tests
Three new tests in
tests/client/test_auth.py:post/basic),0= never expires, absent expiry, still-live expiry, and"none"-method registrations;POST /registerinstead of reusing the dead record;_initializealready loaded it is discarded by the 401 handler, which then re-registers.ruff format/ruff checkclean,pyrightstrict 0 errors,tests/client/test_auth.py+tests/client/auth+tests/interaction/authpass with 100% branch coverage onsrc/mcp/client/auth/oauth2.py.