feat(client): SEP-2350 scope step-up — union, retry cap, superset-gated refresh bypass#2356
Conversation
🦋 Changeset detectedLatest commit: 8a647f6 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: |
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
46b6b01 to
d62ce60
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
from
52b1cfc to
a81ff07
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
d62ce60 to
b69058a
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
2 times, most recently
from
67c8e16 to
401f456
Compare
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
b69058a to
1ac3c91
Compare
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
1ac3c91 to
9626ed5
Compare
|
|
||
| /** | ||
| * Whether `union` contains at least one scope token not present in `current`. | ||
| * Both arguments are space-delimited scope strings per RFC 6749 §3.3. | ||
| * | ||
| * Used to gate the step-up refresh bypass: when the union of previously-requested | ||
| * and newly-challenged scopes is a strict superset of the current token's | ||
| * granted scope, refreshing cannot widen the grant (RFC 6749 §6), so the | ||
| * transport must force a fresh authorization request instead. When the current | ||
| * token already covers the union, refresh remains valid. | ||
| * | ||
| * An undefined or empty `current` is treated as the empty set, so any non-empty | ||
| * `union` is a strict superset. Note that per RFC 6749 §3.3 an authorization | ||
| * server MAY omit the token's `scope` field when it equals the requested scope; | ||
| * this helper is conservative and treats an absent token `scope` as empty, so | ||
| * step-up always forces a fresh authorization request in that case rather than | ||
| * risking a refresh that silently drops the widened scope. | ||
| */ | ||
| export function isStrictScopeSuperset(union: string | undefined, current: string | undefined): boolean { | ||
| if (!union) return false; | ||
| const currentSet = new Set((current ?? '').split(/\s+/).filter(Boolean)); | ||
| for (const token of union.split(/\s+/)) { | ||
| if (token && !currentSet.has(token)) return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 isStrictScopeSuperset(union, current) is named and documented as a strict-superset check, but the implementation only tests whether union contains a token absent from current (union ⊄ current) — e.g. isStrictScopeSuperset('read write', 'read admin') returns true even though it is not a superset. Since this is a brand-new public export that the forceReauthorization JSDoc and docs/migration.md tell hosts in 'throw' mode to call with their own inputs, either rename/re-document it to its actual semantics (the first JSDoc sentence is already accurate) or implement a real strict-superset check before the name ossifies.
Extended reasoning...
What the bug is. The new public export isStrictScopeSuperset(union, current) (packages/client/src/client/auth.ts:534-541) is named — and described in the JSDoc body, the changeset, docs/migration.md, the AuthOptions.forceReauthorization JSDoc, and the client-auth:stepup:refresh-bypass-on-superset requirement text — as testing whether union is a strict superset of current. The implementation answers a weaker predicate: it returns true iff union contains at least one token not present in current (i.e. union ⊄ current). It never verifies the superset half of the condition (current ⊆ union). The first JSDoc sentence ("Whether union contains at least one scope token not present in current") is the only consumer-facing description that matches the code.
Step-by-step proof.
- Call
isStrictScopeSuperset('read write', 'read admin'). currentSet = {read, admin}.- The loop reaches token
'write', which is not incurrentSet, so the function returnstrue. - But
'read write'is not a superset of'read admin'—currentholdsadmin, whichunionlacks — so a strict-superset check would returnfalse. - The new unit table in
auth.test.tsnever exercises a shape where each side has a token the other lacks (the closest case, union'read write'vs current'write read admin', returnsfalsebecause every union token is covered), so nothing pins the semantics the name promises.
Why the SDK's own path never diverges. Inside the transport, _stepUpAuthorize always builds the first argument via computeScopeUnion(this._scope, tokens?.scope, challenge.scope), and the conformance fixture builds it via computeScopeUnion(previousTokens?.scope, challengedScope) — so union ⊇ current holds by construction and the two predicates coincide. There is no behavioral bug on any in-repo call path, which is why this is a nit, not a blocker.
Why it still matters. The helper is a deliberate public export (packages/client/src/index.ts), and the forceReauthorization JSDoc plus the migration guide explicitly direct hosts driving step-up themselves (onInsufficientScope: 'throw') to call it with their own inputs ("set this when isStrictScopeSuperset of the union over the current token's scope is true"). The precondition that the first argument must be a true union folding in the current grant is implied only by the parameter name — it is neither stated nor checked. A host that passes, say, only the challenged scope (or its own requested scope) as the first argument gets a result the name misdescribes. Renaming a public export after release is a breaking change, so the window to fix the name/prose cheaply is now.
On the refutation. A reviewer argued this is non-actionable bikeshedding because (a) the first JSDoc sentence documents the real semantics, (b) every documented usage feeds a true union so the predicates coincide, and (c) even on non-union inputs the implemented predicate ("requested scope has tokens the current grant lacks") is the right gate for the refresh bypass. Points (b) and (c) are correct as far as behavior goes — and they are exactly why the right fix is a rename/re-document rather than changing the implementation. But the function's name, its summary phrase ("strict superset"), and all the consumer-facing prose describe a different predicate than the one shipped, on an API where the union-construction precondition is unstated, and the divergent case is untested. For a brand-new, deliberately exported helper, name/contract accuracy at introduction is the cheap moment to get right; afterward it requires a deprecation cycle.
How to fix. Either:
- Rename to something that matches the implementation (e.g.
requiresScopesBeyondGrant/hasScopesNotGranted) and align the JSDoc summary, changeset,docs/migration.md, theforceReauthorizationJSDoc, and the requirement text with the already-accurate first sentence; or - Keep the name and implement a true strict-superset check (also require
current ⊆ union), and add a unit case like{ union: 'read write', current: 'read admin', expected: false }to pin it. Note the in-transport call sites are unaffected either way since their unions always covercurrent.
| /** | ||
| * SEP-2350 step-up: compute the union scope, decide whether refresh must be | ||
| * bypassed, and run {@linkcode auth}. Returns the auth result so the caller | ||
| * can decide whether to retry. Shared by the POST `_send` path and the GET | ||
| * `_startOrAuthSse` path so both apply the same `'throw'` short-circuit, | ||
| * the same superset-gated refresh bypass, and the same retry cap. | ||
| */ | ||
| private async _stepUpAuthorize( | ||
| challenge: { scope?: string; resourceMetadataUrl?: URL; errorDescription?: string; statusText?: string; text?: string | null }, | ||
| stepUpRetries: number | ||
| ): Promise<'AUTHORIZED' | 'REDIRECT'> { | ||
| if (this._onInsufficientScope === 'throw') { | ||
| throw new InsufficientScopeError({ |
There was a problem hiding this comment.
🟡 The PR description's first sentence claims onInsufficientScope step-up handling is added "on both HTTP transports" with "a bounded per-transport retry cap", but the diff only modifies StreamableHTTPClientTransport — SSEClientTransport (packages/client/src/client/sse.ts) is untouched and has no 403 insufficient_scope handling, no onInsufficientScope/maxStepUpRetries, and no computeScopeUnion/InsufficientScopeError; the retry counter is also per-send/per-stream-open, not per-transport. The shipped artifacts (changeset, docs/migration.md, e2e requirements) correctly scope the feature to StreamableHTTPClientTransport, so either correct the description prose (which often becomes the squash commit message) or, if SSE was intended, extend sse.ts.
Extended reasoning...
What the discrepancy is. The PR description opens with: "Adds onInsufficientScope handling on both HTTP transports: on 403 insufficient_scope the transport computes previously-granted ∪ challenged via the new exported computeScopeUnion(), applies a bounded per-transport retry cap…". The diff, however, ships the entire step-up machinery (_stepUpAuthorize, onInsufficientScope, maxStepUpRetries, InsufficientScopeError, the computeScopeUnion/isStrictScopeSuperset wiring) only in packages/client/src/client/streamableHttp.ts. The SDK's second HTTP client transport, SSEClientTransport (packages/client/src/client/sse.ts), is not touched by this PR at all: it has no 403/insufficient_scope branch, no onInsufficientScope/maxStepUpRetries options on SSEClientTransportOptions, no use of computeScopeUnion or InsufficientScopeError, and its 401 handlers (around sse.ts:159 and sse.ts:322) still do a plain this._scope = scope rather than the union-preserving assignment this PR adds to the streamable transport's 401 paths.\n\nStep-by-step proof.\n1. The changed-files list contains 19 files; packages/client/src/client/sse.ts is not among them.\n2. Grep sse.ts for insufficient_scope, onInsufficientScope, maxStepUpRetries, computeScopeUnion, InsufficientScopeError — zero hits. The only WWW-Authenticate handling there is the 401 path with this._scope = scope.\n3. Construct an SSEClientTransport with an OAuthClientProvider and have the RS answer a request with 403 + WWW-Authenticate: Bearer error=\"insufficient_scope\", scope=\"files:write\": no scope union is computed, no retry-capped re-authorization runs, no typed InsufficientScopeError is thrown — the consumer gets the transport's generic non-OK error, exactly as before this PR.\n4. So the description's "both HTTP transports" is not backed by the diff. The "per-transport retry cap" phrase is also slightly off: stepUpRetries is threaded per _send chain / per _startOrAuthSse open (the new test "caps step-up retries per send (bounded counter)" pins exactly that), not per transport instance.\n\nWhy this is a nit, not a blocker (and addressing the refutation). A refuting reviewer correctly pointed out that every shipped, consumer-facing artifact is accurately scoped: the changeset names StreamableHTTPClientTransport/StreamableHTTPClientTransportOptions only, docs/migration.md's step-up section says "StreamableHTTPClientTransport now accepts onInsufficientScope", and all five new e2e requirements declare transports: ['streamableHttp']. It is also true that sse.ts never had the pre-PR upscoping/verbatim-header path (that lived only in streamableHttp.ts), so this is not a partial migration leaving a buggy sibling, and excluding the legacy SSE transport from a brand-new opt-in feature is a plausible, deliberate scoping choice. The refutation's reading that "both HTTP transports" might be shorthand for the two HTTP request paths (POST _send and GET _startOrAuthSse) is conceivable, but the natural reading of "HTTP transports" in this SDK is the two exported HTTP client transport classes, and "per-transport retry cap" reinforces the wrong reading. All of that is why this is filed as a nit confined to the PR description — but the description still matters: it frequently becomes the squash commit message and is what future readers (and the SEP-2350 tracking issue) will cite when asking whether SSE clients get step-up.\n\nHow to fix. Editorial only: reword the first sentence to "Adds onInsufficientScope handling on StreamableHTTPClientTransport (both the POST send path and the GET listen-stream open path)… applies a bounded per-send retry cap…", and optionally state explicitly that SSEClientTransport step-up is out of scope / a follow-up. Alternatively, if SSE was actually intended to receive SEP-2350 step-up, extend sse.ts with the same handling (and add sse-transport e2e rows) — but given the changeset, docs, and tests all scope to streamableHttp, the wording fix is the smaller and likely intended change.
…ed refresh bypass Adds onInsufficientScope handling: computeScopeUnion (previously-granted ∪ challenged), per-transport retry cap, and superset-gated refresh bypass when the held token already covers the challenged scope. InsufficientScopeError joins the other flow errors in authErrors.ts. Includes the scoped-tools example story (server + demo AS + client). The demo authorization server is hardened: DCR rejects any redirect_uri that is not a loopback host (127.0.0.1, localhost, [::1]) with http/https scheme; /authorize validates the request's redirect_uri exactly against the per-client registered set; both the demo AS and the MCP resource server bind to 127.0.0.1 only. Also wires the conformance fixture's fetch-level withOAuthRetry to use the exported computeScopeUnion on 403 insufficient_scope (the fixture intercepts before the transport's _stepUpAuthorize sees it). The 2026-07-28 leg's auth/scope-step-up entry stays in the expected-failures baseline — it is gated on the fixture's 2025 lifecycle (-32020 before scope escalation is observed), orthogonal to this change. Claude-Session: https://claude.ai/code/session_01XBib5gRe8AMPPJhySCz3EJ
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
9626ed5 to
8a647f6
Compare
…ed refresh bypass (#2356)
1 participant
a0266c630(squash of07fdc0d2d+ae5453f21+9be8a7e97)Adds
onInsufficientScopehandling on both HTTP transports: on403 insufficient_scopethe transport computespreviously-granted ∪ challengedvia the new exportedcomputeScopeUnion(), applies a bounded per-transport retry cap, and — when the union is a strict superset of the held token's scope — bypasses the refresh branch and forces a freshstartAuthorization(RFC 6749 §6: refresh cannot widen scope). New exportedInsufficientScopeError. Adds theexamples/scoped-toolsheadless story (server + demo AS + client) with the demo AS hardened to loopback-onlyredirect_urisat DCR. Wires the conformance fixture'swithOAuthRetryto usecomputeScopeUnionsoauth/scope-step-upburns on the 2026 leg.Motivation and Context
SEP-2350 (modelcontextprotocol/modelcontextprotocol#2350).
Normative sentences implemented:
The superset-gated refresh bypass is the security-review-mandated guard from the PRD: a refresh grant cannot widen scope per RFC 6749 §6, so step-up to a strict superset must go through
startAuthorization.How Has This Been Tested?
computeScopeUnionset semantics; retry-cap counter.client-auth:step-up:{union, retry-cap, superset-bypass-refresh, throw-mode, get-stream-403},hosting-entry-authstep-up rows.examples/scoped-toolsis a self-verifying headless OAuth leg inpnpm run:examples.auth/scope-step-upburns on the 2026-07-28 leg (the fixture's fetch-levelwithOAuthRetrynow calls the exportedcomputeScopeUnionon 403, exercising the same helper a real consumer would).Breaking Changes
None.
onInsufficientScopedefaults to'reauthorize', which is the existing behavior at the plan pin (PRD user stories 30–31: the named consumer'swrapFetchWithStepUpDetectionkeeps working without an opt-in).'throw'is the new opt-in.Types of changes
Checklist
Additional context
Demo AS hardening (folded in from
ae5453f21). Theexamples/scoped-toolsdemo authorization server now: rejects any DCRredirect_urithat is not a loopback host (127.0.0.1,localhost,[::1]) with http/https scheme; validates/authorize'sredirect_uriexactly against the per-client registered set; binds both the demo AS and the MCP resource server to127.0.0.1only. This prevents an unauthenticated DCR from registering an external redirect target and exfiltrating authorization codes from the demo.Fixture wiring (folded in from
9be8a7e97). The conformance fixture's fetch-levelwithOAuthRetryintercepts 403 before the transport's_stepUpAuthorizesees it, so itshandle401now calls the exportedcomputeScopeUniondirectly.Re-opened from #2346, which was auto-closed by a transient stack-branch mis-push. Same branch, same content. Paul's review is on #2346.