fix(client): preserve scopes across authorization retries#2448
fix(client): preserve scopes across authorization retries#2448mattzcarey wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 110dfb4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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.
mattzcarey
force-pushed
the
fix/sep-2350-scope-accumulation
branch
from
8499a5f to
9d3352f
Compare
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
Thank you! Good find.
| const challenge = extractWWWAuthenticateParams(ctx.response); | ||
| const tokens = await provider.tokens(); | ||
| const result = await auth(provider, { | ||
| serverUrl: ctx.serverUrl, | ||
| resourceMetadataUrl, | ||
| scope, | ||
| resourceMetadataUrl: challenge.resourceMetadataUrl ?? ctx.resourceMetadataUrl, | ||
| scope: computeScopeUnion(tokens?.scope, ctx.scope, challenge.scope), | ||
| fetchFn: ctx.fetchFn, | ||
| ...extraAuthOptions | ||
| }); |
There was a problem hiding this comment.
🟡 handleOAuthUnauthorized now unions the token, transport, and challenge scopes, but unlike the two sibling paths this PR touches (_stepUpAuthorize and withOAuth) it never passes forceReauthorization when the union strictly exceeds the token's granted scope. When a refresh_token is present, auth() takes the refresh branch (which cannot widen scope per RFC 6749 §6), so a 401 challenge requesting a wider scope refreshes the same narrow token, retries once, and throws SdkHttpError instead of forcing a fresh authorization — this matters most for legacy SSE, which has no 403 insufficient_scope step-up path. Consider adding the same isStrictScopeSuperset-gated forceReauthorization here for consistency.
Extended reasoning...
What the code does today
handleOAuthUnauthorized (auth.ts:186-194) now computes computeScopeUnion(tokens?.scope, ctx.scope, challenge.scope) and passes it to auth(), but it never sets forceReauthorization. In authInternal (auth.ts:~1310), the refresh-token branch is taken whenever tokens?.refresh_token is present and forceReauthorization is falsy, and it returns 'AUTHORIZED' immediately after the refresh. As the SDK's own comments (and RFC 6749 §6) note, a refresh cannot widen the granted scope. The two sibling paths this same PR touches handle the identical situation differently: StreamableHTTPClientTransport._stepUpAuthorize computes forceReauthorization = isStrictScopeSuperset(unionScope, tokens?.scope), and withOAuth in middleware.ts is explicitly updated by this PR to add forceReauthorization when the union strictly exceeds the stored token scope.
Step-by-step failure scenario
- The client holds tokens
{ scope: 'read', refresh_token: '...' }for a Streamable HTTP or legacy SSE transport. - The server responds
401withWWW-Authenticate: Bearer scope="read admin". - The transport calls
onUnauthorized→handleOAuthUnauthorized, which computes the union'read admin'and callsauth()with it — but withoutforceReauthorization. authInternalsees a refresh token and no force flag, so it takes the refresh branch and gets back a token still scoped to'read'.- The transport retries once with
isAuthRetry=true, receives401again, and throwsSdkHttpError('Server returned 401 after re-authentication')— instead of forcing a fresh authorization request with the widened scope, as the 403 step-up path andwithOAuthdo.
Why it matters (and why it's only a nit)
The impact is real but narrow. RFC 6750 directs servers to signal insufficient scope with 403 error="insufficient_scope", and that path is handled correctly by _stepUpAuthorize for Streamable HTTP. The scenario above requires a server that steps up via a 401 with a wider scope challenge, plus a stored refresh token. Also, the refresh-first behavior on 401 predates this PR — before this change, handleOAuthUnauthorized also passed no forceReauthorization — so nothing that worked before regresses. One refutation argued the 403 path is a sufficient backstop for the transports; that is true for Streamable HTTP, but legacy SSE has no 403 insufficient_scope step-up path at all, so the 401 handler is its only step-up avenue, and the changeset/migration text ("Automatic 401 handling and legacy SSE preserve that accumulated scope") implies the widened scope is actually obtainable there — with a refresh token present it is not.
Suggested fix
Mirror the sibling paths: compute const forceReauthorization = isStrictScopeSuperset(unionScope, tokens?.scope) and pass it to auth() when true. This is safe for the common expired-token 401 case: when the challenge does not exceed the granted scope, the union equals the granted scope, isStrictScopeSuperset returns false, and the refresh-first behavior is preserved unchanged. The AuthOptions doc for forceReauthorization already tells callers to set it exactly in this situation, so the omission looks like an oversight rather than an intentional divergence.
2 participants
Previously unaddressed
#2356 accumulated token, transport, and challenge scopes during 403 step-up, but could still lose earlier permissions when a token response omitted
scope. It also did not carry accumulated scope and resource metadata through automatic 401, legacy SSE, andwithOAuthpaths.This PR
withOAuthVerification
pnpm check:allpnpm build:allFollow-up to #2265, #2356, and #2286.