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 5 commits into
Conversation
📚 Documentation preview
|
![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
…eshing The 401 branch of OAuthClientProvider discovered metadata but never tried the stored refresh token, and the pre-request refresh tried the refresh token but never discovered metadata. After a restart nothing restores an expiry, so the pre-request path is skipped, the stale bearer draws a 401, and the flow went straight to interactive authorization with a usable refresh token in hand; headless clients failed outright. When an application forced the pre-request path, the refresh was posted to a path guessed from the server origin, which 404s against an authorization server under a path, and the refresh token was dropped. The 401 branch now tries the refresh_token grant after discovery and registration and runs the full authorization only when there is no refresh token or the server rejects it. The pre-request refresh runs only when authorization server metadata is already known, so it never guesses an endpoint; a cold start takes the 401 and refreshes there. A fresh dynamic registration clears any held tokens, which belonged to a previous client. Closes #3240, #3250, #1318.
07d639a to
2bc618a
Compare
There was a problem hiding this comment.
Beyond the inline findings, this run also examined and ruled out: clear_tokens() after re-registration leaving the old tokens in TokenStorage (in-memory state governs the rest of the flow, so it is a persistence nit, not a correctness break); the Step 5 refresh running without a metadata guard when every discovery URL fails (it falls back to the same guessed-endpoint behaviour as before, not worse); and the refresh request going out unauthenticated for PrivateKeyJWTOAuthProvider (pre-existing — _refresh_token() was never overridden there, this PR only widens exposure).
Extended reasoning...
This run confirmed two findings on the new 401-branch refresh in src/mcp/client/auth/oauth2.py (posted inline at lines 757–758), so approval is not on the table — additionally, verified findings exist that will not be posted, which independently blocks approval. The body is limited to the informational ruled-out note covering candidates newly investigated this run that do not overlap the posted inline comments or the prior review's comments: the storage-vs-memory scope of the new clear_tokens() call after dynamic re-registration, the absence of a discovered-metadata guard on the Step 5 refresh when discovery fails entirely, and the unauthenticated refresh for the private-key-JWT provider, which predates this PR. The remaining ruled-out candidate (refresh token presented to a newly discovered AS for CIMD/unstamped registrations) substantially matches the confirmed finding at line 757 and is therefore not listed as ruled out.
…ests - A failed refresh resets _initialized inside _handle_refresh_response, so the 401-path refresh site behaves like the pre-request one: the next request re-reads storage and can retry a refresh token that is still good instead of leaving a long-lived headless provider with no tokens. - A CIMD client_id is portable across authorization servers but its tokens are not: when the discovered issuer differs from the record's stamp, keep the record, drop the tokens, and re-stamp before deciding to refresh. - Tests pinning: rejected 401-path refresh falls back to authorization; a headless provider retries the refresh on its next connection; a fresh registration does not present tokens left from a previous client; the CIMD issuer-change case. Manifest wording for the discovered-endpoint entry no longer overclaims about the no-metadata fallback.
There was a problem hiding this comment.
Beyond the inline findings, I also checked two things new to this revision and ruled them out: (1) the new clear_tokens() calls (fresh DCR registration, CIMD issuer change) drop tokens only in memory while the client record is persisted — but the TokenStorage protocol has no token-removal API, so an in-process clear is the strongest drop available today, and the PR already flags a storage "forget tokens" capability as follow-up work; (2) _handle_refresh_response leaving _initialized = False after a failure that the same flow then recovers from via full authorization is harmless, since the next request's re-_initialize re-reads the tokens _handle_token_response just persisted.
Extended reasoning...
Two confirmed findings are posted inline (the CIMD issuer-change block persisting a new issuer stamp without dropping stale old-AS metadata, and the double-refresh when the pre-request refresh succeeds but the RS still 401s), so the inline comments already signal that a human should look. The hunt exited on max_rounds, so approval is off the table regardless. This note only records two concrete candidates investigated and ruled out in this run that were not covered in my two earlier reviews: the storage-persistence gap of the new token drops (bounded by TokenStorage having only get_tokens/set_tokens — verified in src/mcp/client/auth/oauth2.py:127-134 — and explicitly deferred by the PR to the stored-token-contract follow-up) and the non-restored _initialized flag after an in-flow recovery (benign because _initialize re-reads the storage that _handle_token_response just updated). Neither is a repeat of prior feedback, and naming them saves a human reviewer from re-chasing the same questions about the new clear_tokens() call sites.
| # Step 5: Refresh with the stored refresh token first (RFC 6749 §6); run the full | ||
| # authorization only when there is none or the server rejects it. | ||
| refreshed = False | ||
| if self.context.can_refresh_token(): | ||
| refresh_response = yield await self._refresh_token() | ||
| refreshed = await self._handle_refresh_response(refresh_response) |
There was a problem hiding this comment.
🟡 nit: 401-branch Step 5 re-refreshes a token the pre-request branch minted seconds earlier in the same flow — no flag records that a refresh already ran this request, so a 401 on the fresh token triggers an immediate second refresh_token POST instead of falling through to authorization.
Extended reasoning...
Concrete cost: doubled token-endpoint traffic and refresh-token rotation churn with no recovery path. When the pre-request refresh (lines 594-600) succeeds but the resource server still 401s the freshly minted access token (verifier/introspection lag, RS-side revocation, audience misconfig), can_refresh_token() is still True at line 769 (the carried-forward refresh_token from lines 539-540), so every request performs refresh POST -> 401 -> second refresh POST -> retry 401, and because the AS keeps answering 200 to refreshes, refreshed stays True and the interactive re-authorization at line 772 is never reached — the caller just sees repeating 401s at twice the token-endpoint cost. Tracking 'refreshed this flow' (skip Step 5's refresh when the pre-request one already succeeded) removes the duplicate POST.
Verification: nit — the claim is factually true. In /home/claude/python-sdk/src/mcp/client/auth/oauth2.py the pre-request branch (lines 594-600) refreshes an expired token when oauth_metadata is known; on success _handle_refresh_response stores the fresh token and carries the refresh token forward (lines 539-540: `if token_response.refresh_token is None and prior is not None: token_response.refresh_token =
…data is dropped too The CIMD token drop ran after authorization-server metadata rediscovery, so when that rediscovery failed the previous server's cached metadata survived under a record already re-stamped with the new issuer. Doing it at the same point as the existing bound-credentials guard, right after PRM names the issuer, drops the cached metadata as that guard does. A unit test pins all three effects.
…okens, then stamp it Records this SDK creates for a client ID metadata document always carry the issuer stamp, so an unstamped one comes from an older store and its tokens have no confirmed origin. Dropping them once and stamping the record converges after a single re-authorization instead of presenting a refresh token of unknown provenance to whichever server PRM names.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/client/test_auth.py">
<violation number="1" location="tests/client/test_auth.py:3332">
P3: Asserting `mock_storage._client_info is provider.context.client_info` reaches into the mock's private attribute and checks object identity, which only passes because `set_client_info` stores the exact mutated object. Use the public API instead so the test verifies the re-stamped issuer is actually persisted rather than that the same object was assigned: `stored = await mock_storage.get_client_info(); assert stored is not None; assert stored.client_id == cimd_url; assert stored.issuer == "https://new-as.example.com"`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| cimd_url, | ||
| "https://new-as.example.com", | ||
| ) | ||
| assert mock_storage._client_info is provider.context.client_info |
There was a problem hiding this comment.
P3: Asserting mock_storage._client_info is provider.context.client_info reaches into the mock's private attribute and checks object identity, which only passes because set_client_info stores the exact mutated object. Use the public API instead so the test verifies the re-stamped issuer is actually persisted rather than that the same object was assigned: stored = await mock_storage.get_client_info(); assert stored is not None; assert stored.client_id == cimd_url; assert stored.issuer == "https://new-as.example.com".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/client/test_auth.py, line 3332:
<comment>Asserting `mock_storage._client_info is provider.context.client_info` reaches into the mock's private attribute and checks object identity, which only passes because `set_client_info` stores the exact mutated object. Use the public API instead so the test verifies the re-stamped issuer is actually persisted rather than that the same object was assigned: `stored = await mock_storage.get_client_info(); assert stored is not None; assert stored.client_id == cimd_url; assert stored.issuer == "https://new-as.example.com"`.</comment>
<file context>
@@ -3279,3 +3279,55 @@ async def test_expired_token_is_not_refreshed_ahead_of_the_request_before_metada
+ cimd_url,
+ "https://new-as.example.com",
+ )
+ assert mock_storage._client_info is provider.context.client_info
+ await auth_flow.aclose()
</file context>
| assert mock_storage._client_info is provider.context.client_info | |
| stored = await mock_storage.get_client_info() | |
| assert stored is not None | |
| assert (stored.client_id, stored.issuer) == (cimd_url, "https://new-as.example.com") |
…iscovery points The bound-credential discard and the CIMD keep-but-rebind case were spelled out inline where PRM names the issuer, and only the former where the issuer is first learned from AS metadata on the legacy no-PRM path. One method now applies both rules wherever the issuer becomes known, so a CIMD record's carried-over tokens are dropped on that path too. A unit test covers it.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/mcp/client/auth/oauth2.py— The new CIMD issuer-change guard (elif at lines 655-666: drop tokens, drop cached metadata, re-stamp the record) is gated onauth_server_url is not None, so it only runs on the PRM discovery path. The legacy no-PRM path's mirrored SEP-2352 re-evaluation block (lines 692-704) re-checkscredentials_match_issueragainst the discovered ASM issuer, but that helper short-circuits True for CIMD records (utils.py:350-351) and no CIMD branch was mirrored there — so a stamped CIMD record whose issuer changed is neither re-stamped nor has its tokens dropped on the legacy path, and the PR's new Step 5 refresh (lines 766-768) then presents the old AS's refresh token to the new AS. Distinct from the prior finding about unstamped/pre-registered records: here the issuer stamp exists and the mismatch…Extended reasoning...
A client uses
client_metadata_urlagainst a 2025-03-26-era server that publishes no protected-resource metadata but serves origin AS metadata advertising client_id_metadata_document_supported; Step 4's legacy stamp records issuer=OLD (line 719 usesstr(oauth_metadata.issuer)when auth_server_url is None) and storage holds a refresh token minted by OLD. The deployment later migrates to a different authorization server: PRM probes still 404 (auth_server_url stays None) and the origin ASM now returns metadata with issuer=NEW and NEW's endpoints. On the next 401, the elif at 655-666 is skipped (auth_server_url is None), the legacy re-evaluation at 692-704 returns without clearing anything because credentials_match_issuer short-circuits True for the CIMD client_id, Step 4 is skipped (client_info present), and the new Step 5 sees can_refresh_token() True and POSTs OLD's long-lived refresh token to NEW's token_endpoint — disclosing the credential to a server it was never issued by (exactly what the elif was added to prevent; before this PR the 401 branch never refreshed, so no disclosurVerification: nit — the asymmetry is real and mechanically verifiable at HEAD in /home/claude/python-sdk/src/mcp/client/auth/oauth2.py. (1) The new CIMD issuer-change branch is gated to the PRM path: the elif at lines 655-660 requires
self.context.auth_server_url is not None(line 658) before it clears tokens, drops cached metadata, and re-stamps the record (lines 663-666). (2) The legacy no-PRM re-evaluation -
🟡
src/mcp/client/auth/oauth2.py— Pre-existing, extended by this PR:_refresh_token(lines 497-501) re-implements_get_token_endpoint()(lines 433-439) verbatim — same metadata check, same origin fallback — so the token-endpoint selection rule this PR is correcting (#3240) lives in two places, and the fix was applied as a gate at one call site (line 597) instead of once in the shared helper.Extended reasoning...
Concrete cost: duplicated logic plus a fix at the wrong depth. The PR's whole point is "post the refresh to the discovered token endpoint", yet the endpoint choice for refresh remains a private copy of _get_token_endpoint's body; any future change to endpoint selection (e.g. deriving the fallback from auth_server_url, or dropping the origin guess) must be made twice or the refresh path silently diverges from the code-exchange path — exactly the divergence that produced #3240. Replacing lines 497-501 with
token_url = self._get_token_endpoint()removes the copy and gives one place to fix the fallback.Verification: nit — the factual claim is verbatim true.
_get_token_endpoint()at src/mcp/client/auth/oauth2.py:433-439 is:if self.context.oauth_metadata and self.context.oauth_metadata.token_endpoint: token_url = str(...token_endpoint)elseauth_base_url = self.context.get_authorization_base_url(self.context.server_url); token_url = urljoin(auth_base_url, "/token")._refresh_tokenat lines 497-501 re
| # Step 5: Refresh with the stored refresh token first (RFC 6749 §6); run the full | ||
| # authorization only when there is none or the server rejects it. | ||
| refreshed = False | ||
| if self.context.can_refresh_token(): | ||
| refresh_response = yield await self._refresh_token() | ||
| refreshed = await self._handle_refresh_response(refresh_response) |
There was a problem hiding this comment.
🔴 The new 401-branch Step-5 refresh POSTs the refresh token (and client secret) to the origin-guessed {resource-server-origin}/token whenever AS metadata discovery fails, because _refresh_token (lines 497-501) still falls back to urljoin(get_authorization_base_url(self.context.server_url), "/token") and, unlike the newly gated pre-request site (lines 594-598), the Step-5 call site is not conditioned on oauth_metadata is not None. When PRM succeeded and named an AS that is not at the resource server's origin (different host, or a path-based AS), that guess targets the wrong server entirely — contradicting the PR's own claim that the client "never guesses {origin}/token"; the manifest note in tests/interaction/_requirements.py only blesses the fallback for the legacy…
Extended reasoning...
A restarted headless client holds a valid persisted refresh token for an AS at https://as.example.com/oauth2/v1 (RS at https://rs.example.com). The stale bearer draws a 401; PRM discovery succeeds and sets auth_server_url to the AS; the AS's metadata endpoint returns a transient 502, so handle_auth_metadata_response (utils.py:233-234) returns (False, None), the Step-2 loop breaks, and oauth_metadata stays None (OAuthMetadata.token_endpoint is required, so the fallback fires exactly when discovery failed). Step 5 then runs: can_refresh_token() is True, _refresh_token() builds token_url = "https://rs.example.com/token" — the resource server's origin, never the AS — and POSTs grant_type=refresh_token with the refresh token and the client secret (prepare_token_auth) to that host, disclosing long-lived credentials to a party that was only ever meant to see the access token. The guaranteed 404/non-200 makes _handle_refresh_response discard the tokens and fall through to _perform_authorization, which raises OAuthFlowError for the headless client — so one transient metadata 5xx both leaks
Verification: normal — the candidate is mechanically accurate and the failure is newly reachable through the diff-added Step-5 call site. Chain, all in /home/claude/python-sdk/src/mcp/client/auth/oauth2.py at HEAD: (1) the new 401-branch Step 5 (lines 765-768) is if self.context.can_refresh_token(): refresh_response = yield await self._refresh_token() — unlike the pre-request site this PR gated (lines 594-598
| if self.context.can_refresh_token(): | ||
| refresh_response = yield await self._refresh_token() |
There was a problem hiding this comment.
🟡 nit, pre-existing extended by this diff: the new 401-branch Step 5 refresh (and the pre-request refresh it complements) builds the refresh request via the base _refresh_token() -> prepare_token_auth(), which adds no client authentication for token_endpoint_auth_method="private_key_jwt" (oauth2.py:271-274 explicitly defers the assertion to "the provider that implements it"), but PrivateKeyJWTOAuthProvider only adds its client_assertion in _exchange_token_client_credentials (src/mcp/client/auth/extensions/client_credentials.py:303-315) and never overrides _refresh_token. Every refresh_token grant sent by a private_key_jwt client is therefore unauthenticated and is rejected by the AS with invalid_client, so refresh can never succeed for these clients; the diff adds a second…
Extended reasoning...
A PrivateKeyJWTOAuthProvider talks to an AS that issues refresh tokens on the client_credentials grant (e.g. Keycloak's legacy default). When the access token expires server-side, the next request 401s; discovery runs and Step 5 at oauth2.py:766-768 sees can_refresh_token() True and POSTs grant_type=refresh_token with only client_id in the body — no client_assertion — so the AS answers 400/401 invalid_client. _handle_refresh_response logs a warning, clears tokens and resets _initialized, and the flow falls back to a fresh client_credentials exchange, which succeeds. Net effect on every token expiry: one guaranteed-rejected token-endpoint round trip plus a spurious "Token refresh failed" warning, and the refresh token the SDK deliberately carries forward (lines 539-540) is dead weight that can never be used. Fix belongs in _refresh_token (add the RFC 7523 assertion for private_key_jwt, mirroring _add_client_authentication_jwt) or in can_refresh_token for that provider.
Verification: nit — the factual claim is verifiable in code, though the consequence is milder than a brick because the client_credentials fallback recovers headlessly. Chain, all in HEAD: (1) The new Step 5 at /home/claude/python-sdk/src/mcp/client/auth/oauth2.py:766-768 gates only on can_refresh_token() (line 191-193: tokens with a refresh_token plus client_info — no auth-method check), then yields `self._re
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/mcp/client/auth/oauth2.py— Step 4's CIMD comment "the issuer stamp is informational" is made false by this PR: the new _apply_issuer_binding (lines 569-573) reads that exact stamp to decide whether to drop held tokens and re-persist the record, so the stamp is now load-bearing SEP-2352 state, not informational.Extended reasoning...
Concrete cost: a maintainer misled by the comment. The comment at src/mcp/client/auth/oauth2.py:714-715 predates this PR, but the PR adds the first consumer of the CIMD issuer stamp: _apply_issuer_binding line 569 compares
client_info.client_id == self.context.client_metadata_url and client_info.issuer != issuerand on mismatch clears tokens and re-stamps. A future change to the CIMD creation path (line 721client_information.issuer = discovered_issuer) that skips or mis-sets the stamp — plausible exactly because the adjacent comment says it is informational — would silently disable the token-drop guard, so a refresh token minted at one authorization server would be presented to a different one on the next 401. Fix is one line: reword the comment to say the stamp records which issuer the record's tokens came from and drives _apply_issuer_binding's token drop.Verification: nit — the claim is factually accurate. The comment at /home/claude/python-sdk/src/mcp/client/auth/oauth2.py:714-715 ("CIMD records are portable across authorization servers, so the issuer stamp is informational") predates the PR (present at base line 709, untouched by the diff —
git diff 0d92192..HEADcontains no hunk touching it), and at base it was true: the only reader of the stamp was `cre -
🟡
tests/interaction/auth/_harness.py— connect_with_oauth's docstring ("in that case ...headless[is] unused (the yielded HeadlessOAuth is never invoked)") is made stale by this PR: the new test passes both auth= and headless= and depends on headless.bind(http_client) firing so the provider's redirect/callback handlers work.Extended reasoning...
Concrete cost: the harness contract now contradicts its own usage. tests/interaction/auth/test_lifecycle.py's new test_a_refresh_the_server_rejects_on_the_401_path_falls_back_to_authorization builds auth=restarted_interactive_provider(storage, headless) AND passes headless=headless to connect_with_oauth; the flow only completes because connect_with_oauth still calls headless.bind(http_client) and the supplied provider invokes that same HeadlessOAuth's handlers. A test author following the docstring at tests/interaction/auth/_harness.py:485-488 ("
headless[is] unused" when auth is given) would omit headless= in the next such restart-with-browser test, get an unbound HeadlessOAuth whose redirect_handler assertsself._http is not None, and burn time on a confusing in-harness AssertionError. One-sentence docstring update (headless is still bound and is used when the supplied auth references its handlers) removes the trap.Verification: nit. The claim is factually true. The pre-existing docstring at tests/interaction/auth/_harness.py:485-488 states: "
auth: supply a pre-builthttpx2.Auth... in that casestorage,client_metadata,client_metadata_url, andheadlessare unused (the yieldedHeadlessOAuthis never invoked and itsauthorize_urlstays None)." This PR adds two tests that violate that contract: tests/inte
| if self.context.can_refresh_token(): | ||
| refresh_response = yield await self._refresh_token() | ||
| refreshed = await self._handle_refresh_response(refresh_response) |
There was a problem hiding this comment.
🔴 Security: 401-branch Step 5 still POSTs a refresh token of unconfirmed issuer provenance to whatever AS the resource server's PRM currently names — for pre-registered/unstamped registrations always, and for CIMD across restarts because the SEP-2352 token drop in _apply_issuer_binding is memory-only (clear_tokens never touches storage). [also at: src/mcp/client/auth/oauth2.py:756 - Re-filing still-present security gap: for pre-registered/unstamped (non-CIMD) client_info, the new 401-branch Step 5…; src/mcp/client/auth/oauth2.py:723 - Fresh-registration token clearing is applied only in the DCR branch of Step 4 (line 751) — the CIMD branch (lines…; +1 more]
Extended reasoning...
A client with pre-registered credentials (client_info.issuer is None) holds a persisted refresh token. A compromised or malicious resource server changes its PRM to name an attacker-controlled AS; credentials_match_issuer (src/mcp/client/auth/utils.py:352-353) returns True for the unstamped record, _apply_issuer_binding's token-drop branch (oauth2.py:569-573) applies only when client_id == client_metadata_url, so tokens survive, and Step 5 (oauth2.py:756-758) silently POSTs the long-lived refresh token (plus client secret via prepare_token_auth) to the attacker's advertised token_endpoint with no user-visible signal. The CIMD case is only fixed in-process: clear_tokens (oauth2.py:195-198) does not delete tokens from storage while the re-stamped record IS persisted (line 572), so the next restarted process reloads the old-issuer refresh token under a record now stamped with the new issuer, _apply_issuer_binding finds issuer == issuer and keeps it, and Step 5 presents the previous issuer's refresh token to the new AS anyway. Prior to this PR the 401 branch never refreshed, so this harv
Verification: normal — both prongs are mechanically real at HEAD. (1) Pre-registered/unstamped: utils.py:352-353 (if client_info.issuer is None: return True) makes credentials_match_issuer pass, and the token-drop branch in _apply_issuer_binding (oauth2.py:569, if client_info.client_id == self.context.client_metadata_url ...) is CIMD-only, so tokens survive; the RS's PRM alone sets the AS (oauth2.py:657
| # SEP-2352: on the legacy no-PRM path the issuer is only known after ASM | ||
| # discovery, so re-evaluate the binding here using the discovered metadata | ||
| # issuer (mirroring the bound_issuer fallback in Step 4). | ||
| if ( | ||
| self.context.client_info is not None | ||
| and self.context.auth_server_url is None | ||
| and self.context.oauth_metadata is not None | ||
| and not credentials_match_issuer( | ||
| self.context.client_info, | ||
| str(self.context.oauth_metadata.issuer), | ||
| self.context.client_metadata_url, | ||
| ) | ||
| ): | ||
| logger.debug("Authorization server changed; discarding bound credentials and re-registering") | ||
| self.context.client_info = None | ||
| self.context.clear_tokens() | ||
| # discovery (mirroring the bound_issuer fallback in Step 4). | ||
| if self.context.auth_server_url is None and self.context.oauth_metadata is not None: | ||
| await self._apply_issuer_binding(str(self.context.oauth_metadata.issuer)) |
There was a problem hiding this comment.
🔴 Legacy no-PRM path trusts the resource-server-origin ASM's self-declared issuer without validation (validate_metadata_issuer at lines 684-685 is skipped when auth_server_url is None), so the SEP-2352 stamp check in _apply_issuer_binding is spoofable and the new Step-5 refresh (lines 756-758) silently POSTs the stored refresh token plus client secret to the forged metadata's token_endpoint — defeating issuer binding even for stamped, SDK-minted registrations, which the PRM path does protect (there SEP-2468 forces asm.issuer to equal the discovery URL, so a stamped mismatch is discarded).
Extended reasoning...
A client previously authorized legitimately, so storage holds an SDK-minted registration stamped issuer=https://legit-as.example.com plus a refresh token. The resource server is later compromised. On the next 401 it serves no PRM (all PRM URLs 404), so auth_server_url stays None and discovery falls back to https://{rs-origin}/.well-known/oauth-authorization-server (utils.py:166-170), where the attacker serves ASM with issuer="https://legit-as.example.com" (the AS it formerly used, which it knows) and token_endpoint=https://attacker.example/token. The SEP-2468 check at oauth2.py:684-685 is skipped because auth_server_url is None; handle_auth_metadata_response accepts the document; _apply_issuer_binding(str(asm.issuer)) at lines 693-694 compares the forged issuer to the stamp, matches, and keeps credentials AND tokens. The new Step 5 (lines 756-758) then builds _refresh_token() with token_url = oauth_metadata.token_endpoint (line 498) and POSTs grant_type=refresh_token with the refresh token and, via prepare_token_auth, the client secret — to the attacker's endpoint, with no user-vis
Verification: normal — security gap newly reachable through this diff's Step-5 refresh. Chain in src/mcp/client/auth/oauth2.py at HEAD: (1) with all PRM URLs 404ing (attacker-controlled RS), auth_server_url stays None and ASM is fetched from the RS's own origin (utils.py:166-170 returns only "{rs-origin}/.well-known/oauth-authorization-server"); (2) lines 684-685 skip validate_metadata_issuer exactly when aut
1 participant
On a 401,
OAuthClientProvidernow tries the stored refresh token (after rediscovery) before falling back to interactive authorization, and the pre-request refresh only runs once authorization-server metadata is known, so it never guesses{origin}/token. A restarted or headless client with a persisted refresh token recovers from an expired access token without a browser.Closes #3240, closes #3250, closes #1318.
Motivation and Context
The provider had two ways to obtain a token that each lacked half of what they needed: the pre-request branch could refresh but never ran discovery, and the 401 branch ran discovery but never tried the refresh token. After a restart nothing restores an expiry, so the pre-request branch is skipped, the stale bearer draws a 401, and the flow goes to the browser with a usable 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 went to a path derived from the server origin, which 404s for any authorization server under a path, and the refresh token was then discarded. #1318 reported this a year ago; #3240 and #3250 are the same defect from two angles. The 401 branch did refresh in 1.10/1.11; #1071 moved that block ahead of the request instead of duplicating it and nothing tested the restart path.
What changed
if can_refresh_token(): refresh→ on success retry the request; otherwise (no refresh token, or the server rejected it) run the full grant as before.oauth_metadata is not None. In-process behaviour is unchanged (metadata is cached after the first flow); on a cold start the request goes out and the 401 branch discovers, then refreshes._initializedinside_handle_refresh_response, so the next request re-reads storage; a long-lived headless provider whose one refresh hit a transient error retries it rather than staying tokenless for the life of the process. Classifying failures (keep the refresh token on 5xx, clear only oninvalid_grant) is left for the follow-up below.client_idis portable across authorization servers, but tokens issued under it are not: when the issuer that becomes known (from PRM, or from AS metadata on the legacy no-PRM path) differs from the record's SEP-2352 stamp — or the record carries no stamp, so its tokens have no confirmed origin — the record is kept and re-stamped and its tokens (and any cached metadata from the previous server) are dropped before anything is refreshed (the TypeScript SDK'sdiscardIfIssuerMismatch). The existing bound-credential discard and this rule now live in one_apply_issuer_bindingmethod called at both points the issuer becomes known, replacing the two inline copies.src/is +55/−41. No public API change,TokenStorageuntouched. This is the TypeScript SDK's ordering (auth()refreshes beforestartAuthorization) and matches C#/Rust.How Has This Been Tested?
Interaction tests against the SDK's own AS+RS in process, each logging in as a "first process" and then connecting a fresh provider over the same storage: refresh on 401 after restart (no handlers wired); the same against an AS mounted under
/oauth2/v1(bare/token404s) proving the refresh goes to the advertised endpoint; a rejected 401-path refresh falling back to authorization; a headless provider retrying the refresh on its next connection after a failed one; a fresh registration not presenting tokens left from a previous client; and a CIMD client keeping its id but dropping its tokens on an issuer change. Each was mutation-checked against the line it pins. Unit tests pin that an expired token is not refreshed ahead of the request while metadata is unknown, and the CIMD rebinding on both discovery paths (stamped elsewhere, and unstamped). Manifest gainsclient-auth:refresh:on-401andclient-auth:refresh:discovered-endpoint; the harness gains a path-prefixed-AS shim andexpire_access_token.Also driven over real sockets (uvicorn serving the SDK AS with 3 s access tokens, then under a prefix; a separate application process with file-backed storage run repeatedly): headless runs after expiry show
401 → PRM → ASM → POST …/token grant_type=refresh_token → 200; withtokens.jsonrewritten toexpires_in=-1and an app that derives expiry on load (the #3240 setup) there is no guessed/tokenrequest and the refresh lands on/oauth2/v1/token.Breaking Changes
None. Behavioural notes: a 401 with a refresh token in storage now produces one token-endpoint request before any redirect (a
scope=challenge wider than the grant is then met by the 403 step-up on the retry, as in the TypeScript SDK). A cold start with an access token the server no longer accepts costs one 401 round trip before the refresh, unless the application restores both the expiry and the AS metadata itself. Against 2025-03-26-era servers that publish no metadata at all, in-process expiry now also goes through that 401 (and the failing well-known probes) instead of a single pre-request refresh; everywhere else in-process behaviour is unchanged.Types of changes
Checklist
Additional context
An earlier revision of this branch also folded in the #3256 recovery (expired DCR secret /
invalid_client→ re-register once) and a cold-start pre-request discovery mode. Review surfaced enough edge cases in both (403 step-up path, pre-registered credentials, tokens outliving their registration, a coarse-clock test knob) that they're better handled separately, so this PR is cut back to the refresh fix and the rest follows:client_secret_expires_at,invalid_clienthandling incl. the 403 step-up exchange) — separate PR.issuer(and client binding) plusissued_aton the persisted token, following theOAuthClientInformationFull.issuerprecedent, and a way forTokenStorageto forget tokens. Known exposures until then, stated rather than fixed here: (a) for pre-registered / unstampedclient_infothe 401-path refresh presents the stored refresh token (and client secret, as the code exchange already did) to whichever issuer the resource server's PRM now names — needs a token issuer stamp to detect; (b)clear_tokens()is memory-only, so a process that re-registers but cannot complete authorization leaves the previous client'stokens.jsonbeside the newclient_info, and the next process presents that refresh token under the new client once (rejected asinvalid_grant) before authorizing; (c) verbatim storages get no real expiry across restarts. Refresh-failure classification and the{server-origin}fallback base used when PRM named an AS whose metadata could not be fetched belong in the same pass.token_expiry_timeupon context initialization for stored tokens. #1784 and fix: restore OAuth token expiry across process restarts #3248 restore expiry in_initialize, which this PR deliberately doesn't; the stored-token follow-up covers that.)AI Disclaimer