fix(client/auth): discover AS metadata before cold-start token refresh, with issuer-binding check

[claude]

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 #3240. Together with #3248 (which restores token expiry on load), resolves #3250.

Problem

On a cold start — a client restarting with a stored refresh token, before any 401 has triggered discovery — the eager pre-401 refresh in OAuthClientProvider.async_auth_flow builds its token URL from the urljoin(origin, "/token") fallback because context.oauth_metadata has not been discovered yet. Against servers whose token endpoint lives under a path (e.g. https://auth.example.com/oauth2/api/v1/token), the refresh POSTs to the wrong URL, gets a 404, _handle_refresh_response clears the stored tokens, and the client falls back to a full interactive re-auth on every process restart — which headless/gateway clients cannot perform (#3240, #3250).

Fix

Run OAuth discovery before the cold-start refresh, driven through the same httpx auth flow (no side-channel client):

1. PRM discovery (SEP-985 well-known fallbacks; there is no 401 yet, so no WWW-Authenticate resource_metadata hint).
2. ASM discovery (RFC 8414 fallbacks, SEP-2468 issuer validation) → the real token endpoint.
3. Refresh, now targeting the discovered token endpoint.

Because this discovery is unanchored (blind well-known probes — a co-hosted origin can legitimately serve some other resource's documents), its results are treated as best-effort and never destructive or fatal, unlike the 401 path's hint-anchored, authoritative discovery:

• A resource-mismatched PRM or an issuer-mismatched ASM counts as a failed discovery (skip, try the next URL) instead of raising out of the auth flow.
• On a SEP-2352 issuer-binding mismatch — the gap that led the author of Discover auth-server metadata before eager token refresh #3241 to close it after cubic's P1 review comment — the eager refresh is skipped, so stored credentials are never presented to an unvalidated authorization server; the unanchored discovery results (including rejected ASM metadata) are discarded, and the credentials themselves are left for the anchored 401 path to judge — that path re-discovers with the server's hint and drops/re-registers only on a confirmed AS change.
• Servers publishing no metadata at all keep the exact previous behavior (refresh against {origin}/token), and the probes run only once per context (eager_discovery_attempted, recorded only when the probe sequence completes so an interrupted discovery is retried), so repeat in-process refreshes make no extra requests.
• When metadata is already known (normal in-process refresh), nothing changes — no extra requests.

The inner refresh generator is driven under contextlib.aclosing, so it is finalized deterministically when httpx closes the auth flow mid-refresh.

This follows the structure of #3241 (closed by its author) and addresses the issuer-binding concern it was missing, with strictly conservative semantics on the hint-less path.

Tests

Ten new anyio tests in tests/client/test_auth.py:

• cold-start refresh targets the discovered (pathful) token endpoint, then retries the original request with the refreshed bearer;
• legacy server with no metadata anywhere keeps the {origin}/token fallback, and a failed refresh still clears tokens;
• the discovery probes run only once per context — a second expiry refresh goes straight to the fallback;
• an interrupted probe sequence is not recorded as completed and is retried on the next refresh;
• a foreign (resource-mismatched) PRM from a blind probe is skipped as failed discovery;
• an issuer-mismatched ASM (SEP-2468) is skipped as failed discovery instead of failing the request;
• a non-4XX ASM discovery error stops the fallback chain (mirrors the 401 path);
• issuer-binding mismatch after PRM discovery skips the refresh while keeping credentials/tokens for the 401 path (cubic's P1 scenario, non-destructively);
• issuer-binding mismatch on the legacy no-PRM path discards the rejected ASM metadata (mirroring the 401 path's defensive clear);
• metadata already known → immediate refresh, no discovery requests.

ruff format/ruff check clean, pyright strict 0 errors, full local test suite passes with 100% branch coverage on the changed files (tests/interaction/auth/test_lifecycle.py continues to cover the metadata-already-known refresh and failed-refresh paths).

[claude]

Beyond the inline findings, one other candidate was examined and ruled out: whether an ASM discovery failure after a successful PRM discovery leaks the refresh token (and client secret) to the resource origin's /token fallback. It does not introduce new exposure — that fallback is the pre-existing behavior this PR inherits (pre-PR, every cold-start refresh POSTed to {origin}/token unconditionally, since metadata was never discovered before a 401); the PR only narrows it by targeting the discovered endpoint when discovery succeeds.

Extended reasoning...

Bugs were found and posted as inline comments, so human review is already signaled; this note only records what else was examined so a later pass need not re-derive it. The ruled-out concern was verified against _refresh_token (src/mcp/client/auth/oauth2.py:498-501): the {origin}/token fallback fires whenever oauth_metadata is None, which was unconditionally the case on every pre-PR cold-start refresh — so sending the refresh token to the resource origin when ASM discovery fails is the preserved legacy path, not a regression introduced by this PR.

[claude]

Beyond the inline findings, I also examined whether the SEP-2352 eager-skip being one-shot leaks credentials: after a binding-mismatch skip, eager_discovery_attempted means the next in-process refresh POSTs the refresh token to the {origin}/token fallback. That URL is derived from the configured server_url, not from the discarded discovery results, so the credentials go to the same resource origin they went to pre-PR — never to the mismatch-discovered AS. Not a bug.

Extended reasoning...

Bugs were found this run (posted as inline comments), so no approval. This note records one additional candidate that was examined and ruled out: the concern that the SEP-2352 eager refresh-skip only holds for a single refresh. It is true the skip is one-shot — on the next token expiry eager_discovery_attempted is already set, discovery is bypassed, and _refresh_token() runs — but the fallback token URL is built from get_authorization_base_url(server_url), i.e. the configured resource origin, identical to pre-PR behavior. The mismatched AS discovered during the skipped attempt was discarded (auth_server_url and protected_resource_metadata reset to None), so its endpoints cannot receive the stored credentials. This is informational only, not a guarantee of correctness of the rest of the path.

[claude]

All six findings from my earlier review rounds are confirmed fixed in 7e70eae and 137de1f, and a fresh review pass of the post-fix code found no new issues — but this adds ~120 lines of new logic to the OAuth client auth flow, so it should get a human look rather than a bot approval.

What was reviewed this pass:

• The reworked eager path end-to-end: best-effort PRM/ASM validation, both SEP-2352 skip branches, flag-on-completion, and the aclosing drive loop all behave as described in the fix confirmations, with matching tests.
• Checked whether the binding-mismatch skip being one-shot lets a later refresh present the refresh token to the mismatched AS — it cannot: the rejected metadata is discarded, so the follow-up refresh POSTs to the pre-PR {origin}/token fallback.
• Checked whether the eager refresh can send stored credentials to a blindly-discovered foreign AS — the refresh only targets discovered metadata after the resource-match, SEP-2468 issuer, and SEP-2352 binding checks all pass; mismatches skip or fall back.

Extended reasoning...

Overview

This PR touches two files: ~120 new lines in src/mcp/client/auth/oauth2.py (a new _refresh_with_discovery async generator plus a driver loop in async_auth_flow) and ~460 lines of new tests in tests/client/test_auth.py. It makes the eager pre-401 token refresh run PRM/ASM discovery first on a cold start, so the refresh targets the discovered token endpoint instead of the {origin}/token fallback (fixing #3240/#3250 for servers with pathful token endpoints).

Security risks

This is security-sensitive by nature: it decides where a stored refresh token (and potentially a client secret) is POSTed, based on unanchored well-known probes. The current revision handles this conservatively — a resource-mismatched PRM and an issuer-mismatched ASM are treated as failed discovery (skipped, never used), and a SEP-2352 issuer-binding mismatch skips the eager refresh entirely while discarding the unanchored metadata, deferring credential judgment to the hint-anchored 401 path. Two candidate concerns were examined and ruled out this pass: the one-shot nature of the mismatch skip does not route a later refresh to the mismatched AS (that metadata is discarded, so the fallback is the pre-PR {origin}/token), and the refresh cannot target a blindly-discovered foreign AS without passing the resource-match, SEP-2468, and SEP-2352 checks first.

Level of scrutiny

High. This is production client-auth code in a widely used SDK, and the guidelines here are explicit that security-sensitive auth code should not be bot-approved. The two review rounds already surfaced six real issues (one P1-class), which is itself evidence that this logic has subtle failure modes and deserves human sign-off even though the current revision reviews clean.

Other factors

All six prior findings have confirmed fixes with dedicated regression tests (ten new tests total, covering the happy path, legacy no-metadata fallback, once-per-context probing, interrupted-probe retry, and every mismatch branch). The PR description accurately reflects the final design, and the author (the maintainer-operated triage workflow) has been responsive to review feedback. Nothing is outstanding from my side — the deferral is purely about the sensitivity and non-triviality of the code path, not unresolved concerns.