fix(client): accumulate scopes (union) on step-up authorization challenges (SEP-2350)#2265
fix(client): accumulate scopes (union) on step-up authorization challenges (SEP-2350)#2265mattzcarey wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 0c4e8dc 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/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
There was a problem hiding this comment.
The union logic and unit tests look right, but the conformance suite doesn't exercise it yet. Since this is the SEP-2350 PR, could we wire that up here?
It will still warn because for the scenario to pass it also needs #2266, but given this PR is about step up auth, it would be create to wire the auth/scope-step-up as part of this PR as an expected failure in expected-failures.yml and then remove it with #2266
The fix in test/conformance/src/helpers/withOAuthRetry.ts:
-import { auth, extractWWWAuthenticateParams, UnauthorizedError } from '@modelcontextprotocol/client';
+import { auth, extractWWWAuthenticateParams, unionScopes, UnauthorizedError } from '@modelcontextprotocol/client';
@@ handle401
- const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response);
+ const { resourceMetadataUrl, scope: challengedScope } = extractWWWAuthenticateParams(response);
+ const scope = unionScopes((await provider.tokens())?.scope, challengedScope);Checked locally: the SEP-2350 check passes and the full client suite still exits 0. Leave auth/scope-step-up in expected-failures.yaml for now since it still fails the SEP-837 check; #2266 drops it once that lands.
e1ddc2f to
20d8fd1
Compare
|
Rebased onto current |
|
Addressed the remaining scope accumulation review in Changes:
Verified:
|
|
Resolved the latest changeset wording thread in 2a3ed5e. The changeset now describes the actual SEP-2350 union inputs: stored-token scopes, previously requested scopes, protected resource metadata scopes, provider default scopes, and challenged scopes. Verification:
|
There was a problem hiding this comment.
All of my earlier findings (step-up scope drop when the AS omits scope, 401 challenge clobbering accumulated state, resource-metadata URL preservation, changeset wording, SSE test coverage) look addressed in the latest commits — thanks. I'm leaving final sign-off to a maintainer since this changes OAuth re-authorization behavior, widens the step-up scope union to include PRM scopes_supported and clientMetadata.scope, and adds a new public export (unionScopes) plus new UnauthorizedContext fields, which are API/spec-interpretation calls a human should make.
Extended reasoning...
Overview
This PR implements SEP-2350 client-side scope accumulation: the 403 insufficient_scope step-up path in StreamableHTTPClientTransport now requests the union of stored-token scope, previously requested scope, PRM scopes_supported, clientMetadata.scope, and the challenged scope; the 401 paths in both StreamableHTTP and SSE transports now merge (rather than replace) accumulated scope and preserve the resource metadata URL; UnauthorizedContext gains scope/resourceMetadataUrl fields; a unionScopes helper is exported; the conformance OAuth retry helper is wired through it and the auth/scope-step-up baseline entry is removed. New regression tests cover the union, dedup, no-prior-scope, metadata-derived scopes, and 401-without-scope preservation in both transports.
State of prior review feedback
This is a multi-round PR. My earlier rounds flagged (1) the union dropping previously granted scopes when the AS omits scope from the token response, (2) 401 branches clobbering the accumulated scope/resource-metadata state, (3) changeset prose not matching the broadened union, and (4) missing SSE-side coverage. The latest commits (c1355fb, 2a3ed5e, 2f1eb55, c3ff159) address all of these: the union now folds in PRM and client-metadata scopes, the 401 branches use unionScopes / null-coalescing preservation, the changeset enumerates the actual inputs, and a mirrored SSE test was added. The current bug-hunting pass found no new issues.
Security risks
The change is in OAuth client behavior. The main risk vector is over-broad re-authorization: by including scopes_supported and clientMetadata.scope in the step-up union, the consent prompt can request more than was previously granted. This is documented in the changeset and was an intentional tradeoff to handle ASes that omit scope in token responses, but it is a privilege-scope decision a maintainer should consciously accept. There is no injection or data-exposure concern; the existing _lastUpscopingHeader guard still bounds retry loops.
Level of scrutiny
This touches auth-sensitive client transport code, changes default re-authorization behavior, adds public API surface (unionScopes export, UnauthorizedContext fields), and interprets draft-spec SHOULD language — all of which the repo's review conventions treat as requiring maintainer judgment (burden of proof on addition, spec as anchor). Test coverage is good and the conformance scenario now passes, but I am not auto-approving given the security-sensitive area and the API/design calls involved.
Other factors
Conformance expected-failures burn-down looks consistent with the author's verification notes (auth/scope-step-up removed; SEP-837 baseline handled separately per the author). The conformance helper change keeps the test harness aligned with the new client behavior.
…enges (SEP-2350) Per the draft authorization spec's step-up authorization flow, scope accumulation is a client-side responsibility: when re-authorizing after a 403 insufficient_scope challenge, the client SHOULD request the union of previously requested/granted scopes and the newly challenged scopes. Previously the StreamableHTTP transport replaced the requested scope with the challenged scopes only, dropping previously granted permissions. - Add exported unionScopes() helper (opaque-string, order-preserving, deduped union of space-delimited scope strings) - Union stored token scope + previously requested scope + challenged scope in the 403 insufficient_scope retry path - Tests for the step-up union, dedup, and no-prior-scope cases plus unionScopes unit tests Closes #2200
c3ff159 to
0c4e8dc
Compare
There was a problem hiding this comment.
The latest revision addresses all of my earlier feedback (resource-metadata preservation on 401, SSE-side test coverage, and the changeset wording), and I found no new issues — but since this changes the OAuth re-authorization behavior in both client transports and includes a deliberate design choice to fold PRM scopes_supported and the provider's default scope into the step-up union, a maintainer should sign off on that scope-broadening tradeoff.
Extended reasoning...
Overview
This PR implements SEP-2350 client-side scope accumulation: a new exported unionScopes helper in packages/client/src/client/auth.ts, union-based scope accumulation in the 403 insufficient_scope step-up path of StreamableHTTPClientTransport, scope/resource-metadata preservation in the 401 paths of both streamableHttp.ts and sse.ts, two new optional fields on UnauthorizedContext, conformance-helper wiring, and an expected-failures baseline removal. Test coverage is solid: 8 unit tests for unionScopes, six StreamableHTTP integration tests, and an SSE 401-preservation test.
Security risks
This is OAuth/authorization code. The main behavioral consideration is that the step-up union now includes the protected resource's advertised scopes_supported and the provider's configured default scope in addition to granted/requested/challenged scopes — meaning a re-authorization consent prompt can request scopes the user never previously granted. This was a deliberate, review-driven tradeoff to handle authorization servers that omit scope from token responses, and it's documented in the changeset, but it broadens the requested permission set and deserves a maintainer's explicit sign-off. No injection, data-exposure, or auth-bypass vectors were identified; the existing _lastUpscopingHeader loop guard is unchanged.
Level of scrutiny
High — this is production client auth-flow code in the SDK, touches both transports, adds a public API export, and implements a draft-spec SEP. Prior review rounds surfaced real gaps (scope sources missing from the union, 401 clobbering of accumulated state), all of which the author addressed in subsequent commits; the current bug-hunting pass found no remaining issues.
Other factors
All earlier inline comments are resolved in the latest revision: 401 branches now preserve _resourceMetadataUrl and union _scope, an SSE-side regression test was added, and the changeset enumerates the actual union inputs. Conformance auth/scope-step-up SEP-2350 checks now pass per the author's verification. Given the security-sensitive area and the design tradeoff above, this should get a human maintainer's review rather than a shadow approval.
Implements SEP-2350 (tracking issue #2200): client-side scope accumulation in step-up authorization.
Spec
The draft authorization spec (
basic/authorization/index.mdx, "Scope Challenge Handling" note and "Step-Up Authorization Flow" step 2) makes scope accumulation a client-side responsibility:Servers stay stateless and emit only per-operation scopes in
WWW-Authenticate: Bearer error="insufficient_scope", scope="..."challenges.Before / after
Before: the
403 insufficient_scoperetry path inStreamableHTTPClientTransportdidthis._scope = scope— the challenged scopes replaced the requested scope, so the re-authorization dropped everything previously granted. A client holdingread writethat hit a challenge foradminwould re-authorize with onlyadmin.After: the client requests the union of (1) the scope on the stored tokens (previously granted), (2) the previously requested scope, and (3) the newly challenged scope. The same client now re-authorizes with
read write admin. The union is order-preserving and exact-string-deduped — scopes are treated as opaque strings per the spec, so no hierarchy-aware deduplication (repodoes not absorbrepo:read).The helper is exported as
unionScopes(...scopeStrings: Array<string | undefined>): string | undefinedfrom@modelcontextprotocol/client.Retry limiting is unchanged: the existing
_lastUpscopingHeaderguard still prevents infinite upscoping loops.Decision note for reviewers: 401 path left as-is
The
401path (handleOAuthUnauthorized/ thethis._scope = scopeassignments in the 401 branches ofstreamableHttp.tsandsse.ts) is intentionally unchanged. The spec's union language is specifically about re-authorization during step-up after aninsufficient_scopechallenge; a 401 carries initial-auth semantics (no valid grant to preserve), and applying the union there is debatable. Flagging explicitly in case reviewers want union-on-401 too — happy to follow up.Relatedly,
sse.tshas no403 insufficient_scopehandler at all (only 401 paths), so there was nothing to mirror there.Validation
pnpm build:all✅pnpm typecheck:all✅pnpm lint:all✅pnpm --filter @modelcontextprotocol/client test— 378/378 ✅ (new: 3 step-up integration tests instreamableHttp.test.tscovering union, dedup-on-repeated-scope, and no-prior-scope; 8unionScopesunit tests inauth.test.ts)@modelcontextprotocol/clientDownstream impact: cloudflare/agents needs no changes — its DurableObjectOAuthClientProvider persists token scope, so the union picks previously granted scopes up automatically.
A follow-up PR implementing SEP-2468 (iss validation) touches the same transport retry region and will be rebased over this.
Closes #2200