-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fall back to the caller-configured scope when the server advertises none #3172
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,21 +100,30 @@ | |
| protected_resource_metadata: ProtectedResourceMetadata | None, | ||
| authorization_server_metadata: OAuthMetadata | None = None, | ||
| client_grant_types: list[str] | None = None, | ||
| configured_scope: str | None = None, | ||
| ) -> str | None: | ||
| """Select effective scopes and augment for refresh token support.""" | ||
| """Select effective scopes and augment for refresh token support. | ||
|
|
||
| A source that yields no scopes (absent, null, or an empty list) offers no guidance, so | ||
| selection falls through to the next source rather than treating "advertised nothing" as | ||
| "request nothing". | ||
| """ | ||
| selected_scope: str | None = None | ||
|
|
||
| # MCP spec scope selection priority: | ||
| # 1. WWW-Authenticate header scope | ||
| # 2. PRM scopes_supported | ||
| # 3. AS scopes_supported (SDK fallback) | ||
| # 4. Omit scope parameter | ||
| if www_authenticate_scope is not None: | ||
| # 4. Scope the caller configured on the provider (SDK fallback) | ||
| # 5. Omit scope parameter | ||
| if www_authenticate_scope: | ||
| selected_scope = www_authenticate_scope | ||
| elif protected_resource_metadata is not None and protected_resource_metadata.scopes_supported is not None: | ||
| elif protected_resource_metadata is not None and protected_resource_metadata.scopes_supported: | ||
| selected_scope = " ".join(protected_resource_metadata.scopes_supported) | ||
| elif authorization_server_metadata is not None and authorization_server_metadata.scopes_supported is not None: | ||
| elif authorization_server_metadata is not None and authorization_server_metadata.scopes_supported: | ||
| selected_scope = " ".join(authorization_server_metadata.scopes_supported) | ||
| else: | ||
| selected_scope = configured_scope or None | ||
|
Check warning on line 126 in src/mcp/client/auth/utils.py
|
||
|
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. P2: Clients that previously relied on an omitted Prompt for AI agents
Comment on lines
113
to
+126
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. 🟡 This PR changes wire behavior — a caller-configured Extended reasoning...What's missing. The new tier-4 fallback in |
||
|
|
||
| # SEP-2207: append offline_access when the AS supports it and the client can use refresh tokens | ||
| if ( | ||
|
|
||
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.
🟡 The
configured_scope=client_metadata.scopesnapshot can capture a previously discovered scope rather than the caller-configured one: the provider stores the caller'sOAuthClientMetadataby reference and Step 3 mutatesclient_metadata.scopein place, so a second provider constructed from the same metadata object after the first's 401 flow snapshots server A's discovered scopes and the new tier-4 sends them to server B when B advertises nothing — the same stale-scope leak this snapshot was added to prevent, just across provider instances. Considerclient_metadata.model_copy()at construction (or tracking the effective scope in a context field instead of mutatingclient_metadata.scope) to close the gap.Extended reasoning...
What the bug is.
OAuthClientProvider.__init__stores the caller'sOAuthClientMetadatainstance directly intoOAuthContext(nomodel_copy()anywhere undersrc/mcp/client/auth), and snapshotsconfigured_scope=client_metadata.scopeat construction time (src/mcp/client/auth/oauth2.py:281). Step 3 of the 401 flow then mutates that same caller-owned object in place:self.context.client_metadata.scope = get_client_metadata_scopes(...)(oauth2.py:649; the 403 step-up path atoauth2.py:731does the same).OAuthClientMetadatais a plain non-frozen Pydantic model (src/mcp/shared/auth.py), so the assignment silently rewrites the caller's model. If the caller reuses one metadata object across providers, the "configured" snapshot of a later provider is really whatever the earlier provider's discovery wrote.\n\nCode path. The polluted value reaches the wire through the tier-4 fallback this PR adds inget_client_metadata_scopes(src/mcp/client/auth/utils.py): whenWWW-Authenticate, PRMscopes_supported, and ASscopes_supportedall yield nothing,selected_scope = configured_scope or None— andconfigured_scopeis the polluted snapshot.\n\nStep-by-step proof.\n\npython\nMETADATA = OAuthClientMetadata(scope=\"user\", redirect_uris=[...]) # module-level constant\n\np1 = OAuthClientProvider(url_a, METADATA, storage_a, ...)\n# p1.context.client_metadata IS METADATA (same object); p1 configured_scope = \"user\"\n# p1's 401 flow: server A advertises scopes_supported=[\"read\", \"write\"]\n# Step 3 (oauth2.py:649): METADATA.scope = \"read write\" <-- caller's object mutated\n\np2 = OAuthClientProvider(url_b, METADATA, storage_b, ...)\n# oauth2.py:281: configured_scope = METADATA.scope = \"read write\" <-- server A's scopes\n# p2's 401 flow: server B advertises nothing (no WWW-Authenticate scope, no scopes_supported)\n# tier 4 (utils.py): selected_scope = \"read write\"\n# -> p2 requests server A's discovered scopes from server B, not the configured \"user\"\n\n\nWhy this is a consequence of this PR and not purely pre-existing. The in-place mutation of the caller's object predates the PR, but before it the pollution was self-healing: Step 3 unconditionally overwroteclient_metadata.scopewith the fresh selection, and when nothing was advertised the scope becameNoneand was simply omitted from the request — the stale value never reached the wire on this path. After this PR, the polluted value is durably captured as the "configured" fallback tier and actively sent whenever tier 4 fires. That is exactly the cross-flow stale-scope leak the PR description says theconfigured_scopesnapshot exists to prevent — just across provider instances rather than across 401 flows of one instance. (There is also a concurrency variant: two providers sharing one metadata object each hold their ownOAuthContext.lock, so provider A's Step 3 write can race provider B mid-flow reads.)\n\nWhy it's a nit, not blocking. The trigger needs a specific pattern no SDK docs or examples demonstrate — one shared mutable metadata instance, providers constructed at different times, and a second server that advertises no scopes. The docs build metadata inline per provider, andClientCredentialsOAuthProvider/PrivateKeyJWTOAuthProviderconstruct their metadata internally and are immune. The failure mode is requesting wrong/extra scopes, which a server typically rejects or narrows.\n\nFix. Either copy at construction —client_metadata=client_metadata.model_copy()in__init__so the provider owns the object Step 3 mutates — or stop mutatingclient_metadata.scopealtogether and track the effective/selected scope in a separateOAuthContextfield. The second option also removes the surprising side effect of the SDK rewriting a caller-supplied model.