-
Notifications
You must be signed in to change notification settings - Fork 3.6k
OAuth client: harden SEP-2352/SEP-2350 edge cases; fix conformance comment #2936
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
maxisbey
wants to merge
2
commits into
main
Choose a base branch
from
bughunter/followups-2026-06-20
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.
+142
−7
Open
Changes from all commits
Commits
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
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
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.
🟡 When the issuer changes but ASM discovery for the new AS fails in the same flow, Step 4 still falls back to
urljoin(resource-origin, '/register')and stampsclient_information.issuer = auth_server_url(the new AS) on credentials actually registered with the old embedded AS — persisting exactly the mis-bound record theoauth_metadata = Noneclear is meant to prevent, andcredentials_match_issuerwill then accept it on every later 401 with no auto-recovery. Consider failing the flow (or skipping the issuer stamp / the legacy/registerfallback) when the issuer changed but the new AS's metadata could not be discovered.Extended reasoning...
The residual gap. The new
self.context.oauth_metadata = None(oauth2.py:594-596) correctly stops a stale discoveredregistration_endpointfrom being reused after an issuer change. But it only changes where the fallback registration goes, not the fact that a registration performed against an endpoint never derived from the new issuer is still stamped with the new issuer. If ASM discovery for the new AS fails in the same flow, Step 4 registers at the resource origin's legacy/registerand binds that record to the new issuer, persisting it viastorage.set_client_infobefore token exchange.Concrete walkthrough (PRM-success branch, single 401 flow):
client_infois bound to issuer A — the resource server's old embedded AS athttps://api.example.com/. The server has migrated: PRM now advertises AS B (https://new-as.example.com/), soauth_server_url = B(oauth2.py:576).credentials_match_issuerfails →client_info/tokens discarded,oauth_metadatacleared. So far so good.handle_auth_metadata_responsereturns(True, None)for 404s and(False, None)for non-4xx (utils.py:223-233), so the loop exits withoauth_metadatastillNone; no exception. The new post-ASM re-check (lines 619-633) is gated onauth_server_url is None, so it does not run here.bound_issuer = auth_server_url = B(line 649). Withoauth_metadata is None,should_use_client_metadata_urlis False andcreate_client_registration_request(utils.py:284-287) falls back tourljoin("https://api.example.com", "/register")— in the single-origin-migration case, the old embedded AS, which still serves a live DCR endpoint.client_information.issuer = Bis stamped (line 674) and persisted viastorage.set_client_infobefore the authorization/token exchange, so the mis-bound record lands in storage even if the rest of the flow fails. (The same flow then also hits/authorizeand/tokenon the resource origin via the legacy fallbacks in_perform_authorization_code_grant/_get_token_endpoint, despite the resource explicitly advertising B.)credentials_match_issuer(client_info, B, ...)is a simple string compare against the stamped issuer (utils.py:341-345) and returns True — so the SDK keeps presenting AS A'sclient_idto AS B indefinitely with no automatic re-registration. That sticky wedge is exactly what SEP-2352 binding was meant to auto-recover from, and the PR description's second bullet says this change prevents "persisting a mis-bound record".Why existing safeguards don't catch it. The metadata clear stops the old AS's discovered
registration_endpointfrom leaking into Step 4, but the legacy origin-/registerfallback reaches the same old AS in the common case where the old AS lived at the resource origin; andbound_issueris taken fromauth_server_urlregardless of whether the registration endpoint was actually derived from that issuer.On the pre-existing/scope objection. It's true that both ingredients (the origin-
/registerfallback and thebound_issuer = auth_server_urlstamping from #2933) predate this PR, that the trigger is narrow (an AS migration plus complete well-known discovery failure for the new AS plus a still-live legacy/registerat the resource origin, all in one flow), and that this PR strictly narrows the window rather than widening it. This is filed as a non-blocking nit for that reason. It still seems worth a comment because the PR touches this exact block and its stated goal for this change is to stop persisting a mis-bound record when rediscovery for the new AS fails — this is a second route to that same record that remains open.Suggested fix. When the issuer changed (the line-591 block fired) but
oauth_metadatais stillNoneafter Step 2, either fail the flow with anOAuthFlowError, or skip the legacy/registerfallback / skip stampingissuerso a registration not derived from the new AS's metadata is never recorded as bound to it. Any of these keeps the auto-recovery property intact: a later 401 with working discovery would re-register cleanly instead of being wedged on the wrongclient_id.