diff --git a/.changeset/fix-prm-resource-validation.md b/.changeset/fix-prm-resource-validation.md new file mode 100644 index 0000000000..20dc240cca --- /dev/null +++ b/.changeset/fix-prm-resource-validation.md @@ -0,0 +1,11 @@ +--- +'@modelcontextprotocol/client': patch +--- + +Validate protected resource metadata identity before using its authorization servers or caching it (RFC 9728 §3.3). Challenge and path-specific discovery must identify the requested resource; root well-known fallback must identify the root resource. OAuth authorization, code exchange, and refresh continue to send a valid metadata resource unchanged. + +Previously accepted parent-path resources, differing queries, and differing trailing path slashes are now rejected, including when `validateResourceURL` is supplied. That hook still selects the outgoing resource after metadata validation. The shared `checkResourceAllowed` path-containment helper is unchanged. + +Path-specific PRM discovery now preserves a non-root path's trailing slash: `/mcp/` is probed at `/.well-known/oauth-protected-resource/mcp/`, rather than `/.well-known/oauth-protected-resource/mcp`. Root discovery and authorization server metadata URL construction are unchanged. Resource validation rejects ambiguous backslash authority boundaries and checks that the parsed URL also matches the expected resource. + +Persist the complete discovery state, including the optional `resourceMetadataRootFallback` marker. Cached metadata is revalidated before use; old root-resource caches without discovery provenance are rediscovered. diff --git a/docs/clients/oauth.md b/docs/clients/oauth.md index f03a3e8af0..974fbd6ac6 100644 --- a/docs/clients/oauth.md +++ b/docs/clients/oauth.md @@ -2,6 +2,7 @@ shape: how-to description: 'Sign an end user in from a client you build with the OAuth authorization-code flow.' --- + # Authenticate a user with OAuth Protecting a server you run → [Require authorization](../serving/authorization.md). Signing a user in from a client → this page. No user present → [Authenticate without a user](./machine-auth.md). @@ -149,7 +150,13 @@ A mismatch means the callback came from an authorization server you did not star ## Pin the resource indicator -The SDK binds tokens to your server with the RFC 8707 `resource` parameter: when the server publishes protected resource metadata (RFC 9728), the SDK checks the metadata's `resource` against the server URL and attaches it to the authorization redirect and every token request. Override `validateResourceURL` to force the value — return the URL to send, or `undefined` to omit the parameter. +The SDK binds tokens to your server with the RFC 8707 `resource` parameter. Protected resource metadata (RFC 9728) discovered through a challenge must identify the requested server. For well-known discovery, it must identify the resource used to construct that discovery URL. A root well-known fallback must therefore identify the root resource. + +Before using the metadata, the SDK rejects parent paths, different queries, and different trailing path slashes relative to that expected identifier. Valid resource identifiers are sent unchanged on the authorization redirect and every token request. + +Override `validateResourceURL` to select the value after metadata identity validation — return the URL to send, or `undefined` to omit the parameter. The hook cannot bypass metadata identity validation. + +Persist the complete `OAuthDiscoveryState` returned to `saveDiscoveryState`, including `resourceMetadataRootFallback`, so a valid root fallback can be restored after a redirect. Older root-resource caches without that marker are rediscovered before use. ```ts source="../../examples/guides/clients/oauth.examples.ts#validateResourceURL_pin" class PinnedResourceProvider extends MyOAuthProvider { diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 69366521da..f82029dc00 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -14,7 +14,6 @@ import type { } from '@modelcontextprotocol/core-internal'; import { brandedHasInstance, - checkResourceAllowed, fetchWithinOrigin, LATEST_PROTOCOL_VERSION, OAuthClientInformationFullSchema, @@ -391,9 +390,8 @@ export interface OAuthClientProvider { dpop?(): DpopSession | undefined | Promise; /** - * If defined, overrides the selection and validation of the - * RFC 8707 Resource Indicator. If left undefined, default - * validation behavior will be used. + * If defined, overrides selection of the RFC 8707 Resource Indicator + * after protected resource metadata passes RFC 9728 identity validation. * * Implementations must verify the returned resource matches the MCP server. */ @@ -1174,6 +1172,7 @@ async function authInternal( let authorizationServerUrl: string | URL; let metadata: AuthorizationServerMetadata | undefined; let freshDiscoveryState: OAuthDiscoveryState | undefined; + let resourceMetadataRootFallback = false; // If resourceMetadataUrl is not provided, try to load it from cached state // This handles browser redirects where the URL was saved before navigation @@ -1182,41 +1181,57 @@ async function authInternal( effectiveResourceMetadataUrl = new URL(cachedState.resourceMetadataUrl); } - if (cachedState?.authorizationServerUrl) { + // Older caches did not distinguish a root fallback from an invalid path-level response. + const rediscoverRootMetadata = + cachedState?.resourceMetadata !== undefined && + cachedState.resourceMetadataRootFallback === undefined && + effectiveResourceMetadataUrl === undefined && + resourceMatches(new URL('/', serverUrl), cachedState.resourceMetadata.resource) && + !resourceMatches(resourceUrlFromServerUrl(serverUrl), cachedState.resourceMetadata.resource); + + if (cachedState?.authorizationServerUrl && !rediscoverRootMetadata) { // Restore discovery state from cache authorizationServerUrl = cachedState.authorizationServerUrl; resourceMetadata = cachedState.resourceMetadata; - metadata = - cachedState.authorizationServerMetadata ?? - (await discoverAuthorizationServerMetadata(authorizationServerUrl, { - fetchFn, - skipIssuerValidation: skipIssuerMetadataValidation - })); + resourceMetadataRootFallback = cachedState.resourceMetadataRootFallback === true && effectiveResourceMetadataUrl === undefined; + if (resourceMetadata) { + validateResourceMetadata(resourceMetadata, resourceMetadataRootFallback ? new URL('/', serverUrl) : serverUrl); + } // If resource metadata wasn't cached, try to fetch it for selectResourceURL if (!resourceMetadata) { try { - resourceMetadata = await discoverOAuthProtectedResourceMetadata( + const discovered = await discoverProtectedResourceMetadata( serverUrl, { resourceMetadataUrl: effectiveResourceMetadataUrl }, fetchFn ); + resourceMetadata = discovered.metadata; + resourceMetadataRootFallback = discovered.rootFallback; } catch (error) { // Network failures (DNS, connection refused) surface as TypeError — propagate // those rather than masking a transient reachability problem. - if (error instanceof TypeError) { + if (error instanceof TypeError || error instanceof ProtectedResourceMismatchError) { throw error; } // RFC 9728 not available — selectResourceURL will handle undefined } } + metadata = + cachedState.authorizationServerMetadata ?? + (await discoverAuthorizationServerMetadata(authorizationServerUrl, { + fetchFn, + skipIssuerValidation: skipIssuerMetadataValidation + })); + // Re-save if we enriched the cached state with missing metadata if (metadata !== cachedState.authorizationServerMetadata || resourceMetadata !== cachedState.resourceMetadata) { await provider.saveDiscoveryState?.({ authorizationServerUrl: String(authorizationServerUrl), resourceMetadataUrl: effectiveResourceMetadataUrl?.toString(), resourceMetadata, + ...(resourceMetadataRootFallback && { resourceMetadataRootFallback: true }), authorizationServerMetadata: metadata }); } @@ -1230,6 +1245,7 @@ async function authInternal( authorizationServerUrl = serverInfo.authorizationServerUrl; metadata = serverInfo.authorizationServerMetadata; resourceMetadata = serverInfo.resourceMetadata; + resourceMetadataRootFallback = serverInfo.resourceMetadataRootFallback === true; // 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 @@ -1241,6 +1257,7 @@ async function authInternal( authorizationServerUrl: String(authorizationServerUrl), resourceMetadataUrl: effectiveResourceMetadataUrl?.toString(), resourceMetadata, + ...(resourceMetadataRootFallback && { resourceMetadataRootFallback: true }), authorizationServerMetadata: metadata }; } @@ -1291,7 +1308,7 @@ async function authInternal( // `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 // provider's own `validateResourceURL` is used as returned. - const selectedResource = await selectResourceURL(serverUrl, provider, resourceMetadata); + const selectedResource = await selectValidatedResourceURL(serverUrl, provider, resourceMetadata); const resource: string | URL | undefined = selectedResource && resourceMetadata && !provider.validateResourceURL ? resourceMetadata.resource : selectedResource; @@ -1516,8 +1533,8 @@ export function isHttpsUrl(value?: string): boolean { /** * Selects the RFC 8707 resource indicator for an MCP server: the provider's * {@linkcode OAuthClientProvider.validateResourceURL | validateResourceURL} result when - * implemented, otherwise the protected resource metadata's `resource` (checked against the - * server URL with `checkResourceAllowed`), or `undefined` when there is no metadata. + * implemented, otherwise the protected resource metadata's `resource`, or `undefined` when + * there is no metadata. Metadata must identify the supplied server URL; parent paths are rejected. * * The result is a parsed `URL`, so a pathless indicator such as `https://example.com` has * the `href` `https://example.com/`. {@linkcode auth} therefore sends the metadata string @@ -1528,6 +1545,15 @@ export async function selectResourceURL( serverUrl: string | URL, provider: OAuthClientProvider, resourceMetadata?: OAuthProtectedResourceMetadata +): Promise { + if (resourceMetadata) validateResourceMetadata(resourceMetadata, serverUrl); + return selectValidatedResourceURL(serverUrl, provider, resourceMetadata); +} + +async function selectValidatedResourceURL( + serverUrl: string | URL, + provider: OAuthClientProvider, + resourceMetadata?: OAuthProtectedResourceMetadata ): Promise { const defaultResource = resourceUrlFromServerUrl(serverUrl); @@ -1541,14 +1567,33 @@ export async function selectResourceURL( return undefined; } - // Validate that the metadata's resource is compatible with our request - if (!checkResourceAllowed({ requestedResource: defaultResource, configuredResource: resourceMetadata.resource })) { - throw new Error(`Protected resource ${resourceMetadata.resource} does not match expected ${defaultResource} (or origin)`); - } // Prefer the resource from metadata since it's what the server is telling us to request return new URL(resourceMetadata.resource); } +class ProtectedResourceMismatchError extends Error { + override name = 'ProtectedResourceMismatchError'; +} + +function resourceMatches(expected: URL, resource: string): boolean { + // Normalize the authority and empty root path, preserving path spelling, query and fragment. + const match = /^https?:\/\/[^/?#\\\s]+(.*)$/i.exec(resource); + if (!match || match[0] !== resource) return false; + const actual = new URL(resource); + if (actual.username || actual.password || actual.href !== expected.href) return false; + const suffix = match[1]!; + return `${actual.origin}${suffix.startsWith('/') ? suffix : `/${suffix}`}` === expected.href; +} + +function validateResourceMetadata(metadata: OAuthProtectedResourceMetadata, expectedResource: string | URL): void { + const expected = resourceUrlFromServerUrl(expectedResource); + if (!resourceMatches(expected, metadata.resource)) { + throw new ProtectedResourceMismatchError( + `Protected resource ${JSON.stringify(metadata.resource)} does not match expected ${JSON.stringify(expected.href)} (RFC 9728 §3.3)` + ); + } +} + /** Auth-scheme challenge tokens {@linkcode extractWWWAuthenticateParams} recognizes. */ const RECOGNIZED_CHALLENGE_SCHEMES = new Set(['bearer', 'dpop']); @@ -1667,7 +1712,16 @@ export async function discoverOAuthProtectedResourceMetadata( opts?: { protocolVersion?: string; resourceMetadataUrl?: string | URL }, fetchFn: FetchLike = fetch ): Promise { - const response = await discoverMetadataWithFallback(serverUrl, 'oauth-protected-resource', fetchFn, { + const discovered = await discoverProtectedResourceMetadata(serverUrl, opts, fetchFn); + return discovered.metadata; +} + +async function discoverProtectedResourceMetadata( + serverUrl: string | URL, + opts?: { protocolVersion?: string; resourceMetadataUrl?: string | URL }, + fetchFn: FetchLike = fetch +): Promise<{ metadata: OAuthProtectedResourceMetadata; rootFallback: boolean }> { + const { response, rootFallback } = await discoverMetadataWithFallback(serverUrl, 'oauth-protected-resource', fetchFn, { protocolVersion: opts?.protocolVersion, metadataUrl: opts?.resourceMetadataUrl }); @@ -1681,7 +1735,9 @@ export async function discoverOAuthProtectedResourceMetadata( await response.text?.().catch(() => {}); throw new Error(`HTTP ${response.status} trying to load well-known OAuth protected resource metadata.`); } - return OAuthProtectedResourceMetadataSchema.parse(await response.json()); + const metadata = OAuthProtectedResourceMetadataSchema.parse(await response.json()); + validateResourceMetadata(metadata, rootFallback ? new URL('/', serverUrl) : serverUrl); + return { metadata, rootFallback }; } /** @@ -1771,7 +1827,7 @@ async function discoverMetadataWithFallback( wellKnownType: 'oauth-authorization-server' | 'oauth-protected-resource', fetchFn: FetchLike, opts?: { protocolVersion?: string; metadataUrl?: string | URL; metadataServerUrl?: string | URL } -): Promise { +): Promise<{ response: Response | undefined; rootFallback: boolean }> { const issuer = new URL(serverUrl); const protocolVersion = opts?.protocolVersion ?? LATEST_PROTOCOL_VERSION; @@ -1780,20 +1836,25 @@ async function discoverMetadataWithFallback( url = new URL(opts.metadataUrl); } else { // Try path-aware discovery first - const wellKnownPath = buildWellKnownPath(wellKnownType, issuer.pathname); + const wellKnownPath = + wellKnownType === 'oauth-protected-resource' + ? `/.well-known/${wellKnownType}${issuer.pathname === '/' ? '' : issuer.pathname}` + : buildWellKnownPath(wellKnownType, issuer.pathname); url = new URL(wellKnownPath, opts?.metadataServerUrl ?? issuer); url.search = issuer.search; } let response = await tryMetadataDiscovery(url, protocolVersion, fetchFn); + let rootFallback = false; // If path-aware discovery fails (4xx, 502 or a redirect not followed) and we're not already at root, try fallback to root discovery if (!opts?.metadataUrl && shouldAttemptFallback(response, issuer.pathname)) { const rootUrl = new URL(`/.well-known/${wellKnownType}`, issuer); response = await tryMetadataDiscovery(rootUrl, protocolVersion, fetchFn); + rootFallback = true; } - return response; + return { response, rootFallback }; } /** @@ -1826,7 +1887,7 @@ export async function discoverOAuthMetadata( } protocolVersion ??= LATEST_PROTOCOL_VERSION; - const response = await discoverMetadataWithFallback(authorizationServerUrl, 'oauth-authorization-server', fetchFn, { + const { response } = await discoverMetadataWithFallback(authorizationServerUrl, 'oauth-authorization-server', fetchFn, { protocolVersion, metadataServerUrl: authorizationServerUrl }); @@ -2023,6 +2084,9 @@ export interface OAuthServerInfo { * or `undefined` if the server does not support it. */ resourceMetadata?: OAuthProtectedResourceMetadata; + + /** True when the metadata was validated against the root well-known fallback; persist with discovery state. */ + resourceMetadataRootFallback?: boolean; } /** @@ -2059,13 +2123,16 @@ export async function discoverOAuthServerInfo( ): Promise { let resourceMetadata: OAuthProtectedResourceMetadata | undefined; let authorizationServerUrl: string | undefined; + let resourceMetadataRootFallback = false; try { - resourceMetadata = await discoverOAuthProtectedResourceMetadata( + const discovered = await discoverProtectedResourceMetadata( serverUrl, { resourceMetadataUrl: opts?.resourceMetadataUrl }, opts?.fetchFn ); + resourceMetadata = discovered.metadata; + resourceMetadataRootFallback = discovered.rootFallback; if (resourceMetadata.authorization_servers && resourceMetadata.authorization_servers.length > 0) { authorizationServerUrl = resourceMetadata.authorization_servers[0]; } @@ -2073,7 +2140,7 @@ export async function discoverOAuthServerInfo( // Network failures (DNS, connection refused) surface as TypeError from fetch. Those are // transient reachability problems, not "server doesn't support PRM" — propagate so the // caller sees the real error instead of silently falling back to a different auth server. - if (error instanceof TypeError) { + if (error instanceof TypeError || error instanceof ProtectedResourceMismatchError) { throw error; } // RFC 9728 not supported -- fall back to treating the server URL as the authorization server @@ -2093,7 +2160,8 @@ export async function discoverOAuthServerInfo( return { authorizationServerUrl, authorizationServerMetadata, - resourceMetadata + resourceMetadata, + ...(resourceMetadataRootFallback && { resourceMetadataRootFallback: true }) }; } diff --git a/packages/client/test/client/auth.resourceIdentity.test.ts b/packages/client/test/client/auth.resourceIdentity.test.ts new file mode 100644 index 0000000000..82a7a7a027 --- /dev/null +++ b/packages/client/test/client/auth.resourceIdentity.test.ts @@ -0,0 +1,223 @@ +import type { FetchLike, StoredOAuthTokens } from '@modelcontextprotocol/core-internal'; +import { describe, expect, it, vi } from 'vitest'; + +import type { OAuthClientProvider, OAuthDiscoveryState } from '../../src/client/auth'; +import { auth, discoverOAuthProtectedResourceMetadata, discoverOAuthServerInfo, selectResourceURL } from '../../src/client/auth'; + +const SERVER = 'https://resource.example/mcp/tenant'; +const ROOT = 'https://resource.example'; +const PRM = `${ROOT}/.well-known/oauth-protected-resource`; +const CHALLENGE = `${ROOT}/metadata`; +const ISSUER = 'https://auth.example'; +const AS_METADATA = { + issuer: ISSUER, + authorization_endpoint: `${ISSUER}/authorize`, + token_endpoint: `${ISSUER}/token`, + response_types_supported: ['code'], + code_challenge_methods_supported: ['S256'] +}; +const metadata = (resource: string) => ({ resource, authorization_servers: [ISSUER] }); + +function createProvider(initialState?: OAuthDiscoveryState) { + let state = initialState; + let tokens: StoredOAuthTokens | undefined; + const provider = { + redirectUrl: 'http://localhost/callback', + clientMetadata: { redirect_uris: ['http://localhost/callback'], token_endpoint_auth_method: 'none' }, + clientInformation: vi.fn(() => ({ client_id: 'client', issuer: ISSUER })), + tokens: vi.fn(() => tokens), + saveTokens: vi.fn((value: StoredOAuthTokens) => { + tokens = value; + }), + redirectToAuthorization: vi.fn(), + saveCodeVerifier: vi.fn(), + codeVerifier: vi.fn(() => 'verifier'), + discoveryState: vi.fn(() => state), + saveDiscoveryState: vi.fn((value: OAuthDiscoveryState) => { + state = JSON.parse(JSON.stringify(value)); + }) + } satisfies OAuthClientProvider; + return provider; +} + +describe('RFC 9728 protected resource identity (#3077)', () => { + it.each([ + ['challenge parent', SERVER, `${ROOT}/mcp`, true], + ['challenge root', SERVER, ROOT, true], + ['path-aware parent', SERVER, `${ROOT}/mcp`, false], + ['query mismatch', `${ROOT}/mcp?tenant=a`, `${ROOT}/mcp?tenant=b`, true], + ['different origin', SERVER, 'https://other.example/mcp/tenant', true], + ['sibling', SERVER, `${ROOT}/mcp/other`, true], + ['non-segment prefix', SERVER, `${ROOT}/mc`, true], + ['extra trailing slash', SERVER, `${SERVER}/`, true], + ['missing trailing slash', `${SERVER}/`, SERVER, true], + ['dot segment', SERVER, `${ROOT}/mcp/./tenant`, true], + ['encoded segment', SERVER, `${ROOT}/mcp/%74enant`, true], + ['challenge backslash in authority', SERVER, `${ROOT}\\other/mcp/tenant`, true], + ['path-aware backslash in authority', SERVER, `${ROOT}\\other/mcp/tenant`, false], + ['fragment', SERVER, `${SERVER}#fragment`, true] + ])('rejects %s before using metadata or invoking provider hooks', async (_, serverUrl, resource, challenge) => { + const provider = { ...createProvider(), validateResourceURL: vi.fn().mockResolvedValue(new URL(SERVER)) }; + const fetchFn = vi.fn().mockResolvedValue(Response.json(metadata(resource as string))); + + await expect( + auth(provider, { + serverUrl: serverUrl as string, + ...(challenge ? { resourceMetadataUrl: new URL(CHALLENGE) } : {}), + fetchFn + }) + ).rejects.toThrow('does not match expected'); + + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(provider.saveDiscoveryState).not.toHaveBeenCalled(); + expect(provider.clientInformation).not.toHaveBeenCalled(); + expect(provider.validateResourceURL).not.toHaveBeenCalled(); + expect(provider.redirectToAuthorization).not.toHaveBeenCalled(); + expect(provider.saveTokens).not.toHaveBeenCalled(); + }); + + it('rejects a root document claiming a non-root resource', async () => { + const fetchFn = vi + .fn() + .mockResolvedValueOnce(new Response(null, { status: 404 })) + .mockResolvedValueOnce(Response.json(metadata(`${ROOT}/mcp`))); + await expect(discoverOAuthServerInfo(SERVER, { fetchFn })).rejects.toThrow('does not match expected'); + expect(fetchFn.mock.calls.map(([url]) => String(url))).toEqual([`${PRM}/mcp/tenant`, PRM]); + }); + + it('preserves a trailing path slash in the PRM discovery URL', async () => { + const fetchFn = vi.fn().mockResolvedValue(Response.json(metadata(`${SERVER}/`))); + await expect(discoverOAuthProtectedResourceMetadata(`${SERVER}/`, undefined, fetchFn)).resolves.toEqual(metadata(`${SERVER}/`)); + expect(String(fetchFn.mock.calls[0]![0])).toBe(`${PRM}/mcp/tenant/`); + }); + + it.each(['challenge', 'path', 'root'] as const)( + 'preserves resource through authorization, cached code exchange and refresh: %s', + async route => { + const resource = route === 'root' ? ROOT : SERVER; + const provider = createProvider(); + const fetchFn = vi.fn(async input => { + const url = String(input); + if (url === `${ISSUER}/token`) + return Response.json({ access_token: 'token', token_type: 'Bearer', refresh_token: 'refresh' }); + if (url === `${ISSUER}/.well-known/oauth-authorization-server`) return Response.json(AS_METADATA); + if (route === 'root' && url === `${PRM}/mcp/tenant`) return new Response(null, { status: 404 }); + if (url === (route === 'challenge' ? CHALLENGE : route === 'root' ? PRM : `${PRM}/mcp/tenant`)) + return Response.json(metadata(resource)); + throw new Error(`Unexpected request: ${url}`); + }); + const options = { serverUrl: SERVER, fetchFn, ...(route === 'challenge' ? { resourceMetadataUrl: new URL(CHALLENGE) } : {}) }; + await expect(auth(provider, options)).resolves.toBe('REDIRECT'); + const authorizationUrl = provider.redirectToAuthorization.mock.calls[0]![0] as URL; + expect(authorizationUrl.searchParams.get('resource')).toBe(resource); + expect(provider.discoveryState()?.resourceMetadataRootFallback).toBe(route === 'root' ? true : undefined); + + fetchFn.mockClear(); + await expect(auth(provider, { serverUrl: SERVER, fetchFn, authorizationCode: 'code' })).resolves.toBe('AUTHORIZED'); + await expect(auth(provider, { serverUrl: SERVER, fetchFn })).resolves.toBe('AUTHORIZED'); + expect(fetchFn).toHaveBeenCalledTimes(2); + for (const [url, init] of fetchFn.mock.calls) { + expect(String(url)).toBe(`${ISSUER}/token`); + expect(new URLSearchParams(init?.body as string).get('resource')).toBe(resource); + } + expect(new URLSearchParams(fetchFn.mock.calls[1]![1]?.body as string).get('grant_type')).toBe('refresh_token'); + } + ); + + it.each([ + { resourceMetadata: metadata(`${ROOT}/mcp`) }, + { resourceMetadata: metadata(`${ROOT}\\other/mcp/tenant`) }, + { resourceMetadata: metadata('https://other.example'), resourceMetadataRootFallback: true } + ])('rejects invalid cached metadata before any network request', async cached => { + const provider = createProvider({ authorizationServerUrl: ISSUER, ...cached }); + const fetchFn = vi.fn(); + await expect(auth(provider, { serverUrl: SERVER, fetchFn })).rejects.toThrow('does not match expected'); + expect(fetchFn).not.toHaveBeenCalled(); + expect(provider.saveDiscoveryState).not.toHaveBeenCalled(); + }); + + it('does not let cached root provenance bypass a new challenge', async () => { + const provider = createProvider({ + authorizationServerUrl: ISSUER, + resourceMetadata: metadata(ROOT), + resourceMetadataRootFallback: true + }); + const fetchFn = vi.fn(); + await expect(auth(provider, { serverUrl: SERVER, resourceMetadataUrl: new URL(CHALLENGE), fetchFn })).rejects.toThrow( + 'does not match expected' + ); + expect(fetchFn).not.toHaveBeenCalled(); + }); + + it('validates fetched PRM before enriching a partial discovery cache', async () => { + const provider = createProvider({ authorizationServerUrl: ISSUER }); + const fetchFn = vi.fn().mockResolvedValue(Response.json(metadata(`${ROOT}/mcp`))); + await expect(auth(provider, { serverUrl: SERVER, fetchFn })).rejects.toThrow('does not match expected'); + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(provider.saveDiscoveryState).not.toHaveBeenCalled(); + }); + + it.each([true, false])('rediscovers an old root cache instead of assuming its source (valid: %s)', async valid => { + const provider = createProvider({ + authorizationServerUrl: ISSUER, + resourceMetadata: metadata(ROOT), + authorizationServerMetadata: AS_METADATA + }); + const fetchFn = vi.fn(async input => { + const url = String(input); + if (url === `${PRM}/mcp/tenant`) return valid ? new Response(null, { status: 404 }) : Response.json(metadata(ROOT)); + if (url === PRM) return Response.json(metadata(ROOT)); + if (url === `${ISSUER}/.well-known/oauth-authorization-server`) return Response.json(AS_METADATA); + throw new Error(`Unexpected request: ${url}`); + }); + if (valid) { + await expect(auth(provider, { serverUrl: SERVER, fetchFn })).resolves.toBe('REDIRECT'); + expect(provider.discoveryState()?.resourceMetadataRootFallback).toBe(true); + expect(fetchFn).toHaveBeenCalledTimes(3); + } else { + await expect(auth(provider, { serverUrl: SERVER, fetchFn })).rejects.toThrow('does not match expected'); + expect(provider.saveDiscoveryState).not.toHaveBeenCalled(); + expect(fetchFn).toHaveBeenCalledTimes(1); + } + }); + + it('keeps the original server URL for custom resource selection after root discovery', async () => { + const validateResourceURL = vi.fn().mockResolvedValue(undefined); + const provider = { + ...createProvider({ + authorizationServerUrl: ISSUER, + authorizationServerMetadata: AS_METADATA, + resourceMetadata: metadata(ROOT), + resourceMetadataRootFallback: true + }), + validateResourceURL + }; + await expect(auth(provider, { serverUrl: SERVER, fetchFn: vi.fn() })).resolves.toBe('REDIRECT'); + expect(validateResourceURL).toHaveBeenCalledWith(new URL(SERVER), ROOT); + }); + + it('validates identity for direct selectResourceURL callers too', async () => { + await expect(selectResourceURL(SERVER, createProvider(), metadata(`${ROOT}/mcp`))).rejects.toThrow('does not match expected'); + await expect(selectResourceURL(SERVER, createProvider(), metadata(SERVER))).resolves.toEqual(new URL(SERVER)); + }); + + it.each([`${ROOT}\\other/mcp/tenant`, `${ROOT}\\/mcp/tenant`])( + 'rejects ambiguous authority boundaries in direct resource selection: %s', + async resource => { + await expect(selectResourceURL(SERVER, createProvider(), metadata(resource))).rejects.toThrow('does not match expected'); + } + ); + + it('accepts a pathless resource with a query without rewriting it', async () => { + const resource = 'HTTPS://RESOURCE.example?tenant=a'; + const fetchFn = vi.fn().mockResolvedValue(Response.json(metadata(resource))); + await expect(discoverOAuthProtectedResourceMetadata(`${ROOT}?tenant=a`, undefined, fetchFn)).resolves.toEqual(metadata(resource)); + expect(String(fetchFn.mock.calls[0]![0])).toBe(`${PRM}?tenant=a`); + }); + + it('accepts authority case and the empty root path without rewriting the selected indicator', async () => { + const resource = 'https://RESOURCE.example:443'; + const fetchFn = vi.fn().mockResolvedValue(Response.json(metadata(resource))); + await expect(discoverOAuthProtectedResourceMetadata(ROOT, undefined, fetchFn)).resolves.toEqual(metadata(resource)); + }); +}); diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index b3cd42bd78..83fe031dbc 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -353,14 +353,15 @@ describe('OAuth Authorization', () => { }); it('returns metadata when discovery succeeds with path', async () => { + const pathMetadata = { ...validMetadata, resource: 'https://resource.example.com/path/name' }; mockFetch.mockResolvedValueOnce({ ok: true, status: 200, - json: async () => validMetadata + json: async () => pathMetadata }); const metadata = await discoverOAuthProtectedResourceMetadata('https://resource.example.com/path/name'); - expect(metadata).toEqual(validMetadata); + expect(metadata).toEqual(pathMetadata); const calls = mockFetch.mock.calls; expect(calls.length).toBe(1); const [url] = calls[0]!; @@ -368,14 +369,15 @@ describe('OAuth Authorization', () => { }); it('preserves query parameters in path-aware discovery', async () => { + const pathMetadata = { ...validMetadata, resource: 'https://resource.example.com/path?param=value' }; mockFetch.mockResolvedValueOnce({ ok: true, status: 200, - json: async () => validMetadata + json: async () => pathMetadata }); const metadata = await discoverOAuthProtectedResourceMetadata('https://resource.example.com/path?param=value'); - expect(metadata).toEqual(validMetadata); + expect(metadata).toEqual(pathMetadata); const calls = mockFetch.mock.calls; expect(calls.length).toBe(1); const [url] = calls[0]!; @@ -3381,15 +3383,14 @@ describe('OAuth Authorization', () => { warn.mockRestore(); }); - it('skips default PRM resource validation when custom validateResourceURL is provided', async () => { + it('allows custom resource selection after validating PRM identity', async () => { const mockValidateResourceURL = vi.fn().mockResolvedValue(undefined); const providerWithCustomValidation = { ...mockProvider, validateResourceURL: mockValidateResourceURL }; - // Mock protected resource metadata with mismatched resource URL - // This would normally throw an error in default validation, but should be skipped + // A provider may omit the resource parameter after metadata identity is validated. mockFetch.mockImplementation(url => { const urlString = url.toString(); @@ -3398,7 +3399,7 @@ describe('OAuth Authorization', () => { ok: true, status: 200, json: async () => ({ - resource: 'https://different-resource.example.com/mcp-server', // Mismatched resource + resource: 'https://api.example.com/mcp-server', authorization_servers: ['https://auth.example.com'] }) }); @@ -3428,7 +3429,7 @@ describe('OAuth Authorization', () => { (providerWithCustomValidation.saveCodeVerifier as Mock).mockResolvedValue(undefined); (providerWithCustomValidation.redirectToAuthorization as Mock).mockResolvedValue(undefined); - // Call auth - should succeed despite resource mismatch because custom validation overrides default + // Call auth with valid metadata and custom resource selection. const result = await auth(providerWithCustomValidation, { serverUrl: 'https://api.example.com/mcp-server' }); @@ -3438,21 +3439,23 @@ describe('OAuth Authorization', () => { // Verify custom validation method was called expect(mockValidateResourceURL).toHaveBeenCalledWith( new URL('https://api.example.com/mcp-server'), - 'https://different-resource.example.com/mcp-server' + 'https://api.example.com/mcp-server' ); }); - it('uses prefix of server URL from PRM resource as resource parameter', async () => { - // Mock successful metadata discovery with resource URL that is a prefix of requested URL + it('uses the root resource when path-aware discovery falls back to root metadata', async () => { mockFetch.mockImplementation(url => { const urlString = url.toString(); - if (urlString.includes('/.well-known/oauth-protected-resource')) { + if (urlString === 'https://api.example.com/.well-known/oauth-protected-resource/mcp-server/endpoint') { + return Promise.resolve({ ok: false, status: 404 }); + } + if (urlString === 'https://api.example.com/.well-known/oauth-protected-resource') { return Promise.resolve({ ok: true, status: 200, json: async () => ({ - // Resource is a prefix of the requested server URL + // The resource identifies the root discovery target. resource: 'https://api.example.com/', authorization_servers: ['https://auth.example.com'] }) @@ -3511,7 +3514,10 @@ describe('OAuth Authorization', () => { mockFetch.mockImplementation(url => { const urlString = url.toString(); - if (urlString.includes('/.well-known/oauth-protected-resource')) { + if (urlString === 'https://example.com/.well-known/oauth-protected-resource/mcp') { + return Promise.resolve({ ok: false, status: 404 }); + } + if (urlString === 'https://example.com/.well-known/oauth-protected-resource') { return Promise.resolve({ ok: true, status: 200, @@ -3905,7 +3911,7 @@ describe('OAuth Authorization', () => { ok: true, status: 200, json: async () => ({ - resource: 'https://my.resource.com/', + resource: 'https://my.resource.com/path/name', authorization_servers: ['https://auth.example.com/oauth'] }) }); diff --git a/packages/client/test/client/redirects.test.ts b/packages/client/test/client/redirects.test.ts index 02354e6e80..7bd9596b04 100644 --- a/packages/client/test/client/redirects.test.ts +++ b/packages/client/test/client/redirects.test.ts @@ -241,7 +241,7 @@ describe('redirects', () => { }); test('protected resource metadata discovery tries the root well-known URL', async () => { - const metadata = { resource: new URL('/mcp', endpointUrl).href, authorization_servers: [endpointUrl.origin] }; + const metadata = { resource: endpointUrl.origin, authorization_servers: [endpointUrl.origin] }; handle = (req, res, recorded) => req.url === '/.well-known/oauth-protected-resource' ? void res.writeHead(200, { 'content-type': 'application/json' }).end(JSON.stringify(metadata)) @@ -400,7 +400,7 @@ describe('redirects', () => { (req, res, recorded) => { const origin = endpointUrl.origin; const metadata = req.url?.startsWith('/.well-known/oauth-protected-resource') - ? { resource: origin, authorization_servers: [origin] } + ? { resource: origin + req.url.slice('/.well-known/oauth-protected-resource'.length), authorization_servers: [origin] } : req.url?.startsWith('/.well-known/oauth-authorization-server') ? { issuer: origin,