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

[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 #3256.

Problem

OAuthClientProvider persists client_secret_expires_at (RFC 7591) through TokenStorage but 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 with ClientRegistrationOptions.client_secret_expiry_seconds set — every token-endpoint interaction fails with invalid_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, past client_secret_expires_at is treated as absent —

• in _initialize, when loading stored client info (restart case), and
• at the start of the 401 handler, re-checking the in-memory record (long-lived process whose secret lapses after load, so an interactive consent is never burned on an exchange doomed to fail invalid_client).

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 (unlike draft #3260, which makes the setters accept None).

Deliberate scoping:

• 0 means "never expires" and an absent field means no expiry was declared — both are kept (RFC 7591).
• Registrations with token_endpoint_auth_method "none"/absent never present the secret, so a lapsed secret does not invalidate them.
• Stored tokens are kept: a live access token continues to work without client authentication, and with no client info can_refresh_token() is false, so the refresh path that would present the lapsed secret is skipped.
• Pre-registered flows are unaffected: the client-credentials extension providers override _initialize and never load client info from storage.
• The issue's "expected behavior" (2) — reactively invalidating on an invalid_client token 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. a TokenStorage contract decision that deserves its own discussion.

docs/client/oauth-clients.md is updated in the same PR: the TokenStorage tip and the CIMD section's "stored client_info still wins" statement now mention the expired-secret discard.

Tests

Three new tests in tests/client/test_auth.py:

• predicate unit test covering lapsed secret (post/basic), 0 = never expires, absent expiry, still-live expiry, and "none"-method registrations;
• flow regression test: a lapsed stored registration is treated as absent on load (stored access token still used), and after the 401 the flow issues a POST /register instead of reusing the dead record;
• mid-session lapse test: a registration that expires after _initialize already loaded it is discarded by the 401 handler, which then re-registers.

ruff format/ruff check clean, pyright strict 0 errors, tests/client/test_auth.py + tests/client/auth + tests/interaction/auth pass with 100% branch coverage on src/mcp/client/auth/oauth2.py.

[claude]

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.

[claude]

🟡 The new expiry discards (in _initialize and at the top of the 401 handler) null context.client_info before the SEP-2352 issuer-binding checks — which are guarded on client_info is not None and read the issuer stamp from client_info.issuer — so a record that is both expired and issuer-mismatched (AS migration) now skips the SEP-2352 cleanup (clear_tokens() + oauth_metadata = None) that fired pre-PR. The old issuer's kept refresh token can then later be presented to the new AS's token endpoint, and stale oauth_metadata can leak the old registration/token endpoints into Step 4 when ASM rediscovery fails. Fix is a reordering: run the in-flow expiry discard after the SEP-2352 checks (just before Step 4), which is all the discard needs to enable re-registration.

Extended reasoning...

What the bug is

Both new expiry-discard sites set context.client_info = None while deliberately keeping current_tokens and oauth_metadata: _initialize and the in-flow check at the top of the 401 handler (src/mcp/client/auth/oauth2.py:638-646). But the two SEP-2352 issuer-binding checks a few lines below — the primary one after PRM discovery and the legacy no-PRM mirror after ASM discovery — are both guarded on self.context.client_info is not None, and the issuer stamp that credentials_match_issuer() compares lives on client_info (client_info.issuer, utils.py). So when a stored record has both an expired secret and a stale issuer binding (the server migrated to a different AS), the expiry discard destroys the only issuer evidence first, the SEP-2352 block is skipped entirely, and its cleanup — clear_tokens() and oauth_metadata = None — never runs. Pre-PR, the expired-but-present record reached the SEP-2352 check and the cleanup fired; this ordering is a regression introduced by this PR.

Why the PR's scoping rationale doesn't cover this

The PR's "stored tokens are kept" reasoning (a live access token keeps working without client authentication, and with no client info can_refresh_token() is false) is only sound when the issuer is unchanged. When the issuer changed, the kept tokens are another issuer's credentials — exactly what SEP-2352's clear_tokens() exists to remove, per its own comment: "drop them (and the old tokens) so the flow re-registers instead of presenting another server's credentials."

Step-by-step proof (consequence 1 — cross-issuer refresh-token presentation)

Mid-session variant, verified against the code:

1. A prior successful in-session flow against AS-A leaves context with client_info{issuer: AS-A, client_secret_post, client_secret_expires_at}, AS-A tokens (access + refresh, token_expiry_time set), and AS-A's oauth_metadata.
2. The secret lapses in memory; the server migrates its PRM to AS-B. A 401 arrives while the access token is still locally valid (so no refresh fires first).
3. The in-flow discard at lines 638-646 nulls client_info (expired). Tokens and oauth_metadata are kept by design.
4. PRM discovery sets auth_server_url = AS-B. The SEP-2352 check is skipped — client_info is None — so AS-A's tokens are not cleared.
5. Step 4 registers with AS-B (client_info.issuer = AS-B) and persists it. Step 5's interactive authorization then fails (user closes the browser, state mismatch, token-endpoint 5xx — any exception after registration); the except block re-raises, leaving context with AS-B client_info + AS-A tokens, and _initialized still True.
6. Later, token_expiry_time passes. On the next request, can_refresh_token() is true (AS-A refresh_token present AND AS-B client_info present), so _refresh_token() sends AS-A's refresh token to AS-B's token endpoint (oauth_metadata is now AS-B's), authenticated with AS-B's fresh secret. Pre-PR, step 4 cleared the tokens, making step 6 impossible.

(Note: the fresh-restart variant of this trace does not fire — _initialize never sets token_expiry_time, so is_token_valid() stays true for a stored access token and the refresh branch is never entered. The mid-session variant above is the reproducible one; three independent verifiers traced it.)

Consequence 2 — stale oauth_metadata leaking old endpoints into Step 4

Same setup: the discard nulls client_info, SEP-2352 is skipped, so oauth_metadata stays AS-A's. If Step 2's ASM rediscovery for AS-B fails (handle_auth_metadata_response returns not-ok → break leaves oauth_metadata untouched), Step 4 computes discovered_issuer = auth_server_url = AS-B but POSTs the registration to AS-A's registration_endpoint from the stale metadata — and stamps the record issuer = AS-B, a binding to an issuer that never saw the registration, with subsequent token requests aimed at AS-A's token_endpoint. The existing SEP-2352 comment states its oauth_metadata = None cleanup exists precisely "so a failed rediscovery cannot leak the old registration/token endpoints into Step 4."

Impact and fix

The trigger is a narrow conjunction — an AS migration coinciding with a lapsed secret, plus (for consequence 1) a flow failure after registration or (for consequence 2) a failed ASM rediscovery — so nothing breaks in ordinary use; but it silently disables an existing security control rather than merely wasting a round-trip. The fix is a trivial reordering: move the in-flow expiry discard to just before Step 4 (after both SEP-2352 checks) — re-registration is all the discard needs to enable, and the SEP-2352 checks then still see the expired-but-stamped record. For _initialize, either defer the discard to the flow, or remember the issuer stamp when discarding so the flow can still clear tokens/oauth_metadata on mismatch.

[claude]

🟡 The new mid-session stored_registration_expired() re-check only runs inside the 401 branch, but prepare_token_auth() is also reached from two other paths when the secret lapses after _initialize(): the refresh branch (can_refresh_token() ignores expiry, so one refresh presents the dead secret and fails invalid_client before self-healing via the subsequent 401), and the 403 insufficient_scope step-up, where the live access token keeps the 401 discard from ever running — each 403 burns a full interactive user consent that is doomed to fail invalid_client at exchange, with no state reset, until the access token itself expires. Note the 403 fix is not a bare discard: that path has no registration step, so clearing client_info there would raise "No client info available for authorization" — it needs its own re-registration handling or a fall-through to the 401-style flow.

Extended reasoning...

The residual gap

This PR's in-flow expiry re-check (src/mcp/client/auth/oauth2.py:639-647) closes the previously-flagged mid-session gap for the 401 path — but stored_registration_expired() is still only consulted in _initialize() and inside the 401 branch, while prepare_token_auth() (which actually presents the secret) is reachable from two other paths in async_auth_flow after the secret lapses in memory:

1. Refresh path (minor, self-healing)

can_refresh_token() checks only current_tokens + refresh_token + client_info — never expiry. So with an expired access token, a refresh token, and in-memory client_info whose secret lapsed mid-session, the refresh branch (before the 401 handler, ~line 619) runs _refresh_token() -> prepare_token_auth() and presents the dead secret. The AS is guaranteed to answer invalid_client.

Recovery does happen in the same flow: _handle_refresh_response clears tokens, the failed refresh sets _initialized = False, the unauthenticated retry gets 401, and the new in-flow discard fires. Cost: one doomed round-trip per occurrence.

2. 403 insufficient_scope step-up (worse: repeated doomed interactive consents)

The elif response.status_code == 403 branch (~lines 797-827) has no expiry check at all, and — crucially — the access token is still live on this path, so the 401 branch's discard never gets a chance to run. Step-by-step:

1. Provider initializes while the secret is live; _initialized = True, client_info populated, access token valid.
2. client_secret_expires_at lapses in memory.
3. A request hits a SEP-2350 scope challenge: 403 with error=insufficient_scope. The step-up calls _perform_authorization() directly.
4. The user completes a full interactive authorization (redirect_handler + callback_handler).
5. _exchange_token_authorization_code -> prepare_token_auth presents the lapsed secret; the AS answers invalid_client; _handle_token_response raises OAuthTokenError.
6. The except block re-raises without resetting client_info or _initialized — so every subsequent request hitting the 403 repeats steps 3-5 identically.

The client is stuck in exactly the "I re-authenticated and nothing changed" state this PR exists to eliminate, until the access token itself expires and the 401 path finally recovers.

Why the fix isn't a copy-paste of the 401 discard

The 403 step-up has no registration step: if you simply clear context.client_info at the top of that branch, _perform_authorization_code_grant raises OAuthFlowError("No client info available for authorization"). The step-up needs its own re-registration handling when the secret has lapsed (or to fall through to the 401-style flow, which has Step 4). The refresh path is easier: re-check the predicate before the can_refresh_token() branch, mirroring the 401-branch check — with client_info cleared, can_refresh_token() is false and the flow falls straight through to the recovering 401 path.

Why this isn't the deliberately-descoped item

The PR intentionally leaves out reactive invalidation on an invalid_client token response — for servers that expire registrations without declaring it — because that needs a TokenStorage delete. Both gaps here concern a declared expiry, addressable with the exact predicate this PR adds; no storage-contract change is needed for the refresh half, and the 403 half needs flow restructuring, not storage changes.

Severity

Nit: the trigger is doubly narrow (the secret must lapse during the process lifetime, and a refresh or 403 step-up must arrive before any 401), the refresh half self-heals immediately, the 403 half is time-bounded by the access-token lifetime, and pre-PR behavior on both paths was identical or worse — nothing regresses at merge. But since the PR extends exactly this expiry-check pattern to the 401 path, these two remaining token-presenting paths are worth covering (now or in a follow-up).

[claude]

Code review found no issues

No high-confidence issues detected in this change.