feat(client,core): RFC 9207 iss parameter validation on authorization responses (SEP-2468)#2272
feat(client,core): RFC 9207 iss parameter validation on authorization responses (SEP-2468)#2272mattzcarey wants to merge 11 commits into
Conversation
🦋 Changeset detectedLatest commit: 7b3b4c7 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/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
mattzcarey
force-pushed
the
feat/sep-2468-iss-validation
branch
from
848e052 to
a625f46
Compare
|
Rebased on current I checked the implementation against the current upstream spec text for Authorization Response Validation: present Verification:
Conformance note: full client conformance is currently blocked on sibling PR #2266 / SEP-837. A verbose |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Resolved the latest review threads in ede900a. Changes:
Verification:
|
There was a problem hiding this comment.
Thanks — the docs/client.md prose, both runnable examples, and the SSE finishAuth test from my earlier comments are now in the diff, so I have nothing further to flag. Leaving final sign-off to a maintainer since this adds new public API (validateAuthorizationResponseIssuer, the iss option on auth()/finishAuth()) and changes security-relevant OAuth client behavior.
Extended reasoning...
Overview
This PR implements RFC 9207 (SEP-2468) iss-parameter validation for OAuth authorization responses. It adds authorization_response_iss_parameter_supported to the core metadata schemas, a new exported validateAuthorizationResponseIssuer() helper, an optional tri-state iss option on auth(), and an options?: { iss } second argument on both StreamableHTTPClientTransport.finishAuth and SSEClientTransport.finishAuth. Docs, both OAuth examples, and a changeset are updated, and there is substantial new test coverage in auth.test.ts, streamableHttp.test.ts, and sse.test.ts.
Status of prior feedback
My earlier review comments (missing prose docs/example updates, and missing SSE-side finishAuth iss test) are addressed in the current revision: docs/client.md now describes extracting and forwarding iss (including the null vs undefined semantics), simpleOAuthClient.ts and elicitationUrlExample.ts pass { iss } through, and sse.test.ts gained a mismatched-iss rejection test. The validation logic itself matches the RFC 9207 decision table, runs before the code reaches any token endpoint, and uses exact string comparison with no normalization; the mismatch error message does not echo attacker-controlled error_description content, and a dedicated test asserts that.
Security risks
This is security-positive code (mix-up attack mitigation), but it sits squarely in the OAuth client flow. The main design judgment calls a maintainer should own are: (1) the tri-state iss semantics where undefined silently skips validation — sound for backwards compatibility, but it means existing callers get no protection until they opt in; (2) the metadata-provenance caveat the author documents, where without cached discoveryState the comparison anchor is freshly re-discovered metadata rather than the pre-redirect issuer, which weakens the guarantee; (3) new public API surface that is hard to remove later.
Level of scrutiny
High — auth code, new exports across two packages, and a minor version bump for the client. The implementation looks correct and well-tested, but per the repo's own conventions (burden of proof on addition, security-sensitive paths) this should get a human maintainer's review rather than a shadow approval.
Other factors
The bug-hunting system found no bugs in this revision, the changeset text matches the implemented behavior, and the conformance/land-order caveats are clearly documented in the PR description. The cloudflare/agents downstream-compatibility analysis in the description is consistent with the undefined-skips-validation path.
There was a problem hiding this comment.
Thanks — the docs/examples and SSE test gaps from my earlier review are addressed in ede900a, and I have no further findings. Since this adds new public auth API surface (the tri-state iss option, a new export, and finishAuth signature changes) on a security-sensitive path, it should still get a human maintainer's review of the API design rather than an automated approval.
Extended reasoning...
Overview
This PR implements RFC 9207 (SEP-2468) iss parameter validation: a new validateAuthorizationResponseIssuer() export, an optional tri-state iss option on auth(), and an options?: { iss } second parameter on both StreamableHTTPClientTransport.finishAuth and SSEClientTransport.finishAuth. Core's OAuthMetadataSchema/OpenIdProviderMetadataSchema gain authorization_response_iss_parameter_supported. Docs (docs/client.md), both OAuth examples, a changeset, and substantial test coverage (auth.test.ts decision table, StreamableHTTP and SSE transport plumbing tests) are included.
Security risks
This is security-positive in intent (mix-up attack mitigation), but it sits squarely on the OAuth authorization-code exchange path. The validation logic itself looks correct against RFC 9207 Section 2.4 (exact string compare, fail-closed on advertised-but-missing only when the caller asserts null, rejection before any token-endpoint call, no surfacing of forged error params), and the undefined skip path preserves existing behavior for callers that never see the callback URL. The main residual risk is design-level rather than a bug: the protection is opt-in, and the metadata anchor is the freshly re-discovered issuer when no cached discovery state exists, which the PR description itself acknowledges as weaker than the pre-redirect anchor.
Level of scrutiny
High. This touches client auth code, adds three new public API surfaces (export, auth() option, transport method signature), and encodes an API-design decision (tri-state string | null | undefined) that a maintainer should explicitly endorse per the repo's minimalism / burden-of-proof principles. Per my approval criteria, security-sensitive auth changes and new public API design decisions should not be auto-approved regardless of implementation quality.
Other factors
The bug-hunting pass found no defects in the latest revision. My previous review's items (missing prose docs, examples not forwarding iss, missing SSE-side test) were all addressed in ede900a and the threads are resolved. Test coverage is thorough, including the negative paths and the no-normalization requirement. The changeset accurately describes the shipped behavior. Conformance baseline caveats are documented and depend on sibling PRs, which is a sequencing question for maintainers.
mattzcarey
force-pushed
the
feat/sep-2468-iss-validation
branch
from
37f812b to
079b624
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
packages/client/src/client/auth.ts:736-739— Discovery state persisted by an older SDK release via the legacy no-PRM fallback (whereauthorizationServerUrl= MCP origin but the metadata'sissuer= the real IdP) now fails the newvalidateAuthorizationServerMetadataIssuer()check on the cached-state path ofauthInternal()with a plainError, which theauth()recovery wrapper never handles — so everyauth()call (including silent token refresh) fails permanently until the application manually callsinvalidateCredentials('discovery'). Consider invalidating/ignoring the cached discovery state on this mismatch and falling through to full re-discovery (which would now succeed thanks to this PR's ownvalidateIssuer: false+ issuer-swap handling for the fallback path), or at minimum throwing a typed, recoverable error.Extended reasoning...
What the bug is
The cached-discovery-state branch of
authInternal()(packages/client/src/client/auth.ts:736-739) now unconditionally callsvalidateAuthorizationServerMetadataIssuer(metadata, cachedState.authorizationServerUrl). That helper throws a plainErrorwhen the cachedauthorizationServerUrland the metadata'sissuerdiffer (modulo trailing slash). The problem is that there is a legitimate, previously-valid configuration that produces exactly this mismatch in already-persisted state: discovery state written by an earlier SDK release via the legacy no-PRM fallback, whereauthorizationServerUrlwas set to the MCP server origin whileauthorizationServerMetadata.issuerwas the real IdP. The old SDK never validated or swapped these, so providers that implementsaveDiscoveryState()/discoveryState()(cross-session persistence is the documented purpose ofOAuthDiscoveryState) can have this state on disk today.Why this PR makes it inconsistent
The PR itself acknowledges this configuration is legitimate on the fresh-discovery path: commit df9db5b makes
discoverOAuthServerInfo()skip issuer validation when the AS URL came from the legacy fallback (validateIssuer: authorizationServerUrlFromResourceMetadata) and swapsauthorizationServerUrltometadata.issuerwhen they differ, with the test 'uses legacy fallback metadata issuer when it differs from the MCP origin' covering it. So a brand-new client against the same server works fine — but a client upgrading with state persisted by the previous release hits the cached-state check and fails.Why there is no recovery path
The thrown value is a plain
Error, not anOAuthError. The recovery wrapper inauth()only catchesOAuthErrorwith codesInvalidClient/UnauthorizedClient/InvalidGrant, in which case it callsprovider.invalidateCredentials()and retries. For this error nothing is invalidated and there is no fall-through to fresh discovery. Since the cached state is consulted on everyauthInternal()call — 401-triggered re-auth, silent token refresh,finishAuth()— the failure repeats forever. The error message ('Authorization server metadata issuer does not match the expected issuer ... RFC 8414 Section 3.3') gives no hint that the remedy is for the host application to callinvalidateCredentials('discovery').Step-by-step proof
- Under the previous SDK release, a provider connects to
https://mcp.example.comwhich has no protected-resource metadata. The legacy fallback setsauthorizationServerUrl = 'https://mcp.example.com/';/.well-known/oauth-authorization-serverthere returns metadata withissuer: 'https://idp.example.com'(a proxied IdP).saveDiscoveryState()persists{ authorizationServerUrl: 'https://mcp.example.com/', authorizationServerMetadata: { issuer: 'https://idp.example.com', ... } }. Auth completes; tokens with a refresh token are saved. This was fully supported behavior. - The application upgrades to the SDK containing this PR.
- The access token expires; the transport gets a 401 and calls
auth().provider.discoveryState()returns the persisted state, soauthInternal()takes the cached branch and callsvalidateAuthorizationServerMetadataIssuer({ issuer: 'https://idp.example.com', ... }, 'https://mcp.example.com/')→'https://idp.example.com' !== 'https://mcp.example.com'→ throwsError. auth()'s catch sees a non-OAuthErrorand rethrows. No credentials are invalidated, no re-discovery happens. Every subsequentauth()call replays steps 3-4. The connection is permanently broken even though the server configuration is one this PR explicitly continues to support, and even though dropping the cache and re-runningdiscoverOAuthServerInfo()would succeed and persist corrected state (authorizationServerUrl = 'https://idp.example.com').
Why existing handling doesn't cover it
The new test 'rejects cached AS metadata with a mismatched issuer' shows the strict check is intentional for adversarial cached metadata, but it doesn't distinguish the stale-but-previously-valid legacy state, and there is no corresponding self-healing path. The earlier review comment on the changeset asked for documentation/gating of the unconditional RFC 8414 check; the fresh-discovery part of that concern was addressed by df9db5b, but this cached-state recovery gap remains.
How to fix
In the cached-state branch, when
validateAuthorizationServerMetadataIssuer()would fail, treat the cached discovery state as stale: callprovider.invalidateCredentials?.('discovery')(or simply ignore the cached state) and fall through to the fulldiscoverOAuthServerInfo()path, which under this PR's own swap logic will resolve the correct issuer and re-persist consistent state. Alternatively (weaker), throw a typed/OAuthError-style error and document the migration step so applications can recover programmatically. - Under the previous SDK release, a provider connects to
… responses (SEP-2468)
- Add authorization_response_iss_parameter_supported to OAuthMetadataSchema
and OpenIdProviderMetadataSchema (RFC 8414 / RFC 9207)
- New exported validateAuthorizationResponseIssuer() implementing the
RFC 9207 Section 2.4 decision table with exact string comparison
- auth() accepts optional iss, validated against the recorded AS metadata
before the authorization code is sent to any token endpoint
- finishAuth(code, { iss }) optional second argument on both
StreamableHTTPClientTransport and SSEClientTransport
- Tests covering all four decision-table rows and the
error-response-mismatch case
Closes #2197
…caller signal The advertised-but-missing rejection (AS metadata sets authorization_response_iss_parameter_supported: true but no iss was supplied) previously fired whenever iss was omitted from auth()/ finishAuth(). The SDK never sees the authorization response itself, so it cannot distinguish 'the response had no iss' from 'the caller did not plumb response parameters through' — and every existing finishAuth(code) caller falls in the second bucket. This broke the client-conformance auth/pre-registration scenario (the fixture AS advertises RFC 9207 support; the harness never passes iss). iss is now tri-state on validateAuthorizationResponseIssuer(), auth(), and both transports' finishAuth(): - string: exact-match validation against the recorded issuer (unchanged) - null: caller asserts it inspected the response and it had no iss -> RFC 9207 fail-closed rejection applies when support is advertised - undefined: caller had no access to response parameters -> validation is skipped entirely Conformance: client suite back to baseline-green (auth/pre-registration 15/15). Client tests: 386 passed.
mattzcarey
force-pushed
the
feat/sep-2468-iss-validation
branch
from
c09f665 to
68d8bb1
Compare
| let useCachedDiscoveryState = false; | ||
| if (cachedState?.authorizationServerUrl) { | ||
| useCachedDiscoveryState = true; | ||
| // Restore discovery state from cache | ||
| authorizationServerUrl = cachedState.authorizationServerUrl; | ||
| resourceMetadata = cachedState.resourceMetadata; | ||
| metadata = | ||
| cachedState.authorizationServerMetadata ?? (await discoverAuthorizationServerMetadata(authorizationServerUrl, { fetchFn })); | ||
| cachedState.authorizationServerMetadata ?? | ||
| (await discoverAuthorizationServerMetadataInternal(authorizationServerUrl, { fetchFn, validateIssuer: false })); | ||
| try { | ||
| validateAuthorizationServerMetadataIssuer(metadata, authorizationServerUrl); | ||
| } catch (error) { | ||
| if (!isStaleLegacyFallbackDiscoveryState(cachedState, metadata, serverUrl)) { | ||
| throw error; | ||
| } | ||
|
|
||
| await provider.invalidateCredentials?.('discovery'); | ||
| useCachedDiscoveryState = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 The cached-discovery-state issuer check rejects the very state the legacy no-PRM carve-out persists when the origin-hosted metadata has an unparseable issuer (e.g. auth.example.com): the first auth() call tolerates it and saveDiscoveryState() records it (as the PR's own test asserts), but the subsequent finishAuth(code) call hits validateAuthorizationServerMetadataIssuer() in the cached-state branch, and isStaleLegacyFallbackDiscoveryState() cannot forgive the failure because it normalizes metadata.issuer inside its own try block — so the redirect→finishAuth flow the SDK just started throws a plain Error on every retry until the host manually clears persisted discovery state. Consider tolerating an unparseable cached metadata issuer in isStaleLegacyFallbackDiscoveryState() (or the cached-state catch) when the cached state matches the legacy-fallback shape, mirroring the fresh-discovery carve-out.
Extended reasoning...
What the bug is
The legacy no-PRM carve-out and the cached-discovery-state validation are mutually inconsistent for unparseable issuers, and the inconsistency is self-inflicted within a single OAuth flow. The full-discovery branch of authInternal() deliberately tolerates an AS metadata issuer that is not a parseable URL (the legacyIssuerIsMalformed carve-out), and discoverOAuthServerInfo()'s rewrite try/catch keeps the MCP-origin fallback URL in that case. saveDiscoveryState() then persists { authorizationServerUrl: '<MCP origin>', authorizationServerMetadata: { issuer: 'auth.example.com', ... } } — the PR's own test 'keeps the fallback MCP origin when legacy no-PRM auth metadata has an invalid issuer' asserts exactly this persisted shape. But the cached-state branch (packages/client/src/client/auth.ts:757-776) unconditionally re-runs validateAuthorizationServerMetadataIssuer(metadata, authorizationServerUrl) on the next auth() call, and normalizeDiscoveredIssuerIdentifier('auth.example.com') throws. The escape hatch, isStaleLegacyFallbackDiscoveryState() (lines ~625-643), normalizes metadata.issuer inside its own try block, so the same parse failure makes it return false and the error is rethrown.
The code path that triggers it
- A provider implements
discoveryState()/saveDiscoveryState()(the SDK-recommended pattern for browser-redirect flows). The MCP server has no protected-resource metadata, and its origin-hosted AS metadata carries an unparseableissuer(schemeless host likeauth.example.com, or an empty string). - First
auth()call: PRM discovery 404s, the legacy fallback keeps the MCP origin, the carve-out swallows the issuer-validation error, discovery state is persisted with the unparseable issuer, andauth()returns'REDIRECT'. - The user authorizes; the host calls
transport.finishAuth(code)→auth()again.cachedState.authorizationServerUrlis set, so the cached-state branch callsvalidateAuthorizationServerMetadataIssuer(), which throws on the unparseable issuer. - The catch only forgives via
isStaleLegacyFallbackDiscoveryState(), which itself fails to normalize the same issuer and returnsfalse→ the error is rethrown,invalidateCredentials('discovery')at the next line is never reached, and there is no fall-through to re-discovery.
Why existing code doesn't prevent it
The throw is a plain Error, not an OAuthError, so auth()'s recovery wrapper does not invalidate anything or retry. Every subsequent auth() call (the code exchange of the flow the SDK just started, plus all later token refreshes) repeats the same failure until the host manually clears the persisted discovery state. The earlier resolved review threads do not cover this gap: the cached-state escape hatch was added for old-SDK state with a parseable mismatched issuer, and the unparseable-issuer carve-outs were applied only to the fresh-discovery paths (discoverOAuthServerInfo() and the full-discovery branch). The remaining hole is precisely that the cached-state branch rejects the unparseable-issuer state the new code itself writes one call earlier. The PR's malformed-issuer test only wires saveDiscoveryState (the write side), never discoveryState (the read side), so no test catches it.
Step-by-step proof
- Server:
https://resource.example.com, no PRM;/.well-known/oauth-authorization-serverreturns{ issuer: 'auth.example.com', authorization_endpoint: 'https://auth.example.com/oauth/authorize', token_endpoint: 'https://auth.example.com/oauth/token', ... }. auth(provider, { serverUrl }):discoverOAuthServerInfo()keepsauthorizationServerUrl = 'https://resource.example.com/'; the full-discovery carve-out setslegacyIssuerIsMalformed = trueand proceeds;saveDiscoveryState({ authorizationServerUrl: 'https://resource.example.com/', authorizationServerMetadata: { issuer: 'auth.example.com', ... } }); result'REDIRECT'(this exact shape is asserted by the existing test).transport.finishAuth(code)→auth():provider.discoveryState()returns the persisted pair; the cached-state branch callsvalidateAuthorizationServerMetadataIssuer({ issuer: 'auth.example.com' }, 'https://resource.example.com/');new URL('auth.example.com')throws →'Authorization server metadata issuer is not a valid issuer identifier ... (RFC 8414 Section 3.3)'.isStaleLegacyFallbackDiscoveryState()hits the same parse failure in its try block and returnsfalse→ the error propagates;invalidateCredentials('discovery')is never called;auth()'s catch sees a non-OAuthErrorand rethrows. The flow is bricked.
Impact
A legacy deployment whose configuration the changeset and migration prose explicitly promise keeps working (no PRM, origin-hosted metadata) breaks mid-flow when the issuer happens to be unparseable and the host uses persisted discovery state — and it cannot self-heal, because re-discovery is never reached. Severity is limited because the trigger requires a non-RFC-8414-conformant issuer (the spec mandates an https URL) plus a discoveryState-implementing provider, so this should not block the PR; but it defeats the carve-out the PR deliberately ships and tests, so it is worth a small fix.
How to fix
Tolerate an unparseable cached metadata.issuer when the cached state matches the legacy-fallback shape (cached URL == MCP origin, no PRM authorization_servers) — either in isStaleLegacyFallbackDiscoveryState() (e.g. treat a normalize failure of metadata.issuer as "keep using the cached state" rather than "reject") or directly in the cached-state catch, mirroring the legacyIssuerIsMalformed carve-out used by the full-discovery branch. A test driving the full 'REDIRECT' → finishAuth(code) sequence with a discoveryState-persisting provider and an unparseable issuer would lock the behavior in.
1 participant
What
Implements RFC 9207 (OAuth 2.0 Authorization Server Issuer Identification)
issparameter validation for authorization responses, per the draft MCP spec (basic/authorization, "Authorization Response Validation").Before redirecting the user, the client records the
issuerfrom validated authorization server metadata alongside the PKCE verifier/state. Before sending an authorization code to any token endpoint, callers that provide authorization-response parameters can apply RFC 9207 Section 2.4 validation.Decision table implemented by
validateAuthorizationResponseIssuer():authorization_response_iss_parameter_supported: trueississ: null)error/error_descriptionfrom that response eitherOne extra fail-closed row: if an
issis received but no AS metadata was ever recorded, we reject because there is nothing trustworthy to compare against.The
issoption is tri-state (string | null | undefined)The SDK never sees the authorization response itself; the caller does. So the strict advertised-but-missing rejection is gated behind an explicit caller signal:
string- theissfrom the authorization response; validated by exact comparison.null- the caller asserts it inspected the authorization response and it contained noiss. This enables the RFC 9207 fail-closed rejection when the AS advertisesauthorization_response_iss_parameter_supported: true.undefined(omitted) - the caller did not have access to the response parameters, matching existingfinishAuth(code)callers. Validation is skipped because the SDK cannot distinguish "the response had no iss" from "the caller did not plumb it."This keeps existing callers compiling and behaving identically while giving hosts that have the callback URL enough API surface to enforce RFC 9207.
API additions
@modelcontextprotocol/core:OAuthMetadataSchemaandOpenIdProviderMetadataSchemarecognizeauthorization_response_iss_parameter_supported.@modelcontextprotocol/client:validateAuthorizationResponseIssuer(metadata, iss).auth(provider, { ..., iss? })validatesissbeforefetchTokenwhenauthorizationCodeis present andissis provided (stringornull).StreamableHTTPClientTransport.finishAuth(authorizationCode, options?: { iss?: string | null })andSSEClientTransport.finishAuth(...)plumb the optional value through toauth().Metadata provenance
Validation strength depends on where the recorded issuer comes from. With a provider that implements
discoveryState/saveDiscoveryState, the recorded issuer is the one from the provider's validated AS metadata captured before the redirect, which is the intended RFC 9207 anchor. Without cached discovery state,authInternalre-discovers metadata at code-exchange time, so the comparison anchor is the freshly-discovered issuer rather than the one recorded pre-redirect. That still ensures theissmust match the AS the client is about to use, but recording viadiscoveryStateis stronger.Land order / conformance note
This branch is rebased on
origin/mainand is not stacked. Because current conformance includes SEP-837 checks, the full client conformance baseline on this branch still depends on #2266 landing first; the observed local failure isDCR application_type specified, not anissregression. Recommended order: #2265 -> #2266 -> #2271 -> this PR.The
auth/iss-*conformance scenarios remain in the expected-failures baseline because the conformanceeverythingClientdoes not yet plumb the redirect'sissthroughfinishAuth.Validation
After rebasing on current
origin/main:pnpm --filter @modelcontextprotocol/client test -- auth.test.ts streamableHttp.test.ts sse.test.ts tokenProvider.test.ts- 388 passedpnpm --filter @modelcontextprotocol/core test- 477 passedpnpm run typecheck:all- cleanpnpm run lint:all- cleanpnpm run build:all- clean, with the existing AJV/fast-uri missing-export warningspnpm --filter @modelcontextprotocol/test-conformance exec conformance client --command 'node --import tsx ./src/everythingClient.ts' --scenario auth/metadata-default --expected-failures ./expected-failures.yaml --verbose- fails only on SEP-837application_typeomission, fixed by feat(client,core): application_type client metadata with native/web inference (SEP-837) #2266Downstream impact
cloudflare/agents calls
transport.finishAuth(code)inpackages/agents/src/mcp/client-connection.ts, so it keeps compiling and behaving identically through theundefinedpath. Agents will not get RFC 9207 protection until it extractsissfrom the OAuth callback URL inpackages/agents/src/mcp/client.tsand forwards it throughcompleteAuthorization()/finishAuth().Closes #2197