-
Notifications
You must be signed in to change notification settings - Fork 4k
fix(client/auth): discard stored client registrations whose secret has expired #3264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
claude
wants to merge
9
commits into
main
Choose a base branch
from
fix/oauth-discard-expired-client-registration
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,191
−129
Open
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4fac3c7
fix(client/auth): discard stored client registrations with an expired…
claude 50d6b0b
fix(client/auth): re-check secret expiry in the 401 flow; document th…
claude 6b7ab0d
chore: retrigger CI after PyPI download timeout
claude 5a246e6
fix(client/auth): run the expiry discard after the SEP-2352 issuer ch…
claude e53e526
fix(client/auth): drop the orphaned refresh token on expiry discard; …
claude 32a056d
test(client/auth): make the expiry-discard test's storage assertions …
claude 43d9599
fix(client/auth): run discovery before the 403 step-up re-registers w…
claude aedc88c
fix(client/auth): finalize the discovery sub-generator when the flow …
claude 928b7bf
test(client/auth): cover the 403 step-up's exception relay; drop its …
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
fix(client/auth): finalize the discovery sub-generator when the flow …
…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.
- Loading branch information
commit aedc88c780e043e3dede817bcd2525cfd456e5dd
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 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()orclient_info is None), but the restart state that motivated it —_initialize()restores tokens and client info but neveroauth_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 againsturljoin(resource_origin, "/authorize")/"/token"), burning an interactive consent per 403 in the separate-AS topology; widening the guard to run discovery wheneveroauth_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_scopestep-up added in this PR runs the shared discovery sub-generator only under the guard atsrc/mcp/client/auth/oauth2.py:884-886: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 onlycurrent_tokensandclient_info, neveroauth_metadata/protected_resource_metadata, andtoken_expiry_timeisNoneafter reload sois_token_valid()isTrueand the 401 flow's discovery never runs — applies just as much when the stored registration is valid. For that population the guard isFalse, discovery is skipped, and the step-up proceeds to_perform_authorization()withoauth_metadatastillNone.The code path. With
oauth_metadata=None,_perform_authorization_code_grant()falls back tourljoin(resource_origin, "/authorize")(theelsebranch aroundoauth2.py:417-420) and the exchange targetsurljoin(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'sWWW-Authenticateresource_metadatapointer is available on this path —extract_resource_metadata_from_www_authnow 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_timeisNone, so the Bearer header is attached and no 401 occurs — discovery never runs. (2) The request needs a broader scope: 403insufficient_scope. The guard isFalse(record valid,client_infopresent), so no discovery runs. (3) Step 2b calls_perform_authorization(): the user's browser is sent to a deadhttps://resource-origin/authorizeURL — a burned interactive consent — and the flow fails (the callback never completes, or the exchange 404s and_handle_token_responseraisesOAuthTokenError). (4) Theexceptre-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 coversoauth_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 discoveredauthorization_endpoint. Same one-line shape as the existing fix, plus a regression test foroauth_metadata=None+ valid record + 403.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged — this is a real gap: after a restart with a still-valid registration, a 403 step-up authorizes against the
{origin}/authorizefallback 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 anyoauth_metadata is Nonecase 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