| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -424,6 +424,13 @@ | |
| # Parse and validate response with scope validation | ||
| token_response = await handle_token_response_scopes(response) | ||
|
|
||
| # RFC 6749 §5.1: an omitted scope means the granted scope equals the requested | ||
| # scope. Record it explicitly so the persisted token is self-describing — the | ||
| # SEP-2350 step-up union reads it after a restart, when client_metadata.scope | ||
| # has reverted to its constructor value. | ||
| if token_response.scope is None: | ||
| token_response.scope = self.context.client_metadata.scope | ||
|
|
||
| # Store tokens in context | ||
| self.context.current_tokens = token_response | ||
| self.context.update_token_expiry(token_response) | ||
| Expand Down Expand Up | @@ -470,6 +477,12 @@ | |
| content = await response.aread() | ||
| token_response = OAuthToken.model_validate_json(content) | ||
|
|
||
| # RFC 6749 §6: an omitted scope on refresh means the scope is unchanged from | ||
| # the prior access token. Carry it forward so the persisted token stays | ||
| # self-describing for the SEP-2350 step-up union after a restart. | ||
| if token_response.scope is None and self.context.current_tokens is not None: | ||
| token_response.scope = self.context.current_tokens.scope | ||
|
|
||
|
Check notice on line 485 in src/mcp/client/auth/oauth2.py
|
||
| self.context.current_tokens = token_response | ||
| self.context.update_token_expiry(token_response) | ||
| await self.context.storage.set_tokens(token_response) | ||
| Expand Down Expand Up | @@ -578,6 +591,9 @@ | |
| logger.debug("Authorization server changed; discarding bound credentials and re-registering") | ||
| self.context.client_info = None | ||
| self.context.clear_tokens() | ||
| # Any cached AS metadata is for the old server; drop it so a failed | ||
| # rediscovery cannot leak the old registration/token endpoints into Step 4. | ||
| self.context.oauth_metadata = None | ||
|
|
||
| asm_discovery_urls = build_oauth_authorization_server_metadata_discovery_urls( | ||
| self.context.auth_server_url, self.context.server_url | ||
|
Comment thread
claude[bot] marked this conversation as resolved.
|
||
| Expand All | @@ -600,6 +616,23 @@ | |
| else: | ||
| logger.debug(f"OAuth metadata discovery failed: {url}") | ||
|
|
||
| # 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() | ||
|
|
||
| # Step 3: Apply scope selection strategy | ||
| self.context.client_metadata.scope = get_client_metadata_scopes( | ||
| extract_scope_from_www_auth(response), | ||
| Expand All | @@ -610,23 +643,22 @@ | |
|
|
||
| # Step 4: Register client or use URL-based client ID (CIMD) | ||
| if not self.context.client_info: | ||
| # SEP-2352: bind the credentials to the issuing AS. Prefer the PRM-advertised | ||
| # authorization server; on the legacy no-PRM path fall back to the issuer from | ||
| # the discovered metadata so the binding is still recorded. | ||
| bound_issuer = self.context.auth_server_url | ||
| if bound_issuer is None and self.context.oauth_metadata is not None: | ||
| bound_issuer = str(self.context.oauth_metadata.issuer) | ||
| # SEP-2352: the issuer to bind these credentials to, when known. | ||
| discovered_issuer: str | None = None | ||
| if self.context.oauth_metadata is not None: | ||
| discovered_issuer = self.context.auth_server_url or str(self.context.oauth_metadata.issuer) | ||
|
|
||
| if should_use_client_metadata_url( | ||
| self.context.oauth_metadata, self.context.client_metadata_url | ||
| ): | ||
| # Use URL-based client ID (CIMD) | ||
| # Use URL-based client ID (CIMD). CIMD records are portable across | ||
| # authorization servers, so the issuer stamp is informational. | ||
| logger.debug(f"Using URL-based client ID (CIMD): {self.context.client_metadata_url}") | ||
| client_information = create_client_info_from_metadata_url( | ||
| self.context.client_metadata_url, # type: ignore[arg-type] | ||
| redirect_uris=self.context.client_metadata.redirect_uris, | ||
| ) | ||
| client_information.issuer = bound_issuer | ||
| client_information.issuer = discovered_issuer | ||
| self.context.client_info = client_information | ||
| await self.context.storage.set_client_info(client_information) | ||
| else: | ||
| Expand All | @@ -638,7 +670,16 @@ | |
| ) | ||
| registration_response = yield registration_request | ||
| client_information = await handle_registration_response(registration_response) | ||
| client_information.issuer = bound_issuer | ||
| # Only record the issuer when the registration above actually targeted | ||
| # the discovered AS's registration_endpoint. With no metadata, or | ||
| # metadata that omits registration_endpoint, DCR fell back to the | ||
| # resource-server origin's /register — recording that as bound to a | ||
| # PRM-advertised AS would persist a binding that was never established. | ||
| if ( | ||
| self.context.oauth_metadata is not None | ||
| and self.context.oauth_metadata.registration_endpoint is not None | ||
| ): | ||
| client_information.issuer = discovered_issuer | ||
|
Check warning on line 682 in src/mcp/client/auth/oauth2.py
|
||
|
Comment thread
Comment on lines
+673
to
+682
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality🟡 The new registration_endpoint is not None gate also drops the issuer stamp in the legacy same-origin case (PRM 404 → root ASM discovery succeeds with issuer == resource origin, metadata omits the optional registration_endpoint, DCR falls back to that same origin's /register) — there the registration genuinely targets the discovered issuer's host, and pre-#2933-PR the record was correctly bound, so a later AS migration triggered discard + re-registration. Since credentials_match_issuer returns True for issuer is None, the unstamped record is now silently reused after a migration, regressing SEP-2352 auto-recovery for that deployment shape; consider also stamping when the fallback /register origin matches the discovered issuer's origin (the new parametrized test only covers the cross-origin variant). Extended reasoning...The over-suppression. The new gate at oauth2.py:678-682 only stamps client_information.issuer when self.context.oauth_metadata.registration_endpoint is not None. The comment's rationale is to avoid recording "a binding that was never established" when DCR fell back to the resource-server origin's /register instead of the discovered AS's registration_endpoint. But the gate is keyed on registration_endpoint presence, not on whether the fallback URL actually belongs to a different server than the discovered issuer — and that over-suppresses in the legacy case where the AS is the resource origin. There, the fallback /register is on the discovered issuer's own host, so the binding was established; only the path was guessed. Concrete walkthrough (legacy same-origin embedded AS, no PRM):
Why nothing else catches it. The new post-ASM re-check earlier in the flow only helps when a binding exists to compare; once records are persisted unstamped, every later 401 reuses them regardless of which AS the resource now advertises. The PR's new parametrized test (asm-metadata-without-registration-endpoint) only exercises the cross-origin variant (issuer https://new-as.example.com vs resource api.example.com), where leaving the issuer unset is the right call — the same-origin variant, where the old stamp was correct, is untested and regresses unnoticed. Why this is distinct from the existing review comments. The two prior inline comments argued the stamp was wrong when the fallback /register belongs to a different server than the advertised AS (cross-origin case), and this PR implemented that suggestion. This finding is the flip side: the chosen gate (registration_endpoint presence) is too coarse and also drops the stamp when the fallback /register is on the same origin as the discovered issuer, where the binding was both correct and useful. Scope and severity. The trigger is narrow: an AS that publishes RFC 8414 metadata without registration_endpoint yet serves DCR at the legacy /register path, followed by a later AS migration. The failure mode degrades to the documented "unbound credentials are reused" semantics rather than persisting a wrong binding, so this is non-blocking. Suggested fix. Instead of gating purely on registration_endpoint presence, also stamp when the discovered issuer's origin matches get_authorization_base_url(server_url) (i.e. the fallback /register URL is on the issuer's own host) — or compute the actual registration URL once and stamp iff it was derived from / belongs to the discovered issuer. Either keeps the cross-origin suppression this PR adds while preserving #2933's auto-recovery for same-origin embedded-AS deployments.
Sorry, something went wrong.
claude[bot] reacted with thumbs up emoji
claude[bot] reacted with thumbs down emoji
All reactions
|
||
| self.context.client_info = client_information | ||
| await self.context.storage.set_client_info(client_information) | ||
|
|
||
| Expand Down | ||
| Back | FazBrowse Home | New Git URL |
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality🟣 Pre-existing issue in the block this PR touches: _handle_refresh_response now carries an omitted scope forward per RFC 6749 §6, but an omitted refresh_token (also allowed by §6 and common with non-rotating ASes) is not — the parsed token has refresh_token=None and unconditionally overwrites both context.current_tokens and persistent storage, so the next expiry forces a full interactive re-authorization (or fails for headless clients). The fix is the same one-line pattern this PR adds for scope: backfill token_response.refresh_token from self.context.current_tokens.refresh_token when the response omits it.
Extended reasoning...The bug. _handle_refresh_response (src/mcp/client/auth/oauth2.py:469-495) parses the refresh response into a fresh OAuthToken and then unconditionally does self.context.current_tokens = token_response followed by await self.context.storage.set_tokens(token_response). OAuthToken.refresh_token is Optional with a default of None (src/mcp/shared/auth.py), so when the authorization server omits refresh_token from the refresh response, the still-valid stored refresh token is overwritten with None both in memory and in persistent storage. RFC 6749 §6 explicitly allows this server behavior — the AS MAY issue a new refresh token; when it does not, the client is expected to keep using the previously issued one. Non-rotating refresh tokens are common in practice (Google and many enterprise ASes omit refresh_token from refresh responses).
Code path. async_auth_flow → can_refresh_token() true → _refresh_token() builds the request → _handle_refresh_response handles the 200. The handler validates the JSON, applies the new scope carry-forward this PR adds (lines 480-484), and then replaces the stored token wholesale. Nothing anywhere in src/mcp/client/auth preserves the prior refresh_token — the only other uses are can_refresh_token() (which requires it) and building the refresh request itself. The only carry-forward logic in the module is the scope backfill introduced two lines above the overwrite.
Step-by-step proof.
Why nothing prevents it. The existing tests always include refresh_token in mocked refresh responses, so the omitted case is uncovered, and there is no other code path that carries the prior value forward.
Severity / scope. This is pre-existing — the overwrite line predates this PR and the PR does not change runtime behavior here. It is flagged because the PR edits this exact handler and implements the identical RFC 6749 §6 carry-forward principle for the sibling scope field two lines above, so this is the natural place to fix it.
Fix. Mirror the scope backfill the PR adds:
placed alongside the new scope carry-forward, before current_tokens is replaced and the token is persisted. A test analogous to test_handle_refresh_response_carries_prior_scope_when_response_omits_it (asserting the stored token keeps the old refresh_token) would cover it.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.