diff --git a/.changeset/oauth-prm-resource-fragment-validation.md b/.changeset/oauth-prm-resource-fragment-validation.md new file mode 100644 index 000000000..6d28d7763 --- /dev/null +++ b/.changeset/oauth-prm-resource-fragment-validation.md @@ -0,0 +1,6 @@ +--- +'@modelcontextprotocol/core': patch +'@modelcontextprotocol/client': patch +--- + +Tighten OAuth Protected Resource Metadata `resource` validation per RFC 8707 §2: identifiers containing a fragment component are now rejected by `OAuthProtectedResourceMetadataSchema`. The error message thrown by `selectResourceURL` on origin/path mismatch now points at `OAuthClientProvider.validateResourceURL` as the supported override for non-URL RFC 8707 indicators (e.g. `urn:`, `api://`) or identifiers served from a different origin. The `validateResourceURL` JSDoc has been clarified to document this override use case and reference RFC 9728 §3.3 / §7.3 for the strict-default rationale. diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 5f55fb7a0..09424528e 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -234,7 +234,16 @@ export interface OAuthClientProvider { * RFC 8707 Resource Indicator. If left undefined, default * validation behavior will be used. * - * Implementations must verify the returned resource matches the MCP server. + * The default validation accepts URL `resource` values with no fragment that share + * origin with, and are equal to or a parent path of, the MCP server URL. This is + * stricter than RFC 8707 (which permits any absolute URI) and looser than RFC 9728 + * §3.3 (which requires identity with the resource identifier used for discovery). + * Implement this hook to opt in to accepting: + * - non-URL absolute URI resource indicators per RFC 8707 §2 (e.g. `urn:...`, `api://...`) + * - identifiers served from a different origin than the MCP endpoint + * + * Implementations remain responsible for any audience-binding policy required by the + * application. See RFC 9728 §3.3 / §7.3 for the security rationale behind the strict default. */ validateResourceURL?(serverUrl: string | URL, resource?: string): Promise; @@ -857,9 +866,15 @@ export async function selectResourceURL( return undefined; } - // Validate that the metadata's resource is compatible with our request + // Validate that the metadata's resource is compatible with our request. + // Per RFC 9728 §3.3 / §7.3, refusing mismatched PRM resources prevents an attacker-controlled + // endpoint from advertising metadata that impersonates another resource. if (!checkResourceAllowed({ requestedResource: defaultResource, configuredResource: resourceMetadata.resource })) { - throw new Error(`Protected resource ${resourceMetadata.resource} does not match expected ${defaultResource} (or origin)`); + throw new Error( + `Protected resource ${resourceMetadata.resource} does not match expected ${defaultResource} (or origin). ` + + `Override OAuthClientProvider.validateResourceURL to opt in to accepting non-URL RFC 8707 resource indicators ` + + `(e.g. 'urn:' or 'api://') or identifiers served from a different origin.` + ); } // Prefer the resource from metadata since it's what the server is telling us to request return new URL(resourceMetadata.resource); diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 04d7f4a3f..c7de45210 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -250,6 +250,34 @@ describe('OAuth Authorization', () => { await expect(discoverOAuthProtectedResourceMetadata('https://resource.example.com')).rejects.toThrow(); }); + it('rejects metadata whose resource identifier includes a fragment (RFC 8707 §2)', async () => { + mockFetch.mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => ({ + resource: 'https://resource.example.com/mcp#section' + }) + }); + + await expect(discoverOAuthProtectedResourceMetadata('https://resource.example.com')).rejects.toThrow( + "Protected Resource Metadata 'resource' MUST NOT include a fragment component (RFC 8707 §2)" + ); + }); + + it('rejects metadata whose resource identifier ends with an empty fragment', async () => { + mockFetch.mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => ({ + resource: 'https://resource.example.com/mcp#' + }) + }); + + await expect(discoverOAuthProtectedResourceMetadata('https://resource.example.com')).rejects.toThrow( + "Protected Resource Metadata 'resource' MUST NOT include a fragment component (RFC 8707 §2)" + ); + }); + it('returns metadata when discovery succeeds with path', async () => { mockFetch.mockResolvedValueOnce({ ok: true, @@ -2652,6 +2680,54 @@ describe('OAuth Authorization', () => { ); }); + it('throws an actionable error pointing at validateResourceURL when PRM resource origin mismatches', async () => { + // Same setup as the override test above, but with NO override on the provider — + // this exercises the default `selectResourceURL` rejection path and asserts the + // error guides integrators to the documented escape hatch. + mockFetch.mockImplementation(url => { + const urlString = url.toString(); + + if (urlString.includes('/.well-known/oauth-protected-resource')) { + return Promise.resolve({ + ok: true, + status: 200, + json: async () => ({ + resource: 'https://different-resource.example.com/mcp-server', + authorization_servers: ['https://auth.example.com'] + }) + }); + } else if (urlString.includes('/.well-known/oauth-authorization-server')) { + return Promise.resolve({ + ok: true, + status: 200, + json: async () => ({ + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/authorize', + token_endpoint: 'https://auth.example.com/token', + response_types_supported: ['code'], + code_challenge_methods_supported: ['S256'] + }) + }); + } + + return Promise.resolve({ ok: false, status: 404 }); + }); + + (mockProvider.clientInformation as Mock).mockResolvedValue({ + client_id: 'test-client', + client_secret: 'test-secret' + }); + (mockProvider.tokens as Mock).mockResolvedValue(undefined); + (mockProvider.saveCodeVerifier as Mock).mockResolvedValue(undefined); + (mockProvider.redirectToAuthorization as Mock).mockResolvedValue(undefined); + + await expect( + auth(mockProvider, { + serverUrl: 'https://api.example.com/mcp-server' + }) + ).rejects.toThrow(/validateResourceURL/); + }); + 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 mockFetch.mockImplementation(url => { diff --git a/packages/core/src/shared/auth.ts b/packages/core/src/shared/auth.ts index deee583aa..334d3ca01 100644 --- a/packages/core/src/shared/auth.ts +++ b/packages/core/src/shared/auth.ts @@ -28,7 +28,12 @@ export const SafeUrlSchema = z * RFC 9728 OAuth Protected Resource Metadata */ export const OAuthProtectedResourceMetadataSchema = z.looseObject({ - resource: z.string().url(), + resource: z + .string() + .url() + .refine(value => !value.includes('#'), { + message: "Protected Resource Metadata 'resource' MUST NOT include a fragment component (RFC 8707 §2)" + }), authorization_servers: z.array(SafeUrlSchema).optional(), jwks_uri: z.string().url().optional(), scopes_supported: z.array(z.string()).optional(), diff --git a/packages/core/test/shared/auth.test.ts b/packages/core/test/shared/auth.test.ts index 770e0c4d4..4f530ac5f 100644 --- a/packages/core/test/shared/auth.test.ts +++ b/packages/core/test/shared/auth.test.ts @@ -1,6 +1,7 @@ import { OAuthClientMetadataSchema, OAuthMetadataSchema, + OAuthProtectedResourceMetadataSchema, OpenIdProviderMetadataSchema, OptionalSafeUrlSchema, SafeUrlSchema @@ -70,6 +71,46 @@ describe('OAuthMetadataSchema', () => { }); }); +describe('OAuthProtectedResourceMetadataSchema', () => { + it('accepts a resource identifier without a fragment', () => { + const metadata = { + resource: 'https://api.example.com/mcp' + }; + + expect(() => OAuthProtectedResourceMetadataSchema.parse(metadata)).not.toThrow(); + }); + + it('rejects a resource identifier with a fragment', () => { + const metadata = { + resource: 'https://api.example.com/mcp#frag' + }; + + expect(() => OAuthProtectedResourceMetadataSchema.parse(metadata)).toThrow( + "Protected Resource Metadata 'resource' MUST NOT include a fragment component (RFC 8707 §2)" + ); + }); + + it('rejects a resource identifier with an empty fragment', () => { + // RFC 3986 §3.5: presence of "#" delimits a fragment, even when empty. + const metadata = { + resource: 'https://api.example.com/mcp#' + }; + + expect(() => OAuthProtectedResourceMetadataSchema.parse(metadata)).toThrow( + "Protected Resource Metadata 'resource' MUST NOT include a fragment component (RFC 8707 §2)" + ); + }); + + it('accepts percent-encoded "#" inside the path or query', () => { + // %23 is not a fragment delimiter; only a literal "#" is. + const metadata = { + resource: 'https://api.example.com/mcp%23section' + }; + + expect(() => OAuthProtectedResourceMetadataSchema.parse(metadata)).not.toThrow(); + }); +}); + describe('OpenIdProviderMetadataSchema', () => { it('validates complete OpenID Provider metadata', () => { const metadata = {