-
Notifications
You must be signed in to change notification settings - Fork 2k
fix(client): preserve scopes across authorization retries #2448
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
mattzcarey
wants to merge
4
commits into
main
Choose a base branch
from
fix/sep-2350-scope-accumulation
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.
+437
−37
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
9d3352f
fix(client): preserve scopes across authorization retries
mattzcarey 872b9b3
Merge branch 'main' into fix/sep-2350-scope-accumulation
felixweinberger 110dfb4
Merge branch 'main' into fix/sep-2350-scope-accumulation
mattzcarey e87cd73
fix: address PR #2448 review feedback
mattzcarey 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': patch | ||
| --- | ||
|
|
||
| Preserve previously requested OAuth scopes across Streamable HTTP, legacy SSE, automatic 401 handling, scope step-up, and `withOAuth`. Step-up unions explicit token, transport, and challenged scopes; when prior scope is unavailable, it reconstructs the initial protected-resource-metadata/provider fallback without over-requesting both. |
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
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.
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.
🔴 handleOAuthUnauthorized computes forceReauthorization = isStrictScopeSuperset(unionScope, tokens?.scope) with no guard for the case where the stored token has no scope field — which RFC 6749 §5.1 permits (and many ASes do) when granted == requested. Since the transports now thread accumulated _scope/challenge scope into ctx.scope on every 401, an ordinary access-token expiry then bypasses the working refresh token and forces interactive re-authorization (or throws UnauthorizedError), a regression from the pre-PR silent-refresh behavior. Mirror the guard this same PR adds in withOAuth (lastTokenScope !== undefined && isStrictScopeSuperset(...)), or reconstruct the prior requested scope the way _stepUpAuthorize does before comparing.
Extended reasoning...
What the bug is
handleOAuthUnauthorized(auth.ts:186-194) now computes:isStrictScopeSuperset(auth.ts:638-645) treats an undefined/absentcurrentscope as the empty set, so it returnstruewheneverunionScopeis non-empty and the stored token record has noscopefield. Per RFC 6749 §5.1, authorization servers commonly omitscopefrom the token response when the granted scope equals the requested scope, andexchangeAuthorization/saveTokensstore exactly what the AS returned — sotokens?.scope === undefinedis the normal steady state for many providers, not an edge case.The code path that triggers it
This PR newly threads the transports' accumulated
_scope(andresourceMetadataUrl) intoctx.scopeon every 401 for Streamable HTTP_send/_startOrAuthSseand legacy SSE, all of which route throughadaptOAuthProvider→handleOAuthUnauthorized. So the trigger only requires that any earlierWWW-Authenticatechallenge carried ascopeparam (e.g. the very first pre-token 401 that kicked off authorization — RFC 6750 servers routinely include one), or that the current 401 does.Step-by-step regression
401withWWW-Authenticate: Bearer scope=\"read\"→ transport records_scope = 'read', user completes interactive auth.scope(granted == requested, per §5.1) → stored tokens are{ access_token, refresh_token }with noscopefield.401.onUnauthorizedwithctx.scope = 'read'(new in this PR) →unionScope = 'read',tokens?.scope = undefined→isStrictScopeSuperset('read', undefined)→true→forceReauthorization: true.authInternal(auth.ts:1313) skips therefresh_tokenbranch entirely whenforceReauthorizationis set and starts a fresh authorization request; for interactive providers that callsredirectToAuthorizationand returns'REDIRECT', sohandleOAuthUnauthorizedthrowsUnauthorizedErrorand the user is bounced through the consent flow on every ordinary token expiry.Before this commit,
handleOAuthUnauthorizednever passedforceReauthorization, so step 3–5 was handled by a silent refresh + retry. This is therefore a concrete behavioral regression introduced here, not a pre-existing quirk.Why the existing code doesn't prevent it
The
isStrictScopeSupersetdoc comment describes its "conservative" treatment of an absent token scope, but that reasoning was written for the 403insufficient_scopestep-up path, where the server has explicitly said the current grant is insufficient and forcing a fresh authorization is the right call. A plain 401 is overwhelmingly token expiry, where the refresh token is exactly the right tool. The same PR demonstrates awareness of this distinction in the sibling paths:withOAuth(middleware.ts:66) guards withlastTokenScope !== undefined && isStrictScopeSuperset(...), and_stepUpAuthorizereconstructs the previously-requested scope fromdiscoveryState/clientMetadatabefore comparing precisely so an omitted token scope isn't treated as "no prior grant".handleOAuthUnauthorizeddoes neither, so the two 401 paths this PR touches are inconsistent.Impact
For any provider whose token responses omit
scopeand any deployment where a challenge scope has been observed (or the transport accumulated one), every access-token expiry becomes an interactive consent bounce (browser redirect) or a hardUnauthorizedErrorfor non-interactive clients — instead of the silent refresh that worked before this PR. This affects Streamable HTTP, legacy SSE, and the rawhandleOAuthUnauthorizedhelper.How to fix
Gate the force the same way
withOAuthdoes in this PR:or reconstruct the previously-requested scope (transport
ctx.scope, then PRMscopes_supported, thenclientMetadata.scope) the way_stepUpAuthorizedoes and compare the union against that, so a genuinely widened challenge still forces re-authorization while a routine expiry keeps using the refresh token.