feat(client): SEP-2352 per-authorization-server credential isolation#2348
Conversation
🦋 Changeset detectedLatest commit: 9bc98d3 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: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // discoveryState() at REDIRECT; compare it on EVERY callback regardless of `previousIssuer` | ||
| // / `authorizationServerChanged` — an attacker who controls PRM at first contact can flip | ||
| // → AS-B (poison) → AS-A (REDIRECT) → AS-B (callback) so that previousIssuer reads | ||
| // undefined or AS-B and the `authorizationServerChanged` block below never runs. |
There was a problem hiding this comment.
wonder if we want a more explicit binding mechanism than sort of probing around here.
| // Record the resolved authorization-server identity so the previous-issuer read on the | ||
| // next auth() call round-trips exactly. SEP-2352 invariant: the SDK MUST NOT durably | ||
| // record an authorization server as "authenticated-at" until a fresh authorization at | ||
| // that AS has completed (saveTokens) — including FIRST CONTACT. Persisting earlier | ||
| // desynchronizes the per-call `authorizationServerChanged` gate from stateful credential | ||
| // storage: an attacker who poisons the first PRM probe to AS-B then flips → AS-A → AS-B | ||
| // would read previousIssuer === issuer on the callback and the credential-isolation block | ||
| // above never runs. saveAuthorizationServerUrl is therefore written ONLY here, immediately | ||
| // after saveTokens (see :authCodePersist / :refreshPersist below). | ||
| // TODO: resourceMetadataUrl is only populated when explicitly provided via options | ||
| // or loaded from cached state. The URL derived internally by | ||
| // discoverOAuthProtectedResourceMetadata() is not captured back here. |
There was a problem hiding this comment.
these super long comments feel like a smell here. can we make the invalid state harder to get to and not rely on these big comments?
felixweinberger
force-pushed
the
fweinberger/auth-4-sep2468-server
branch
from
b1ecab4 to
0097867
Compare
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
e705921 to
b3547d6
Compare
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
b3547d6 to
edbbd84
Compare
felixweinberger
force-pushed
the
fweinberger/auth-4-sep2468-server
branch
from
0097867 to
646cea6
Compare
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
edbbd84 to
354f0ac
Compare
felixweinberger
force-pushed
the
fweinberger/auth-4-sep2468-server
branch
from
646cea6 to
1196cf1
Compare
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
354f0ac to
794b732
Compare
felixweinberger
force-pushed
the
fweinberger/auth-4-sep2468-server
branch
2 times, most recently
from
6b3f82b to
ef079b3
Compare
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
794b732 to
71b2687
Compare
…(URLSearchParams) overload Claude-Session: https://claude.ai/code/session_01XBib5gRe8AMPPJhySCz3EJ
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
71b2687 to
9bc98d3
Compare
felixweinberger
force-pushed
the
fweinberger/auth-4-sep2468-server
branch
from
ef079b3 to
46b6b01
Compare
felixweinberger
merged commit 9bc98d3
into
fweinberger/auth-1-sep2468-client
| export async function resolveAuthorizationCallbackParams( | ||
| codeOrParams: string | URLSearchParams, | ||
| iss: string | undefined, | ||
| provider: OAuthClientProvider, | ||
| serverUrl: string | URL, | ||
| opts?: { fetchFn?: FetchLike; resourceMetadataUrl?: URL } | ||
| ): Promise<{ authorizationCode: string; iss: string | undefined }> { | ||
| if (typeof codeOrParams === 'string') { | ||
| return { authorizationCode: codeOrParams, iss }; | ||
| } | ||
| const issParam = codeOrParams.get('iss') ?? undefined; | ||
| const code = codeOrParams.get('code'); | ||
| if (code) { | ||
| return { authorizationCode: code, iss: issParam }; | ||
| } | ||
| // No code → error response. Gate the (potentially attacker-supplied) error params on the | ||
| // issuer first. Prefer the provider's recorded discovery state; when absent, mirror auth()'s | ||
| // code-present path and run a fresh discovery so the iss gate has an authentic baseline. | ||
| const discoveryState = await provider.discoveryState?.(); | ||
| let metadata = discoveryState?.authorizationServerMetadata; | ||
| if (!metadata) { | ||
| try { | ||
| const serverInfo = await discoverOAuthServerInfo(serverUrl, opts); | ||
| metadata = serverInfo.authorizationServerMetadata; | ||
| } catch { | ||
| metadata = undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 On the no-code (error-shaped) callback path, resolveAuthorizationCallbackParams() falls back to a fresh discoverOAuthServerInfo(serverUrl, opts), but its opts type only accepts { fetchFn, resourceMetadataUrl }, so the user-configured skipIssuerMetadataValidation flag (which both transports hold and pass to auth() on the code-present leg one statement later) is never forwarded. For the documented known-misconfigured-AS opt-out case, the fresh discovery throws IssuerMismatchError, the bare catch swallows it, and the generic UnauthorizedError('Authorization callback failed and the issuer could not be verified') replaces the AS's real OAuth error — inconsistent behavior between the two legs of the same finishAuth overload. Fix: add skipIssuerMetadataValidation to the resolver's opts and forward this._skipIssuerMetadataValidation from both SSEClientTransport.finishAuth and StreamableHTTPClientTransport.finishAuth.
Extended reasoning...
The gap. The new shared resolver resolveAuthorizationCallbackParams(codeOrParams, iss, provider, serverUrl, opts) (packages/client/src/client/auth.ts:561-588) declares opts as only { fetchFn?: FetchLike; resourceMetadataUrl?: URL }. On the URLSearchParams overload's no-code path (an error-shaped callback), when the provider has no recorded discoveryState, the resolver runs a fresh discoverOAuthServerInfo(serverUrl, opts) to obtain an authentic issuer baseline before surfacing the callback's error params. discoverOAuthServerInfo() accepts a skipIssuerMetadataValidation option and forwards it to discoverAuthorizationServerMetadata as skipIssuerValidation (auth.ts:1744/1777) — but the resolver's opts type doesn't even allow the flag, and neither transport passes it.\n\nThe inconsistency this PR introduces. Both SSEClientTransport and StreamableHTTPClientTransport hold this._skipIssuerMetadataValidation (the documented opt-out for a known-misconfigured AS, per migration.md) and pass it to auth() on the code-present leg of the very same finishAuth call — one statement after the resolveAuthorizationCallbackParams call that omits it (e.g. streamableHttp.ts:860-875, and the same pattern in sse.ts). So a user who explicitly opted out of the RFC 8414 §3.3 issuer-echo check gets it honored when the callback carries a code, but silently ignored when the same callback is error-shaped.\n\nCode path / step-by-step proof.\n1. A user connects to an MCP server whose AS publishes metadata with a mismatched issuer (the documented known-misconfigured-AS case), and sets skipIssuerMetadataValidation: true on the transport options.\n2. The AS rejects the authorization request (e.g. user denies consent, or unauthorized_client) and redirects back with error=access_denied&error_description=... and no code.\n3. The host calls transport.finishAuth(new URL(callbackUrl).searchParams) — the preferred overload this PR adds.\n4. resolveAuthorizationCallbackParams sees no code; the provider has no cached discoveryState (e.g. it doesn't implement the optional method, or state was lost across the redirect), so it calls discoverOAuthServerInfo(serverUrl, { fetchFn, resourceMetadataUrl }) — without the flag.\n5. discoverAuthorizationServerMetadata runs the issuer-echo check, the misconfigured AS fails it, and IssuerMismatchError is thrown (auth.ts:1682).\n6. The resolver's bare catch { metadata = undefined; } swallows it, metadata stays undefined, and the function throws the generic UnauthorizedError('Authorization callback failed and the issuer could not be verified') — replacing the AS's real error/error_description (e.g. access_denied) that the user opted in to receiving from this AS.\n\nWhy nothing else prevents it. The flag is threaded everywhere else on this surface: the transports pass it to adaptOAuthProvider() at construction and to auth() in both finishAuth legs and the 401 path — only this new fresh-discovery fallback misses it. Providers that do implement discoveryState() and have cached metadata from the redirect leg avoid the fresh discovery entirely, which narrows the impact, but the resolver explicitly handles the no-cached-state case (its own comment says it mirrors what auth() does on the code-present path — and auth() does receive the flag there).\n\nImpact. No security regression and no mainline functional break — the flow was failing anyway, and the failure mode is a degraded diagnostic: the operator/user sees a generic "issuer could not be verified" error instead of the AS's actual OAuth error, and the documented opt-out behaves inconsistently between the two legs of one overload. All three verifiers confirmed the trace; none refuted it.\n\nFix (trivial). Add skipIssuerMetadataValidation?: boolean to the resolver's opts type, forward it into discoverOAuthServerInfo(serverUrl, opts) (it already accepts it), and pass skipIssuerMetadataValidation: this._skipIssuerMetadataValidation from both transports' resolveAuthorizationCallbackParams call sites — mirroring what they already do for the auth() call directly below.
2 participants
e7059212cauthInternalresolves the validatedmetadata.issuer(PR 2; raw PRM entry as fallback) and threads it asctx.issuertoOAuthClientProvider.clientInformation()/.saveClientInformation(). It tracks two distinct persisted issuer slots — authenticated-at (authorizationServerUrl(), advanced only aftersaveTokens) and redirected-to (discoveryState().authorizationServerMetadata.issuer, written atREDIRECT) — and gates every credential type against the appropriate slot so no credential issued by one AS is ever sent to another AS's endpoints. For caching providers, an explicitresourceMetadataUrlre-probes PRM so AS changes are detectable through cached discovery. On mismatch: discardclientInformation()for this flow and re-register (CIMD client_ids are portable — skip re-register);invalidateCredentialsis best-effort hygiene only. The fourauthExtensionsm2m providers gain anexpectedIssuerconstructor seed and throwAuthorizationServerMismatchErroron mismatch instead of silently sending the static credential.Motivation and Context
SEP-2352 (modelcontextprotocol/modelcontextprotocol#2352).
Normative sentences implemented:
Security model
The threat model is an attacker who controls (or can poison) the protected-resource metadata's
authorization_servers[]entry and the callback query parameters, but does not control the legitimate AS. The goal is that no credential issued by AS-A is ever transmitted to AS-B:client_id/client_secret,refresh_token, andauthorization_code+ PKCEcode_verifier.authInternalreads two persisted issuer values on entry, before this call's discovery overwrites either:provider.authorizationServerUrl()saveAuthorizationServerUrl(issuer)— only insidepersistAuthorizationServerIdentity(), which runs immediately aftersaveTokens()(including first contact). Never written onREDIRECTor on a throw path.authorizationServerChanged→client_id/client_secretdiscard-and-re-register; the!authorizationServerChangedguard on the refresh branch (so a previous-ASrefresh_tokenis never POSTed to the new AS's token endpoint); the m2m pre-registered throw.discoveryState()?.authorizationServerMetadata?.issuer ?? .authorizationServerUrlsaveDiscoveryState({...})— on everyREDIRECTimmediately beforeredirectToAuthorization, recording which AS the in-flight authorization request targets.if (authorizationCode && redirectedToIssuer && redirectedToIssuer !== issuer) throw AuthorizationServerMismatchError. Runs on every callback regardless ofpreviousIssuer/authorizationServerChanged.The two slots are deliberately decoupled. Advancing authenticated-at at
REDIRECTre-opens a two-callrefresh_tokenleak (an aborted redirect would record AS-B as authenticated whiletokens()still returnsrt-A); advancing it only atsaveTokenswould deadlock a legitimate migration (the callback leg would forever readpreviousIssuer = AS-Aand throw). The redirected-to slot resolves the deadlock — it lets the callback leg recognise "this is the AS we redirected to" — while the hoisted gate keyed on it (not onpreviousIssuer) closes the first-contact flip-flip poison: an attacker who flips PRM → AS-B (poison) → AS-A (REDIRECT) → AS-B (callback) cannot makeredirectedToIssuerread AS-B, so the gate fires even whenauthorizationServerChangedis false.saveDiscoveryStateis also deferred past every SEP-2352 throw path so a rejected migration cannot poison the redirected-to record.Net: the wire MUST holds for every credential type independently of
invalidateCredentials()— the SDK never records an AS as authenticated-at until it has obtained a fresh token from it, and never redeems a code at any AS other than the one the redirect targeted.Consumer obligation
discoveryState()/saveDiscoveryState()is now security-relevant and SHOULD be implemented — it is the only input to the callback-leg gate. Providers that omit it have no callback-leg AS binding on first contact (bothpreviousIssuerandredirectedToIssuerreadundefined, so neither gate can run); after a prior successful authorization they fail closed withAuthorizationServerMismatchErrorbut cannot complete a migration. The SDK emits aconsole.warnon the callback leg when the provider lackssaveDiscoveryStateso the gap is visible at integration time. The referenceInMemoryOAuthClientProviderand the conformance fixture provider now implement both methods.RFC 9207
issvalidation (PR 2) covers the same callback-leg attack independently — when the legitimate AS emitsiss, the mix-up is rejected before the SEP-2352 gate is reached. The SEP-2352 gate is the defence for ASes that do not advertiseauthorization_response_iss_parameter_supported(and against an attacker who stripsissfrom the callback).How Has This Been Tested?
cimd_code_verifier,two_call_refresh,issuer_keyed_map,m2m_first_contact,flip_flip,legitimate_migration,attacker_flips_again— converged to invariant=true / liveness=true with no remaining majors in code.packages/client/test/client/auth.test.ts(describe('SEP-2352 …')), including: first-contactREDIRECTdoes not seed authenticated-at;saveDiscoveryStatenot poisoned on the throw path; flip-flip first-contact poison rejected; legitimate AS migration completes through the callback leg and only then advances authenticated-at; PRM flipped again betweenREDIRECTand callback rejected; no-saveDiscoveryStateprovider warns on the callback leg;refresh_tokennever POSTed to the new AS across calls; one-directional trailing-slash tolerance.client-auth:as-migration:*rows intest/e2e/scenarios/client-auth.test.ts(re-register, CIMD-skip, PRM-rediscovery, m2m-throw, ctx-issuer-threaded, no-token-reuse).auth/authorization-server-migration(31 checks) burns; theauth/iss-*block andauth/scope-retry-limitburn on the 2026 leg.Breaking Changes
None at the type level (PR 1 made the
ctxparameter optional;discoveryState/saveDiscoveryStatewere already optional members). ExistingOAuthClientProviderimplementations compile unchanged.Behaviorally:
discoveryState()/saveDiscoveryState()is promoted from a discovery-cache hint to a security-relevant SHOULD-implement — it is the persistence slot for the callback-leg gate. Providers that omit it receive aconsole.warnon the callback leg and are not protected against PRM hijack on first contact (see Consumer obligation).clientInformationregardless ofctx.issuerwill see it discarded and re-registered when the AS changes — that is the spec-mandated behavior and is what an unmodified provider already experienced as a hard failure at the AS.ClientCredentialsProvider/PrivateKeyJwtProvider/StaticPrivateKeyJwtProvider/CrossAppAccessProviderno longer definesaveClientInformation()(so they are classified pre-registered) and now throwAuthorizationServerMismatchErroron AS change instead of silently sending the credential. They accept a new optionalexpectedIssuerto seed first-call detection.Types of changes
Checklist
Additional context
The wire MUST is enforced structurally regardless of whether the consumer's provider implements issuer-keyed storage;
invalidateCredentials()is best-effort hygiene, not load-bearing. The consumer-side storage obligation (key persisted DCR credentials byctx.issuer; implementdiscoveryState/saveDiscoveryState) is documented inmigration.mdand demonstrated in the reshapedexamples/oauth/simpleOAuthClientProvider.ts. Secondary hardening folded into this commit:assertSecureEndpoint()extended to the registration endpoint, andInsufficientScopeError/RegistrationRejectedErrornowJSON.stringifywire values in their messages (matchingIssuerMismatchError).