feat(client,server-legacy): SEP-2468 server iss emission + finishAuth(URLSearchParams) overload#2357
Conversation
🦋 Changeset detectedLatest commit: 5efd11c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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/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-4-sep2468-server
branch
from
9bc98d3 to
36a0046
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/auth-4-sep2468-server
branch
from
36a0046 to
e7cfdd6
Compare
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
d62ce60 to
b69058a
Compare
| // All validation passed, proceed with authorization. RFC 9207: the metadata | ||
| // advertises `authorization_response_iss_parameter_supported`, so make that claim | ||
| // true from SDK code by appending `iss` to whatever redirect the provider issues | ||
| // back to the client's validated redirect_uri — the provider need not do anything. | ||
| // Redirects elsewhere (e.g. to an upstream authorize endpoint) are left untouched. | ||
| await provider.authorize( | ||
| client, | ||
| { | ||
| state, | ||
| scopes: requestedScopes, | ||
| redirectUri: redirect_uri!, | ||
| codeChallenge: code_challenge, | ||
| resource: resource ? new URL(resource) : undefined | ||
| resource: resource ? new URL(resource) : undefined, | ||
| issuer | ||
| }, | ||
| res | ||
| issuer ? withIssOnCallbackRedirect(res, redirect_uri!, issuer) : res | ||
| ); | ||
| } catch (error) { | ||
| // Post-redirect errors - redirect with error parameters | ||
| if (error instanceof OAuthError) { | ||
| res.redirect(302, createErrorRedirect(redirect_uri!, error, state)); | ||
| res.redirect(302, createErrorRedirect(redirect_uri!, error, state, issuer)); | ||
| } else { | ||
| const serverError = new ServerError('Internal Server Error'); | ||
| res.redirect(302, createErrorRedirect(redirect_uri!, serverError, state)); | ||
| res.redirect(302, createErrorRedirect(redirect_uri!, serverError, state, issuer)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 The bundled authorize handler appends iss to redirects whenever issuerUrl is configured, but never consults the provider's new authorizationResponseIssParameterSupported flag, while createOAuthMetadata gates the metadata advertisement on that same flag — so a provider that opts out (notably ProxyOAuthServerProvider, hard-coded to false) advertises authorization_response_iss_parameter_supported: false yet the handler's Phase-2 validation-error redirects still carry iss=<issuerUrl>, contradicting RFC 9207 §3. Gate emission on the same flag, e.g. const issuer = provider.authorizationResponseIssParameterSupported === false ? undefined : issuerUrl?.href (or have mcpAuthRouter omit issuerUrl when the provider opts out).
Extended reasoning...
What the bug is. The two halves of the RFC 9207 server-side feature added in this PR are driven by different switches. Metadata advertisement is provider-driven: createOAuthMetadata sets authorization_response_iss_parameter_supported: options.provider.authorizationResponseIssParameterSupported ?? true (router.ts). Emission is config-driven: authorizationHandler computes const issuer = issuerUrl?.href and appends iss to the success-redirect wrap (withIssOnCallbackRedirect, authorize.ts:186) and to both error redirects (createErrorRedirect(..., issuer), authorize.ts:191/194) whenever issuer is truthy — nothing in the handler reads provider.authorizationResponseIssParameterSupported. Meanwhile mcpAuthRouter unconditionally passes issuerUrl: options.issuerUrl into the handler (router.ts:149) regardless of the provider flag, so the two switches can disagree.\n\nThe code path that triggers it. Mount ProxyOAuthServerProvider (which this PR hard-codes authorizationResponseIssParameterSupported = false) via mcpAuthRouter — the canonical proxy setup. The published metadata says authorization_response_iss_parameter_supported: false. The proxy's success hop to the upstream authorize endpoint is correctly left untouched (the wrap only matches the client's redirect_uri), but every Phase-2 validation failure handled by the bundled handler itself — response_type=token, missing code_challenge, code_challenge_method=plain — goes through createErrorRedirect(redirect_uri, error, state, issuer) and is sent to the client's redirect_uri with iss=<proxy issuerUrl>.\n\nWhy existing code doesn't prevent it. The flag is only read in router.ts (metadata) and defined in provider.ts/proxyProvider.ts; the handler and the router's handler-wiring never consult it. The new tests only cover the default-true case (appends iss to error redirects) and the proxy success-hop case (leaves redirects to non-callback targets untouched); no test exercises a flag-false provider's error-redirect path, so the asymmetry slipped through. The proxyProvider JSDoc added in this PR reasons only about the upstream-issued success callback, not the locally-issued error redirects.\n\nStep-by-step proof.\n1. app.use(mcpAuthRouter({ provider: new ProxyOAuthServerProvider({...}), issuerUrl: new URL('https://proxy.example.com/') })).\n2. GET /.well-known/oauth-authorization-server → authorization_response_iss_parameter_supported: false (provider flag is false).\n3. GET /authorize?client_id=valid&redirect_uri=https://client.example.com/cb&response_type=token → Phase-2 validation throws InvalidRequestError; the catch calls createErrorRedirect('https://client.example.com/cb', error, state, 'https://proxy.example.com/').\n4. The client receives 302 Location: https://client.example.com/cb?error=invalid_request&error_description=...&iss=https%3A%2F%2Fproxy.example.com%2F — an iss-bearing authorization response from an AS whose metadata advertises false, violating RFC 9207 §3 ("Authorization servers that include the iss parameter MUST advertise this by setting authorization_response_iss_parameter_supported to true") — the very normative sentence the PR description quotes.\n\nWhat the impact would be. Functionally negligible for SDK clients: per the client-side decision table, a present iss is always compared regardless of the advertised flag, and the emitted value equals the proxy's recorded metadata issuer, so callbacks are still accepted; non-9207 clients ignore it. The cost is spec hygiene and consistency — the PR's own stated purpose for the flag is to keep emission and advertisement from disagreeing (no over-claiming), and the inverse disagreement (emit-but-advertise-false) is now reachable in the README's canonical proxy configuration. Hence nit severity, not blocking.\n\nHow to fix. Drive both halves from the same switch. Simplest: in authorizationHandler, compute const issuer = provider.authorizationResponseIssParameterSupported === false ? undefined : issuerUrl?.href; (one-line change in authorize.ts). Alternatively, have mcpAuthRouter omit issuerUrl when options.provider.authorizationResponseIssParameterSupported === false. Either way, a flag-false provider's error redirects then omit iss, matching the advertised metadata, and the default-true behavior is unchanged. A small test mirroring "appends iss to error redirects" but with a flag-false provider would lock this in.
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
b69058a to
1ac3c91
Compare
felixweinberger
force-pushed
the
fweinberger/auth-4-sep2468-server
branch
from
e7cfdd6 to
0bae633
Compare
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
1ac3c91 to
9626ed5
Compare
felixweinberger
force-pushed
the
fweinberger/auth-4-sep2468-server
branch
from
0bae633 to
4fe081b
Compare
| export function authorizationHandler({ provider, issuerUrl, rateLimit: rateLimitConfig }: AuthorizationHandlerOptions): RequestHandler { | ||
| const issuer = issuerUrl?.href; |
There was a problem hiding this comment.
🟡 The new issuerUrl option is serialized via URL.href (line 82), which normalizes the value — an origin-only issuer like new URL('https://auth.example.com') becomes 'https://auth.example.com/' with a trailing slash. Since RFC 9207 clients (including this SDK's validateAuthorizationResponseIssuer) compare iss against the published metadata issuer with strict string equality, a standalone authorizationHandler({provider, issuerUrl}) wiring whose separately-authored metadata uses the conventional no-trailing-slash issuer will have every callback (success and error) rejected with IssuerMismatchError. Consider accepting string | URL and emitting it verbatim, or documenting on AuthorizationHandlerOptions.issuerUrl / AuthorizationParams.issuer that the .href serialization must byte-for-byte equal the published issuer string (mcpAuthRouter users are unaffected since both sides derive from the same .href).
Extended reasoning...
What the bug is. authorizationHandler computes const issuer = issuerUrl?.href (packages/server-legacy/src/auth/handlers/authorize.ts:81-82) and emits that value as the RFC 9207 iss parameter on every redirect to the client's redirect_uri — both via withIssOnCallbackRedirect on the success path and via createErrorRedirect on the error path. URL.href is a normalized serialization: an origin-only issuer gains a trailing slash (new URL('https://auth.example.com').href === 'https://auth.example.com/'), the host is lower-cased, default ports are elided, and IDNs are punycoded. The option is typed as URL, so there is no way to emit a no-trailing-slash issuer at all.
Why the client rejects it. This SDK's own validateAuthorizationResponseIssuer (packages/client/src/client/auth.ts) compares the callback iss against the issuer recorded from the AS's metadata using simple string equality — its JSDoc explicitly states that trailing-slash/case/port normalization is not applied — and it runs whenever iss is present, regardless of whether authorization_response_iss_parameter_supported was advertised. So 'https://auth.example.com/' vs 'https://auth.example.com' is a hard IssuerMismatchError before the code is ever exchanged.
The configuration that triggers it. Inside mcpAuthRouter this can never bite: createOAuthMetadata publishes issuer: issuerUrl.href (router.ts), so the advertised issuer and the emitted iss are the same string by construction. But the issuerUrl option is also exposed on the standalone authorizationHandler — exactly the wiring where the operator authors and serves their own AS metadata (e.g. via mcpAuthMetadataRouter with a hand-written OAuthMetadata object, or an external metadata document). RFC 8414 issuer identifiers are conventionally written without a trailing slash, and the client's discovery-time issuer-echo check explicitly tolerates a no-slash issuer fetched from a slash-terminated URL, so such metadata passes discovery cleanly — and then every callback fails.
Step-by-step proof.
- Operator publishes AS metadata with
issuer: 'https://auth.example.com'(no trailing slash — the common form) and mountsapp.use('/authorize', authorizationHandler({ provider, issuerUrl: new URL('https://auth.example.com') })). - The SDK client discovers the metadata; the RFC 8414 §3.3 echo check passes (its trailing-slash tolerance is one-directional and accepts this), and it records expected issuer
'https://auth.example.com'. - The handler appends
iss=https%3A%2F%2Fauth.example.com%2F(trailing slash fromURL.href) to the success redirect. finishAuth/auth()runsvalidateAuthorizationResponseIssuer:'https://auth.example.com/' !== 'https://auth.example.com'→IssuerMismatchError. The code is never exchanged. The same happens on error redirects, so the operator also loses the AS'serror/error_description.
Why nothing catches it today. The new unit tests use ISSUER = 'https://auth.example.com/' (already in href form), and the e2e test exercises the mcpAuthRouter path where both sides derive from the same .href — so the divergence is never observable in CI. Neither AuthorizationHandlerOptions.issuerUrl nor AuthorizationParams.issuer JSDoc warns that the .href serialization must byte-for-byte equal the published issuer string.
How to fix. Either accept string | URL for issuerUrl and emit a string verbatim when given (preserving the operator's exact issuer identifier), or add a sentence to the issuerUrl / AuthorizationParams.issuer JSDoc (and the migration note) stating that the URL's .href must exactly equal the issuer value published in the AS metadata, since RFC 9207 clients compare with simple string equality. Severity is a nit: mcpAuthRouter users (the primary path) are consistent by construction, and the fix is a small typing or documentation change.
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
9626ed5 to
8a647f6
Compare
felixweinberger
force-pushed
the
fweinberger/auth-4-sep2468-server
branch
from
4fe081b to
bd5b903
Compare
…(URLSearchParams) overload Claude-Session: https://claude.ai/code/session_01XBib5gRe8AMPPJhySCz3EJ
felixweinberger
force-pushed
the
fweinberger/auth-4-sep2468-server
branch
from
bd5b903 to
5efd11c
Compare
| } catch (error) { | ||
| // Post-redirect errors - redirect with error parameters | ||
| if (error instanceof OAuthError) { | ||
| res.redirect(302, createErrorRedirect(redirect_uri!, error, state)); | ||
| res.redirect(302, createErrorRedirect(redirect_uri!, error, state, issuer)); | ||
| } else { | ||
| const serverError = new ServerError('Internal Server Error'); | ||
| res.redirect(302, createErrorRedirect(redirect_uri!, serverError, state)); | ||
| res.redirect(302, createErrorRedirect(redirect_uri!, serverError, state, issuer)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing issue: when Phase-2 validation fails (e.g. response_type=token, missing code_challenge, code_challenge_method=plain), the catch block calls createErrorRedirect with state still undefined — it is only assigned after RequestAuthorizationParamsSchema.safeParse succeeds — so the error redirect drops the client's state even though RFC 6749 §4.1.2.1 requires echoing it when the client sent one. Since this PR touches these exact catch-block lines (adding iss) and its migration text now tells hosts to compare state before finishAuth, consider leniently reading state from req.query/req.body before the strict Phase-2 parse so the catch can echo it.
Extended reasoning...
What the bug is. In authorizationHandler Phase 2, let state; is declared before the try block and only assigned via state = parseResult.data.state after RequestAuthorizationParamsSchema.safeParse succeeds. Zod's safeParse fails atomically: when the request has response_type !== 'code', a missing code_challenge, code_challenge_method !== 'S256', or a malformed resource, parseResult.success is false, the InvalidRequestError is thrown before the assignment, and state remains undefined. The catch block then calls createErrorRedirect(redirect_uri!, error, state /* undefined */, issuer), and createErrorRedirect only sets state when truthy — so the error redirect omits it even when the client included a valid state in the request.
Why this matters. RFC 6749 §4.1.2.1 makes state REQUIRED in the error response "if a 'state' parameter was present in the client authorization request." A client doing standard CSRF/state correlation will fail to match the error callback against its stored state and will discard or mis-handle it, so the user sees a generic failure instead of the AS's invalid_request explanation. Notably, this PR's own migration text now instructs hosts to compare state before calling finishAuth(params) — exactly the pattern that makes the omission user-visible: the host's state check fails before the SDK ever surfaces the error.
Why it is pre-existing rather than introduced. The catch block passed the same undefined state before this PR; the diff only adds the issuer argument to both createErrorRedirect calls. But the PR meaningfully interacts with this code: it modifies these exact lines, and its new tests exercise precisely this parse-failure error-redirect path — the unit test "appends iss to error redirects" sends a state-less request, and the e2e error leg of hosting:auth:as-iss-emission sends state=xyz but only asserts error and iss, so the missing state goes unnoticed. The error redirect now carries the new iss parameter while still dropping the spec-required state.
Step-by-step proof.
- Client sends
GET /authorize?client_id=valid-client&redirect_uri=https://example.com/callback&response_type=token&state=xyz789. - Phase 1 passes (client and redirect_uri are valid). Phase 2's
RequestAuthorizationParamsSchema.safeParsefails becauseresponse_typeis not'code'→InvalidRequestErroris thrown at theif (!parseResult.success)check, beforestate = parseResult.data.stateruns. - The catch calls
createErrorRedirect('https://example.com/callback', error, undefined, issuer). - The client receives
302 Location: https://example.com/callback?error=invalid_request&error_description=...&iss=<issuer>— nostate=xyz789. Its CSRF correlation fails and the AS's error explanation is lost.
Why existing code doesn't prevent it. state is only ever read from the strict Phase-2 schema result, which is all-or-nothing. Nothing reads it leniently for the error path, and no test asserts state on an error redirect (the existing "preserves state parameter in response" test covers the success path only).
How to fix. Before the strict Phase-2 parse, do a lenient read — e.g. const rawState = typeof source.state === 'string' ? source.state : undefined; from req.query/req.body — and use that (or the parsed value once available) in the catch's createErrorRedirect calls. This mirrors how redirect_uri is already resolved in Phase 1 before Phase 2 runs, so the error redirect can echo both state and the new iss. A small test mirroring "appends iss to error redirects" but sending state and asserting it appears in the redirect would lock the behavior in.
…(URLSearchParams) overload (#2357)
1 participant
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.
Re-opened from #2347, which was auto-closed by a transient stack-branch mis-push. Same branch, same content. Paul's review is on #2347.