Repository navigation
fix(client): optionally keep previously granted scopes when a refused refresh leads to a new authorization - #2448
mattzcarey wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 9f7c248 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find bugs in the implementation, but this changes OAuth scope-request behavior across several auth paths (notably unioning in PRM scopes_supported and provider default scope during step-up, and forcing reauthorization in withOAuth), so it deserves a human look at the policy implications.
Extended reasoning...
Overview
This PR modifies the OAuth retry/step-up handling in the client package: handleOAuthUnauthorized now unions the stored token scope with accumulated transport scope and the challenge scope, _stepUpAuthorize in the Streamable HTTP transport additionally unions protected-resource-metadata scopes_supported and the provider's default clientMetadata.scope, both transports preserve accumulated scope/resource-metadata URL instead of overwriting them on 401, and withOAuth now computes the scope union and forces reauthorization when the union strictly exceeds the granted scope. Tests are added for each path (Streamable HTTP, legacy SSE, withOAuth, handleOAuthUnauthorized), plus a changeset and a migration-doc update that matches the new behavior.
Security risks
This is authorization-flow code, so it is inherently security-sensitive. The main policy question is scope widening: including PRM scopes_supported and the provider's default clientMetadata.scope in the step-up union means the client may request broader permissions than the specific challenge requires, which trades least-privilege for not losing previously granted permissions. That is a deliberate design choice stated in the PR description and documented in the migration guide, but it is the kind of behavioral/security decision a maintainer should confirm rather than a bot. The forceReauthorization addition in withOAuth also changes when the refresh-token path is bypassed. I did not find injection, token-leak, or bypass issues in the diff itself.
Level of scrutiny
High. The change touches auth code paths in a published SDK (401 handling, 403 step-up, legacy SSE, and the fetch middleware), and the behavior change (broader scope requests, refresh bypass) affects every OAuth-enabled client. This is well beyond the config-tweak/mechanical threshold for shadow approval.
Other factors
The implementation is internally consistent: resourceMetadataUrl ?? this._resourceMetadataUrl and computeScopeUnion(this._scope, scope) are applied uniformly across the 401 sites, the new UnauthorizedContext fields are optional and backward compatible, and the docs/changeset text matches the code. Test coverage for the new behavior is reasonable. The bug-hunting system found no issues, and there are no prior reviewer comments to weigh.
8499a5f to
9d3352f
Compare
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
Thank you! Good find.
- force fresh authorization when a 401 widens the granted scope - cover refresh-token scope widening with a regression test
claude
Bot
added
the
v2
e87cd73 to
4715991
Compare
mattzcarey
changed the title
4715991 to
84381b1
Compare
mattzcarey
changed the title
There was a problem hiding this comment.
3 optional suggestions (nits or notes on pre-existing code) were found and not posted.
Still open from earlier reviews (2):
- 🔴
packages/client/src/client/streamableHttp.ts:1093—Machine-to-machine clients can lose the connection for good after one refused step-up: every later token renewal re-req… - Also unresolved: 1 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
84381b1 to
a0c1d99
Compare
There was a problem hiding this comment.
3 optional suggestions (nits or notes on pre-existing code) were found and not posted.
The latest push brings back an issue flagged in an earlier review; replied in that comment's thread.
Still open from earlier reviews (2):
- 🔴
packages/client/src/client/auth.ts:1326—Non-interactive clients can fail every token renewal after the server drops or narrows a scope, where the base renewed… - Also unresolved: 1 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
a0c1d99 to
0956210
Compare
There was a problem hiding this comment.
I reviewed the latest push (0956210) and found no bugs this time. Because it changes OAuth scope selection across auth(), both transports and withOAuth, and adds public API (retainScope, two UnauthorizedContext fields), a maintainer look is still worthwhile.
What was reviewed:
handleOAuthUnauthorized/authInternal:retainScopeis only unioned into the selected scope, never setsforceReauthorization, so the refresh-token branch still runs first; it is dropped for providers withoutredirectUrland filtered to the authorization server'sscopes_supportedwhen advertised.- Streamable HTTP step-up:
_scopeis now written only onAUTHORIZED; a redirect parks the union in_pendingStepUpScope, whichfinishAuth()clears before resolving the callback and commits only on success. A thrownauth()leaves both untouched. - SSE and Streamable HTTP 401 paths:
_scopeaccumulates and_resourceMetadataUrlsurvives a challenge without one; both are passed toonUnauthorizedon connect and send. - One narrow edge for the human pass, not confirmed as a failure:
_pendingStepUpScopeis cleared only insidefinishAuth(), so a step-up redirect the user abandons (no callback at all) would leave it for the next unrelatedfinishAuth()to commit into_scope.
Extended reasoning...
The change is a single commit touching packages/client auth.ts, middleware.ts, sse.ts and streamableHttp.ts plus five test files and a patch changeset (+658/-37). It sits squarely on a security-sensitive surface: which OAuth scopes the client requests on 401 re-authorization and step-up, and when a widened scope is recorded. This run's bug hunt ran to a dry streak with no findings, and the latest commit follows three earlier rounds of findings on this PR. Deferring rather than approving because it is auth code with new public options, the whole repo is owned by the typescript-sdk CODEOWNERS team, and the one earlier human approval was dismissed with no human review of the current commit.
0956210 to
0f0b669
Compare
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
The fix works for the reported case. But two cases that work on main break with this PR:
- A retained scope that the server no longer accepts. A user has
read write, an admin removeswrite, and the refresh fails.- On
main: the client asks forread, and the user signs in. - With this PR: the client asks for
read write, and the server answersinvalid_scope. Each later attempt asks for the same set, so the user cannot sign in until the stored tokens are cleared.
- On
- Parallel step-ups. Three calls at the same time need
write,adminandbilling.- On
main: the client ends withread write admin billing, and all three calls work. - With this PR: the client asks for
read write,read adminandread billing. It ends withread billing, so two of the calls redirect again.
- On
Can you confirm these and decide how to handle them?
Generated by Claude Code
c1128b3 to
3731069
Compare
3731069 to
fccb2da
Compare
fccb2da to
dfb8587
Compare
mattzcarey
changed the title
| When a refresh is refused (`invalid_grant` or `invalid_dpop_proof`), the SDK drops the stored tokens and sends the user to sign in again for the scope it would request on a first sign-in. Permissions granted earlier through step-up are not part of that, so the user steps up again for them. Set `includePreviouslyGrantedScopes` to request them in that sign-in too, as the MCP specification recommends. | ||
|
|
||
| ```ts source="../../examples/guides/clients/oauth.examples.ts#includePreviouslyGrantedScopes" | ||
| class KeepGrantedScopesProvider extends MyOAuthProvider { | ||
| includePreviouslyGrantedScopes = true; | ||
| } | ||
| ``` | ||
|
|
||
| The SDK reads the scope saved with the dropped tokens and leaves out `offline_access` and any value the authorization server does not list in `scopes_supported`. It requests that scope once: if the server refuses it, the next sign-in requests the usual scope alone. Nothing is added when the sign-in response had no `scope`, when there is no refresh token, or when no scope would be requested at all. |
There was a problem hiding this comment.
🟡 (optional) Developers who copy the new example get an option that never takes effect: a refused refresh throws OAuthError instead of a sign-in that keeps earlier scopes. KeepGrantedScopesProvider extends MyOAuthProvider, which has no invalidateCredentials, so the tokens are never dropped and auth.ts:1490 adds nothing. Fix: make the documented path work and say what it needs, by adding an invalidateCredentials that clears tokens to the example and naming that requirement in this section and the includePreviouslyGrantedScopes JSDoc (auth.ts:261-273).
Why this was flagged
A provider built from the page's own example sets includePreviouslyGrantedScopes = true (docs/clients/oauth.md:188-190) on top of MyOAuthProvider, which defines no invalidateCredentials (examples/guides/clients/oauth.examples.ts:43-98). On invalid_grant, packages/client/src/client/auth.ts:1114 calls provider.invalidateCredentials?.('tokens'), a no-op, so the retry at auth.ts:1115 finds the same tokens at auth.ts:1421, refreshes again at auth.ts:1433 and rethrows at auth.ts:1468. No redirect happens, and the tokens || guard at auth.ts:1490 would drop the carried scope anyway. The new test 'does not redirect when the provider cannot drop the grant' (packages/client/test/client/auth.test.ts:5780) pins exactly this. The base throws here too, but had no text claiming otherwise: line 185 says the SDK "drops the stored tokens and sends the user to sign in again", and the "Nothing is added when" list at line 193 omits this case.
Verification: examples/guides/clients/oauth.examples.ts:43-98 — MyOAuthProvider defines tokens, saveTokens, state, discovery and verifier methods, but no invalidateCredentials. auth.ts:1421 — the retry reads the same stored tokens; auth.ts:1433 sees tokens?.refresh_token and refreshes again at 1438; the same refusal is rethrown at 1468. Nothing regresses against the base.
Finding ID: f-65e95c33
| * Values the authorization server does not list in `scopes_supported` and `offline_access` are | ||
| * left out. The scope is requested only once: if the server refuses it, the next authorization | ||
| * requests the selected scope alone. |
There was a problem hiding this comment.
🟡 nit (optional): readers whose authorization server publishes no scopes_supported are told unlisted values are left out, yet every dropped scope is requested again. The JSDoc at auth.ts:267-268 and docs/clients/oauth.md:193 state the filter without that case. filterToSupportedScopes (auth.ts:1134) keeps all values when the list is absent or empty. Fix: say in both places that every value except offline_access is kept when the server advertises no scopes_supported, which covers the 2 sites listed. Same instruction at 2 sites (auth.ts:268, docs/clients/oauth.md:193).
Why this was flagged
No runtime failure; this is a check of public wording the diff adds. It applies when the authorization server metadata has no scopes_supported or an empty one, which RFC 8414 allows. auth.ts:267-268 says "Values the authorization server does not list in scopes_supported and offline_access are left out", and docs/clients/oauth.md:193 says the SDK "leaves out offline_access and any value the authorization server does not list in scopes_supported". Read literally, nothing is listed, so nothing would be carried. The code at auth.ts:1134 is !supported?.length || supported.includes(value), so every value except offline_access is carried. Only the JSDoc on the private helper (auth.ts:1126) mentions "all of them when it advertises none". The base has no such text. The instruction guards against docs that describe something other than what the code does.
Verification: packages/client/src/client/auth.ts:1134 filters with value !== 'offline_access' && (!supported?.length || supported.includes(value)). When scopes_supported is absent or empty, !supported?.length is true, so every value except offline_access is kept and requested again. Neither public text mentions the keep-all case.
Finding ID: f-f55a0271
… refresh leads to a new authorization When a refresh was refused (invalid_grant or invalid_dpop_proof), auth() dropped the stored tokens and started a new authorization for the selected scope only, so permissions granted earlier through step-up were lost and the user was sent back through step-up for them. Add OAuthClientProvider.includePreviouslyGrantedScopes (default false, so the request is unchanged unless a provider opts in). When true, auth() reads the token's scope before dropping it and adds it to that one authorization request (MCP 2026-07-28: re-authorization includes previously granted scopes). Once the tokens are gone nothing stored can supply it again, so a scope the server refuses is requested only once; it is carried only when no tokens remain and a scope is requested at all. offline_access and values the authorization server does not list in scopes_supported are left out. Code exchange, non-interactive token requests, the transports, step-up and withOAuth are unaffected.
dfb8587 to
9f7c248
Compare
Adds
OAuthClientProvider.includePreviouslyGrantedScopes. When a refresh is refused (invalid_grantorinvalid_dpop_proof),auth()drops the stored tokens and starts a new authorization for the selected scope only, so permissions granted earlier through step-up are lost and the user goes back through step-up for them. With the flag set,auth()reads the dropped token'sscopefirst and asks for it in that authorization too. The default isfalse, so nothing changes unless a provider opts in.The 2026-07-28 spec says a client re-authorizing should include the challenged scopes "alongside any previously granted scopes". We hit this in the Cloudflare Agents SDK (cloudflare/agents#2663): a user grants a write scope through step-up, the refresh later fails, and the new link asks for the base scope only. cloudflare/agents#2698 sets the flag and runs the Agents SDK's MCP suite and client conformance lanes against this PR's pkg.pr.new build. Follow-up to #2265, #2356 and #2286; same opt-in shape as #2877.
Earlier versions of this PR kept scope on every
401, by default, and needed transport bookkeeping to undo it, which is where the reviewed regressions came from. This version carries the scope only from the point where the grant is dropped, so nothing stored can supply it again, and only for providers that ask.With the flag set:
invalid_scope(RFC 6749 §4.1.2.1: the value is invalid or unknown), the next authorization asks for the selected scope alone. A server that grants less instead (§3.3) costs nothing.server_error, a provider withoutinvalidateCredentials, or nothing selecting a scope (the server's default grant applies), the request is the same as without the flag.offline_accessand values the authorization server does not list inscopes_supportedare left out, so expanded forms a token reports (api://app/Readforapi://app/.default) are not requested back, and the flag never addsprompt=consent(Unconditionalprompt=consent(whenoffline_accessin scope) blocks OAuth in Entra tenants with user consent disabled + admin consent granted #2867). Servers may leave supported values out of that list (RFC 8414); those are not carried.scope(RFC 6749 §5.1 allows that when it equals the request).finishAuthandwithOAuthare unchanged.pnpm check:all, the client tests and the client conformance suite pass.