From 620ad0c2d6ff6e435a5fe5abef5b56c8dc4d7433 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Mon, 28 Sep 2026 14:22:32 +0000 Subject: [PATCH 1/4] [v1.x] Bind stored OAuth credentials to the authorization server that issued them v1.x port of the per-authorization-server credential binding that shipped in 2.0 (SEP-2352, #2286), plus the `expectedIssuer` option of the 2.x bundled providers. auth() now records the authorization server a credential was obtained from on everything it hands to saveClientInformation() / saveTokens(), as an `issuer` property. On read it ignores client information or tokens whose `issuer` names a different authorization server than the one resolved for the current call, so a DCR client registers again and an interactive session authorizes again there instead of presenting the earlier registration's secret or refresh token. fetchToken() throws, before preparing or sending anything, when the client information is bound to a different authorization server. OAuthTokensSchema and OAuthClientInformationSchema accept the optional `issuer`, so providers that read storage back through them keep it. auth() overwrites the property on every save, so a value in a registration or token response never becomes the stamp of what auth() stores. ClientCredentialsProvider, PrivateKeyJwtProvider and StaticPrivateKeyJwtProvider accept `expectedIssuer`, the issuer URL of the authorization server the credentials were registered with. Omitting it is deprecated: the constructor logs a console.warn and that call signature is marked @deprecated. An empty or null value throws at construction. Behaviour changes - After an MCP server advertises a different authorization server, DCR clients register again and interactive users authorize again rather than being refreshed silently (2.x already behaves this way). - auth() throws an Error naming both authorization servers, instead of proceeding, when the stored client information is bound to another authorization server and cannot be re-created by registering: no saveClientInformation(), an addClientAuthentication() implementation, or a non-interactive provider (no redirectUrl). The three bundled providers therefore stay with `expectedIssuer`, or without it with the first authorization server they are used with in the process. - Storage written by earlier versions has no `issuer`. It is used as-is and written back with the property on first use, so saveClientInformation() / saveTokens() can be called with values that were not newly issued. A failing saveClientInformation() at that point is ignored. For tokens this happens only when there is a refresh token, and logs a console.warn. A provider that never returns the property gets the write and the warning on every auth() call and is not bound. - Objects passed to saveClientInformation() / saveTokens(), and the OAuthTokens / OAuthClientInformation types, carry the optional `issuer`. A string `issuer` in a token or registration response is no longer stripped, so exchangeAuthorization(), refreshAuthorization(), registerClient() and ProxyOAuthServerProvider pass it through. Differences from 2.x - The binding key, and what `expectedIssuer` is compared with, is the authorization server URL used for discovery rather than metadata.issuer, because 1.x does not compare the metadata issuer with the URL it was fetched from (RFC 8414 section 3.3). Where the two agree, one trailing slash aside, values written here are readable by 2.x. - No `ctx` argument on provider methods, no AuthorizationServerMismatchError class, no callback-leg discoveryState check. - The 1.x bundled providers implement saveClientInformation(), hence the non-interactive clause and the first-use binding above; the 2.x ones are bound only by `expectedIssuer`. --- .../bind-oauth-credentials-to-issuer.md | 7 + docs/client.md | 5 + src/client/auth-extensions.ts | 72 +++- src/client/auth.ts | 108 ++++- .../client/simpleClientCredentials.ts | 14 +- src/shared/auth.ts | 14 +- test/client/auth-extensions.test.ts | 137 +++++- test/client/auth.test.ts | 399 +++++++++++++++++- test/client/sse.test.ts | 9 +- test/client/streamableHttp.test.ts | 6 +- test/e2e/scenarios/client-auth.test.ts | 10 +- 11 files changed, 745 insertions(+), 36 deletions(-) create mode 100644 .changeset/bind-oauth-credentials-to-issuer.md diff --git a/.changeset/bind-oauth-credentials-to-issuer.md b/.changeset/bind-oauth-credentials-to-issuer.md new file mode 100644 index 0000000000..7025217c6f --- /dev/null +++ b/.changeset/bind-oauth-credentials-to-issuer.md @@ -0,0 +1,7 @@ +--- +'@modelcontextprotocol/sdk': patch +--- + +Bind stored OAuth client credentials to the authorization server that issued them. `auth()` adds an `issuer` property to what it passes to `saveClientInformation()` / `saveTokens()` and does not reuse a stored value with a different authorization server: the client registers or +authorizes again, or `auth()` throws when the credentials cannot be re-created by registering. `OAuthTokensSchema` and `OAuthClientInformationSchema` accept the optional `issuer`. `ClientCredentialsProvider`, `PrivateKeyJwtProvider` and `StaticPrivateKeyJwtProvider` accept +`expectedIssuer`; omitting it is deprecated. diff --git a/docs/client.md b/docs/client.md index 8f1c653273..c1c4739ba3 100644 --- a/docs/client.md +++ b/docs/client.md @@ -46,6 +46,11 @@ For OAuth-secured MCP servers, the client `auth` module exposes: - `PrivateKeyJwtProvider` - `StaticPrivateKeyJwtProvider` +Pass `expectedIssuer` (the issuer URL of the authorization server the credentials were registered with) to each of these. `auth()` then only presents the credentials to that authorization server and throws if the MCP server advertises a different one. Omitting it is deprecated. + +If you implement `OAuthClientProvider` yourself, store the objects passed to `saveClientInformation()` and `saveTokens()` unchanged: they carry an `issuer` property naming the authorization server they came from, and `auth()` does not reuse them with a different one. If +`clientInformation()` returns pre-registered credentials, include `issuer` yourself. + Examples: - [`simpleOAuthClient.ts`](../src/examples/client/simpleOAuthClient.ts) diff --git a/src/client/auth-extensions.ts b/src/client/auth-extensions.ts index c90404e940..5d8ab82f19 100644 --- a/src/client/auth-extensions.ts +++ b/src/client/auth-extensions.ts @@ -90,6 +90,24 @@ export function createPrivateKeyJwtAuth(options: { }; } +/** + * The `issuer` stamp for constructor-supplied client information. Omitting `expectedIssuer` is + * deprecated: nothing else says which authorization server such credentials belong to. + */ +function checkedExpectedIssuer(expectedIssuer: string | undefined): string | undefined { + if (expectedIssuer === undefined) { + // eslint-disable-next-line no-console + console.warn( + '[mcp-sdk] Omitting `expectedIssuer` is deprecated. Without it, the MCP server decides which authorization ' + + "server receives this client's credentials; pass your authorization server's issuer URL as " + + '`expectedIssuer` so they are only sent there.' + ); + } else if (!expectedIssuer) { + throw new Error("expectedIssuer must be the authorization server's issuer URL"); + } + return expectedIssuer; +} + /** * Options for creating a ClientCredentialsProvider. */ @@ -113,6 +131,16 @@ export interface ClientCredentialsProviderOptions { * Space-separated scopes values requested by the client. */ scope?: string; + + /** + * The issuer URL of the authorization server these credentials were registered with. + * When set, `auth()` only presents them to that authorization server and throws if the + * MCP server advertises a different URL (one trailing slash aside). + * + * Omitting it is deprecated: the credentials then go to whichever authorization server + * the MCP server advertises first. + */ + expectedIssuer?: string; } /** @@ -124,7 +152,8 @@ export interface ClientCredentialsProviderOptions { * @example * const provider = new ClientCredentialsProvider({ * clientId: 'my-client', - * clientSecret: 'my-secret' + * clientSecret: 'my-secret', + * expectedIssuer: 'https://auth.example.com' * }); * * const transport = new StreamableHTTPClientTransport(serverUrl, { @@ -136,10 +165,14 @@ export class ClientCredentialsProvider implements OAuthClientProvider { private _clientInfo: OAuthClientInformation; private _clientMetadata: OAuthClientMetadata; + constructor(options: ClientCredentialsProviderOptions & { expectedIssuer: string }); + /** @deprecated Pass `expectedIssuer` so the credentials are only sent to that authorization server. */ + constructor(options: ClientCredentialsProviderOptions); constructor(options: ClientCredentialsProviderOptions) { this._clientInfo = { client_id: options.clientId, - client_secret: options.clientSecret + client_secret: options.clientSecret, + issuer: checkedExpectedIssuer(options.expectedIssuer) }; this._clientMetadata = { client_name: options.clientName ?? 'client-credentials-client', @@ -227,6 +260,16 @@ export interface PrivateKeyJwtProviderOptions { * Space-separated scopes values requested by the client. */ scope?: string; + + /** + * The issuer URL of the authorization server these credentials were registered with. + * When set, `auth()` only presents them to that authorization server and throws if the + * MCP server advertises a different URL (one trailing slash aside). + * + * Omitting it is deprecated: the credentials then go to whichever authorization server + * the MCP server advertises first. + */ + expectedIssuer?: string; } /** @@ -239,7 +282,8 @@ export interface PrivateKeyJwtProviderOptions { * const provider = new PrivateKeyJwtProvider({ * clientId: 'my-client', * privateKey: pemEncodedPrivateKey, - * algorithm: 'RS256' + * algorithm: 'RS256', + * expectedIssuer: 'https://auth.example.com' * }); * * const transport = new StreamableHTTPClientTransport(serverUrl, { @@ -252,9 +296,13 @@ export class PrivateKeyJwtProvider implements OAuthClientProvider { private _clientMetadata: OAuthClientMetadata; addClientAuthentication: AddClientAuthentication; + constructor(options: PrivateKeyJwtProviderOptions & { expectedIssuer: string }); + /** @deprecated Pass `expectedIssuer` so the credentials are only sent to that authorization server. */ + constructor(options: PrivateKeyJwtProviderOptions); constructor(options: PrivateKeyJwtProviderOptions) { this._clientInfo = { - client_id: options.clientId + client_id: options.clientId, + issuer: checkedExpectedIssuer(options.expectedIssuer) }; this._clientMetadata = { client_name: options.clientName ?? 'private-key-jwt-client', @@ -341,6 +389,16 @@ export interface StaticPrivateKeyJwtProviderOptions { * Space-separated scopes values requested by the client. */ scope?: string; + + /** + * The issuer URL of the authorization server these credentials were registered with. + * When set, `auth()` only presents them to that authorization server and throws if the + * MCP server advertises a different URL (one trailing slash aside). + * + * Omitting it is deprecated: the credentials then go to whichever authorization server + * the MCP server advertises first. + */ + expectedIssuer?: string; } /** @@ -356,9 +414,13 @@ export class StaticPrivateKeyJwtProvider implements OAuthClientProvider { private _clientMetadata: OAuthClientMetadata; addClientAuthentication: AddClientAuthentication; + constructor(options: StaticPrivateKeyJwtProviderOptions & { expectedIssuer: string }); + /** @deprecated Pass `expectedIssuer` so the credentials are only sent to that authorization server. */ + constructor(options: StaticPrivateKeyJwtProviderOptions); constructor(options: StaticPrivateKeyJwtProviderOptions) { this._clientInfo = { - client_id: options.clientId + client_id: options.clientId, + issuer: checkedExpectedIssuer(options.expectedIssuer) }; this._clientMetadata = { client_name: options.clientName ?? 'static-private-key-jwt-client', diff --git a/src/client/auth.ts b/src/client/auth.ts index 214a289cb7..ddd8ee66d8 100644 --- a/src/client/auth.ts +++ b/src/client/auth.ts @@ -74,6 +74,10 @@ export interface OAuthClientProvider { * Loads information about this OAuth client, as registered already with the * server, or returns `undefined` if the client is not registered with the * server. + * + * A provider that returns pre-registered credentials should include `issuer`, the URL of + * the authorization server they were registered with; {@linkcode auth} then does not + * present them to any other. */ clientInformation(): OAuthClientInformationMixed | undefined | Promise; @@ -84,6 +88,10 @@ export interface OAuthClientProvider { * * This method is not required to be implemented if client information is * statically known (e.g., pre-registered). + * + * The object carries `issuer`, the authorization server it was obtained from. Store it + * unchanged so that {@linkcode auth} does not reuse the registration with a different one. + * Client information stored without `issuer` is passed here once, with it, on first use. */ saveClientInformation?(clientInformation: OAuthClientInformationMixed): void | Promise; @@ -96,6 +104,10 @@ export interface OAuthClientProvider { /** * Stores new OAuth tokens for the current session, after a successful * authorization. + * + * The object carries `issuer`, the authorization server that issued the tokens. Store it + * unchanged so that {@linkcode auth} does not present the refresh token to a different one. + * A refresh token stored without `issuer` is passed here once, with it, on first use. */ saveTokens(tokens: OAuthTokens): void | Promise; @@ -392,6 +404,33 @@ export async function parseErrorResponse(input: Response | string): Promise(stored: T | null | undefined, issuer: string): T | undefined { + // `null`: a `JSON.parse(storage.getItem(...))`-style getter with nothing stored. + if (!stored) return undefined; + return stored.issuer === undefined || issuersMatch(stored.issuer, issuer) ? stored : undefined; +} + +function boundElsewhereError(stored: { issuer?: string }, issuer: string): Error { + return new Error( + `OAuth client information is bound to authorization server ${stored.issuer} and is not presented to ${issuer}. ` + + 'Clear the stored client information, or correct `expectedIssuer`, if the authorization server has moved.' + ); +} + /** * Orchestrates the full auth flow with a server. * @@ -503,6 +542,12 @@ async function authInternal( }); } + // Binding key for stored credentials: the authorization server URL that discovery used + // (advertised by the resource server, restored from discoveryState(), or the legacy + // resource-origin fallback). `metadata.issuer` is deliberately not used: this version does + // not check that the metadata document echoes the URL it was fetched from. + const issuer = String(authorizationServerUrl); + // Send the metadata's resource indicator verbatim: `selectResourceURL` returns a parsed // `URL`, and `URL.href` appends "/" to a pathless indicator such as `https://example.com`, // which exact-match authorization servers reject (#1968). A URL returned by the @@ -518,8 +563,28 @@ async function authInternal( // The resolved scope is used consistently for both DCR and the authorization request. const resolvedScope = scope || resourceMetadata?.scopes_supported?.join(' ') || provider.clientMetadata.scope; - // Handle client registration if needed - let clientInformation = await Promise.resolve(provider.clientInformation()); + // Handle client registration if needed. Client information stamped for a different + // authorization server reads back as `undefined`, so the flow re-registers exactly as if + // nothing were stored. + const storedClientInformation = await Promise.resolve(provider.clientInformation()); + let clientInformation = discardIfIssuerMismatch(storedClientInformation, issuer); + const canRegisterAgain = + provider.saveClientInformation !== undefined && provider.addClientAuthentication === undefined && !!provider.redirectUrl; + if (storedClientInformation && !clientInformation && !canRegisterAgain) { + // Pre-registered credentials, custom client authentication provisioned for the stored + // registration, or a non-interactive client running on configured credentials: none of + // these can be re-created by registering with this authorization server. + throw boundElsewhereError(storedClientInformation, issuer); + } + if (clientInformation && clientInformation.issuer === undefined) { + // Saved by an earlier version: bind it to the first authorization server it is used with. + clientInformation = { ...clientInformation, issuer }; + try { + await provider.saveClientInformation?.(clientInformation); + } catch { + // A provider that only expects this call after a registration keeps working, unbound. + } + } if (!clientInformation) { if (authorizationCode !== undefined) { throw new Error('Existing OAuth client information is required when exchanging an authorization code'); @@ -538,9 +603,7 @@ async function authInternal( if (shouldUseUrlBasedClientId) { // SEP-991: URL-based Client IDs - clientInformation = { - client_id: clientMetadataUrl - }; + clientInformation = { client_id: clientMetadataUrl, issuer }; await provider.saveClientInformation?.(clientInformation); } else { // Fallback to dynamic registration @@ -555,8 +618,8 @@ async function authInternal( fetchFn }); - await provider.saveClientInformation(fullInformation); - clientInformation = fullInformation; + clientInformation = { ...fullInformation, issuer }; + await provider.saveClientInformation(clientInformation); } } @@ -572,11 +635,24 @@ async function authInternal( fetchFn }); - await provider.saveTokens(tokens); + await provider.saveTokens({ ...tokens, issuer }); return 'AUTHORIZED'; } - const tokens = await provider.tokens(); + // A refresh token stamped for a different authorization server reads back as `undefined`, + // so it is never posted to this one's token endpoint. + let tokens = discardIfIssuerMismatch(await provider.tokens(), issuer); + if (tokens?.refresh_token && tokens.issuer === undefined) { + // Saved by an earlier version: bind it to the first authorization server it is used with. + // eslint-disable-next-line no-console + console.warn( + "[mcp-sdk] stored OAuth tokens have no 'issuer' property (saved by an earlier version, or by a provider that " + + 'does not keep it). They are used as-is and bound to the current authorization server; make sure your ' + + 'OAuthClientProvider stores what saveTokens() and saveClientInformation() receive unchanged.' + ); + tokens = { ...tokens, issuer }; + await provider.saveTokens(tokens); + } // Handle token refresh or new authorization if (tokens?.refresh_token) { @@ -591,7 +667,7 @@ async function authInternal( fetchFn }); - await provider.saveTokens(newTokens); + await provider.saveTokens({ ...newTokens, issuer }); return 'AUTHORIZED'; } catch (error) { // If this is a ServerError, or an unknown type, log it out and try to continue. Otherwise, escalate so we can fix things and retry. @@ -1415,6 +1491,14 @@ export async function fetchToken( fetchFn?: FetchLike; } = {} ): Promise { + // Nothing is prepared for or sent to an authorization server other than the one the client + // information is stamped for. + const storedClientInformation = await provider.clientInformation(); + const clientInformation = discardIfIssuerMismatch(storedClientInformation, String(authorizationServerUrl)); + if (storedClientInformation && !clientInformation) { + throw boundElsewhereError(storedClientInformation, String(authorizationServerUrl)); + } + const scope = provider.clientMetadata.scope; // Use provider's prepareTokenRequest if available, otherwise fall back to authorization_code @@ -1435,12 +1519,10 @@ export async function fetchToken( tokenRequestParams = prepareAuthorizationCodeRequest(authorizationCode, codeVerifier, provider.redirectUrl); } - const clientInformation = await provider.clientInformation(); - return executeTokenRequest(authorizationServerUrl, { metadata, tokenRequestParams, - clientInformation: clientInformation ?? undefined, + clientInformation, addClientAuthentication: provider.addClientAuthentication, resource, fetchFn diff --git a/src/examples/client/simpleClientCredentials.ts b/src/examples/client/simpleClientCredentials.ts index 7defcc41f9..70967f1fb4 100644 --- a/src/examples/client/simpleClientCredentials.ts +++ b/src/examples/client/simpleClientCredentials.ts @@ -16,6 +16,7 @@ * * Common: * MCP_SERVER_URL - Server URL (default: http://localhost:3000/mcp) + * MCP_EXPECTED_ISSUER - Issuer URL of the authorization server the credentials were registered with (required) */ import { Client } from '../../client/index.js'; @@ -32,6 +33,13 @@ function createProvider(): OAuthClientProvider { process.exit(1); } + // The authorization server these credentials were registered with + const expectedIssuer = process.env.MCP_EXPECTED_ISSUER; + if (!expectedIssuer) { + console.error('MCP_EXPECTED_ISSUER environment variable is required'); + process.exit(1); + } + // If private key is provided, use private_key_jwt authentication const privateKeyPem = process.env.MCP_CLIENT_PRIVATE_KEY_PEM; if (privateKeyPem) { @@ -40,7 +48,8 @@ function createProvider(): OAuthClientProvider { return new PrivateKeyJwtProvider({ clientId, privateKey: privateKeyPem, - algorithm + algorithm, + expectedIssuer }); } @@ -54,7 +63,8 @@ function createProvider(): OAuthClientProvider { console.log('Using client_secret_basic authentication'); return new ClientCredentialsProvider({ clientId, - clientSecret + clientSecret, + expectedIssuer }); } diff --git a/src/shared/auth.ts b/src/shared/auth.ts index c546c8608b..99959be189 100644 --- a/src/shared/auth.ts +++ b/src/shared/auth.ts @@ -134,7 +134,12 @@ export const OAuthTokensSchema = z token_type: z.string(), expires_in: z.coerce.number().optional(), scope: z.string().optional(), - refresh_token: z.string().optional() + refresh_token: z.string().optional(), + /** + * Not part of the wire format: the authorization server this value was obtained from, added by + * the client's `auth()` before it is stored and compared when it is read back. + */ + issuer: z.string().optional().catch(undefined) }) .strip(); @@ -184,7 +189,12 @@ export const OAuthClientInformationSchema = z client_id: z.string(), client_secret: z.string().optional(), client_id_issued_at: z.number().optional(), - client_secret_expires_at: z.number().optional() + client_secret_expires_at: z.number().optional(), + /** + * Not part of the wire format: the authorization server this value was obtained from, added by + * the client's `auth()` before it is stored and compared when it is read back. + */ + issuer: z.string().optional().catch(undefined) }) .strip(); diff --git a/test/client/auth-extensions.test.ts b/test/client/auth-extensions.test.ts index 623d5e4da4..c99ff9e7b2 100644 --- a/test/client/auth-extensions.test.ts +++ b/test/client/auth-extensions.test.ts @@ -1,5 +1,5 @@ -import { describe, it, expect } from 'vitest'; -import { auth } from '../../src/client/auth.js'; +import { afterEach, beforeEach, describe, it, expect, vi, type MockInstance } from 'vitest'; +import { auth, type OAuthClientProvider } from '../../src/client/auth.js'; import { ClientCredentialsProvider, PrivateKeyJwtProvider, @@ -14,6 +14,7 @@ const AUTH_SERVER_URL = 'https://auth.example.com'; describe('auth-extensions providers (end-to-end with auth())', () => { it('authenticates using ClientCredentialsProvider with client_secret_basic', async () => { const provider = new ClientCredentialsProvider({ + expectedIssuer: AUTH_SERVER_URL, clientId: 'my-client', clientSecret: 'my-secret', clientName: 'test-client' @@ -51,6 +52,7 @@ describe('auth-extensions providers (end-to-end with auth())', () => { it('sends scope in token request when ClientCredentialsProvider is configured with scope', async () => { const provider = new ClientCredentialsProvider({ + expectedIssuer: AUTH_SERVER_URL, clientId: 'my-client', clientSecret: 'my-secret', clientName: 'test-client', @@ -80,6 +82,7 @@ describe('auth-extensions providers (end-to-end with auth())', () => { it('authenticates using PrivateKeyJwtProvider with private_key_jwt', async () => { const provider = new PrivateKeyJwtProvider({ + expectedIssuer: AUTH_SERVER_URL, clientId: 'client-id', privateKey: 'a-string-secret-at-least-256-bits-long', algorithm: 'HS256', @@ -123,6 +126,7 @@ describe('auth-extensions providers (end-to-end with auth())', () => { it('sends scope in token request when PrivateKeyJwtProvider is configured with scope', async () => { const provider = new PrivateKeyJwtProvider({ + expectedIssuer: AUTH_SERVER_URL, clientId: 'client-id', privateKey: 'a-string-secret-at-least-256-bits-long', algorithm: 'HS256', @@ -155,6 +159,7 @@ describe('auth-extensions providers (end-to-end with auth())', () => { it('fails when PrivateKeyJwtProvider is configured with an unsupported algorithm', async () => { const provider = new PrivateKeyJwtProvider({ + expectedIssuer: AUTH_SERVER_URL, clientId: 'client-id', privateKey: 'a-string-secret-at-least-256-bits-long', algorithm: 'none', @@ -178,6 +183,7 @@ describe('auth-extensions providers (end-to-end with auth())', () => { const staticAssertion = 'header.payload.signature'; const provider = new StaticPrivateKeyJwtProvider({ + expectedIssuer: AUTH_SERVER_URL, clientId: 'static-client', jwtBearerAssertion: staticAssertion, clientName: 'static-private-key-jwt-client' @@ -215,6 +221,7 @@ describe('auth-extensions providers (end-to-end with auth())', () => { const staticAssertion = 'header.payload.signature'; const provider = new StaticPrivateKeyJwtProvider({ + expectedIssuer: AUTH_SERVER_URL, clientId: 'static-client', jwtBearerAssertion: staticAssertion, clientName: 'static-private-key-jwt-client', @@ -245,6 +252,132 @@ describe('auth-extensions providers (end-to-end with auth())', () => { }); }); +describe('auth-extensions providers are bound to one authorization server', () => { + const SERVER_URL = 'https://api.example.com/mcp'; + const AS_ONE = 'https://as-one.example.com'; + const AS_TWO = 'https://as-two.example.com'; + + function createMigratingFetch() { + let active = AS_ONE; + const requests: string[] = []; + const tokenCalls: Array<{ origin: string; body: string; authorization: string | null }> = []; + const fetchFn = async (url: string | URL, init?: RequestInit): Promise => { + const u = new URL(String(url)); + requests.push(`${init?.method ?? 'GET'} ${u.origin}${u.pathname}`); + if (u.pathname.includes('/.well-known/oauth-protected-resource')) { + return Response.json({ resource: SERVER_URL, authorization_servers: [active] }); + } + if (u.pathname.includes('/.well-known/')) { + return Response.json({ + issuer: u.origin, + authorization_endpoint: `${u.origin}/authorize`, + token_endpoint: `${u.origin}/token`, + registration_endpoint: `${u.origin}/register`, + response_types_supported: ['code'], + grant_types_supported: ['client_credentials'] + }); + } + if (u.pathname === '/register') { + return Response.json({ client_id: `cid-${u.host}`, client_secret: `secret-${u.host}`, redirect_uris: [] }, { status: 201 }); + } + if (u.pathname === '/token') { + tokenCalls.push({ + origin: u.origin, + body: String(init?.body), + authorization: new Headers(init?.headers).get('authorization') + }); + return Response.json({ access_token: `at-${u.host}`, token_type: 'Bearer' }); + } + return new Response(null, { status: 404 }); + }; + return { fetchFn, requests, tokenCalls, switchTo: (as: string) => (active = as) }; + } + + type Options = { expectedIssuer?: string }; + const providers: Array<[string, (options: Options) => OAuthClientProvider]> = [ + [ + 'ClientCredentialsProvider', + o => new ClientCredentialsProvider({ clientId: 'client-one', clientSecret: 'configured-secret', ...o }) + ], + [ + 'PrivateKeyJwtProvider', + o => new PrivateKeyJwtProvider({ clientId: 'client-one', privateKey: 'k'.repeat(64), algorithm: 'HS256', ...o }) + ], + [ + 'StaticPrivateKeyJwtProvider', + o => new StaticPrivateKeyJwtProvider({ clientId: 'client-one', jwtBearerAssertion: 'assertion-one', ...o }) + ] + ]; + + let warn: MockInstance; + beforeEach(() => { + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + afterEach(() => { + warn.mockRestore(); + }); + + it.each(providers)('%s with expectedIssuer is only used with that authorization server', async (_name, create) => { + const srv = createMigratingFetch(); + const provider = create({ expectedIssuer: AS_ONE }); + + srv.switchTo(AS_TWO); + await expect(auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).rejects.toThrow( + `OAuth client information is bound to authorization server ${AS_ONE} and is not presented to ${AS_TWO}` + ); + expect(srv.requests.filter(r => r.includes(AS_TWO))).toEqual([`GET ${AS_TWO}/.well-known/oauth-authorization-server`]); + + srv.switchTo(AS_ONE); + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + expect(srv.tokenCalls.map(c => c.origin)).toEqual([AS_ONE]); + expect(warn).not.toHaveBeenCalled(); + }); + + it('accepts expectedIssuer spelled with a trailing slash', async () => { + const srv = createMigratingFetch(); + const provider = new ClientCredentialsProvider({ + clientId: 'client-one', + clientSecret: 'configured-secret', + expectedIssuer: `${AS_ONE}/` + }); + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + }); + + it.each(providers)('%s without expectedIssuer stays with the first authorization server it is used with', async (_name, create) => { + const srv = createMigratingFetch(); + const provider = create({}); + + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + + srv.switchTo(AS_TWO); + await expect(auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).rejects.toThrow( + `OAuth client information is bound to authorization server ${AS_ONE} and is not presented to ${AS_TWO}` + ); + expect(srv.requests.filter(r => r.includes(AS_TWO))).toEqual([`GET ${AS_TWO}/.well-known/oauth-authorization-server`]); + + // Back at the first authorization server the configured credential is used as before. + srv.switchTo(AS_ONE); + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + expect(srv.tokenCalls.map(c => c.origin)).toEqual([AS_ONE, AS_ONE]); + }); + + it.each(providers)('%s logs one deprecation message at construction when expectedIssuer is omitted', async (_name, create) => { + const srv = createMigratingFetch(); + const provider = create({}); + await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn }); + await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn }); + + expect(warn).toHaveBeenCalledTimes(1); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Omitting `expectedIssuer` is deprecated')); + }); + + it.each([null, ''])('rejects expectedIssuer %j at construction', value => { + expect(() => new ClientCredentialsProvider({ clientId: 'c', clientSecret: 's', expectedIssuer: value as string })).toThrow( + 'expectedIssuer must be' + ); + }); +}); + describe('createPrivateKeyJwtAuth', () => { const baseOptions = { issuer: 'client-id', diff --git a/test/client/auth.test.ts b/test/client/auth.test.ts index 56d1c5b943..74ec60f697 100644 --- a/test/client/auth.test.ts +++ b/test/client/auth.test.ts @@ -11,14 +11,23 @@ import { discoverOAuthServerInfo, extractWWWAuthenticateParams, auth, + fetchToken, type OAuthClientProvider, + type OAuthDiscoveryState, selectClientAuthMethod, isHttpsUrl } from '../../src/client/auth.js'; import { createPrivateKeyJwtAuth } from '../../src/client/auth-extensions.js'; import { InvalidClientMetadataError, ServerError } from '../../src/server/auth/errors.js'; -import { AuthorizationServerMetadata, OAuthClientMetadata, OAuthTokens } from '../../src/shared/auth.js'; -import { expect, vi, type Mock } from 'vitest'; +import { + AuthorizationServerMetadata, + OAuthClientInformationMixed, + OAuthClientInformationSchema, + OAuthClientMetadata, + OAuthTokens, + OAuthTokensSchema +} from '../../src/shared/auth.js'; +import { expect, vi, type Mock, type MockInstance } from 'vitest'; // Mock pkce-challenge vi.mock('pkce-challenge', () => ({ @@ -3556,7 +3565,8 @@ describe('OAuth Authorization', () => { // Should save URL-based client info expect(mockProvider.saveClientInformation).toHaveBeenCalledWith({ - client_id: 'https://example.com/client-metadata.json' + client_id: 'https://example.com/client-metadata.json', + issuer: 'https://server.example.com/' }); }); @@ -3602,7 +3612,8 @@ describe('OAuth Authorization', () => { expect(mockProvider.saveClientInformation).toHaveBeenCalledWith({ client_id: 'generated-uuid', client_secret: 'generated-secret', - redirect_uris: ['http://localhost:3000/callback'] + redirect_uris: ['http://localhost:3000/callback'], + issuer: 'https://server.example.com/' }); }); @@ -3758,8 +3769,386 @@ describe('OAuth Authorization', () => { expect(mockProvider.saveClientInformation).toHaveBeenCalledWith({ client_id: 'generated-uuid', client_secret: 'generated-secret', - redirect_uris: ['http://localhost:3000/callback'] + redirect_uris: ['http://localhost:3000/callback'], + issuer: 'https://server.example.com/' + }); + }); + }); + describe('auth: credentials are bound to the authorization server that issued them', () => { + const SERVER_URL = 'https://api.example.com/mcp'; + const AS_ONE = 'https://as-one.example.com'; + const AS_TWO = 'https://as-two.example.com'; + + const asMetadata = (issuer: string, endpoints = issuer): AuthorizationServerMetadata => ({ + issuer, + authorization_endpoint: `${endpoints}/authorize`, + token_endpoint: `${endpoints}/token`, + registration_endpoint: `${endpoints}/register`, + response_types_supported: ['code'], + code_challenge_methods_supported: ['S256'], + grant_types_supported: ['authorization_code', 'refresh_token', 'client_credentials'] + }); + + /** + * Resource server whose protected resource metadata advertises `active` as the authorization + * server; every origin answers authorization server metadata for itself. Records where + * registrations and token requests were sent. + */ + function createMigratingFetch(opts: { prm?: boolean; claimedIssuer?: Record } = {}) { + let active = AS_ONE; + const registerCalls: string[] = []; + const tokenCalls: Array<{ origin: string; body: URLSearchParams; authorization: string | null }> = []; + const requests: string[] = []; + const fetchFn = async (url: string | URL, init?: RequestInit): Promise => { + const u = new URL(String(url)); + requests.push(`${init?.method ?? 'GET'} ${u.origin}${u.pathname}`); + if (u.pathname.includes('/.well-known/oauth-protected-resource')) { + if (opts.prm === false) return new Response(null, { status: 404 }); + return Response.json({ resource: SERVER_URL, authorization_servers: [active] }); + } + if (u.pathname.includes('/.well-known/')) { + return Response.json(asMetadata(opts.claimedIssuer?.[u.origin] ?? u.origin, u.origin)); + } + if (u.pathname === '/register') { + registerCalls.push(u.origin); + return Response.json( + { + client_id: `cid-${u.host}`, + client_secret: `secret-${u.host}`, + redirect_uris: ['http://localhost:3000/callback'] + }, + { status: 201 } + ); + } + if (u.pathname === '/token') { + const body = new URLSearchParams(String(init?.body)); + tokenCalls.push({ origin: u.origin, body, authorization: new Headers(init?.headers).get('authorization') }); + return Response.json({ access_token: `at-${u.host}`, token_type: 'Bearer', refresh_token: `rt-${u.host}` }); + } + return new Response(null, { status: 404 }); + }; + return { fetchFn, registerCalls, tokenCalls, requests, switchTo: (as: string) => (active = as) }; + } + + type Stored = { info?: OAuthClientInformationMixed; tokens?: OAuthTokens }; + + /** Single-slot provider that round-trips whatever auth() saves. */ + function createBlobProvider(withDiscoveryState = true): OAuthClientProvider & { redirected: URL[]; stored: Stored } { + const stored: Stored = {}; + 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; + } + }) + }; + } + + /** Whether `needle` appears in the body or Basic credentials of any recorded token request (optionally ignoring one origin). */ + const sentAnywhere = (srv: ReturnType, needle: string, exceptOrigin?: string) => + srv.tokenCalls + .filter(c => c.origin !== exceptOrigin) + .some( + c => + String(c.body).includes(needle) || + (c.authorization !== null && atob(c.authorization.replace(/^Basic /, '')).includes(needle)) + ); + + let warn: MockInstance; + beforeEach(() => { + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + afterEach(() => { + warn.mockRestore(); + }); + + it('stamps the authorization server onto saved client information and tokens', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('REDIRECT'); + expect(provider.stored.info).toEqual(expect.objectContaining({ client_id: 'cid-as-one.example.com', issuer: AS_ONE })); + + expect(await auth(provider, { serverUrl: SERVER_URL, authorizationCode: 'code', fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + expect(provider.stored.tokens).toEqual({ + access_token: 'at-as-one.example.com', + token_type: 'Bearer', + refresh_token: 'rt-as-one.example.com', + issuer: AS_ONE + }); + expect(warn).not.toHaveBeenCalled(); + }); + + it('a refresh token issued through AS-one is never posted to AS-two', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'cid', client_secret: 'secret-one', issuer: AS_ONE }; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-one', issuer: AS_ONE }; + srv.switchTo(AS_TWO); + + const result = await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn }); + expect(srv.tokenCalls.filter(c => c.origin === AS_TWO)).toHaveLength(0); + expect(sentAnywhere(srv, 'rt-one')).toBe(false); + expect(sentAnywhere(srv, 'secret-one')).toBe(false); + expect(result).toBe('REDIRECT'); + expect(provider.redirected.at(-1)?.origin).toBe(AS_TWO); + }); + + it('client information issued through AS-one is not reused at AS-two; the client re-registers', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + + await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn }); + expect(srv.registerCalls).toEqual([AS_ONE]); + + srv.switchTo(AS_TWO); + provider.invalidateCredentials?.('discovery'); + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('REDIRECT'); + expect(srv.registerCalls).toEqual([AS_ONE, AS_TWO]); + expect(provider.stored.info).toEqual(expect.objectContaining({ client_id: 'cid-as-two.example.com', issuer: 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("the binding key is the discovery URL, not the metadata's issuer", async () => { + const srv = createMigratingFetch({ claimedIssuer: { [AS_TWO]: AS_ONE } }); + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'cid', client_secret: 'secret-one', issuer: AS_ONE }; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-one', issuer: AS_ONE }; + srv.switchTo(AS_TWO); + + const result = await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn }); + expect(srv.tokenCalls).toHaveLength(0); + expect(sentAnywhere(srv, 'rt-one')).toBe(false); + expect(result).toBe('REDIRECT'); + expect(srv.registerCalls).toEqual([AS_TWO]); + }); + + it('the resource-origin fallback is a distinct binding key', async () => { + const srv = createMigratingFetch({ prm: false }); + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'cid', client_secret: 'secret-one', issuer: AS_ONE }; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-one', issuer: AS_ONE }; + + const result = await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn }); + expect(srv.tokenCalls).toHaveLength(0); + expect(result).toBe('REDIRECT'); + expect(srv.registerCalls).toEqual(['https://api.example.com']); + expect(provider.stored.info?.issuer).toBe('https://api.example.com/'); + }); + + it('unstamped stored credentials are bound on first use', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'legacy-cid', client_secret: 'legacy-secret' }; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-legacy' }; + + // First use: refreshed at the resolved authorization server and written back with its stamp. + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + expect(srv.tokenCalls.map(c => c.origin)).toEqual([AS_ONE]); + expect(provider.stored.info).toEqual({ client_id: 'legacy-cid', client_secret: 'legacy-secret', issuer: AS_ONE }); + expect(provider.stored.tokens?.issuer).toBe(AS_ONE); + expect(srv.registerCalls).toHaveLength(0); + expect(warn.mock.calls.filter(c => String(c[0]).includes("no 'issuer' property"))).toHaveLength(1); + + // From then on the stamp applies. + srv.switchTo(AS_TWO); + provider.invalidateCredentials?.('discovery'); + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('REDIRECT'); + expect(srv.tokenCalls.filter(c => c.origin === AS_TWO)).toHaveLength(0); + expect(sentAnywhere(srv, 'legacy-secret', AS_ONE)).toBe(false); + expect(sentAnywhere(srv, 'rt-as-one.example.com')).toBe(false); + expect(srv.registerCalls).toEqual([AS_TWO]); + }); + + it('cached discovery state keeps its authorization server binding', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + provider.stored.info = { client_id: 'cid', client_secret: 'secret-one', issuer: AS_ONE }; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-one', issuer: AS_ONE }; + provider.saveDiscoveryState?.({ authorizationServerUrl: AS_ONE, authorizationServerMetadata: asMetadata(AS_ONE) }); + srv.switchTo(AS_TWO); + + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + expect(srv.tokenCalls.map(c => c.origin)).toEqual([AS_ONE]); + expect(srv.requests.some(r => r.includes(AS_TWO))).toBe(false); + }); + + it('a provider that cannot re-register reports the authorization server its client information is bound to', async () => { + const srv = createMigratingFetch(); + const provider: OAuthClientProvider = { ...createBlobProvider(), saveClientInformation: undefined }; + provider.clientInformation = () => ({ client_id: 'cid', client_secret: 'secret-one', issuer: AS_ONE }); + srv.switchTo(AS_TWO); + + await expect(auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).rejects.toThrow( + `OAuth client information is bound to authorization server ${AS_ONE}` + ); + expect(srv.tokenCalls).toHaveLength(0); + }); + + it('custom client authentication is not presented to a different authorization server than its client information', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(); + const addClientAuthentication = vi.fn>((_headers, params) => { + params.set('client_assertion', 'assertion-one'); + params.set('client_assertion_type', 'urn:ietf:params:oauth:client-assertion-type:jwt-bearer'); + }); + provider.addClientAuthentication = addClientAuthentication; + provider.stored.info = { client_id: 'cid', issuer: AS_ONE }; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-one', issuer: AS_ONE }; + srv.switchTo(AS_TWO); + + await expect(auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).rejects.toThrow( + `OAuth client information is bound to authorization server ${AS_ONE}` + ); + expect(addClientAuthentication).not.toHaveBeenCalled(); + expect(srv.registerCalls).toHaveLength(0); + expect(srv.tokenCalls).toHaveLength(0); + }); + + it('fetchToken sends nothing to an authorization server other than the one the client information is bound to', async () => { + const srv = createMigratingFetch(); + const addClientAuthentication = vi.fn>(); + const provider: OAuthClientProvider = { ...createBlobProvider(), addClientAuthentication }; + provider.clientInformation = () => ({ client_id: 'cid', client_secret: 'bound-secret', issuer: AS_ONE }); + + await expect( + fetchToken(provider, AS_TWO, { metadata: asMetadata(AS_TWO), authorizationCode: 'code', fetchFn: srv.fetchFn }) + ).rejects.toThrow(`OAuth client information is bound to authorization server ${AS_ONE}`); + expect(srv.requests).toEqual([]); + expect(addClientAuthentication).not.toHaveBeenCalled(); + + // A matching stamp is used as before (one trailing slash is tolerated). + provider.addClientAuthentication = undefined; + await fetchToken(provider, `${AS_ONE}/`, { metadata: asMetadata(AS_ONE), authorizationCode: 'code', fetchFn: srv.fetchFn }); + expect(srv.tokenCalls.map(c => [c.origin, c.authorization])).toEqual([[AS_ONE, `Basic ${btoa('cid:bound-secret')}`]]); + expect(warn).not.toHaveBeenCalled(); + }); + + it('a provider that reads storage back through the SDK schemas keeps the binding', async () => { + const storage = new Map(); + const provider: OAuthClientProvider = { + ...createBlobProvider(false), + clientInformation: () => + storage.has('info') ? OAuthClientInformationSchema.parseAsync(JSON.parse(storage.get('info')!)) : undefined, + saveClientInformation: i => void storage.set('info', JSON.stringify(i)), + tokens: () => (storage.has('tokens') ? OAuthTokensSchema.parseAsync(JSON.parse(storage.get('tokens')!)) : undefined), + saveTokens: t => void storage.set('tokens', JSON.stringify(t)) + }; + const srv = createMigratingFetch(); + await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn }); + await auth(provider, { serverUrl: SERVER_URL, authorizationCode: 'code', fetchFn: srv.fetchFn }); + + srv.switchTo(AS_TWO); + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('REDIRECT'); + expect(srv.tokenCalls.map(c => c.origin)).toEqual([AS_ONE]); + expect(warn).not.toHaveBeenCalled(); + }); + + it('an issuer property in a registration or token response does not become the stamp', async () => { + const srv = createMigratingFetch(); + const fetchFn = async (url: string | URL, init?: RequestInit) => { + const response = await srv.fetchFn(url, init); + return init?.method === 'POST' ? Response.json({ ...(await response.json()), issuer: AS_ONE }, response) : response; + }; + const provider = createBlobProvider(false); + srv.switchTo(AS_TWO); + + await auth(provider, { serverUrl: SERVER_URL, fetchFn }); + await auth(provider, { serverUrl: SERVER_URL, authorizationCode: 'code', fetchFn }); + expect(provider.stored.info?.issuer).toBe(AS_TWO); + expect(provider.stored.tokens?.issuer).toBe(AS_TWO); + }); + + it('an issuer that is not a string is dropped from a response, and an empty stored one matches nothing', async () => { + expect(OAuthTokensSchema.parse({ access_token: 'at', token_type: 'Bearer', issuer: null })).toEqual({ + access_token: 'at', + token_type: 'Bearer' + }); + expect(OAuthClientInformationSchema.parse({ client_id: 'cid', issuer: 5 })).toEqual({ client_id: 'cid' }); + + const srv = createMigratingFetch(); + const provider: OAuthClientProvider = { ...createBlobProvider(false), saveClientInformation: undefined }; + provider.clientInformation = () => ({ client_id: 'cid', client_secret: 'secret-one', issuer: '' }); + await expect(auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).rejects.toThrow( + 'OAuth client information is bound' + ); + expect(srv.tokenCalls).toEqual([]); + }); + + it('binding unstamped storage on first use never fails a flow that worked without it', async () => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(false); + provider.stored.info = { client_id: 'pre-registered' }; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer' }; + provider.saveClientInformation = () => { + throw new Error('client is pre-registered'); + }; + const saveTokens = vi.spyOn(provider, 'saveTokens'); + + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('REDIRECT'); + // Without a refresh token there is nothing to bind: no write, no message. + expect(saveTokens).not.toHaveBeenCalled(); + expect(warn).not.toHaveBeenCalled(); + }); + + it('a provider whose storage getters return null is treated as having nothing stored', async () => { + // The `JSON.parse(storage.getItem(key))` idiom yields `null`, not `undefined`, for an empty slot. + const storage = new Map(); + const read = (key: string) => JSON.parse(storage.get(key) ?? 'null'); + const provider: OAuthClientProvider = { + ...createBlobProvider(), + clientInformation: () => read('info'), + saveClientInformation: i => void storage.set('info', JSON.stringify(i)), + tokens: () => read('tokens'), + saveTokens: t => void storage.set('tokens', JSON.stringify(t)) + }; + const srv = createMigratingFetch(); + + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('REDIRECT'); + expect(srv.registerCalls).toEqual([AS_ONE]); + expect(read('info')).toEqual(expect.objectContaining({ client_id: 'cid-as-one.example.com', issuer: AS_ONE })); + + // Registered, still no tokens: a fresh authorization is started again. + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('REDIRECT'); + expect(srv.registerCalls).toEqual([AS_ONE]); + + // Without saveClientInformation the pre-existing registration error is reported unchanged. + storage.clear(); + const preRegisteredOnly: OAuthClientProvider = { ...provider, saveClientInformation: undefined }; + await expect(auth(preRegisteredOnly, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).rejects.toThrow( + 'OAuth client information must be saveable for dynamic registration' + ); + + // fetchToken sends the request without client authentication rather than failing. + await fetchToken(provider, AS_ONE, { + metadata: asMetadata(AS_ONE), + authorizationCode: 'code', + fetchFn: srv.fetchFn }); + expect(srv.tokenCalls.at(-1)?.authorization).toBeNull(); }); }); }); diff --git a/test/client/sse.test.ts b/test/client/sse.test.ts index 92ca5a8833..0b1a1364a6 100644 --- a/test/client/sse.test.ts +++ b/test/client/sse.test.ts @@ -776,7 +776,8 @@ describe('SSEClientTransport', () => { expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith({ access_token: 'new-token', token_type: 'Bearer', - refresh_token: 'new-refresh-token' + refresh_token: 'new-refresh-token', + issuer: `${authBaseUrl}` }); expect(connectionAttempts).toBe(1); expect(lastServerRequest.headers.authorization).toBe('Bearer new-token'); @@ -928,7 +929,8 @@ describe('SSEClientTransport', () => { expect(mockAuthProvider.saveTokens).toHaveBeenCalledWith({ access_token: 'new-token', token_type: 'Bearer', - refresh_token: 'new-refresh-token' + refresh_token: 'new-refresh-token', + issuer: `${authBaseUrl}` }); expect(postAttempts).toBe(1); expect(lastServerRequest.headers.authorization).toBe('Bearer new-token'); @@ -1515,7 +1517,8 @@ describe('SSEClientTransport', () => { access_token: 'new-access-token', token_type: 'Bearer', expires_in: 3600, - refresh_token: 'new-refresh-token' + refresh_token: 'new-refresh-token', + issuer: authBaseUrl.href }); // Global fetch should never have been called diff --git a/test/client/streamableHttp.test.ts b/test/client/streamableHttp.test.ts index 52c8f10748..3c7a2ff964 100644 --- a/test/client/streamableHttp.test.ts +++ b/test/client/streamableHttp.test.ts @@ -1346,7 +1346,8 @@ describe('StreamableHTTPClientTransport', () => { access_token: 'new-access-token', token_type: 'Bearer', expires_in: 3600, - refresh_token: 'new-refresh-token' + refresh_token: 'new-refresh-token', + issuer: 'http://localhost:1234' }); // Global fetch should never have been called @@ -1619,7 +1620,8 @@ describe('StreamableHTTPClientTransport', () => { access_token: 'new-access-token', token_type: 'Bearer', expires_in: 3600, - refresh_token: 'refresh-token' // Refresh token is preserved + refresh_token: 'refresh-token', // Refresh token is preserved + issuer: 'http://localhost:1234/' }); }); }); diff --git a/test/e2e/scenarios/client-auth.test.ts b/test/e2e/scenarios/client-auth.test.ts index 5e9fb6fdb3..0bf6378523 100644 --- a/test/e2e/scenarios/client-auth.test.ts +++ b/test/e2e/scenarios/client-auth.test.ts @@ -612,7 +612,7 @@ verifies('client-auth:client-credentials', async (_args: TestArgs) => { return baseFetch(url, init); }; - const provider = new ClientCredentialsProvider({ clientId: CLIENT_ID, clientSecret: CLIENT_SECRET }); + const provider = new ClientCredentialsProvider({ expectedIssuer: ISSUER, clientId: CLIENT_ID, clientSecret: CLIENT_SECRET }); const client = new Client({ name: 'c', version: '0' }); const transport = new StreamableHTTPClientTransport(new URL(MCP_URL), { authProvider: provider, fetch: combinedFetch }); @@ -844,7 +844,12 @@ verifies('client-auth:private-key-jwt', async (_args: TestArgs) => { const privateKeyPem = keyPair.privateKey.export({ type: 'pkcs8', format: 'pem' }).toString(); const publicKeyPem = keyPair.publicKey.export({ type: 'spki', format: 'pem' }).toString(); - const provider = new PrivateKeyJwtProvider({ clientId: CLIENT_ID, privateKey: privateKeyPem, algorithm: 'RS256' }); + const provider = new PrivateKeyJwtProvider({ + expectedIssuer: ISSUER, + clientId: CLIENT_ID, + privateKey: privateKeyPem, + algorithm: 'RS256' + }); const client = new Client({ name: 'c', version: '0' }); const transport = new StreamableHTTPClientTransport(new URL(MCP_URL), { authProvider: provider, fetch: combinedFetch }); @@ -1282,6 +1287,7 @@ verifies('client-auth:private-key-jwt:static-assertion', async (_args: TestArgs) const combinedFetch = createCombinedFetch({ as, mcpHost, validToken: ISSUED }); const provider = new StaticPrivateKeyJwtProvider({ + expectedIssuer: ISSUER, clientId: CLIENT_ID, jwtBearerAssertion: preBuiltJwt }); From 4b38d0b2842fd7acdf26094f6a66a176eb95ad30 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Mon, 28 Sep 2026 16:00:14 +0000 Subject: [PATCH 2/4] Bind unstamped credentials only after the authorization server accepts them The issuer stamp for storage written by earlier versions was saved before any token request had succeeded. A discovery failure that fell back to the resource origin therefore bound the credentials to that origin, and the next call, resolving the real authorization server, treated them as belonging elsewhere. The stamp is now written after a successful token request. Tokens need no separate write: the success path already saves them stamped. A stored `issuer` of null counts as no stamp. --- src/client/auth.ts | 31 +++++++++--------- test/client/auth-extensions.test.ts | 12 +++++++ test/client/auth.test.ts | 51 +++++++++++++++++++++++------ 3 files changed, 68 insertions(+), 26 deletions(-) diff --git a/src/client/auth.ts b/src/client/auth.ts index ddd8ee66d8..609f6458de 100644 --- a/src/client/auth.ts +++ b/src/client/auth.ts @@ -91,7 +91,7 @@ export interface OAuthClientProvider { * * The object carries `issuer`, the authorization server it was obtained from. Store it * unchanged so that {@linkcode auth} does not reuse the registration with a different one. - * Client information stored without `issuer` is passed here once, with it, on first use. + * Client information stored without `issuer` is passed here, with it, after its first successful use. */ saveClientInformation?(clientInformation: OAuthClientInformationMixed): void | Promise; @@ -107,7 +107,6 @@ export interface OAuthClientProvider { * * The object carries `issuer`, the authorization server that issued the tokens. Store it * unchanged so that {@linkcode auth} does not present the refresh token to a different one. - * A refresh token stored without `issuer` is passed here once, with it, on first use. */ saveTokens(tokens: OAuthTokens): void | Promise; @@ -416,12 +415,13 @@ function issuersMatch(a: string, b: string): boolean { * {@linkcode auth} stamps everything it passes to `saveClientInformation` / `saveTokens` with * `issuer`, the authorization server URL used for discovery. This returns `stored` unless its * stamp names a different authorization server, in which case the caller behaves as if nothing - * were stored. A value without a stamp (saved by an earlier version) is returned as-is. + * were stored. A value without a stamp (saved by an earlier version) is returned as-is, and + * is stamped once this authorization server has accepted it. */ function discardIfIssuerMismatch(stored: T | null | undefined, issuer: string): T | undefined { // `null`: a `JSON.parse(storage.getItem(...))`-style getter with nothing stored. if (!stored) return undefined; - return stored.issuer === undefined || issuersMatch(stored.issuer, issuer) ? stored : undefined; + return stored.issuer == null || issuersMatch(stored.issuer, issuer) ? stored : undefined; } function boundElsewhereError(stored: { issuer?: string }, issuer: string): Error { @@ -576,15 +576,15 @@ async function authInternal( // these can be re-created by registering with this authorization server. throw boundElsewhereError(storedClientInformation, issuer); } - if (clientInformation && clientInformation.issuer === undefined) { - // Saved by an earlier version: bind it to the first authorization server it is used with. - clientInformation = { ...clientInformation, issuer }; + // Saved by an earlier version: bound to the first authorization server that accepts it. + const unstampedClientInformation = clientInformation?.issuer == null ? clientInformation : undefined; + const bindClientInformation = async () => { try { - await provider.saveClientInformation?.(clientInformation); + if (unstampedClientInformation) await provider.saveClientInformation?.({ ...unstampedClientInformation, issuer }); } catch { // A provider that only expects this call after a registration keeps working, unbound. } - } + }; if (!clientInformation) { if (authorizationCode !== undefined) { throw new Error('Existing OAuth client information is required when exchanging an authorization code'); @@ -635,23 +635,21 @@ async function authInternal( fetchFn }); + await bindClientInformation(); await provider.saveTokens({ ...tokens, issuer }); return 'AUTHORIZED'; } // A refresh token stamped for a different authorization server reads back as `undefined`, // so it is never posted to this one's token endpoint. - let tokens = discardIfIssuerMismatch(await provider.tokens(), issuer); - if (tokens?.refresh_token && tokens.issuer === undefined) { - // Saved by an earlier version: bind it to the first authorization server it is used with. + const tokens = discardIfIssuerMismatch(await provider.tokens(), issuer); + if (tokens?.refresh_token && tokens.issuer == null) { // eslint-disable-next-line no-console console.warn( "[mcp-sdk] stored OAuth tokens have no 'issuer' property (saved by an earlier version, or by a provider that " + - 'does not keep it). They are used as-is and bound to the current authorization server; make sure your ' + - 'OAuthClientProvider stores what saveTokens() and saveClientInformation() receive unchanged.' + 'does not keep it) and are used as-is; make sure your OAuthClientProvider stores what saveTokens() and ' + + 'saveClientInformation() receive unchanged.' ); - tokens = { ...tokens, issuer }; - await provider.saveTokens(tokens); } // Handle token refresh or new authorization @@ -667,6 +665,7 @@ async function authInternal( fetchFn }); + await bindClientInformation(); await provider.saveTokens({ ...newTokens, issuer }); return 'AUTHORIZED'; } catch (error) { diff --git a/test/client/auth-extensions.test.ts b/test/client/auth-extensions.test.ts index c99ff9e7b2..630690b697 100644 --- a/test/client/auth-extensions.test.ts +++ b/test/client/auth-extensions.test.ts @@ -361,6 +361,18 @@ describe('auth-extensions providers are bound to one authorization server', () = expect(srv.tokenCalls.map(c => c.origin)).toEqual([AS_ONE, AS_ONE]); }); + it.each(providers)('%s without expectedIssuer recovers after a failed first attempt', async (_name, create) => { + const srv = createMigratingFetch(); + const provider = create({}); + + // Discovery fails once and falls back to the resource origin, which has no token endpoint. + const unavailable = async (url: string | URL) => + new Response(null, { status: String(url).includes('oauth-protected-resource') ? 503 : 404 }); + await expect(auth(provider, { serverUrl: SERVER_URL, fetchFn: unavailable })).rejects.toThrow(); + + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + }); + it.each(providers)('%s logs one deprecation message at construction when expectedIssuer is omitted', async (_name, create) => { const srv = createMigratingFetch(); const provider = create({}); diff --git a/test/client/auth.test.ts b/test/client/auth.test.ts index 74ec60f697..edd224563b 100644 --- a/test/client/auth.test.ts +++ b/test/client/auth.test.ts @@ -3796,6 +3796,7 @@ describe('OAuth Authorization', () => { */ function createMigratingFetch(opts: { prm?: boolean; claimedIssuer?: Record } = {}) { let active = AS_ONE; + let rejecting: string | undefined; const registerCalls: string[] = []; const tokenCalls: Array<{ origin: string; body: URLSearchParams; authorization: string | null }> = []; const requests: string[] = []; @@ -3823,11 +3824,19 @@ describe('OAuth Authorization', () => { if (u.pathname === '/token') { const body = new URLSearchParams(String(init?.body)); tokenCalls.push({ origin: u.origin, body, authorization: new Headers(init?.headers).get('authorization') }); + if (u.origin === rejecting) return new Response(null, { status: 404 }); return Response.json({ access_token: `at-${u.host}`, token_type: 'Bearer', refresh_token: `rt-${u.host}` }); } return new Response(null, { status: 404 }); }; - return { fetchFn, registerCalls, tokenCalls, requests, switchTo: (as: string) => (active = as) }; + return { + fetchFn, + registerCalls, + tokenCalls, + requests, + switchTo: (as: string) => (active = as), + rejectTokenRequestsAt: (origin: string) => (rejecting = origin) + }; } type Stored = { info?: OAuthClientInformationMixed; tokens?: OAuthTokens }; @@ -3983,6 +3992,30 @@ describe('OAuth Authorization', () => { expect(srv.registerCalls).toEqual([AS_TWO]); }); + it('unstamped stored credentials are only bound to an authorization server that accepted them', async () => { + const srv = createMigratingFetch(); + let prmAvailable = false; + const fetchFn = async (url: string | URL, init?: RequestInit) => + !prmAvailable && String(url).includes('oauth-protected-resource') + ? new Response(null, { status: 503 }) + : srv.fetchFn(url, init); + const provider = createBlobProvider(false); + provider.stored.info = { client_id: 'legacy-cid', client_secret: 'legacy-secret', issuer: null as unknown as undefined }; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-legacy' }; + + // Discovery falls back to the resource origin, where the refresh is not accepted. + srv.rejectTokenRequestsAt('https://api.example.com'); + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn })).toBe('REDIRECT'); + expect(provider.stored.info?.issuer).toBeNull(); + expect(provider.stored.tokens?.issuer).toBeUndefined(); + + prmAvailable = true; + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn })).toBe('AUTHORIZED'); + expect(srv.registerCalls).toEqual([]); + expect(provider.stored.info?.issuer).toBe(AS_ONE); + expect(provider.stored.tokens?.issuer).toBe(AS_ONE); + }); + it('cached discovery state keeps its authorization server binding', async () => { const srv = createMigratingFetch(); const provider = createBlobProvider(); @@ -4098,20 +4131,18 @@ describe('OAuth Authorization', () => { expect(srv.tokenCalls).toEqual([]); }); - it('binding unstamped storage on first use never fails a flow that worked without it', async () => { + it('binding unstamped client information never fails a flow that worked without it', async () => { const srv = createMigratingFetch(); const provider = createBlobProvider(false); provider.stored.info = { client_id: 'pre-registered' }; - provider.stored.tokens = { access_token: 'at', token_type: 'Bearer' }; - provider.saveClientInformation = () => { + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-legacy' }; + provider.saveClientInformation = vi.fn(() => { throw new Error('client is pre-registered'); - }; - const saveTokens = vi.spyOn(provider, 'saveTokens'); + }); - expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('REDIRECT'); - // Without a refresh token there is nothing to bind: no write, no message. - expect(saveTokens).not.toHaveBeenCalled(); - expect(warn).not.toHaveBeenCalled(); + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + expect(provider.saveClientInformation).toHaveBeenCalledWith({ client_id: 'pre-registered', issuer: AS_ONE }); + expect(provider.stored.tokens?.issuer).toBe(AS_ONE); }); it('a provider whose storage getters return null is treated as having nothing stored', async () => { From 1c12c08b7335f815cf419d7b18f63e9c58c76716 Mon Sep 17 00:00:00 2001 From: Max Isbey <224885523+maxisbey@users.noreply.github.com> Date: Mon, 28 Sep 2026 16:36:07 +0000 Subject: [PATCH 3/4] Let interactive clients with custom client authentication register again A provider with addClientAuthentication() was treated as unable to register with a newly advertised authorization server. That also stopped URL-based client IDs and DCR clients whose key is published in their metadata. The bundled JWT providers are non-interactive and stay covered by the redirectUrl condition. --- src/client/auth.ts | 8 +++----- test/client/auth.test.ts | 20 -------------------- 2 files changed, 3 insertions(+), 25 deletions(-) diff --git a/src/client/auth.ts b/src/client/auth.ts index 609f6458de..4f8727884a 100644 --- a/src/client/auth.ts +++ b/src/client/auth.ts @@ -568,12 +568,10 @@ async function authInternal( // nothing were stored. const storedClientInformation = await Promise.resolve(provider.clientInformation()); let clientInformation = discardIfIssuerMismatch(storedClientInformation, issuer); - const canRegisterAgain = - provider.saveClientInformation !== undefined && provider.addClientAuthentication === undefined && !!provider.redirectUrl; + const canRegisterAgain = provider.saveClientInformation !== undefined && !!provider.redirectUrl; if (storedClientInformation && !clientInformation && !canRegisterAgain) { - // Pre-registered credentials, custom client authentication provisioned for the stored - // registration, or a non-interactive client running on configured credentials: none of - // these can be re-created by registering with this authorization server. + // Pre-registered credentials, or a non-interactive client running on configured + // credentials: neither can be re-created by registering with this authorization server. throw boundElsewhereError(storedClientInformation, issuer); } // Saved by an earlier version: bound to the first authorization server that accepts it. diff --git a/test/client/auth.test.ts b/test/client/auth.test.ts index edd224563b..e0f9b64984 100644 --- a/test/client/auth.test.ts +++ b/test/client/auth.test.ts @@ -4041,26 +4041,6 @@ describe('OAuth Authorization', () => { expect(srv.tokenCalls).toHaveLength(0); }); - it('custom client authentication is not presented to a different authorization server than its client information', async () => { - const srv = createMigratingFetch(); - const provider = createBlobProvider(); - const addClientAuthentication = vi.fn>((_headers, params) => { - params.set('client_assertion', 'assertion-one'); - params.set('client_assertion_type', 'urn:ietf:params:oauth:client-assertion-type:jwt-bearer'); - }); - provider.addClientAuthentication = addClientAuthentication; - provider.stored.info = { client_id: 'cid', issuer: AS_ONE }; - provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt-one', issuer: AS_ONE }; - srv.switchTo(AS_TWO); - - await expect(auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).rejects.toThrow( - `OAuth client information is bound to authorization server ${AS_ONE}` - ); - expect(addClientAuthentication).not.toHaveBeenCalled(); - expect(srv.registerCalls).toHaveLength(0); - expect(srv.tokenCalls).toHaveLength(0); - }); - it('fetchToken sends nothing to an authorization server other than the one the client information is bound to', async () => { const srv = createMigratingFetch(); const addClientAuthentication = vi.fn>(); From a271c27cb58991d3ab91256ed10a9b53df7ceb92 Mon Sep 17 00:00:00 2001 From: Felix Weinberger <3823880+felixweinberger@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:22:47 +0000 Subject: [PATCH 4/4] fix(client): ignore issuer in authorization server responses `issuer` on stored tokens and client information is written by `auth()` when it saves them. Token and registration responses are now parsed without it, in the client and in `ProxyOAuthServerProvider`, so the exported helpers and the proxy return the same fields as 1.30.1. A stored or configured `issuer` and the authorization server URL are compared as parsed URLs, so the result does not depend on how scheme, host or default port are spelled, whether by the caller or by the installed zod version. One trailing slash is still tolerated; if either value is not a URL, both are compared as strings, as before. `fetchToken()` reads the provider's client information again after the token request is prepared when the first read returned nothing, and applies the same check to it. A stored `issuer` that is not a string is treated as not set. The bundled providers reject an `expectedIssuer` that is not a string when they are constructed. --- src/client/auth-extensions.ts | 2 +- src/client/auth.ts | 41 ++++-- src/server/auth/providers/proxyProvider.ts | 10 +- test/client/auth-extensions.test.ts | 9 +- test/client/auth.test.ts | 118 ++++++++++++++++++ .../auth/providers/proxyProvider.test.ts | 39 ++++++ 6 files changed, 203 insertions(+), 16 deletions(-) diff --git a/src/client/auth-extensions.ts b/src/client/auth-extensions.ts index 5d8ab82f19..6b8a875667 100644 --- a/src/client/auth-extensions.ts +++ b/src/client/auth-extensions.ts @@ -102,7 +102,7 @@ function checkedExpectedIssuer(expectedIssuer: string | undefined): string | und "server receives this client's credentials; pass your authorization server's issuer URL as " + '`expectedIssuer` so they are only sent there.' ); - } else if (!expectedIssuer) { + } else if (typeof expectedIssuer !== 'string' || !expectedIssuer) { throw new Error("expectedIssuer must be the authorization server's issuer URL"); } return expectedIssuer; diff --git a/src/client/auth.ts b/src/client/auth.ts index 4f8727884a..19c4d23f0d 100644 --- a/src/client/auth.ts +++ b/src/client/auth.ts @@ -408,7 +408,14 @@ export async function parseErrorResponse(input: Response | string): Promise(stored: T | null | undefined, issuer: string): T | undefined { // `null`: a `JSON.parse(storage.getItem(...))`-style getter with nothing stored. if (!stored) return undefined; - return stored.issuer == null || issuersMatch(stored.issuer, issuer) ? stored : undefined; + // A stamp that is not a string (raw storage) counts as no stamp. + if (typeof stored.issuer !== 'string') return stored.issuer == null ? stored : { ...stored, issuer: undefined }; + return issuersMatch(stored.issuer, issuer) ? stored : undefined; } +// `issuer` is added by the client when it stores a value; authorization server responses are parsed without it. +const TokenResponseSchema = OAuthTokensSchema.omit({ issuer: true }); +const RegistrationResponseSchema = OAuthClientInformationFullSchema.omit({ issuer: true }); + function boundElsewhereError(stored: { issuer?: string }, issuer: string): Error { return new Error( `OAuth client information is bound to authorization server ${stored.issuer} and is not presented to ${issuer}. ` + @@ -1349,7 +1362,7 @@ async function executeTokenRequest( throw await parseErrorResponse(response); } - return OAuthTokensSchema.parse(await response.json()); + return TokenResponseSchema.parse(await response.json()); } /** @@ -1488,13 +1501,16 @@ export async function fetchToken( fetchFn?: FetchLike; } = {} ): Promise { - // Nothing is prepared for or sent to an authorization server other than the one the client - // information is stamped for. - const storedClientInformation = await provider.clientInformation(); - const clientInformation = discardIfIssuerMismatch(storedClientInformation, String(authorizationServerUrl)); - if (storedClientInformation && !clientInformation) { - throw boundElsewhereError(storedClientInformation, String(authorizationServerUrl)); - } + // Nothing is sent to an authorization server other than the one the client information is stamped for. + const readClientInformation = async () => { + const storedClientInformation = await provider.clientInformation(); + const checked = discardIfIssuerMismatch(storedClientInformation, String(authorizationServerUrl)); + if (storedClientInformation && !checked) { + throw boundElsewhereError(storedClientInformation, String(authorizationServerUrl)); + } + return checked; + }; + let clientInformation = await readClientInformation(); const scope = provider.clientMetadata.scope; @@ -1516,6 +1532,9 @@ export async function fetchToken( tokenRequestParams = prepareAuthorizationCodeRequest(authorizationCode, codeVerifier, provider.redirectUrl); } + // A provider may fill in its client information while the request is prepared. + clientInformation ??= await readClientInformation(); + return executeTokenRequest(authorizationServerUrl, { metadata, tokenRequestParams, @@ -1574,5 +1593,5 @@ export async function registerClient( throw await parseErrorResponse(response); } - return OAuthClientInformationFullSchema.parse(await response.json()); + return RegistrationResponseSchema.parse(await response.json()); } diff --git a/src/server/auth/providers/proxyProvider.ts b/src/server/auth/providers/proxyProvider.ts index 855856c89e..9e8e67ea82 100644 --- a/src/server/auth/providers/proxyProvider.ts +++ b/src/server/auth/providers/proxyProvider.ts @@ -12,6 +12,10 @@ import { AuthorizationParams, OAuthServerProvider } from '../provider.js'; import { ServerError } from '../errors.js'; import { FetchLike } from '../../../shared/transport.js'; +// `issuer` is added by a client when it stores a value; upstream responses are parsed without it. +const TokenResponseSchema = OAuthTokensSchema.omit({ issuer: true }); +const RegistrationResponseSchema = OAuthClientInformationFullSchema.omit({ issuer: true }); + export type ProxyEndpoints = { authorizationUrl: string; tokenUrl: string; @@ -113,7 +117,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { } const data = await response.json(); - return OAuthClientInformationFullSchema.parse(data); + return RegistrationResponseSchema.parse(data); } }) }; @@ -188,7 +192,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { } const data = await response.json(); - return OAuthTokensSchema.parse(data); + return TokenResponseSchema.parse(data); } async exchangeRefreshToken( @@ -229,7 +233,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { } const data = await response.json(); - return OAuthTokensSchema.parse(data); + return TokenResponseSchema.parse(data); } async verifyAccessToken(token: string): Promise { diff --git a/test/client/auth-extensions.test.ts b/test/client/auth-extensions.test.ts index 630690b697..7d323f9c92 100644 --- a/test/client/auth-extensions.test.ts +++ b/test/client/auth-extensions.test.ts @@ -343,6 +343,13 @@ describe('auth-extensions providers are bound to one authorization server', () = expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); }); + it.each(['https://AS-ONE.example.com', 'https://as-one.example.com:443'])('accepts expectedIssuer spelled %s', async expectedIssuer => { + const srv = createMigratingFetch(); + const provider = new ClientCredentialsProvider({ clientId: 'client-one', clientSecret: 'configured-secret', expectedIssuer }); + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + expect(srv.tokenCalls.map(c => c.origin)).toEqual([AS_ONE]); + }); + it.each(providers)('%s without expectedIssuer stays with the first authorization server it is used with', async (_name, create) => { const srv = createMigratingFetch(); const provider = create({}); @@ -383,7 +390,7 @@ describe('auth-extensions providers are bound to one authorization server', () = expect(warn).toHaveBeenCalledWith(expect.stringContaining('Omitting `expectedIssuer` is deprecated')); }); - it.each([null, ''])('rejects expectedIssuer %j at construction', value => { + it.each([null, '', 42, new URL(AS_ONE)])('rejects expectedIssuer %j at construction', value => { expect(() => new ClientCredentialsProvider({ clientId: 'c', clientSecret: 's', expectedIssuer: value as string })).toThrow( 'expectedIssuer must be' ); diff --git a/test/client/auth.test.ts b/test/client/auth.test.ts index e0f9b64984..45ec2806d0 100644 --- a/test/client/auth.test.ts +++ b/test/client/auth.test.ts @@ -4161,5 +4161,123 @@ describe('OAuth Authorization', () => { }); expect(srv.tokenCalls.at(-1)?.authorization).toBeNull(); }); + + it.each(['https://other.example.com', 42])('an issuer of %j in a token or registration response is not returned', async issuer => { + const srv = createMigratingFetch(); + const fetchFn = async (url: string | URL, init?: RequestInit) => { + const response = await srv.fetchFn(url, init); + return init?.method === 'POST' ? Response.json({ ...(await response.json()), issuer }, response) : response; + }; + const metadata = asMetadata(AS_ONE); + const clientInformation = { client_id: 'cid' }; + const provider: OAuthClientProvider = { + ...createBlobProvider(false), + clientInformation: () => clientInformation, + prepareTokenRequest: () => new URLSearchParams({ grant_type: 'client_credentials' }) + }; + + const results = [ + await registerClient(AS_ONE, { metadata, clientMetadata: { redirect_uris: [] }, fetchFn }), + await exchangeAuthorization(AS_ONE, { + metadata, + clientInformation, + authorizationCode: 'code', + codeVerifier: 'v', + redirectUri: 'http://localhost:3000/callback', + fetchFn + }), + await refreshAuthorization(AS_ONE, { metadata, clientInformation, refreshToken: 'rt', fetchFn }), + await fetchToken(provider, AS_ONE, { metadata, fetchFn }) + ]; + for (const result of results) { + expect(result).not.toHaveProperty('issuer'); + } + }); + + it('a stamp and the authorization server are compared as parsed URLs', async () => { + const requestsSent = async (issuer: string, authorizationServerUrl: string) => { + const srv = createMigratingFetch(); + const provider: OAuthClientProvider = { + ...createBlobProvider(false), + clientInformation: () => ({ client_id: 'cid', client_secret: 'bound-secret', issuer }) + }; + const options = { metadata: asMetadata(AS_ONE), authorizationCode: 'code', fetchFn: srv.fetchFn }; + await fetchToken(provider, authorizationServerUrl, options).catch(() => {}); + return [issuer, authorizationServerUrl, srv.tokenCalls.length]; + }; + + for (const [issuer, url] of [ + ['https://AS-ONE.example.com', AS_ONE], + ['HTTPS://as-one.example.com:443', AS_ONE], + [AS_ONE, 'https://AS-ONE.example.com/'], + [AS_ONE, 'https://as-one.example.com:443'], + ['https://as-one.example.com/a/./b', 'https://as-one.example.com/a/b/'], + ['as-one.example.com', 'as-one.example.com/'] + ]) { + expect(await requestsSent(issuer, url)).toEqual([issuer, url, 1]); + } + + // Another scheme, host, port or path is another authorization server. + for (const [issuer, url] of [ + [AS_ONE, 'http://as-one.example.com'], + [AS_ONE, 'https://as-one.example.com.'], + [AS_ONE, 'https://as-one.example.com.example.org'], + [AS_ONE, 'https://as-one.example.com@example.org'], + [AS_ONE, 'https://a@as-one.example.com'], + [AS_ONE, 'https://as-one.example.com/#f'], + [AS_ONE, 'https://as-one.example.com:8443'], + [AS_ONE, 'https://as-one.example.com/tenant'], + ['https://as-one.example.com/Tenant', 'https://as-one.example.com/tenant'], + ['https://as-one.example.com/tenant', 'https://as-one.example.com/tenant//'], + ['https://as-one.example.com/?tenant=a', 'https://as-one.example.com/?tenant=b'], + ['as-one.example.com', AS_ONE] + ]) { + expect(await requestsSent(issuer, url)).toEqual([issuer, url, 0]); + } + }); + + const notStrings = [{ issuer: 42 }, { issuer: false }, { issuer: { href: AS_ONE } }, { issuer: [AS_ONE] }]; + it.each(notStrings)('a stored issuer of $issuer counts as no stamp', async ({ issuer }) => { + const srv = createMigratingFetch(); + const provider = createBlobProvider(false); + provider.stored.info = { client_id: 'cid', client_secret: 's', issuer } as unknown as OAuthClientInformationMixed; + provider.stored.tokens = { access_token: 'at', token_type: 'Bearer', refresh_token: 'rt', issuer } as unknown as OAuthTokens; + + await fetchToken(provider, AS_ONE, { metadata: asMetadata(AS_ONE), authorizationCode: 'code', fetchFn: srv.fetchFn }); + expect(srv.tokenCalls.map(c => c.authorization)).toEqual([`Basic ${btoa('cid:s')}`]); + + // auth() binds it after its first successful use, as it does for a value with no stamp. + expect(await auth(provider, { serverUrl: SERVER_URL, fetchFn: srv.fetchFn })).toBe('AUTHORIZED'); + expect(provider.stored.info?.issuer).toBe(AS_ONE); + expect(provider.stored.tokens?.issuer).toBe(AS_ONE); + }); + + it('fetchToken uses client information that the provider fills in while preparing the request', async () => { + const srv = createMigratingFetch(); + let filled: OAuthClientInformationMixed | undefined; + const prepareTokenRequest = vi.fn(() => { + filled = { client_id: 'cid', client_secret: 'lazy-secret', issuer: AS_ONE }; + return new URLSearchParams({ grant_type: 'client_credentials' }); + }); + const provider: OAuthClientProvider = { ...createBlobProvider(false), clientInformation: () => filled, prepareTokenRequest }; + + await fetchToken(provider, AS_ONE, { metadata: asMetadata(AS_ONE), fetchFn: srv.fetchFn }); + expect(srv.tokenCalls.map(c => [c.origin, c.authorization])).toEqual([[AS_ONE, `Basic ${btoa('cid:lazy-secret')}`]]); + + // The value read after preparing goes through the same check. + filled = undefined; + await expect(fetchToken(provider, AS_TWO, { metadata: asMetadata(AS_TWO), fetchFn: srv.fetchFn })).rejects.toThrow( + `OAuth client information is bound to authorization server ${AS_ONE}` + ); + expect(srv.tokenCalls).toHaveLength(1); + + // A value that is there before preparing is checked before the request is prepared. + prepareTokenRequest.mockClear(); + await expect(fetchToken(provider, AS_TWO, { metadata: asMetadata(AS_TWO), fetchFn: srv.fetchFn })).rejects.toThrow( + `OAuth client information is bound to authorization server ${AS_ONE}` + ); + expect(prepareTokenRequest).not.toHaveBeenCalled(); + expect(srv.tokenCalls).toHaveLength(1); + }); }); }); diff --git a/test/server/auth/providers/proxyProvider.test.ts b/test/server/auth/providers/proxyProvider.test.ts index 40fb55d572..30975fcd8b 100644 --- a/test/server/auth/providers/proxyProvider.test.ts +++ b/test/server/auth/providers/proxyProvider.test.ts @@ -225,6 +225,23 @@ describe('Proxy OAuth Server Provider', () => { ); expect(tokens).toEqual(mockTokenResponse); }); + + it.each(['https://upstream.example.com', 42])('does not forward an issuer of %j from the upstream token response', async issuer => { + (global.fetch as Mock).mockImplementation(() => + Promise.resolve({ + ok: true, + json: () => Promise.resolve({ ...mockTokenResponse, issuer }) + }) + ); + + const exchanged = await provider.exchangeAuthorizationCode(validClient, 'test-code', 'test-verifier'); + const refreshed = await provider.exchangeRefreshToken(validClient, 'test-refresh-token'); + + expect(exchanged).toEqual(mockTokenResponse); + expect(exchanged).not.toHaveProperty('issuer'); + expect(refreshed).toEqual(mockTokenResponse); + expect(refreshed).not.toHaveProperty('issuer'); + }); }); describe('client registration', () => { @@ -256,6 +273,28 @@ describe('Proxy OAuth Server Provider', () => { expect(result).toEqual(newClient); }); + it.each(['https://upstream.example.com', 42])( + 'does not forward an issuer of %j from the upstream registration response', + async issuer => { + const newClient: OAuthClientInformationFull = { + client_id: 'new-client', + redirect_uris: ['https://new-client.com/callback'] + }; + + (global.fetch as Mock).mockImplementation(() => + Promise.resolve({ + ok: true, + json: () => Promise.resolve({ ...newClient, issuer }) + }) + ); + + const result = await provider.clientsStore.registerClient!(newClient); + + expect(result).toEqual(newClient); + expect(result).not.toHaveProperty('issuer'); + } + ); + it('handles registration failure', async () => { mockFailedResponse(); const newClient: OAuthClientInformationFull = {