Uh oh!
There was an error while loading. Please reload this page.
feat(client,server-legacy): SEP-2468 server iss emission + finishAuth(URLSearchParams) overload - #2347
Conversation
🦋 Changeset detectedLatest commit: 46b6b01 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/nodecommit: |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| * present. Validated per RFC 9207 against the recorded issuer before the code is redeemed. | ||
| */ | ||
| async finishAuth(authorizationCode: string, iss?: string): Promise<void> { | ||
| async finishAuth(authorizationCode: string, iss?: string): Promise<void>; |
There was a problem hiding this comment.
wondering if we want the "base" one to be an object, and then also allow params.
1416e4a to
e06add7Compareb1ecab4 to
0097867CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
e06add7 to
2bffe4cCompare0097867 to
646cea6CompareUh oh!
There was an error while loading. Please reload this page.
2bffe4c to
f9285f6Compare646cea6 to
1196cf1CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
f9285f6 to
8b2f271Compare1196cf1 to
6b3f82bCompare8b2f271 to
a2c6968Compare6b3f82b to
ef079b3CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…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
a2c6968 to
52b1cfcCompareef079b3 to
46b6b01Compare46b6b01
into
fweinberger/auth-1-sep2468-clientUh oh!
There was an error while loading. Please reload this page.
| if (stepUpRetries >= this._maxStepUpRetries) { | ||
| throw new SdkHttpError( | ||
| SdkErrorCode.ClientHttpForbidden, | ||
| `Server returned 403 insufficient_scope after step-up re-authorization (retry limit ${this._maxStepUpRetries} reached)`, | ||
| { status: 403, statusText: challenge.statusText ?? 'Forbidden', text: challenge.text } | ||
| ); |
There was a problem hiding this comment.
🟡 Setting maxStepUpRetries: 0 (accepted by the Math.max(0, ...) clamp) makes the very first 403 insufficient_scope throw an SdkHttpError whose message claims "after step-up re-authorization (retry limit 0 reached)" even though no re-authorization was attempted, and a host using 0 to disable automatic step-up gets that opaque error instead of the typed InsufficientScopeError (with requiredScope/resourceMetadataUrl/errorDescription) that the 'throw' mode and no-provider paths produce. Consider treating a cap of 0 as equivalent to onInsufficientScope: 'throw', rewording the message, or documenting/disallowing 0.
Extended reasoning...
What happens. The constructor clamps the option with Math.max(0, opts?.maxStepUpRetries ?? DEFAULT_MAX_STEP_UP_RETRIES), so 0 is an accepted (and, given that negatives are explicitly clamped to 0, apparently sanctioned) configuration. In _stepUpAuthorize, the retry-cap check if (stepUpRetries >= this._maxStepUpRetries) runs before any auth() call. Both call sites — _send and _startOrAuthSse — invoke _stepUpAuthorize with stepUpRetries = 0 on the first 403, so with maxStepUpRetries: 0 the comparison 0 >= 0 fires immediately and the transport throws SdkHttpError(ClientHttpForbidden) with the message "Server returned 403 insufficient_scope after step-up re-authorization (retry limit 0 reached)".\n\nWhy the message is wrong. In that configuration no re-authorization was ever attempted — the error text describes a step-up that never happened. The JSDoc on maxStepUpRetries neither documents nor prohibits 0, and no test covers it (the new tests exercise the default cap of 1 and maxStepUpRetries: 2 only), so this isn't an intentional contract.\n\nWhy the error type matters.0 is the natural value a host would reach for to mean "don't step up automatically; let me drive it." But the two adjacent paths that serve exactly that intent — onInsufficientScope: 'throw' and the no-OAuthClientProvider case — both throw the typed InsufficientScopeError carrying the challenge parameters (requiredScope, resourceMetadataUrl, errorDescription). The cap-0 path instead surfaces only an SdkHttpError with status/text, so the host cannot drive step-up itself from the error contents and gets a misleading explanation of why the request failed.\n\nStep-by-step.\n1. Host constructs new StreamableHTTPClientTransport(url, { authProvider: oauthProvider, maxStepUpRetries: 0 }) intending to disable automatic step-up.\n2. The server answers a POST with 403 + WWW-Authenticate: Bearer error=\"insufficient_scope\", scope=\"files:write\".\n3. _send extracts the challenge and calls _stepUpAuthorize(challenge, 0).\n4. onInsufficientScope is the default 'reauthorize' and an OAuth provider exists, so the first two guards fall through; the cap check 0 >= 0 is true.\n5. The transport throws SdkHttpError(ClientHttpForbidden, "Server returned 403 insufficient_scope after step-up re-authorization (retry limit 0 reached)", ...) — no auth() call ever ran, and the challenge's scope/resource_metadata/error_description are not on the error.\n\nWhy nothing else prevents it. The clamp accepts 0, the JSDoc is silent on it, and the cap check sits ahead of every other branch in _stepUpAuthorize, so there is no path by which a 0-cap configuration reaches either the typed error or an accurate message.\n\nImpact and mitigations. This is a polish issue, not a correctness blocker: the documented way to disable automatic step-up is onInsufficientScope: 'throw', 0 is an unusual deliberate configuration, the request still fails loudly, and "(retry limit 0 reached)" lets a careful reader infer the cause. No incorrect auth occurs.\n\nHow to fix (any of these is small).\n- Treat maxStepUpRetries: 0 as equivalent to 'throw': when stepUpRetries === 0 and the cap is 0, throw InsufficientScopeError with the challenge params instead of the SdkHttpError.\n- Or reword the cap message so it doesn't claim a re-authorization happened when stepUpRetries === 0.\n- Or document on maxStepUpRetries that 0 is disallowed (and clamp to 1) or that hosts wanting to disable step-up should use onInsufficientScope: 'throw'.
| --- | ||
| '@modelcontextprotocol/client': minor | ||
| --- | ||
| SEP-2350 scope step-up: on `403 insufficient_scope`, `StreamableHTTPClientTransport` now re-authorizes with the **union** of the previously-requested and challenged scopes (`computeScopeUnion`), bypassing the refresh-token branch when the union is a strict superset of the current token's granted scope (`isStrictScopeSuperset`, `AuthOptions.forceReauthorization`). New `onInsufficientScope: 'reauthorize' | 'throw'` (default `'reauthorize'`) and `maxStepUpRetries` (default 1) on `StreamableHTTPClientTransportOptions`; `'throw'` raises the new `InsufficientScopeError`. The GET listen-stream open path now applies the same step-up handling. The previous verbatim-header retry guard is replaced by the bounded per-send counter. |
There was a problem hiding this comment.
🟡 The changeset presents SEP-2350 as purely additive, but this PR also unconditionally changes what a 403 insufficient_scope throws for transports configured without an OAuthClientProvider (minimal AuthProvider, requestInit-only headers, or no authProvider): previously SdkHttpError(ClientHttpNotImplemented), now the new InsufficientScopeError, which does not extend SdkError, so existing instanceof SdkError/SdkHttpError handlers stop matching. docs/migration.md documents this and the PR checks the breaking-change box — consider adding one sentence to the changeset (the published CHANGELOG text) so the default-mode error-class change is not a silent surprise on upgrade.
Extended reasoning...
What changed. Pre-PR, the 403 branch in _send was gated on response.status === 403 && this._oauthProvider. A transport configured with a minimal AuthProvider, requestInit-only headers, or no authProvider at all therefore fell through to the generic SdkHttpError(SdkErrorCode.ClientHttpNotImplemented, 'Error POSTing to endpoint: …') on a 403 insufficient_scope. Post-PR the gate is just response.status === 403, and _stepUpAuthorize (packages/client/src/client/streamableHttp.ts, ~line 380) throws the new InsufficientScopeError whenever this._oauthProvider is absent — even in the default'reauthorize' mode, with no opt-in by the consumer. The GET listen-stream open path gains the same behavior (it previously threw SdkHttpError(ClientHttpFailedToOpenStream)).\n\nWhy this is an error-class change consumers can hit.InsufficientScopeError extends OAuthClientFlowError → Error, not SdkError/SdkHttpError. A host using one of the affected configurations whose error handling does if (error instanceof SdkHttpError) (the pattern the migration guide itself recommends) will stop matching this case after upgrading, with no code change on their side.\n\nThe documentation gap.docs/migration.md covers this precisely ("If you pass a non-OAuth authProvider (or only requestInit headers), a 403 insufficient_scope now throws InsufficientScopeError instead of the previous generic SdkHttpError(ClientHttpNotImplemented) … existing instanceof SdkError catches no longer match this case"), docs/migration-SKILL.md has the matching throw-site row, and the PR description checks the Breaking-change box. But .changeset/sep-2350-scope-step-up.md — the text that lands in the published CHANGELOG that upgraders actually read — describes the feature as purely additive and only mentions InsufficientScopeError in connection with the opt-in 'throw' mode ("'throw' raises the new InsufficientScopeError"), which under-describes the implementation.\n\nStep-by-step example.\n1. A host today uses new StreamableHTTPClientTransport(url, { requestInit: { headers: { Authorization: 'Bearer …' } } }) (no OAuthClientProvider) and wraps sends in catch (e) { if (e instanceof SdkHttpError) … }.\n2. The RS responds 403 with WWW-Authenticate: Bearer error=\"insufficient_scope\", scope=\"files:write\".\n3. Pre-PR: the 403 branch is skipped (no _oauthProvider), the request falls through to SdkHttpError(ClientHttpNotImplemented) and the handler matches.\n4. Post-PR: _stepUpAuthorize runs (the gate no longer requires _oauthProvider), hits the if (!this._oauthProvider) branch, and throws InsufficientScopeError. The instanceof SdkHttpError handler no longer matches; the host's fallback path runs instead. Reading the changeset alone gives no hint of this — it implies InsufficientScopeError only appears when onInsufficientScope: 'throw' is set.\n\nOn the counter-argument that this is editorial. It is true that the repo's required upgrade documentation (migration.md / migration-SKILL.md) covers the change, that the changeset makes no false claim, and that the new typed error is strictly more informative — which is why this is filed as a nit, not a blocker. But the changeset is the only one of those documents that ships in the package CHANGELOG that consumers see at upgrade time, and it currently scopes InsufficientScopeError to the opt-in 'throw' mode while the default mode also throws it for a whole class of transport configurations. That is a meaningful omission for the audience the changeset serves, and the fix is a single sentence.\n\nSuggested fix. Append one sentence to .changeset/sep-2350-scope-step-up.md, e.g.: "For transports without an OAuthClientProvider (minimal AuthProvider, requestInit-only headers, or no authProvider), a 403 insufficient_scope now throws InsufficientScopeError (not SdkError) instead of the previous generic SdkHttpError(ClientHttpNotImplemented) — see the migration guide's Scope step-up section."
27b2cbe00mcpAuthRouter/createOAuthMetadatain@modelcontextprotocol/server-legacyemitisson authorization and error redirects and advertiseauthorization_response_iss_parameter_supported: true.finishAuthonStreamableHTTPClientTransportandSSEClientTransportgains a(callbackParams: URLSearchParams)overload that extractscode+iss, validatesissfirst, and on mismatch throws a sanitizedIssuerMismatchErrorwithout surfacingerror/error_description/error_uri.Motivation and Context
SEP-2468 (modelcontextprotocol/modelcontextprotocol#2468, spec wording in #2646).
Normative sentences implemented:
The
URLSearchParamsoverload exists so hosts can hand the SDK the whole callback query and have the SDK enforce the MUST-NOT-display rule structurally.How Has This Been Tested?
hosting:auth:iss-emission,hosting:auth:iss-metadata-advertised,client-auth:finishAuth:{urlsearchparams, sanitized-error}.authorize.tsredirect param assertions.Breaking Changes
Behavior change for
@modelcontextprotocol/server-legacyconsumers with a customOAuthServerProvider.authorize(): the bundled metadata now advertisesauthorization_response_iss_parameter_supported: trueby default. The SDK appendsissto authorization redirects via theres.redirect()it passes you — if yourauthorize()redirects another way (e.g., writesLocationdirectly), appendparams.issueryourself or setauthorizationResponseIssParameterSupported: falseinmcpAuthRouteroptions. Otherwise SEP-2468 clients will reject callbacks (advertised-but-missingiss→ row-2 reject).The
finishAuth(URLSearchParams)overload is additive —finishAuth(code: string, iss?: string)still compiles.Types of changes
Checklist
Additional context
Split from PR 2 to keep the client-side MUSTs (which are the breaking-change surface) reviewable independently of the server-legacy SHOULD.