diff --git a/.changeset/auth-sep-2352-credential-isolation.md b/.changeset/auth-sep-2352-credential-isolation.md new file mode 100644 index 000000000..3fe2449e3 --- /dev/null +++ b/.changeset/auth-sep-2352-credential-isolation.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/client': minor +--- + +Per-authorization-server credential isolation (SEP-2352). `auth()` now stamps an `issuer` field onto every value it passes to `saveTokens()` / `saveClientInformation()` and threads `{ issuer }` to `tokens()` / `clientInformation()`; on read, a stored credential whose stamp names a different authorization server is treated as `undefined`, so a `client_id` / `refresh_token` issued by one AS is never sent to another. Providers that round-trip stored values verbatim are protected with no code change; multi-AS providers may key storage on `ctx.issuer`. New `discardIfIssuerMismatch()` helper and `AuthorizationServerMismatchError` (callback-leg gate). `OAuthClientProvider.saveAuthorizationServerUrl()` / `authorizationServerUrl()` are deprecated (still written, never read). `ClientCredentialsProvider`, `PrivateKeyJwtProvider`, `StaticPrivateKeyJwtProvider`, and `CrossAppAccessProvider` gain `expectedIssuer` and no longer define `saveClientInformation()`. diff --git a/docs/migration-SKILL.md b/docs/migration-SKILL.md index 821923811..bd2775c3e 100644 --- a/docs/migration-SKILL.md +++ b/docs/migration-SKILL.md @@ -223,6 +223,7 @@ The OAuth client flow additionally throws dedicated classes from `@modelcontextp | `exchangeAuthorization()` / `refreshAuthorization()` / `fetchToken()` / `requestJwtAuthorizationGrant()` / `exchangeJwtAuthGrant()` non-https token endpoint | `InsecureTokenEndpointError` (`tokenEndpoint`) | | RFC 9207 `iss` mismatch / RFC 8414 §3.3 issuer-echo mismatch | `IssuerMismatchError` (`kind`, `expected`, `received`) | | Transport 403 `insufficient_scope` with `onInsufficientScope: 'throw'`, or default mode without an `OAuthClientProvider` | `InsufficientScopeError` (`requiredScope`, `resourceMetadataUrl`, `errorDescription`) | +| `auth()` callback leg: discovery resolves a different AS than the recorded redirect target | `AuthorizationServerMismatchError` (`recordedIssuer`, `currentIssuer`) | Update OAuth error handling: @@ -595,6 +596,8 @@ OAuth callback handling: pass the callback URL's `URLSearchParams` to `transport Token-exchange / refresh now refuse to send credentials to a non-`https:` token endpoint (loopback `localhost` / `127.0.0.1` / `::1` exempt), throwing `InsecureTokenEndpointError` with no opt-out. `auth()` surfaces this on every path including refresh — switch any plain-`http:` AS on a non-loopback host to TLS. +`auth()` stamps an `issuer` field onto every value it passes to `saveTokens()` / `saveClientInformation()` and threads `{ issuer }` as the `ctx` argument to those methods plus `tokens()` / `clientInformation()` (SEP-2352). On read, a stored value whose `issuer` names a different AS is treated as `undefined` and the flow re-registers / re-authorizes. Round-trip the stored object verbatim and you're protected; multi-AS providers key storage on `ctx.issuer`. `OAuthClientProvider.saveAuthorizationServerUrl()` / `authorizationServerUrl()` are `@deprecated` (still written for back-compat, never read by the SDK). `ClientCredentialsProvider` / `PrivateKeyJwtProvider` / `StaticPrivateKeyJwtProvider` / `CrossAppAccessProvider` gain `expectedIssuer?: string` and no longer define `saveClientInformation()`. + No code changes required; wire-behavior note: on a 2026-07-28 Streamable HTTP connection, aborting an in-flight client request (caller `signal` / timeout) closes that request's SSE response stream as the spec cancellation signal — `notifications/cancelled` is no longer POSTed there. 2025-era connections and stdio at any era still send `notifications/cancelled`. Custom `Transport` implementations that open one underlying request per outbound message and honor `TransportSendOptions.requestSignal` may declare `readonly hasPerRequestStream = true` to opt into the same routing. diff --git a/docs/migration.md b/docs/migration.md index 12e03acb4..1d0916935 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -1560,10 +1560,28 @@ Step-up retries are now hard-capped per send (`maxStepUpRetries`, default 1) reg The GET listen-stream open path now applies the same step-up handling as the POST send path. +### Credentials are bound to the issuing authorization server (SEP-2352) + +`auth()` now stamps an `issuer` field onto every value it passes to `saveTokens()` and `saveClientInformation()`, and passes `{ issuer }` as the second (`ctx`) argument to `tokens()` / `saveTokens()` / `clientInformation()` / `saveClientInformation()`. On read, a stored value whose `issuer` stamp names a different authorization server is treated as `undefined` — the flow re-registers / re-authorizes exactly as if nothing were stored. A `client_id` or `refresh_token` issued by one authorization server is therefore never sent to another. + +**Round-trip the stored object unchanged and you're protected** — single-slot storage works. To hold credentials for several authorization servers at once, key your storage on `ctx.issuer` instead (and treat **`ctx === undefined` as "return the most-recently-saved token set"** — the transport's per-request `Authorization: Bearer` read (`adaptOAuthProvider().token()`) calls `tokens()` with no `ctx`). + +Implement `discoveryState()` / `saveDiscoveryState()` so the callback leg can verify it is exchanging the authorization code at the same AS the redirect targeted; without it the SDK `console.warn`s once per callback. **`discoveryState` must persist with the same durability as `codeVerifier`** — it has to survive the redirect round-trip (page navigation, app restart). A provider that implements `saveDiscoveryState()` but cannot return it on the callback leg fails closed with `AuthorizationServerMismatchError`. Hosts that drive 401 handling themselves (custom middleware / `withOAuthRetry`) should call `provider.invalidateCredentials?.('discovery')` on a fresh 401 so a changed `authorization_servers` list is picked up on re-discovery. The built-in `StreamableHTTPClientTransport` / `SSEClientTransport` 401 path does not currently invalidate discovery state. + +`OAuthClientProvider.saveAuthorizationServerUrl()` / `authorizationServerUrl()` are **deprecated** — `auth()` still calls `saveAuthorizationServerUrl()` for back-compat with providers that read it internally (Cross-App Access), but the SDK never reads `authorizationServerUrl()`. The call timing changed in v2: it was post-discovery, it is now post-`saveTokens` (after a successful token exchange). Read the `issuer` stamp on stored tokens, or `ctx.issuer`, instead. + +The bundled `ClientCredentialsProvider`, `PrivateKeyJwtProvider`, `StaticPrivateKeyJwtProvider`, and `CrossAppAccessProvider` gain an `expectedIssuer` option that stamps the constructor-supplied credential; when set, `auth()` against any other authorization server fails before the credential leaves the process. These providers no longer define `saveClientInformation()`. + ### Conformance obligations for `OAuthClientProvider` implementers +#### SEP-2352 — per-authorization-server credential isolation + +**No code change required for the common case.** If your `saveTokens()` / `saveClientInformation()` persist the value passed to them verbatim and your `tokens()` / `clientInformation()` return it verbatim, the SDK-stamped `issuer` round-trips and the binding holds. + +If you serialise to a custom format, persist the `issuer` field alongside the rest of the value. If you key storage by `ctx.issuer`, return `undefined` for an issuer you have no entry for, and treat **`ctx === undefined` as "return the most-recently-saved token set"** — the transport's per-request `Authorization: Bearer` read (`adaptOAuthProvider().token()`) calls `tokens()` with no `ctx`. + ## Using an LLM to migrate your code An LLM-optimized version of this guide is available at [`docs/migration-SKILL.md`](migration-SKILL.md). It contains dense mapping tables designed for tools like Claude Code to mechanically apply all the changes described above. You can paste it into your LLM context or load it as diff --git a/examples/oauth/simpleOAuthClientProvider.ts b/examples/oauth/simpleOAuthClientProvider.ts index 1ef08279f..5efbdaefd 100644 --- a/examples/oauth/simpleOAuthClientProvider.ts +++ b/examples/oauth/simpleOAuthClientProvider.ts @@ -1,14 +1,28 @@ -import type { OAuthClientInformationMixed, OAuthClientMetadata, OAuthClientProvider, OAuthTokens } from '@modelcontextprotocol/client'; +import type { + OAuthClientInformationMixed, + OAuthClientMetadata, + OAuthClientProvider, + OAuthDiscoveryState, + OAuthTokens +} from '@modelcontextprotocol/client'; import { validateClientMetadataUrl } from '@modelcontextprotocol/client'; /** - * In-memory OAuth client provider for demonstration purposes - * In production, you should persist tokens securely + * In-memory OAuth client provider for demonstration purposes. + * In production, you should persist tokens and client credentials securely. + * + * Tokens and client credentials are stored as single-slot blobs. The SDK stamps an + * `issuer` field onto every value it saves; round-tripping the blob unchanged means + * a credential issued by one authorization server is never reused at another (the + * SDK reads the stamp back as a key-not-found and re-registers / re-authorizes). + * To hold credentials for several authorization servers at once, key your storage + * on the `ctx.issuer` argument instead. */ export class InMemoryOAuthClientProvider implements OAuthClientProvider { private _clientInformation?: OAuthClientInformationMixed; private _tokens?: OAuthTokens; private _codeVerifier?: string; + private _discoveryState?: OAuthDiscoveryState; constructor( private readonly _redirectUrl: string | URL, @@ -66,4 +80,19 @@ export class InMemoryOAuthClientProvider implements OAuthClientProvider { } return this._codeVerifier; } + + saveDiscoveryState(state: OAuthDiscoveryState): void { + this._discoveryState = state; + } + + discoveryState(): OAuthDiscoveryState | undefined { + return this._discoveryState; + } + + invalidateCredentials(scope: 'all' | 'client' | 'tokens' | 'verifier' | 'discovery'): void { + if (scope === 'all' || scope === 'client') this._clientInformation = undefined; + if (scope === 'all' || scope === 'tokens') this._tokens = undefined; + if (scope === 'all' || scope === 'verifier') this._codeVerifier = undefined; + if (scope === 'all' || scope === 'discovery') this._discoveryState = undefined; + } } diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 2c1169f23..d16ab0674 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -27,10 +27,20 @@ import { } from '@modelcontextprotocol/core'; import pkceChallenge from 'pkce-challenge'; -import { InsecureTokenEndpointError, IssuerMismatchError, RegistrationRejectedError } from './authErrors.js'; +import { + AuthorizationServerMismatchError, + InsecureTokenEndpointError, + IssuerMismatchError, + RegistrationRejectedError +} from './authErrors.js'; // Re-exported for back-compat — the canonical home is ./authErrors.js. -export { InsecureTokenEndpointError, IssuerMismatchError, RegistrationRejectedError } from './authErrors.js'; +export { + AuthorizationServerMismatchError, + InsecureTokenEndpointError, + IssuerMismatchError, + RegistrationRejectedError +} from './authErrors.js'; /** * Function type for adding client authentication to token requests. @@ -105,6 +115,55 @@ export interface OAuthClientInformationContext { issuer: string; } +/** + * SEP-2352 stamp check: returns `stored` only when its `issuer` stamp matches the + * resolved authorization server. A stamp that names a *different* issuer reads back + * as `undefined`, so a credential issued by one authorization server is never reused + * at another — the flow falls through to re-registration / re-authorization exactly + * as if nothing were stored. An unstamped value (legacy provider or pre-SEP-2352 + * storage) is returned as-is with a `console.warn`; {@linkcode auth} writes the + * stamp back on first use so the window closes after one call. + * + * {@linkcode auth} stamps every value it writes via `saveTokens` / `saveClientInformation`, + * so a provider that round-trips the stored object verbatim is protected with no extra + * code. Providers that hold credentials for multiple authorization servers key their + * storage on `ctx.issuer` instead. + * + * @param opts.canPersistStamp - When `false`, suppresses the unstamped-credential + * warning: the caller cannot back-stamp (no `saveClientInformation`), so the + * "binding on first use" claim would be false and would fire on every call. + */ +export function discardIfIssuerMismatch( + stored: T | undefined, + issuer: string, + opts?: { canPersistStamp?: boolean } +): T | undefined { + if (stored === undefined) return undefined; + if (stored.issuer === undefined) { + if (opts?.canPersistStamp !== false) { + console.warn( + `[mcp-sdk] SEP-2352: stored OAuth credential has no 'issuer' stamp (pre-upgrade storage or ` + + `provider not round-tripping the value). SEP-2352 isolation is inactive for this read; ` + + `ensure your provider round-trips the issuer field.` + ); + } + return stored; + } + return issuersMatch(stored.issuer, issuer) ? stored : undefined; +} + +/** + * SEP-2352 issuer-identity comparison. Tolerates a single trailing `/` difference, + * mirroring the RFC 8414 §3.3 "one narrow tolerance" applied at metadata-echo + * validation in {@linkcode discoverAuthorizationServerMetadata}: when the SDK + * derives an issuer from `String(new URL(...))` (always slash-suffixed) and the AS + * publishes a slash-free `metadata.issuer`, the two name the same authorization + * server. + */ +function issuersMatch(a: string, b: string): boolean { + return a === b || (a.endsWith('/') && a.slice(0, -1) === b) || (b.endsWith('/') && b.slice(0, -1) === a); +} + /** * Type guard distinguishing `OAuthClientProvider` from a minimal `AuthProvider`. * Transports use this at construction time to classify the `authProvider` option. @@ -146,6 +205,15 @@ export async function handleOAuthUnauthorized( * transports consume. Called once at transport construction — the transport stores * the adapted provider for `_commonHeaders()` and 401 handling, while keeping the * original `OAuthClientProvider` for OAuth-specific paths (`finishAuth()`, 403 `insufficient_scope` step-up). + * + * SEP-2352 note: `token()` here is the per-request `Authorization: Bearer …` read for + * the *resource server* (the MCP transport URL), not an authorization server. No OAuth + * discovery has run at this layer, so there is no `issuer` to pass as `ctx` and no + * {@linkcode discardIfIssuerMismatch} check to apply — the access token is sent only to + * the resource server, never to an AS, so the SEP-2352 cross-AS isolation invariant is + * not in scope. Providers that key storage on `ctx.issuer` MUST treat `ctx === undefined` + * as "return the most-recently-saved token set" (the only consumer is the resource server + * the token was minted for); providers that round-trip a single blob need no change. */ export function adaptOAuthProvider( provider: OAuthClientProvider, @@ -228,6 +296,9 @@ export interface OAuthClientProvider { * @param ctx - Carries the resolved authorization-server `issuer`. Providers * that persist tokens per authorization server should return the entry * keyed by `ctx.issuer`. Providers with a single token set may ignore it. + * When called with no `ctx` — the transport's per-request bearer-token + * read — return the most-recently-saved token set; do not return + * `undefined` for `ctx === undefined`. */ tokens(ctx?: OAuthClientInformationContext): StoredOAuthTokens | undefined | Promise; @@ -333,24 +404,23 @@ export interface OAuthClientProvider { prepareTokenRequest?(scope?: string): URLSearchParams | Promise | undefined; /** - * Saves the authorization server URL after RFC 9728 discovery. - * This method is called by {@linkcode auth} after successful discovery of the - * authorization server via protected resource metadata. + * Saves the resolved authorization-server **issuer**. Called after a successful + * token exchange (timing changed in v2: was post-discovery, now post-`saveTokens`). * - * Providers implementing Cross-App Access or other flows that need access to - * the discovered authorization server URL should implement this method. - * - * @param authorizationServerUrl - The authorization server URL discovered via RFC 9728 + * @deprecated Superseded by the `issuer` stamp on stored tokens / client credentials + * (SEP-2352). {@linkcode auth} still **writes** this for back-compat with providers + * that read it (e.g. Cross-App Access), but the SDK never reads it. Prefer reading + * the `issuer` field on the value passed to {@linkcode saveTokens} / + * {@linkcode saveClientInformation}, or the `ctx.issuer` argument. */ saveAuthorizationServerUrl?(authorizationServerUrl: string): void | Promise; /** * Returns the previously saved authorization server URL, if available. * - * Providers implementing Cross-App Access can use this to access the - * authorization server URL discovered during the OAuth flow. - * - * @returns The authorization server URL, or `undefined` if not available + * @deprecated Superseded by the `issuer` stamp on stored tokens / client credentials + * (SEP-2352). The SDK never reads this method; it remains for provider implementations + * that consume the value internally (e.g. Cross-App Access). */ authorizationServerUrl?(): string | undefined | Promise; @@ -385,6 +455,9 @@ export interface OAuthClientProvider { * external configuration) to bootstrap the OAuth flow without discovery. * * Called by {@linkcode auth} after successful discovery. + * + * MUST persist with the same durability as `codeVerifier` (survives the redirect + * round-trip). */ saveDiscoveryState?(state: OAuthDiscoveryState): void | Promise; @@ -395,9 +468,12 @@ export interface OAuthClientProvider { * URL, resource metadata, etc.) instead of performing RFC 9728 discovery, reducing * latency on subsequent calls. * - * Providers should clear cached discovery state on repeated authentication failures - * (via {@linkcode invalidateCredentials} with scope `'discovery'` or `'all'`) to allow - * re-discovery in case the authorization server has changed. + * Hosts should call {@linkcode invalidateCredentials} with scope `'discovery'` + * on repeated 401s so a changed `authorization_servers` list is picked up; the + * SDK does not invoke that scope itself. + * + * MUST persist with the same durability as `codeVerifier` (survives the redirect + * round-trip). */ discoveryState?(): OAuthDiscoveryState | undefined | Promise; } @@ -901,7 +977,11 @@ export async function auth(provider: OAuthClientProvider, options: AuthOptions): // Handle recoverable error types by invalidating credentials and retrying if (error instanceof OAuthError) { if (error.code === OAuthErrorCode.InvalidClient || error.code === OAuthErrorCode.UnauthorizedClient) { - await provider.invalidateCredentials?.('all'); + // Not 'all' — preserve discoveryState so the callback-leg gate on retry doesn't + // fire a false 'discoveryState was not available on the callback leg' AuthorizationServerMismatchError that masks the + // real invalid_client. + await provider.invalidateCredentials?.('client'); + await provider.invalidateCredentials?.('tokens'); return await authInternal(provider, options); } else if (error.code === OAuthErrorCode.InvalidGrant) { await provider.invalidateCredentials?.('tokens'); @@ -971,6 +1051,7 @@ async function authInternal( let resourceMetadata: OAuthProtectedResourceMetadata | undefined; let authorizationServerUrl: string | URL; let metadata: AuthorizationServerMetadata | undefined; + let freshDiscoveryState: OAuthDiscoveryState | undefined; // If resourceMetadataUrl is not provided, try to load it from cached state // This handles browser redirects where the URL was saved before navigation @@ -1028,20 +1109,61 @@ async function authInternal( metadata = serverInfo.authorizationServerMetadata; resourceMetadata = serverInfo.resourceMetadata; - // Persist discovery state for future use + // Captured now, persisted only after the SEP-2352 callback-leg gate below — so a + // gate throw cannot leave a freshly resolved (potentially PRM-poisoned) AS recorded + // for the retry to read back as `recordedIssuer`. // 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. - await provider.saveDiscoveryState?.({ + freshDiscoveryState = { authorizationServerUrl: String(authorizationServerUrl), resourceMetadataUrl: effectiveResourceMetadataUrl?.toString(), resourceMetadata, authorizationServerMetadata: metadata - }); + }; + } + + // SEP-2352: the canonical authorization-server identity for this flow. `metadata.issuer` + // is RFC 8414 §3.3-validated to equal the discovery URL; when no metadata document was + // found (legacy fallback) the discovery URL itself is the only identifier available. + const issuer = metadata?.issuer ?? String(authorizationServerUrl); + const infoCtx: OAuthClientInformationContext = { issuer }; + + // Deprecated write-only hook, kept for providers (e.g. Cross-App Access) that read it + // internally. The SDK never reads `authorizationServerUrl()`. + await provider.saveAuthorizationServerUrl?.(issuer); + + // SEP-2352 callback-leg gate. Stored credentials are protected structurally by the + // issuer stamp, but the in-flight `authorization_code` + PKCE `code_verifier` are not + // stored — they are bound to the AS the redirect targeted, recorded in `discoveryState()`. + // Fail-closed: a provider that implements saveDiscoveryState but returned no discovery + // state on the callback leg (e.g. not persisted alongside codeVerifier across page navigation) MUST NOT + // proceed — fresh discovery may have resolved a different AS than the one the user + // approved at /authorize, and the clientInformation stamp alone does not protect a keyed + // multi-AS provider here. Providers that do not implement saveDiscoveryState at all keep + // the (legacy) warn-and-proceed behavior. + if (authorizationCode !== undefined) { + const recordedIssuer = cachedState?.authorizationServerMetadata?.issuer ?? cachedState?.authorizationServerUrl; + if (recordedIssuer === undefined) { + if (provider.saveDiscoveryState !== undefined) { + throw new AuthorizationServerMismatchError( + 'discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier', + issuer + ); + } + console.warn( + '[mcp-sdk] OAuthClientProvider does not implement saveDiscoveryState()/discoveryState(); ' + + 'the SEP-2352 callback-leg authorization-server binding cannot be checked. ' + + 'Implement discoveryState (persist alongside codeVerifier) — see migration.md §SEP-2352.' + ); + } else if (!issuersMatch(recordedIssuer, issuer)) { + throw new AuthorizationServerMismatchError(recordedIssuer, issuer); + } } - // Save authorization server URL for providers that need it (e.g., CrossAppAccessProvider) - await provider.saveAuthorizationServerUrl?.(String(authorizationServerUrl)); + if (freshDiscoveryState) { + await provider.saveDiscoveryState?.(freshDiscoveryState); + } const resource: URL | undefined = await selectResourceURL(serverUrl, provider, resourceMetadata); @@ -1058,8 +1180,26 @@ async function authInternal( clientMetadata: provider.clientMetadata }); - // Handle client registration if needed - let clientInformation = await Promise.resolve(provider.clientInformation()); + // Handle client registration if needed. SEP-2352: a stored credential whose `issuer` + // stamp names a different authorization server reads back as `undefined`, so the flow + // re-registers exactly as if nothing were stored. + const rawClientInfo = await Promise.resolve(provider.clientInformation(infoCtx)); + let clientInformation = discardIfIssuerMismatch(rawClientInfo, issuer, { + canPersistStamp: provider.saveClientInformation !== undefined + }); + if (clientInformation === undefined && rawClientInfo?.issuer && provider.saveClientInformation === undefined) { + // Static-credential provider (no DCR) whose `expectedIssuer` stamp names a different + // AS — surface the typed error with both issuers rather than the generic + // "client information must be saveable for dynamic registration" fallback. + throw new AuthorizationServerMismatchError(rawClientInfo.issuer, issuer); + } + if (clientInformation && clientInformation.issuer === undefined) { + // SEP-2352 back-stamp: legacy (pre-SEP-2352) storage returned an unstamped value. + // Bind it to the first AS resolved after upgrade so subsequent calls have a real + // stamp to compare against — closes the otherwise-permanent unstamped window. + clientInformation = { ...clientInformation, issuer }; + await provider.saveClientInformation?.(clientInformation, infoCtx); + } if (!clientInformation) { if (authorizationCode !== undefined) { throw new Error('Existing OAuth client information is required when exchanging an authorization code'); @@ -1079,10 +1219,8 @@ async function authInternal( if (shouldUseUrlBasedClientId) { // SEP-991: URL-based Client IDs - clientInformation = { - client_id: clientMetadataUrl - }; - await provider.saveClientInformation?.(clientInformation); + clientInformation = { client_id: clientMetadataUrl, issuer }; + await provider.saveClientInformation?.(clientInformation, infoCtx); } else { // Fallback to dynamic registration if (!provider.saveClientInformation) { @@ -1096,8 +1234,8 @@ async function authInternal( fetchFn }); - await provider.saveClientInformation(fullInformation); - clientInformation = fullInformation; + clientInformation = { ...fullInformation, issuer }; + await provider.saveClientInformation(clientInformation, infoCtx); } } @@ -1126,11 +1264,19 @@ async function authInternal( fetchFn }); - await provider.saveTokens(tokens); + await provider.saveTokens({ ...tokens, issuer }, infoCtx); return 'AUTHORIZED'; } - const tokens = await provider.tokens(); + // SEP-2352: a refresh_token stamped for a different authorization server reads back + // as `undefined`, so it is never POSTed to this AS's token endpoint. + let tokens = discardIfIssuerMismatch(await provider.tokens(infoCtx), issuer); + if (tokens && tokens.issuer === undefined) { + // SEP-2352 back-stamp: bind a legacy unstamped token set to the first-resolved AS + // so the stamp check is effective from the next call onward. + tokens = { ...tokens, issuer }; + await provider.saveTokens(tokens, infoCtx); + } // Handle token refresh or new authorization. The step-up path sets // `forceReauthorization` when the requested scope strictly exceeds the @@ -1148,7 +1294,7 @@ async function authInternal( fetchFn }); - await provider.saveTokens(newTokens); + await provider.saveTokens({ ...newTokens, issuer }, infoCtx); return 'AUTHORIZED'; } catch (error) { // A non-TLS token endpoint is a configuration error — re-authorizing cannot @@ -2134,7 +2280,7 @@ export async function fetchToken( tokenRequestParams = prepareAuthorizationCodeRequest(authorizationCode, codeVerifier, provider.redirectUrl); } - const clientInformation = await provider.clientInformation(); + const clientInformation = await provider.clientInformation({ issuer: metadata?.issuer ?? String(authorizationServerUrl) }); return executeTokenRequest(authorizationServerUrl, { metadata, diff --git a/packages/client/src/client/authErrors.ts b/packages/client/src/client/authErrors.ts index 41acb2cbc..4ced2d6f0 100644 --- a/packages/client/src/client/authErrors.ts +++ b/packages/client/src/client/authErrors.ts @@ -134,6 +134,31 @@ export class InsecureTokenEndpointError extends OAuthClientFlowError { * treat them as untrusted input when displaying or logging (this includes * `requiredScope`, which appears in the error message). */ +/** + * Thrown by `auth()` on the authorization-code callback leg when the + * authorization server resolved by discovery differs from the one recorded in + * `discoveryState()` at redirect time. The `authorization_code` and PKCE + * `code_verifier` are bound to the AS that minted the code (RFC 7636); sending + * them to a different AS's token endpoint is a credential-exfiltration vector. + * + * This is the only runtime check left in the SEP-2352 model — stored tokens and + * client credentials are protected structurally by the `issuer` stamp instead. + */ +export class AuthorizationServerMismatchError extends OAuthClientFlowError { + constructor( + /** The issuer recorded in `discoveryState()` when the authorization redirect was issued. */ + public readonly recordedIssuer: string, + /** The issuer resolved by discovery on this call. */ + public readonly currentIssuer: string + ) { + super( + `Authorization server changed between redirect and callback ` + + `(redirected to ${JSON.stringify(recordedIssuer)}, callback resolved ${JSON.stringify(currentIssuer)}); ` + + `refusing to send authorization_code/code_verifier to a different token endpoint` + ); + } +} + export class InsufficientScopeError extends OAuthClientFlowError { /** The `scope` value from the `WWW-Authenticate` challenge — the scopes the resource server says are required. */ readonly requiredScope?: string; diff --git a/packages/client/src/client/authExtensions.ts b/packages/client/src/client/authExtensions.ts index cb476c12f..74cec964a 100644 --- a/packages/client/src/client/authExtensions.ts +++ b/packages/client/src/client/authExtensions.ts @@ -5,7 +5,7 @@ * for common machine-to-machine authentication scenarios. */ -import type { FetchLike, OAuthClientInformation, OAuthClientMetadata, OAuthTokens } from '@modelcontextprotocol/core'; +import type { FetchLike, OAuthClientMetadata, StoredOAuthClientInformation, StoredOAuthTokens } from '@modelcontextprotocol/core'; import type { CryptoKey, JWK } from 'jose'; import type { AddClientAuthentication, OAuthClientProvider } from './auth.js'; @@ -117,6 +117,15 @@ export interface ClientCredentialsProviderOptions { * Space-separated scopes values requested by the client. */ scope?: string; + + /** + * The authorization server's `issuer` identifier these credentials were registered with. + * Stamped onto the stored client information so `auth()`'s SEP-2352 issuer check + * refuses to send the credential to any other authorization server. Hosts supplying + * static client credentials SHOULD set this; omitting it preserves the legacy + * (no-binding) behaviour for back-compat. + */ + expectedIssuer?: string; } /** @@ -138,14 +147,15 @@ export interface ClientCredentialsProviderOptions { * ``` */ export class ClientCredentialsProvider implements OAuthClientProvider { - private _tokens?: OAuthTokens; - private _clientInfo: OAuthClientInformation; + private _tokens?: StoredOAuthTokens; + private _clientInfo: StoredOAuthClientInformation; private _clientMetadata: OAuthClientMetadata; constructor(options: ClientCredentialsProviderOptions) { this._clientInfo = { client_id: options.clientId, - client_secret: options.clientSecret + client_secret: options.clientSecret, + issuer: options.expectedIssuer }; this._clientMetadata = { client_name: options.clientName ?? 'client-credentials-client', @@ -164,19 +174,20 @@ export class ClientCredentialsProvider implements OAuthClientProvider { return this._clientMetadata; } - clientInformation(): OAuthClientInformation { + clientInformation(): StoredOAuthClientInformation { return this._clientInfo; } - saveClientInformation(info: OAuthClientInformation): void { - this._clientInfo = info; - } + // No saveClientInformation: credentials are constructor-supplied and bound to a single + // authorization server. When `expectedIssuer` is set and the resolved AS differs, the + // SEP-2352 stamp check discards `clientInformation()` and auth() throws + // AuthorizationServerMismatchError(expectedIssuer, resolved) rather than sending the credential. - tokens(): OAuthTokens | undefined { + tokens(): StoredOAuthTokens | undefined { return this._tokens; } - saveTokens(tokens: OAuthTokens): void { + saveTokens(tokens: StoredOAuthTokens): void { this._tokens = tokens; } @@ -243,6 +254,12 @@ export interface PrivateKeyJwtProviderOptions { * with finer granularity than what scopes alone allow. */ claims?: Record; + + /** + * The authorization server's `issuer` identifier these credentials were registered with. + * Seeds the SEP-2352 issuer stamp — see {@linkcode ClientCredentialsProviderOptions.expectedIssuer}. + */ + expectedIssuer?: string; } /** @@ -266,14 +283,15 @@ export interface PrivateKeyJwtProviderOptions { * ``` */ export class PrivateKeyJwtProvider implements OAuthClientProvider { - private _tokens?: OAuthTokens; - private _clientInfo: OAuthClientInformation; + private _tokens?: StoredOAuthTokens; + private _clientInfo: StoredOAuthClientInformation; private _clientMetadata: OAuthClientMetadata; addClientAuthentication: AddClientAuthentication; constructor(options: PrivateKeyJwtProviderOptions) { this._clientInfo = { - client_id: options.clientId + client_id: options.clientId, + issuer: options.expectedIssuer }; this._clientMetadata = { client_name: options.clientName ?? 'private-key-jwt-client', @@ -300,19 +318,19 @@ export class PrivateKeyJwtProvider implements OAuthClientProvider { return this._clientMetadata; } - clientInformation(): OAuthClientInformation { + clientInformation(): StoredOAuthClientInformation { return this._clientInfo; } - saveClientInformation(info: OAuthClientInformation): void { - this._clientInfo = info; - } + // No saveClientInformation: credentials are constructor-supplied; the SEP-2352 stamp + // check enforces `expectedIssuer` and auth() throws + // AuthorizationServerMismatchError(expectedIssuer, resolved) on mismatch. - tokens(): OAuthTokens | undefined { + tokens(): StoredOAuthTokens | undefined { return this._tokens; } - saveTokens(tokens: OAuthTokens): void { + saveTokens(tokens: StoredOAuthTokens): void { this._tokens = tokens; } @@ -361,6 +379,12 @@ export interface StaticPrivateKeyJwtProviderOptions { * Space-separated scopes values requested by the client. */ scope?: string; + + /** + * The authorization server's `issuer` identifier this assertion was minted for. + * Seeds the SEP-2352 issuer stamp — see {@linkcode ClientCredentialsProviderOptions.expectedIssuer}. + */ + expectedIssuer?: string; } /** @@ -371,14 +395,15 @@ export interface StaticPrivateKeyJwtProviderOptions { * uses it directly for authentication. */ export class StaticPrivateKeyJwtProvider implements OAuthClientProvider { - private _tokens?: OAuthTokens; - private _clientInfo: OAuthClientInformation; + private _tokens?: StoredOAuthTokens; + private _clientInfo: StoredOAuthClientInformation; private _clientMetadata: OAuthClientMetadata; addClientAuthentication: AddClientAuthentication; constructor(options: StaticPrivateKeyJwtProviderOptions) { this._clientInfo = { - client_id: options.clientId + client_id: options.clientId, + issuer: options.expectedIssuer }; this._clientMetadata = { client_name: options.clientName ?? 'static-private-key-jwt-client', @@ -403,19 +428,19 @@ export class StaticPrivateKeyJwtProvider implements OAuthClientProvider { return this._clientMetadata; } - clientInformation(): OAuthClientInformation { + clientInformation(): StoredOAuthClientInformation { return this._clientInfo; } - saveClientInformation(info: OAuthClientInformation): void { - this._clientInfo = info; - } + // No saveClientInformation: credentials are constructor-supplied; the SEP-2352 stamp + // check enforces `expectedIssuer` and auth() throws + // AuthorizationServerMismatchError(expectedIssuer, resolved) on mismatch. - tokens(): OAuthTokens | undefined { + tokens(): StoredOAuthTokens | undefined { return this._tokens; } - saveTokens(tokens: OAuthTokens): void { + saveTokens(tokens: StoredOAuthTokens): void { this._tokens = tokens; } @@ -527,6 +552,12 @@ export interface CrossAppAccessProviderOptions { * Custom fetch implementation. Defaults to global fetch. */ fetchFn?: FetchLike; + + /** + * The MCP authorization server's `issuer` identifier these credentials were registered with. + * Seeds the SEP-2352 issuer stamp — see {@linkcode ClientCredentialsProviderOptions.expectedIssuer}. + */ + expectedIssuer?: string; } /** @@ -569,8 +600,8 @@ export interface CrossAppAccessProviderOptions { * ``` */ export class CrossAppAccessProvider implements OAuthClientProvider { - private _tokens?: OAuthTokens; - private _clientInfo: OAuthClientInformation; + private _tokens?: StoredOAuthTokens; + private _clientInfo: StoredOAuthClientInformation; private _clientMetadata: OAuthClientMetadata; private _assertionCallback: AssertionCallback; private _fetchFn: FetchLike; @@ -581,7 +612,8 @@ export class CrossAppAccessProvider implements OAuthClientProvider { constructor(options: CrossAppAccessProviderOptions) { this._clientInfo = { client_id: options.clientId, - client_secret: options.clientSecret + client_secret: options.clientSecret, + issuer: options.expectedIssuer }; this._clientMetadata = { client_name: options.clientName ?? 'cross-app-access-client', @@ -601,19 +633,19 @@ export class CrossAppAccessProvider implements OAuthClientProvider { return this._clientMetadata; } - clientInformation(): OAuthClientInformation { + clientInformation(): StoredOAuthClientInformation { return this._clientInfo; } - saveClientInformation(info: OAuthClientInformation): void { - this._clientInfo = info; - } + // No saveClientInformation: credentials are constructor-supplied; the SEP-2352 stamp + // check enforces `expectedIssuer` and auth() throws + // AuthorizationServerMismatchError(expectedIssuer, resolved) on mismatch. - tokens(): OAuthTokens | undefined { + tokens(): StoredOAuthTokens | undefined { return this._tokens; } - saveTokens(tokens: OAuthTokens): void { + saveTokens(tokens: StoredOAuthTokens): void { this._tokens = tokens; } @@ -633,14 +665,14 @@ export class CrossAppAccessProvider implements OAuthClientProvider { * Saves the authorization server URL discovered during OAuth flow. * This is called by the auth() function after RFC 9728 discovery. */ - saveAuthorizationServerUrl?(authorizationServerUrl: string): void { + saveAuthorizationServerUrl(authorizationServerUrl: string): void { this._authorizationServerUrl = authorizationServerUrl; } /** * Returns the cached authorization server URL if available. */ - authorizationServerUrl?(): string | undefined { + authorizationServerUrl(): string | undefined { return this._authorizationServerUrl; } diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 3160cb282..1c5148d53 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -22,6 +22,7 @@ export { auth, buildDiscoveryUrls, computeScopeUnion, + discardIfIssuerMismatch, discoverAuthorizationServerMetadata, discoverOAuthMetadata, discoverOAuthProtectedResourceMetadata, @@ -45,6 +46,7 @@ export { validateClientMetadataUrl } from './client/auth.js'; export { + AuthorizationServerMismatchError, InsecureTokenEndpointError, InsufficientScopeError, IssuerMismatchError, diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index d92724f51..5a65a7671 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -1,4 +1,11 @@ -import type { AuthorizationServerMetadata, OAuthClientMetadata, OAuthTokens } from '@modelcontextprotocol/core'; +import type { + AuthorizationServerMetadata, + OAuthClientInformationMixed, + OAuthClientMetadata, + OAuthTokens, + StoredOAuthClientInformation, + StoredOAuthTokens +} from '@modelcontextprotocol/core'; import { LATEST_PROTOCOL_VERSION, OAuthError, OAuthErrorCode } from '@modelcontextprotocol/core'; import type { Mock } from 'vitest'; import { expect, vi } from 'vitest'; @@ -7,9 +14,11 @@ import type { OAuthClientProvider } from '../../src/client/auth.js'; import { assertSecureTokenEndpoint, auth, + AuthorizationServerMismatchError, buildDiscoveryUrls, computeScopeUnion, determineScope, + discardIfIssuerMismatch, discoverAuthorizationServerMetadata, discoverOAuthMetadata, discoverOAuthProtectedResourceMetadata, @@ -31,7 +40,8 @@ import { validateAuthorizationResponseIssuer, validateClientMetadataUrl } from '../../src/client/auth.js'; -import { createPrivateKeyJwtAuth } from '../../src/client/authExtensions.js'; +import type { OAuthClientInformationContext, OAuthDiscoveryState } from '../../src/client/auth.js'; +import { ClientCredentialsProvider, createPrivateKeyJwtAuth } from '../../src/client/authExtensions.js'; // Mock pkce-challenge vi.mock('pkce-challenge', () => ({ @@ -4040,10 +4050,11 @@ describe('OAuth Authorization', () => { serverUrl: 'https://server.example.com' }); - // Should save URL-based client info - expect(mockProvider.saveClientInformation).toHaveBeenCalledWith({ - client_id: 'https://example.com/client-metadata.json' - }); + // Should save URL-based client info (stamped with the resolved issuer + ctx) + expect(mockProvider.saveClientInformation).toHaveBeenCalledWith( + { client_id: 'https://example.com/client-metadata.json', issuer: 'https://server.example.com' }, + { issuer: 'https://server.example.com' } + ); }); it('falls back to DCR when server does not support URL-based client IDs', async () => { @@ -4085,11 +4096,15 @@ describe('OAuth Authorization', () => { }); // Should save DCR client info - expect(mockProvider.saveClientInformation).toHaveBeenCalledWith({ - client_id: 'generated-uuid', - client_secret: 'generated-secret', - redirect_uris: ['http://localhost:3000/callback'] - }); + expect(mockProvider.saveClientInformation).toHaveBeenCalledWith( + { + client_id: 'generated-uuid', + client_secret: 'generated-secret', + redirect_uris: ['http://localhost:3000/callback'], + issuer: 'https://server.example.com' + }, + { issuer: 'https://server.example.com' } + ); }); it('throws an error when clientMetadataUrl is not an HTTPS URL', async () => { @@ -4241,11 +4256,15 @@ describe('OAuth Authorization', () => { }); // Should fall back to DCR - expect(mockProvider.saveClientInformation).toHaveBeenCalledWith({ - client_id: 'generated-uuid', - client_secret: 'generated-secret', - redirect_uris: ['http://localhost:3000/callback'] - }); + expect(mockProvider.saveClientInformation).toHaveBeenCalledWith( + { + client_id: 'generated-uuid', + client_secret: 'generated-secret', + redirect_uris: ['http://localhost:3000/callback'], + issuer: 'https://server.example.com' + }, + { issuer: 'https://server.example.com' } + ); }); }); @@ -4663,4 +4682,357 @@ describe('OAuth Authorization', () => { }); }); }); + + describe('SEP-2352: per-authorization-server credential isolation (issuer-stamped)', () => { + const AS_ONE = 'https://as-one.example.com'; + const AS_TWO = 'https://as-two.example.com'; + + const asMetadata = (issuer: string): AuthorizationServerMetadata => ({ + issuer, + authorization_endpoint: `${issuer}/authorize`, + token_endpoint: `${issuer}/token`, + registration_endpoint: `${issuer}/register`, + response_types_supported: ['code'], + code_challenge_methods_supported: ['S256'], + grant_types_supported: ['authorization_code', 'refresh_token', 'client_credentials'] + }); + + function createMigratingFetch() { + let active = AS_ONE; + const registerCalls: string[] = []; + const tokenCalls: Array<{ issuer: string; body: URLSearchParams }> = []; + const fetchFn = async (url: string | URL, init?: RequestInit): Promise => { + const u = new URL(String(url)); + if (u.pathname.includes('/.well-known/oauth-protected-resource')) { + return Response.json({ resource: 'https://api.example.com/mcp', authorization_servers: [active] }); + } + if (u.pathname.includes('/.well-known/')) { + return Response.json(asMetadata(u.origin)); + } + if (u.pathname === '/register') { + registerCalls.push(u.origin); + return Response.json({ client_id: `cid-${u.host}`, client_secret: 's', redirect_uris: [] }, { status: 201 }); + } + if (u.pathname === '/token') { + const body = new URLSearchParams(String(init?.body)); + tokenCalls.push({ issuer: u.origin, body }); + return Response.json({ access_token: 'at', token_type: 'Bearer' }); + } + return new Response(null, { status: 404 }); + }; + return { fetchFn, registerCalls, tokenCalls, switchTo: (i: string) => (active = i) }; + } + + /** Single-slot blob provider — round-trips the SDK-stamped values verbatim. */ + function createBlobProvider(withDiscoveryState = true): OAuthClientProvider & { + redirected: URL[]; + stored: { info?: StoredOAuthClientInformation; tokens?: StoredOAuthTokens }; + } { + const stored: { info?: StoredOAuthClientInformation; tokens?: StoredOAuthTokens } = {}; + const redirected: URL[] = []; + let discovery: OAuthDiscoveryState | undefined; + let verifier: string | undefined; + return { + redirected, + stored, + get redirectUrl() { + return 'http://localhost:3000/callback'; + }, + get clientMetadata() { + return { client_name: 't', redirect_uris: ['http://localhost:3000/callback'] }; + }, + clientInformation: () => stored.info, + saveClientInformation: i => void (stored.info = i), + tokens: () => stored.tokens, + saveTokens: t => void (stored.tokens = t), + redirectToAuthorization: u => void redirected.push(u), + saveCodeVerifier: v => void (verifier = v), + codeVerifier: () => verifier ?? 'v', + ...(withDiscoveryState && { + saveDiscoveryState: (s: OAuthDiscoveryState) => void (discovery = s), + discoveryState: () => discovery, + invalidateCredentials: (s: string) => { + if (s === 'client' || s === 'all') stored.info = undefined; + if (s === 'tokens' || s === 'all') stored.tokens = undefined; + if (s === 'discovery' || s === 'all') discovery = undefined; + } + }) + }; + } + + it('discardIfIssuerMismatch: returns undefined only on a different stamp; warns on unstamped', () => { + const stamped: StoredOAuthTokens = { access_token: 'a', token_type: 'Bearer', issuer: AS_ONE }; + const unstamped: StoredOAuthTokens = { access_token: 'a', token_type: 'Bearer' }; + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(discardIfIssuerMismatch(stamped, AS_ONE)).toBe(stamped); + expect(discardIfIssuerMismatch(stamped, AS_TWO)).toBeUndefined(); + expect(discardIfIssuerMismatch(unstamped, AS_TWO)).toBe(unstamped); + expect(discardIfIssuerMismatch(undefined, AS_TWO)).toBeUndefined(); + expect(warn).toHaveBeenCalledTimes(1); + warn.mockRestore(); + }); + + it('clientInformation stamped for AS-one is discarded at AS-two → re-registers (single-slot blob provider)', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + + expect(await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn })).toBe('REDIRECT'); + expect(provider.stored.info?.issuer).toBe(AS_ONE); + expect(srv.registerCalls).toEqual([AS_ONE]); + + srv.switchTo(AS_TWO); + provider.invalidateCredentials?.('discovery'); + expect(await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn })).toBe('REDIRECT'); + expect(srv.registerCalls).toEqual([AS_ONE, AS_TWO]); + expect(provider.stored.info?.issuer).toBe(AS_TWO); + expect(provider.redirected.at(-1)?.origin).toBe(AS_TWO); + expect(provider.redirected.at(-1)?.searchParams.get('client_id')).toBe('cid-as-two.example.com'); + }); + + it('refresh_token stamped for AS-one is never POSTed to AS-two', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'cid', issuer: AS_TWO }; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-one', issuer: AS_ONE }; + srv.switchTo(AS_TWO); + + expect(await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn })).toBe('REDIRECT'); + for (const { issuer, body } of srv.tokenCalls) { + expect(issuer).not.toBe(AS_TWO); + expect(body.get('refresh_token')).not.toBe('rt-one'); + } + }); + + it('issuer-keyed provider holds independent credentials per AS', async () => { + const srv = createMigratingFetch(); + const map = new Map(); + const provider: OAuthClientProvider = { + get redirectUrl() { + return 'http://localhost:3000/callback'; + }, + get clientMetadata() { + return { client_name: 't', redirect_uris: ['http://localhost:3000/callback'] }; + }, + clientInformation: (ctx?: OAuthClientInformationContext) => (ctx ? map.get(ctx.issuer) : undefined), + saveClientInformation: (i, ctx) => void (ctx && map.set(ctx.issuer, i)), + tokens: () => undefined, + saveTokens: () => {}, + redirectToAuthorization: () => {}, + saveCodeVerifier: () => {}, + codeVerifier: () => 'v' + }; + + await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn }); + srv.switchTo(AS_TWO); + await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn }); + expect(map.get(AS_ONE)?.client_id).toBe('cid-as-one.example.com'); + expect(map.get(AS_TWO)?.client_id).toBe('cid-as-two.example.com'); + }); + + it('callback-leg gate throws when discoveryState issuer differs from resolved issuer', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'cid', issuer: AS_ONE }; + // Recorded redirect target = AS-one, but cached state lacks an authorizationServerUrl + // so authInternal runs fresh discovery → AS-two. + provider.saveDiscoveryState?.({ + authorizationServerUrl: '', + authorizationServerMetadata: asMetadata(AS_ONE) + } as OAuthDiscoveryState); + srv.switchTo(AS_TWO); + + await expect( + auth(provider, { serverUrl: 'https://api.example.com/mcp', authorizationCode: 'code', fetchFn: srv.fetchFn }) + ).rejects.toBeInstanceOf(AuthorizationServerMismatchError); + }); + + it('callback-leg gate fails closed when provider implements saveDiscoveryState but discoveryState() is undefined', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'cid', issuer: AS_ONE }; + // Provider implements saveDiscoveryState/discoveryState, but the recorded state was + // lost (e.g. fresh process / page navigation between redirect and callback). The + // gate must fail closed rather than silently re-discover. + // (createBlobProvider starts with discoveryState() → undefined.) + const err = await auth(provider, { + serverUrl: 'https://api.example.com/mcp', + authorizationCode: 'code', + fetchFn: srv.fetchFn + }).then( + () => undefined, + e => e + ); + expect(err).toBeInstanceOf(AuthorizationServerMismatchError); + expect((err as AuthorizationServerMismatchError).recordedIssuer).toContain( + 'discoveryState was not available on the callback leg' + ); + expect(srv.tokenCalls).toHaveLength(0); + }); + + it('warns once on the callback leg when the provider has no discoveryState', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(false); + provider.stored.info = { client_id: 'cid', issuer: AS_ONE }; + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await auth(provider, { + serverUrl: 'https://api.example.com/mcp', + authorizationCode: 'code', + iss: AS_ONE, + fetchFn: srv.fetchFn + }); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0]?.[0]).toContain('saveDiscoveryState'); + warn.mockRestore(); + }); + + it('back-stamps a legacy unstamped clientInformation on first use after upgrade', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + // Pre-SEP-2352 storage: no `issuer` field on the stored blob. + provider.stored.info = { client_id: 'legacy-cid', client_secret: 'legacy-secret' }; + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + expect(await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn })).toBe('REDIRECT'); + // First use binds the unstamped value to the resolved AS — closes the permanent window. + expect(provider.stored.info).toEqual({ client_id: 'legacy-cid', client_secret: 'legacy-secret', issuer: AS_ONE }); + // The legacy value was used, not re-registered. + expect(srv.registerCalls).toHaveLength(0); + expect(warn.mock.calls.some(c => String(c[0]).includes("no 'issuer' stamp"))).toBe(true); + warn.mockRestore(); + + // Subsequent call against AS-two now sees a stamped value and re-registers. + srv.switchTo(AS_TWO); + provider.invalidateCredentials?.('discovery'); + await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn }); + expect(srv.registerCalls).toEqual([AS_TWO]); + }); + + it('back-stamps a legacy unstamped token set on first use', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'cid', issuer: AS_ONE }; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-legacy' }; + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn }); + // The unstamped token set is written back with the resolved issuer before refresh. + expect(provider.stored.tokens?.issuer).toBe(AS_ONE); + warn.mockRestore(); + }); + + it('callback-leg gate: saveDiscoveryState is NOT called when AuthorizationServerMismatchError throws', async () => { + // Case 1: cachedState undefined → fail-closed '(none recorded)' → fresh discovery + // result must NOT have been persisted (a retry would otherwise read it back as + // recordedIssuer and the gate would pass). + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + const saveSpy = vi.fn(provider.saveDiscoveryState!); + provider.saveDiscoveryState = saveSpy; + provider.stored.info = { client_id: 'cid', issuer: AS_ONE }; + + await expect( + auth(provider, { serverUrl: 'https://api.example.com/mcp', authorizationCode: 'code', fetchFn: srv.fetchFn }) + ).rejects.toBeInstanceOf(AuthorizationServerMismatchError); + expect(saveSpy).not.toHaveBeenCalled(); + expect(provider.discoveryState?.()).toBeUndefined(); + + // Case 2: cachedState records AS-one (forces full discovery via empty + // authorizationServerUrl), discovery resolves AS-two → throw → AS-one record is + // untouched. + const srv2 = createMigratingFetch(); + const provider2 = createBlobProvider(); + provider2.stored.info = { client_id: 'cid', issuer: AS_ONE }; + provider2.saveDiscoveryState?.({ + authorizationServerUrl: '', + authorizationServerMetadata: asMetadata(AS_ONE) + } as OAuthDiscoveryState); + srv2.switchTo(AS_TWO); + + await expect( + auth(provider2, { serverUrl: 'https://api.example.com/mcp', authorizationCode: 'code', fetchFn: srv2.fetchFn }) + ).rejects.toBeInstanceOf(AuthorizationServerMismatchError); + expect((provider2.discoveryState?.() as OAuthDiscoveryState).authorizationServerMetadata?.issuer).toBe(AS_ONE); + }); + + it('callback-leg gate: trailing-slash difference between recorded fallback URL and metadata issuer is tolerated', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'cid', issuer: AS_ONE }; + // Redirect leg recorded the SDK-derived String(URL) form (slash-suffixed) with no + // metadata; callback leg sees metadata.issuer (slash-free). Same AS — must not throw. + provider.saveDiscoveryState?.({ authorizationServerUrl: AS_ONE + '/' } as OAuthDiscoveryState); + + await expect( + auth(provider, { serverUrl: 'https://api.example.com/mcp', authorizationCode: 'code', iss: AS_ONE, fetchFn: srv.fetchFn }) + ).resolves.toBe('AUTHORIZED'); + }); + + it('discardIfIssuerMismatch: trailing-slash difference does not discard', () => { + const stamped = { client_id: 'x', issuer: 'https://as.example.com' }; + expect(discardIfIssuerMismatch(stamped, 'https://as.example.com/')).toBe(stamped); + expect(discardIfIssuerMismatch({ client_id: 'x', issuer: 'https://as.example.com/' }, 'https://as.example.com')).toBeDefined(); + }); + + it('invalid_client on code exchange does not surface AuthorizationServerMismatchError', async () => { + const base = createMigratingFetch(); + const fetchFn = async (url: string | URL, init?: RequestInit): Promise => { + if (new URL(String(url)).pathname === '/token') { + return Response.json({ error: 'invalid_client' }, { status: 400 }); + } + return base.fetchFn(url, init); + }; + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'cid', issuer: AS_ONE }; + provider.saveDiscoveryState?.({ + authorizationServerUrl: AS_ONE, + authorizationServerMetadata: asMetadata(AS_ONE) + } as OAuthDiscoveryState); + + const err = await auth(provider, { + serverUrl: 'https://api.example.com/mcp', + authorizationCode: 'code', + iss: AS_ONE, + fetchFn + }).then( + () => undefined, + e => e + ); + // The retry surfaces the (comprehensible) missing-client-information error, not a + // false '(none recorded)' AS-change. + expect(err).not.toBeInstanceOf(AuthorizationServerMismatchError); + expect((provider.discoveryState?.() as OAuthDiscoveryState).authorizationServerUrl).toBe(AS_ONE); + }); + + it('ClientCredentialsProvider without expectedIssuer: no SEP-2352 warn on auth()', async () => { + const srv = createMigratingFetch(); + const provider = new ClientCredentialsProvider({ clientId: 'static', clientSecret: 's' }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn }); + await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn }); + expect(warn.mock.calls.filter(c => /no 'issuer' stamp/.test(String(c[0])))).toHaveLength(0); + warn.mockRestore(); + }); + + it('m2m expectedIssuer: ClientCredentialsProvider refuses to send the credential to a different AS', async () => { + const srv = createMigratingFetch(); + srv.switchTo(AS_TWO); + const provider = new ClientCredentialsProvider({ clientId: 'static', clientSecret: 's', expectedIssuer: AS_ONE }); + + const err = await auth(provider, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn }).then( + () => undefined, + e => e + ); + expect(err).toBeInstanceOf(AuthorizationServerMismatchError); + expect((err as AuthorizationServerMismatchError).recordedIssuer).toBe(AS_ONE); + expect((err as AuthorizationServerMismatchError).currentIssuer).toBe(AS_TWO); + expect(srv.tokenCalls.filter(c => c.issuer === AS_TWO)).toHaveLength(0); + + // Matching expectedIssuer proceeds. + srv.switchTo(AS_ONE); + const ok = new ClientCredentialsProvider({ clientId: 'static', clientSecret: 's', expectedIssuer: AS_ONE }); + expect(await auth(ok, { serverUrl: 'https://api.example.com/mcp', fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + }); + }); }); diff --git a/packages/client/test/client/sse.test.ts b/packages/client/test/client/sse.test.ts index 4d7b1c36b..1be1f03c7 100644 --- a/packages/client/test/client/sse.test.ts +++ b/packages/client/test/client/sse.test.ts @@ -780,11 +780,14 @@ describe('SSEClientTransport', () => { await transport.start(); - expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith({ - access_token: 'new-token', - token_type: 'Bearer', - refresh_token: 'new-refresh-token' - }); + expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith( + expect.objectContaining({ + access_token: 'new-token', + token_type: 'Bearer', + refresh_token: 'new-refresh-token' + }), + expect.anything() + ); expect(connectionAttempts).toBe(1); expect(lastServerRequest.headers.authorization).toBe('Bearer new-token'); }); @@ -933,11 +936,14 @@ describe('SSEClientTransport', () => { await transport.send(message); - expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith({ - access_token: 'new-token', - token_type: 'Bearer', - refresh_token: 'new-refresh-token' - }); + expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith( + expect.objectContaining({ + access_token: 'new-token', + token_type: 'Bearer', + refresh_token: 'new-refresh-token' + }), + expect.anything() + ); expect(postAttempts).toBe(1); expect(lastServerRequest.headers.authorization).toBe('Bearer new-token'); }); @@ -1020,7 +1026,7 @@ describe('SSEClientTransport', () => { expect(mockAuthProvider.redirectToAuthorization).toHaveBeenCalled(); }); - it('invalidates all credentials on OAuthErrorCode.InvalidClient during token refresh', async () => { + it('invalidates client+tokens (not discovery) on OAuthErrorCode.InvalidClient during token refresh', async () => { // Mock tokens() to return token with refresh token mockAuthProvider.tokens.mockResolvedValue({ access_token: 'expired-token', @@ -1069,10 +1075,14 @@ describe('SSEClientTransport', () => { }); await expect(() => transport.start()).rejects.toMatchObject(expectedError); - expect(mockAuthProvider.invalidateCredentials).toHaveBeenCalledWith('all'); + // SEP-2352: 'client'+'tokens' (not 'all') so discoveryState survives for the + // callback-leg gate on retry. + expect(mockAuthProvider.invalidateCredentials).toHaveBeenCalledWith('client'); + expect(mockAuthProvider.invalidateCredentials).toHaveBeenCalledWith('tokens'); + expect(mockAuthProvider.invalidateCredentials).not.toHaveBeenCalledWith('all'); }); - it('invalidates all credentials on OAuthErrorCode.UnauthorizedClient during token refresh', async () => { + it('invalidates client+tokens (not discovery) on OAuthErrorCode.UnauthorizedClient during token refresh', async () => { // Mock tokens() to return token with refresh token mockAuthProvider.tokens.mockResolvedValue({ access_token: 'expired-token', @@ -1120,7 +1130,11 @@ describe('SSEClientTransport', () => { }); await expect(() => transport.start()).rejects.toMatchObject(expectedError); - expect(mockAuthProvider.invalidateCredentials).toHaveBeenCalledWith('all'); + // SEP-2352: 'client'+'tokens' (not 'all') so discoveryState survives for the + // callback-leg gate on retry. + expect(mockAuthProvider.invalidateCredentials).toHaveBeenCalledWith('client'); + expect(mockAuthProvider.invalidateCredentials).toHaveBeenCalledWith('tokens'); + expect(mockAuthProvider.invalidateCredentials).not.toHaveBeenCalledWith('all'); }); it('invalidates tokens on OAuthErrorCode.InvalidGrant during token refresh', async () => { @@ -1519,12 +1533,15 @@ describe('SSEClientTransport', () => { expect(tokenCalls.length).toBeGreaterThan(0); // Verify tokens were saved - expect(authProviderWithCode.saveTokens).toHaveBeenCalledWith({ - access_token: 'new-access-token', - token_type: 'Bearer', - expires_in: 3600, - refresh_token: 'new-refresh-token' - }); + expect(authProviderWithCode.saveTokens).toHaveBeenCalledWith( + expect.objectContaining({ + access_token: 'new-access-token', + token_type: 'Bearer', + expires_in: 3600, + refresh_token: 'new-refresh-token' + }), + expect.anything() + ); // Global fetch should never have been called expect(globalFetchSpy).not.toHaveBeenCalled(); diff --git a/packages/client/test/client/streamableHttp.test.ts b/packages/client/test/client/streamableHttp.test.ts index 8c99dc130..866ca52be 100644 --- a/packages/client/test/client/streamableHttp.test.ts +++ b/packages/client/test/client/streamableHttp.test.ts @@ -2085,12 +2085,15 @@ describe('StreamableHTTPClientTransport', () => { expect(tokenCalls.length).toBeGreaterThan(0); // Verify tokens were saved - expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith({ - access_token: 'new-access-token', - token_type: 'Bearer', - expires_in: 3600, - refresh_token: 'new-refresh-token' - }); + expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith( + expect.objectContaining({ + access_token: 'new-access-token', + token_type: 'Bearer', + expires_in: 3600, + refresh_token: 'new-refresh-token' + }), + expect.anything() + ); // Global fetch should never have been called expect(globalThis.fetch).not.toHaveBeenCalled(); @@ -2364,12 +2367,15 @@ describe('StreamableHTTPClientTransport', () => { expect((error as SdkHttpError).code).toBe(SdkErrorCode.ClientHttpAuthentication); expect((error as SdkHttpError).status).toBe(401); expect((error as SdkHttpError).statusText).toBe('Unauthorized'); - expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith({ - access_token: 'new-access-token', - token_type: 'Bearer', - expires_in: 3600, - refresh_token: 'refresh-token' // Refresh token is preserved - }); + expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith( + expect.objectContaining({ + access_token: 'new-access-token', + token_type: 'Bearer', + expires_in: 3600, + refresh_token: 'refresh-token' // Refresh token is preserved + }), + expect.anything() + ); }); }); diff --git a/test/conformance/expected-failures.2026-07-28.yaml b/test/conformance/expected-failures.2026-07-28.yaml index 7095552f2..f32031559 100644 --- a/test/conformance/expected-failures.2026-07-28.yaml +++ b/test/conformance/expected-failures.2026-07-28.yaml @@ -25,21 +25,9 @@ # milestone. client: - # --- Auth scenarios cut short by the 2026 connection lifecycle --- - # The fixture's auth flow drives the 2025 stateful lifecycle; the - # 2026-mode mock rejects the MCP POST (-32001, missing - # MCP-Protocol-Version header) before the scope-escalation behaviour these - # scenarios measure, so no authorization requests are observed. Unblocks - # when the auth fixture flow speaks the 2026 per-request lifecycle. - - auth/scope-step-up - - auth/scope-retry-limit - # --- Same gaps as the 2025 baseline (fail identically when forced to 2026-07-28) --- # SEP-2106 (JSON Schema $ref handling): no fixture handler for the scenario yet. - json-schema-ref-no-deref - # SEP-2352 (authorization server migration): client does not re-register - # when PRM authorization_servers changes. - - auth/authorization-server-migration server: # --- Carried-forward scenarios (also run by the 2025 legs) --- diff --git a/test/conformance/expected-failures.yaml b/test/conformance/expected-failures.yaml index 4cad1170e..19f3da9bf 100644 --- a/test/conformance/expected-failures.yaml +++ b/test/conformance/expected-failures.yaml @@ -21,9 +21,6 @@ client: # --- Draft-spec scenarios (in `--suite draft`, also part of `--suite all`) --- # SEP-2106 (JSON Schema $ref handling): client still dereferences network $refs. - json-schema-ref-no-deref - # SEP-2352 (authorization server migration): client does not re-register when - # PRM authorization_servers changes. - - auth/authorization-server-migration # SEP-2468 RFC 9207 `iss` validation is now default-ON. The referee's # 2025-03-26 backcompat mock emits a callback `iss` with a `/oauth` path # component that does not match its own metadata `issuer` (conformance#359 diff --git a/test/conformance/src/everythingClient.ts b/test/conformance/src/everythingClient.ts index 99b1b9afa..9c87fd3e5 100644 --- a/test/conformance/src/everythingClient.ts +++ b/test/conformance/src/everythingClient.ts @@ -384,7 +384,7 @@ registerScenario('sep-2322-client-request-state', runMrtrClient); // ============================================================================ async function runAuthClient(serverUrl: string): Promise { - const client = new Client({ name: 'test-auth-client', version: '1.0.0' }, { capabilities: {} }); + const client = new Client({ name: 'test-auth-client', version: '1.0.0' }, { capabilities: {}, versionNegotiation: { mode: 'auto' } }); const oauthFetch = withOAuthRetry('test-auth-client', new URL(serverUrl), handle401, CIMD_CLIENT_METADATA_URL)(fetch); @@ -437,7 +437,11 @@ registerScenarios( 'auth/iss-wrong-issuer', 'auth/iss-unexpected', 'auth/iss-normalized', - 'auth/metadata-issuer-mismatch' + 'auth/metadata-issuer-mismatch', + // SEP-2352: PRM `authorization_servers` switches between calls; the client's + // issuer-stamped credential storage reads back as undefined at the new AS and + // re-registers there. + 'auth/authorization-server-migration' ], runAuthClient ); diff --git a/test/conformance/src/helpers/conformanceOAuthProvider.ts b/test/conformance/src/helpers/conformanceOAuthProvider.ts index b9f9b23af..457ed58a5 100644 --- a/test/conformance/src/helpers/conformanceOAuthProvider.ts +++ b/test/conformance/src/helpers/conformanceOAuthProvider.ts @@ -3,16 +3,21 @@ import type { OAuthClientInformationFull, OAuthClientMetadata, OAuthClientProvider, + OAuthDiscoveryState, OAuthTokens } from '@modelcontextprotocol/client'; export class ConformanceOAuthProvider implements OAuthClientProvider { + // Single-slot blob storage. The SDK stamps `issuer` onto saved values; round-tripping + // them unchanged means a credential issued by AS-A reads back as undefined at AS-B + // (SEP-2352) and the flow re-registers. private _clientInformation?: OAuthClientInformationFull; private _tokens?: OAuthTokens; private _codeVerifier?: string; private _authCode?: string; private _iss?: string; private _authCodePromise?: Promise; + private _discoveryState?: OAuthDiscoveryState; constructor( private readonly _redirectUrl: string | URL, @@ -48,6 +53,21 @@ export class ConformanceOAuthProvider implements OAuthClientProvider { this._tokens = tokens; } + saveDiscoveryState(state: OAuthDiscoveryState): void { + this._discoveryState = state; + } + + discoveryState(): OAuthDiscoveryState | undefined { + return this._discoveryState; + } + + invalidateCredentials(scope: 'all' | 'client' | 'tokens' | 'verifier' | 'discovery'): void { + if (scope === 'all' || scope === 'client') this._clientInformation = undefined; + if (scope === 'all' || scope === 'tokens') this._tokens = undefined; + if (scope === 'all' || scope === 'verifier') this._codeVerifier = undefined; + if (scope === 'all' || scope === 'discovery') this._discoveryState = undefined; + } + async redirectToAuthorization(authorizationUrl: URL): Promise { try { const response = await fetch(authorizationUrl.toString(), { diff --git a/test/conformance/src/helpers/withOAuthRetry.ts b/test/conformance/src/helpers/withOAuthRetry.ts index df435b5c4..1b2688376 100644 --- a/test/conformance/src/helpers/withOAuthRetry.ts +++ b/test/conformance/src/helpers/withOAuthRetry.ts @@ -22,6 +22,12 @@ export const handle401 = async ( // union degenerates to the challenged scope. const previousTokens = await provider.tokens(); const scope = response.status === 403 ? computeScopeUnion(previousTokens?.scope, challengedScope) : challengedScope; + // A 401 after we already held a token means it no longer authenticates the resource; + // drop cached discovery so auth() re-probes PRM and can detect an authorization-server + // migration (SEP-2352). 403 is a step-up at the same AS — keep the cache. + if (response.status === 401) { + provider.invalidateCredentials('discovery'); + } let result = await auth(provider, { serverUrl, resourceMetadataUrl, diff --git a/test/e2e/requirements.ts b/test/e2e/requirements.ts index 5e5d6ceec..91956d194 100644 --- a/test/e2e/requirements.ts +++ b/test/e2e/requirements.ts @@ -2209,6 +2209,46 @@ export const REQUIREMENTS: Record = { transports: ['streamableHttp'], note: 'Verify-only pin of behavior already correct at the v2 baseline. Runs as a single streamableHttp-labelled cell.' }, + 'client-auth:as-migration:reregister': { + addedInSpecVersion: '2026-07-28', + source: 'https://modelcontextprotocol.io/specification/draft/basic/authorization/client-registration#authorization-server-migration', + behavior: + "When the protected resource's authorization_servers list changes to a different issuer, auth() reads back the issuer-stamped client credential as undefined (key not found) and re-runs Dynamic Client Registration at the new authorization server.", + transports: ['streamableHttp'], + note: 'This exercises the HTTP hosting/auth layer and OAuth client; the matrix transport arg is ignored, so it runs as a single streamableHttp-labelled cell to avoid duplicate runs.' + }, + 'client-auth:as-migration:no-cred-reuse': { + addedInSpecVersion: '2026-07-28', + source: 'https://modelcontextprotocol.io/specification/draft/basic/authorization/client-registration#authorization-server-migration', + behavior: + 'A single-slot OAuthClientProvider that round-trips the SDK-stamped value is protected: the previous-AS client_id is never transmitted to any endpoint of the new authorization server because the issuer stamp reads back as undefined.', + transports: ['streamableHttp'], + note: 'This exercises the HTTP hosting/auth layer and OAuth client; the matrix transport arg is ignored, so it runs as a single streamableHttp-labelled cell to avoid duplicate runs.' + }, + 'client-auth:as-migration:no-token-reuse': { + addedInSpecVersion: '2026-07-28', + source: 'https://modelcontextprotocol.io/specification/draft/basic/authorization/client-registration#authorization-server-migration', + behavior: + "auth() never POSTs a refresh_token to a different authorization server's token endpoint: a token whose issuer stamp does not match the resolved AS reads back as undefined and the refresh branch is skipped.", + transports: ['streamableHttp'], + note: 'This exercises the HTTP hosting/auth layer and OAuth client; the matrix transport arg is ignored, so it runs as a single streamableHttp-labelled cell to avoid duplicate runs.' + }, + 'client-auth:as-migration:cimd-portable': { + addedInSpecVersion: '2026-07-28', + source: 'https://modelcontextprotocol.io/specification/draft/basic/authorization/client-registration#authorization-server-migration', + behavior: + 'CIMD (URL-based) client_ids are portable across authorization servers: when the issuer changes, auth() re-saves the same clientMetadataUrl as the client_id at the new AS without dynamic registration.', + transports: ['streamableHttp'], + note: 'This exercises the HTTP hosting/auth layer and OAuth client; the matrix transport arg is ignored, so it runs as a single streamableHttp-labelled cell to avoid duplicate runs.' + }, + 'client-auth:as-migration:m2m-expected-issuer': { + addedInSpecVersion: '2026-07-28', + source: 'https://modelcontextprotocol.io/specification/draft/basic/authorization/client-registration#authorization-server-migration', + behavior: + 'ClientCredentialsProvider (and the other m2m providers) constructed with expectedIssuer refuse to send the static credential to a different authorization server: the issuer-stamped clientInformation() is discarded and auth() fails before any token request.', + transports: ['streamableHttp'], + note: 'This exercises the HTTP hosting/auth layer and OAuth client; the matrix transport arg is ignored, so it runs as a single streamableHttp-labelled cell to avoid duplicate runs.' + }, // Client middleware (SDK) diff --git a/test/e2e/scenarios/client-auth.test.ts b/test/e2e/scenarios/client-auth.test.ts index b2fa498fa..b3e94a078 100644 --- a/test/e2e/scenarios/client-auth.test.ts +++ b/test/e2e/scenarios/client-auth.test.ts @@ -8,10 +8,11 @@ import { createHash, generateKeyPairSync, sign } from 'node:crypto'; -import type { AuthProvider, OAuthClientProvider } from '@modelcontextprotocol/client'; +import type { AuthProvider, OAuthClientProvider, OAuthDiscoveryState } from '@modelcontextprotocol/client'; import { applyMiddlewares, auth, + AuthorizationServerMismatchError, Client, ClientCredentialsProvider, computeScopeUnion, @@ -46,7 +47,9 @@ import type { OAuthClientInformationFull, OAuthClientInformationMixed, OAuthClientMetadata, - OAuthTokens + OAuthTokens, + StoredOAuthClientInformation, + StoredOAuthTokens } from '@modelcontextprotocol/server'; import { LATEST_PROTOCOL_VERSION, McpServer } from '@modelcontextprotocol/server'; import { importSPKI, jwtVerify } from 'jose'; @@ -1056,9 +1059,11 @@ verifies('client-auth:invalid-client-clears-all', async (_args: TestArgs) => { expect(as.tokenCalls).toHaveLength(1); expect(defined(as.tokenCalls[0], 'token call').body.get('grant_type')).toBe('refresh_token'); - // Everything is invalidated: tokens are gone and the stale client_id was discarded, - // forcing a fresh dynamic registration on the retry. - expect(provider.invalidatedCredentials).toContain('all'); + // Client + tokens are invalidated (NOT 'all', so discoveryState survives — SEP-2352): + // tokens are gone and the stale client_id was discarded, forcing a fresh dynamic + // registration on the retry. + expect(provider.invalidatedCredentials).toContain('client'); + expect(provider.invalidatedCredentials).toContain('tokens'); expect(provider.saved.tokens).toBeUndefined(); expect(as.registerCalls).toHaveLength(1); expect(provider.saved.clientInformation?.client_id).toBe('registered-client-id'); @@ -2618,3 +2623,228 @@ verifies('client-auth:scope:offline-access-gate', async (_args: TestArgs) => { const redirectWithout = defined(providerWithout.redirectedTo[0], 'authorization redirect URL'); expect((redirectWithout.searchParams.get('scope') ?? '').split(' ')).not.toContain('offline_access'); }); + +// --------------------------------------------------------------------------- +// SEP-2352 — per-authorization-server credential isolation. Stored tokens and +// client credentials carry an SDK-stamped `issuer`; a value stamped for a +// different AS reads back as undefined, so it is never reused on the wire. +// --------------------------------------------------------------------------- + +/** + * A protected resource whose `authorization_servers` PRM entry can be swapped + * between calls. Each issuer hosts its own DCR endpoint that mints a distinct + * `client_id`, so reuse of an old-AS `client_id` is observable on the wire. + */ +function createMigratingAuthorizationServer() { + const issuers = { one: 'https://as-one.example.com', two: 'https://as-two.example.com' } as const; + let active: keyof typeof issuers = 'one'; + const registerCalls: Array<{ issuer: string }> = []; + const clientIdsSeen: Array<{ issuer: string; clientId: string | null }> = []; + const tokenCalls: Array<{ issuer: string; body: URLSearchParams }> = []; + + const asMetadata = (issuer: string): AuthorizationServerMetadata => ({ + issuer, + authorization_endpoint: `${issuer}/authorize`, + token_endpoint: `${issuer}/token`, + registration_endpoint: `${issuer}/register`, + response_types_supported: ['code'], + code_challenge_methods_supported: ['S256'], + client_id_metadata_document_supported: true, + token_endpoint_auth_methods_supported: ['client_secret_basic', 'client_secret_post', 'none'], + grant_types_supported: ['authorization_code', 'refresh_token', 'client_credentials'] + }); + + const fetchFn = async (url: URL | string, init?: RequestInit): Promise => { + const u = typeof url === 'string' ? new URL(url) : url; + if (u.pathname.includes('/.well-known/oauth-protected-resource')) { + return Response.json({ resource: RESOURCE, authorization_servers: [issuers[active]] }); + } + if (u.pathname.includes('/.well-known/oauth-authorization-server') || u.pathname.includes('/.well-known/openid-configuration')) { + return Response.json(asMetadata(u.origin)); + } + if (u.pathname === '/register' && init?.method === 'POST') { + const body = z.record(z.string(), z.unknown()).parse(JSON.parse(String(init.body))); + registerCalls.push({ issuer: u.origin }); + return Response.json({ ...body, client_id: `cid-at-${u.host}`, client_secret: `secret-at-${u.host}` }, { status: 201 }); + } + if (u.pathname === '/authorize') { + clientIdsSeen.push({ issuer: u.origin, clientId: u.searchParams.get('client_id') }); + return new Response('Authorize', { status: 200 }); + } + if (u.pathname === '/token' && init?.method === 'POST') { + const body = new URLSearchParams(String(init.body)); + clientIdsSeen.push({ issuer: u.origin, clientId: body.get('client_id') }); + tokenCalls.push({ issuer: u.origin, body }); + return Response.json({ access_token: `tok-${u.host}`, token_type: 'Bearer' }); + } + return new Response('Not Found', { status: 404 }); + }; + + return { + issuers, + registerCalls, + clientIdsSeen, + tokenCalls, + fetchFn, + switchTo(which: keyof typeof issuers) { + active = which; + } + }; +} + +/** Single-slot blob provider — round-trips the SDK-stamped values verbatim. */ +class StampedBlobProvider implements OAuthClientProvider { + redirectedTo: URL[] = []; + info?: StoredOAuthClientInformation; + storedTokens?: StoredOAuthTokens; + discovery?: OAuthDiscoveryState; + private _verifier?: string; + + constructor(public readonly clientMetadataUrl?: string) {} + + get redirectUrl() { + return 'http://localhost:3000/callback'; + } + get clientMetadata() { + return { client_name: 'Test Client', redirect_uris: [this.redirectUrl] }; + } + clientInformation() { + return this.info; + } + saveClientInformation(i: OAuthClientInformationMixed) { + this.info = i; + } + tokens() { + return this.storedTokens; + } + saveTokens(t: OAuthTokens) { + this.storedTokens = t; + } + redirectToAuthorization(u: URL) { + this.redirectedTo.push(u); + } + saveCodeVerifier(v: string) { + this._verifier = v; + } + codeVerifier() { + if (!this._verifier) throw new Error('no verifier'); + return this._verifier; + } + discoveryState() { + return this.discovery; + } + saveDiscoveryState(s: OAuthDiscoveryState) { + this.discovery = s; + } + invalidateCredentials(scope: 'all' | 'client' | 'tokens' | 'verifier' | 'discovery') { + if (scope === 'all' || scope === 'discovery') this.discovery = undefined; + } +} + +verifies('client-auth:as-migration:reregister', async (_args: TestArgs) => { + const server = createMigratingAuthorizationServer(); + const provider = new StampedBlobProvider(); + + expect(await auth(provider, { serverUrl: MCP_URL, fetchFn: server.fetchFn })).toBe('REDIRECT'); + expect(server.registerCalls).toEqual([{ issuer: server.issuers.one }]); + expect(provider.info?.issuer).toBe(server.issuers.one); + + // PRM now lists AS-two. Drop cached discovery (as a host would on a fresh 401) so + // re-discovery picks up the new AS. + server.switchTo('two'); + provider.invalidateCredentials('discovery'); + expect(await auth(provider, { serverUrl: MCP_URL, fetchFn: server.fetchFn })).toBe('REDIRECT'); + + // Stamp mismatch → undefined → re-registers at AS-two, redirect carries the fresh client_id. + expect(server.registerCalls).toEqual([{ issuer: server.issuers.one }, { issuer: server.issuers.two }]); + const redirect = defined(provider.redirectedTo.at(-1), 'second redirect'); + expect(redirect.origin).toBe(server.issuers.two); + expect(redirect.searchParams.get('client_id')).toBe('cid-at-as-two.example.com'); +}); + +verifies('client-auth:as-migration:no-cred-reuse', async (_args: TestArgs) => { + const server = createMigratingAuthorizationServer(); + const provider = new StampedBlobProvider(); + + expect(await auth(provider, { serverUrl: MCP_URL, fetchFn: server.fetchFn })).toBe('REDIRECT'); + expect(provider.info?.client_id).toBe('cid-at-as-one.example.com'); + + server.switchTo('two'); + provider.invalidateCredentials('discovery'); + await auth(provider, { serverUrl: MCP_URL, fetchFn: server.fetchFn }); + + // Wire MUST: AS-two never received AS-one's client_id at any endpoint. + for (const seen of server.clientIdsSeen.filter(c => c.issuer === server.issuers.two)) { + expect(seen.clientId).not.toBe('cid-at-as-one.example.com'); + } + expect(provider.info?.client_id).toBe('cid-at-as-two.example.com'); + expect(provider.info?.issuer).toBe(server.issuers.two); +}); + +verifies('client-auth:as-migration:no-token-reuse', async (_args: TestArgs) => { + const server = createMigratingAuthorizationServer(); + const provider = new StampedBlobProvider(); + + // Completed flow against AS-one — provider holds an AS-one-stamped refresh_token. + expect(await auth(provider, { serverUrl: MCP_URL, fetchFn: server.fetchFn })).toBe('REDIRECT'); + expect(await auth(provider, { serverUrl: MCP_URL, fetchFn: server.fetchFn, authorizationCode: 'code-a' })).toBe('AUTHORIZED'); + provider.storedTokens = { ...defined(provider.storedTokens, 'tokens'), refresh_token: 'rt-one' }; + expect(provider.storedTokens.issuer).toBe(server.issuers.one); + + server.switchTo('two'); + provider.invalidateCredentials('discovery'); + expect(await auth(provider, { serverUrl: MCP_URL, fetchFn: server.fetchFn })).toBe('REDIRECT'); + + // Wire MUST: AS-two's /token never received a refresh_token grant or rt-one. + for (const { body } of server.tokenCalls.filter(c => c.issuer === server.issuers.two)) { + expect(body.get('grant_type')).not.toBe('refresh_token'); + expect(body.get('refresh_token')).not.toBe('rt-one'); + } +}); + +verifies('client-auth:as-migration:cimd-portable', async (_args: TestArgs) => { + const cimdUrl = 'https://client.example.com/.well-known/client-metadata.json'; + const server = createMigratingAuthorizationServer(); + const provider = new StampedBlobProvider(cimdUrl); + + expect(await auth(provider, { serverUrl: MCP_URL, fetchFn: server.fetchFn })).toBe('REDIRECT'); + expect(server.registerCalls).toHaveLength(0); + expect(provider.info?.client_id).toBe(cimdUrl); + + server.switchTo('two'); + provider.invalidateCredentials('discovery'); + expect(await auth(provider, { serverUrl: MCP_URL, fetchFn: server.fetchFn })).toBe('REDIRECT'); + + // No DCR; the same URL-based client_id is presented to the new AS (re-stamped). + expect(server.registerCalls).toHaveLength(0); + const redirect = defined(provider.redirectedTo.at(-1), 'second redirect'); + expect(redirect.origin).toBe(server.issuers.two); + expect(redirect.searchParams.get('client_id')).toBe(cimdUrl); + expect(provider.info?.issuer).toBe(server.issuers.two); +}); + +verifies('client-auth:as-migration:m2m-expected-issuer', async (_args: TestArgs) => { + const server = createMigratingAuthorizationServer(); + + // expectedIssuer = AS-one, but PRM points to AS-two → stamp mismatch → undefined → + // no saveClientInformation → AuthorizationServerMismatchError before any token request. + server.switchTo('two'); + const provider = new ClientCredentialsProvider({ + clientId: 'static-cid', + clientSecret: 'static-secret', + expectedIssuer: server.issuers.one + }); + await expect(auth(provider, { serverUrl: MCP_URL, fetchFn: server.fetchFn })).rejects.toThrow(AuthorizationServerMismatchError); + expect(server.tokenCalls.filter(c => c.issuer === server.issuers.two)).toHaveLength(0); + expect(server.clientIdsSeen.filter(c => c.issuer === server.issuers.two)).toHaveLength(0); + + // Matching expectedIssuer proceeds and stamps the saved tokens. + server.switchTo('one'); + const ok = new ClientCredentialsProvider({ + clientId: 'static-cid', + clientSecret: 'static-secret', + expectedIssuer: server.issuers.one + }); + expect(await auth(ok, { serverUrl: MCP_URL, fetchFn: server.fetchFn })).toBe('AUTHORIZED'); + expect(ok.tokens()?.issuer).toBe(server.issuers.one); +});