OAuth client: refresh before re-authorizing, and discover before refreshing - #3328
OAuth client: refresh before re-authorizing, and discover before refreshing#3328maxisbey wants to merge 1 commit into
Conversation
…eshing OAuthClientProvider had two disconnected ways to obtain a token: a pre-request branch that could refresh but never discovered metadata, and a 401 branch that discovered but never tried the refresh token. After a process restart the first is unreachable (no expiry is restored, so the loaded token always reads as valid) and the second goes straight to interactive authorization, so every restarted client re-opened the browser once its access token lapsed and headless clients failed outright. When an application forced the pre-request path, the refresh was posted to a path guessed from the server origin and 404ed against any authorization server mounted under a path. Separately, a dynamically registered client whose secret had expired was reused forever: the stored client_secret_expires_at was never read and invalid_client from the token endpoint was not recognised, so each attempt spent a consent and failed at the code exchange. Both entry points now drive one sequence, _reacquire_tokens: discover (always on a 401, and before a cold-start refresh when nothing is cached), drop a registration the SDK minted whose secret has lapsed, try the refresh_token grant when one is held, and only then run the provider's full grant. invalid_client from the token endpoint discards an SDK-minted registration and its tokens and runs registration once more; pre-registered credentials surface the error instead. _initialize derives an expiry from the loaded token unless the application already set one. The 2025-03-26 origin-path fallbacks remain for servers with no metadata but are no longer reached merely because state was not loaded. Closes #3240, #3250, #3256, #1318.
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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, two adjacent concerns were examined and ruled out: the invalid_client discard-and-reregister path also replacing CIMD registrations (harmless — Step 4 deterministically re-derives the same registration from the client metadata URL, so no extra consent or identity change), and the post-401 pass re-running PRM/ASM discovery when a cold-start pass already ran it within the same request (matches the stated design — a 401's WWW-Authenticate hints may point at a changed AS, so rediscovery there is intentional).
Extended reasoning...
Findings were confirmed and will be posted inline, so the body is limited to recording what else was investigated and ruled out. The CIMD variant of the invalid_client recovery concern (src/mcp/client/auth/oauth2.py around lines 744-765) was ruled out because create_client_info_from_metadata_url rebuilds the identical URL-based registration non-interactively, unlike the pre-registered-credentials case that did survive as a finding. The duplicate-discovery concern (lines 640-642 and the 855 entry point) was ruled out as intended behavior per the code's own Step 1-2 comment ("Always on a 401 — the AS may have changed"). The bug hunt exited on max_rounds and several verified findings were pruned from posting, so this change should not be approved without a human pass over the auth flow; the inline comments already signal that.
Additional findings (outside current diff — PR may have been updated during review):
-
🟣
src/mcp/client/auth/oauth2.py— The new lapsed-secret discard and invalid_client re-registration recovery apply only inside _reacquire_tokens (401/cold-start paths); the 403 insufficient_scope step-up branch still calls _perform_authorization() and _handle_token_response() directly with no client_secret_lapsed() check and no token_error_code() handling, so an SDK-minted registration whose secret has lapsed spends a user consent at /authorize and then dies with OAuthTokenError at the code exchange - the exact #3256 failure this PR fixes elsewhere, and a direct contradiction of the new docs/client/oauth-clients.md claim that a registration whose secret expired or that the token endpoint rejects with invalid_client is always discarded and re-registered. (The 403 branch itself predates this PR, but the recovery mechanism itExtended reasoning...
A client registered against an AS that issues expiring DCR secrets (the PR's own auth_settings(client_secret_expiry_seconds=...) scenario) holds a still-valid access token when the secret window passes. The server answers a request with 403 insufficient_scope (SEP-2350 step-up). async_auth_flow's 403 branch sends the user through redirect_handler/callback_handler - the AS's /authorize does not check the secret, so consent is granted - then the code exchange authenticates with the dead secret, the token endpoint answers 400/401 invalid_client, and _handle_token_response raises OAuthTokenError (lines 891-892). The consent is wasted and the operation fails; every subsequent step-up repeats the consent-then-error cycle until the access token itself expires and a 401 finally routes through _reacquire_tokens, which discards and re-registers. Routing the 403 exchange through the same lapsed-secret check and token_error_code recovery (or through _reacquire_tokens) would make the step-up recover headlessly like the 401 path does.
Verification: pre-existing — the candidate is factually correct, though the defective branch itself is untouched by this diff. The 403 insufficient_scope step-up branch at src/mcp/client/auth/oauth2.py:869-899 calls
token_response = yield await self._perform_authorization()thenawait self._handle_token_response(token_response)(lines 891-892) directly, with noclient_secret_lapsed()discard (that check l
| if self.tokens is not None and self.report_expired_on_load: | ||
| return self.tokens.model_copy(update={"expires_in": 0}) |
There was a problem hiding this comment.
🔴 report_expired_on_load stands in for "already expired" with expires_in=0, but that makes _initialize set token_expiry_time = time.time() and is_token_valid() uses the inclusive test time.time() <= token_expiry_time microseconds later - on Windows CPython 3.10-3.12, where time.time() is backed by GetSystemTimeAsFileTime with ~15.6 ms granularity (the precise clock was only adopted in 3.13), both calls return the same value, so the token is judged still valid and the cold-start refresh path never runs.
Extended reasoning...
On the windows-latest x {3.10, 3.11, 3.12} CI entries (matrix at .github/workflows/shared.yml line 75), test_a_refresh_before_the_first_request_discovers_metadata_and_posts_to_the_advertised_token_endpoint and test_a_stored_registration_with_a_lapsed_secret_is_replaced_before_authorizing enter async_auth_flow, _initialize() derives expiry A from the copied token's expires_in=0, and the not is_token_valid() gate evaluates time.time() (== A within the same 15.6 ms clock tick) <= A as True. The cold-start _reacquire_tokens(challenge=None) pass is skipped, the stale bearer (still valid server-side in both tests - neither calls expire_access_token) is sent to /mcp and succeeds, so recorded[0] is ("POST", "/mcp") instead of the discovery GET and the snapshot assertions fail - a near-deterministic failure on those matrix entries, not just a flake. Fix: return the copy with a strictly negative expires_in (e.g. -1), which OAuthToken permits (expires_in: int | None has no ge=0 constraint).
Verification: normal. The chain is real and each link checks out. (1) /home/claude/python-sdk/tests/interaction/auth/_harness.py:131-134: get_tokens returns self.tokens.model_copy(update={"expires_in": 0}). (2) /home/claude/python-sdk/src/mcp/client/auth/oauth2.py:584-585 (new in this PR): _initialize derives expiry via update_token_expiry -> calculate_token_expiry(0) which is time.time() + int(0)
| if ( | ||
| may_reregister | ||
| and self._sdk_minted_registration() | ||
| and await token_error_code(refresh_response) == "invalid_client" | ||
| ): | ||
| self._discard_registration("Token endpoint rejected the client credentials") | ||
| may_reregister = False | ||
| continue |
There was a problem hiding this comment.
🟡 Pre-existing, extended by this diff: when the token endpoint answers invalid_client to the Step 5a refresh but the registration is NOT SDK-minted (pre-registered credentials, or the client-credentials providers' fixed client_info which carries no issuer stamp), the new guard at lines 803-810 skips the discard and the flow falls through to Step 5b _perform_authorization(), running the full grant with the exact same client authentication the server just rejected. For the interactive provider this launches a browser consent that is guaranteed to fail at the code exchange with the same invalid_client (the exchange uses the same prepare_token_auth secret/client_id), only then raising OAuthTokenError. The PR and the updated client-auth:invalid-client-clears-all manifest entry claim
Extended reasoning...
An operator pre-registers a confidential client (no issuer stamp) and its secret is rotated or revoked on the AS. The stored access token expires; the next request 401s; _reacquire_tokens(challenge=response) tries the held refresh token, the token endpoint answers 401 {"error":"invalid_client"}, the guard at line 803-810 does nothing (not SDK-minted), and execution reaches Step 5b at line 817: the user's browser is opened, they complete a consent at /authorize, and the subsequent code exchange fails with the same invalid_client, surfacing OAuthTokenError only after the pointless interactive round trip. A headless deployment with pre-registered credentials instead crashes at the missing redirect handler rather than getting the documented OAuthTokenError.
Verification: nit — the mechanics check out but the end state is the designed one, just after one doomed consent. At src/mcp/client/auth/oauth2.py:803-810 the invalid_client re-register guard requires self._sdk_minted_registration() (line 621: client_info.issuer is not None), so a pre-registered client whose secret was rotated gets no discard and no early error; _handle_refresh_response (548-551) merely c
| client_info = self.context.client_info | ||
| if client_info is not None and client_info.issuer is not None and client_secret_lapsed(client_info): |
There was a problem hiding this comment.
🟡 [quality] Secret-lapse check inlines the SDK-minted provenance test (client_info.issuer is not None) instead of calling the _sdk_minted_registration() helper defined ~100 lines above for exactly this purpose
Extended reasoning...
Duplicated provenance logic: the same "is this registration ours to replace" rule now lives in two places (the helper used by the invalid_client paths at lines 805/820, and the inline copy at line 728). If the provenance test ever changes (e.g. an additional stamp or a CIMD carve-out), the secret-lapse site can silently diverge from the invalid_client sites. if self._sdk_minted_registration() and client_secret_lapsed(client_info) (with the existing client_info local for narrowing) does the same job with one source of truth.
Verification: nit — the claim is factually accurate. /home/claude/python-sdk/src/mcp/client/auth/oauth2.py:613-621 defines _sdk_minted_registration() whose body is exactly return self.context.client_info is not None and self.context.client_info.issuer is not None, documented as the SEP-2352 provenance test for registrations the SDK may replace. Line 728 re-states that same test inline: `if client_info is no
| """ | ||
| # Step 1-2: discovery. Always on a 401 (the AS may have changed); before a cold-start | ||
| # refresh only when nothing has been discovered yet in this process. | ||
| if challenge is not None or self.context.oauth_metadata is None: |
There was a problem hiding this comment.
🟡 [quality] Cold-start discovery gate challenge is not None or self.context.oauth_metadata is None only remembers successful discovery, so against a legacy server that publishes no metadata (both PRM and ASM discovery 404), every pre-request refresh in the process re-runs the entire failing PRM+ASM discovery URL chain before posting the refresh.
Extended reasoning...
Concrete cost: wasted repeated I/O. For a 2025-03-26-style server with no /.well-known metadata, oauth_metadata stays None forever, so each time the access token expires in a long-lived process the pre-request branch in async_auth_flow (line 838) issues several guaranteed-404 GETs (all PRM discovery URLs plus all ASM fallback URLs) before the refresh POST to {origin}/token — every token lifetime, for the life of the process. Before this change the cold refresh posted directly. Tracking 'discovery attempted this process' (a flag set after the discovery block runs) instead of gating on oauth_metadata is None gives the intended 'only when nothing has been discovered yet in this process' behavior without the repeated failing round-trips.
Verification: nit — the claim is factually accurate. The cold-start gate at src/mcp/client/auth/oauth2.py:642 is if challenge is not None or self.context.oauth_metadata is None:, and the only thing that ever suppresses re-discovery on the pre-request path is a successful ASM discovery (self.context.oauth_metadata = asm inside the Step-2 loop). Against a legacy 2025-03-26 server that publishes no metadata,
| # Derive an expiry from the loaded token unless the application already set one (some | ||
| # storages assign `context.token_expiry_time` before or during `get_tokens`). A storage | ||
| # that persists tokens verbatim yields a stale window here; that costs one 401 round-trip, | ||
| # after which `_reacquire_tokens` refreshes. | ||
| if self.context.current_tokens is not None and self.context.token_expiry_time is None: | ||
| self.context.update_token_expiry(self.context.current_tokens) |
There was a problem hiding this comment.
🟡 [quality] The #3250 fix (derive token_expiry_time from the loaded token) is applied only in OAuthClientProvider._initialize; ClientCredentialsOAuthProvider._initialize and PrivateKeyJWTOAuthProvider._initialize (src/mcp/client/auth/extensions/client_credentials.py lines 79-81 and 291-295) override it with duplicated token-loading code that still leaves token_expiry_time None, so the exact defect this PR fixes for the base class - a persisted token treated as valid forever - persists for the M2M providers. Fix at shared depth: put load-plus-derivation in one helper (or in OAuthContext) that all three _initialize implementations call.
Extended reasoning...
Concrete cost: divergent behavior plus duplicated logic. A restarted ClientCredentialsOAuthProvider/PrivateKeyJWTOAuthProvider whose storage reports the token's remaining lifetime via expires_in (the same storage shape the new report_expired_on_load models) still gets token_expiry_time=None, is_token_valid() reads that as valid, and the stale bearer goes out and draws a 401 on every restart instead of the documented cold-start refresh (docs/client/oauth-clients.md now promises 'an expired access token is refreshed... before the next request'). Meanwhile the three near-identical _initialize bodies mean the next change to token loading must be made in three places - the identity_assertion extension already derives expiry on load, showing a fourth divergent copy of the same logic.
Verification: nit — the factual claim checks out. The new derivation lives only in the base class: src/mcp/client/auth/oauth2.py:584-585 (if self.context.current_tokens is not None and self.context.token_expiry_time is None: self.context.update_token_expiry(...)). Both M2M subclasses override _initialize with duplicated token-loading that skips it: src/mcp/client/auth/extensions/client_credentials.py:77-81
1 participant
OAuthClientProvidernow recovers from an expired access token the way the other SDKs do: on a 401 it rediscovers, tries the stored refresh token, and only falls back to interactive authorization if that fails. A refresh attempted before the first request of a process discovers metadata first instead of guessing{origin}/token. A dynamically registered client whose secret has expired (or that the token endpoint rejects withinvalid_client) is discarded and re-registered once instead of being reused forever.Closes #3240, closes #3250, closes #3256, closes #1318.
Motivation and Context
The provider had two disconnected ways to obtain a token. The pre-request branch could refresh but never ran discovery; the 401 branch ran discovery but never tried the refresh token. After a restart the first branch is unreachable (
_initializerestores no expiry, andis_token_valid()reads "no expiry" as valid), so the stale bearer goes out, the server 401s, and the flow goes straight to the browser with a perfectly good refresh token in hand. Headless clients just fail. If an application forced the pre-request path by reporting the loaded token as expired, the refresh was posted to a path derived from the server origin, which 404s for any authorization server under a path, and the refresh token was then thrown away. #1318 reported this a year ago; #3240 and #3250 are the same defect from different angles.The 401 branch did try the refresh token in 1.10/1.11; #1071 moved that block ahead of the request rather than duplicating it, and nothing tested the restart path, so it went unnoticed.
#3256 is adjacent:
client_secret_expires_atwas persisted but never read, and neither token-response handler looked at the RFC 6749errormember, so once a DCR secret lapsed every attempt spent a consent at/authorizeand died at the code exchange withinvalid_client.What changed
_reacquire_tokens(challenge), an async generator both entry points drive. Order: PRM/ASM discovery (always on a 401; before a cold-start refresh only when nothing is cached) → SEP-2352 issuer-binding checks (unchanged) → drop an SDK-minted registration whose secret has lapsed → scope selection (401 only) → register/CIMD if needed → refresh_token grant if one is held → the provider's full grant. Withchallenge=None(cold start) it never registers or authorizes unprompted; it refreshes if it can and otherwise lets the request go out so a real 401, with itsWWW-Authenticatehints, drives the rest.invalid_clientfrom the token endpoint (refresh or code exchange, 400 or 401) discards the registration and its tokens and runs registration once more. Only registrations the SDK minted are treated this way — they carry the SEP-2352issuerstamp, the same provenance testcredentials_match_issueruses — so pre-registered credentials and the client-credentials providers' fixedclient_infosurfaceOAuthTokenErrorrather than being swapped for a dynamic client._initializederivestoken_expiry_timefrom the loaded token unless the application already set it. For a storage that persists the token verbatim that window is stale, which now costs one 401 round trip rather than a re-authorization.client_secret_lapsed()andtoken_error_code(); two duplicated "discard bound credentials" blocks fold into_discard_registration().{origin}/token|/authorize|/registerfallbacks are untouched; they're just no longer reached because state wasn't loaded.No public API changes;
TokenStorageis unchanged. Most of theoauth2.pyline count is the existing 401 body re-indented into the new method (git diff -wis +150/−37).How Has This Been Tested?
Six interaction tests against the SDK's own AS+RS in process (a first "process" logs in, a fresh provider over the same storage is the restart): refresh on 401 with no handlers wired; cold-start refresh against an AS under
/oauth2/v1hitting the advertised endpoint; lapsedclient_secret_expires_atreplaced before any consent;invalid_clienton refresh → re-register;invalid_clientat the code exchange → re-register and authorize once more; pre-registered credentials → error surfaces, no/register. Unit tests for the two helpers and the_initializeguard. Manifest gainsclient-auth:refresh:on-401,client-auth:refresh:discovered-endpoint,client-auth:registration:secret-expiry, andclient-auth:invalid-client-clears-allloses its deferral. The harness grows a path-prefixed-AS shim, areport_expired_on_loadstorage knob andclient_secret_expiry_secondsonauth_settings(); the server-side "Client secret has expired" branch is now executed, so its pragma goes.Also driven over real sockets: uvicorn serving the SDK AS (3 s access tokens, expiring DCR secrets, then the AS under a prefix), and a separate application process with file-backed storage run repeatedly against it. Headless runs after expiry show
401 → PRM → ASM → POST …/token grant_type=refresh_token → 200; after the secret window, one consent with a new client id and headless refreshes thereafter; with the AS restarted (all registrations forgotten) the headless run re-registers and stops cleanly asking for a browser.The restart path had no coverage before this (no test loaded tokens through
_initialize, none entered the 401 branch holding a refresh token), which is why nothing existing changes.Breaking Changes
None intended. Behavioural notes: a 401 with a refresh token in storage now produces a token-endpoint request before any redirect; applications that pre-seed
context.token_expiry_timeorcontext.oauth_metadatakeep working (both are respected);_initialized = Falseafter a failed refresh is preserved.Types of changes
Checklist
Additional context
TokenStorage/stored-token contract question — Critical Token Refresh Bugs - Prevents Proactive Refresh #1318 has the discussion); a way for storage to forget tokens, so a registration discarded in a process that can't complete authorization doesn't leave a staletokens.jsonthat costs one rejected refresh next time; classifying refresh failures so a 5xx doesn't drop the refresh token in memory.token_expiry_timeupon context initialization for stored tokens. #1784, feat(auth): persist oauth_metadata via TokenStorage to fix refresh after restart #2492, fix(client/auth): use stored refresh_token on 401 instead of full re-authorization #2875, fix: restore OAuth token expiry across process restarts #3248, fix(client/auth): discover AS metadata before cold-start token refresh, with issuer-binding check #3263, fix(client/auth): discard stored client registrations whose secret has expired #3264.AI Disclaimer