From 9383b54d50c76bc779e34d9f39cbf8fa18fd13d4 Mon Sep 17 00:00:00 2001 From: Gal Zahavi <38544478+galz10@users.noreply.github.com> Date: Fri, 19 Dec 2025 11:33:20 -0800 Subject: [PATCH] Validate OAuth resource parameter matches MCP server URL (#15289) --- packages/core/src/mcp/oauth-provider.test.ts | 5 +- packages/core/src/mcp/oauth-provider.ts | 8 ++- packages/core/src/mcp/oauth-utils.test.ts | 54 ++++++++++++++++++++ packages/core/src/mcp/oauth-utils.ts | 38 ++++++++++++++ 4 files changed, 103 insertions(+), 2 deletions(-) diff --git a/packages/core/src/mcp/oauth-provider.test.ts b/packages/core/src/mcp/oauth-provider.test.ts index 5ddc607064..018c2d8c71 100644 --- a/packages/core/src/mcp/oauth-provider.test.ts +++ b/packages/core/src/mcp/oauth-provider.test.ts @@ -255,6 +255,7 @@ describe('MCPOAuthProvider', () => { delete configWithoutAuth.tokenUrl; const mockResourceMetadata = { + resource: 'https://api.example.com/', authorization_servers: ['https://discovered.auth.com'], }; @@ -513,7 +514,7 @@ describe('MCPOAuthProvider', () => { delete configWithoutClientAndAuthorizationUrl.authorizationUrl; const mockResourceMetadata: OAuthProtectedResourceMetadata = { - resource: 'https://api.example.com', + resource: 'https://api.example.com/', authorization_servers: ['https://auth.example.com'], }; @@ -1474,6 +1475,7 @@ describe('MCPOAuthProvider', () => { delete configWithUserScopes.tokenUrl; const mockResourceMetadata = { + resource: 'https://api.example.com/', authorization_servers: ['https://discovered.auth.com'], }; @@ -1562,6 +1564,7 @@ describe('MCPOAuthProvider', () => { delete configWithoutScopes.tokenUrl; const mockResourceMetadata = { + resource: 'https://api.example.com/', authorization_servers: ['https://discovered.auth.com'], }; diff --git a/packages/core/src/mcp/oauth-provider.ts b/packages/core/src/mcp/oauth-provider.ts index 6c20745843..bdadd0d54c 100644 --- a/packages/core/src/mcp/oauth-provider.ts +++ b/packages/core/src/mcp/oauth-provider.ts @@ -13,7 +13,7 @@ import { openBrowserSecurely } from '../utils/secure-browser-launcher.js'; import type { OAuthToken } from './token-storage/types.js'; import { MCPOAuthTokenStorage } from './oauth-token-storage.js'; import { getErrorMessage } from '../utils/errors.js'; -import { OAuthUtils } from './oauth-utils.js'; +import { OAuthUtils, ResourceMismatchError } from './oauth-utils.js'; import { coreEvents } from '../utils/events.js'; import { debugLogger } from '../utils/debugLogger.js'; @@ -744,6 +744,7 @@ export class MCPOAuthProvider { const discoveredConfig = await OAuthUtils.discoverOAuthFromWWWAuthenticate( wwwAuthenticate, + mcpServerUrl, ); if (discoveredConfig) { // Merge discovered config with existing config, preserving clientId and clientSecret @@ -760,6 +761,11 @@ export class MCPOAuthProvider { } } } catch (error) { + // Re-throw security validation errors + if (error instanceof ResourceMismatchError) { + throw error; + } + debugLogger.debug( `Failed to check endpoint for authentication requirements: ${getErrorMessage(error)}`, ); diff --git a/packages/core/src/mcp/oauth-utils.test.ts b/packages/core/src/mcp/oauth-utils.test.ts index bec8ef9f4b..0e17f3c32e 100644 --- a/packages/core/src/mcp/oauth-utils.test.ts +++ b/packages/core/src/mcp/oauth-utils.test.ts @@ -210,6 +210,60 @@ describe('OAuthUtils', () => { }); }); + describe('discoverOAuthConfig', () => { + const mockResourceMetadata: OAuthProtectedResourceMetadata = { + resource: 'https://example.com/mcp', + authorization_servers: ['https://auth.example.com'], + bearer_methods_supported: ['header'], + }; + + const mockAuthServerMetadata: OAuthAuthorizationServerMetadata = { + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/authorize', + token_endpoint: 'https://auth.example.com/token', + scopes_supported: ['read', 'write'], + }; + + it('should succeed when resource metadata matches server URL', async () => { + mockFetch + // fetchProtectedResourceMetadata + .mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve(mockResourceMetadata), + }) + // discoverAuthorizationServerMetadata + .mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve(mockAuthServerMetadata), + }); + + const config = await OAuthUtils.discoverOAuthConfig( + 'https://example.com/mcp', + ); + + expect(config).toEqual({ + authorizationUrl: 'https://auth.example.com/authorize', + tokenUrl: 'https://auth.example.com/token', + scopes: ['read', 'write'], + }); + }); + + it('should throw error when resource metadata does not match server URL', async () => { + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => + Promise.resolve({ + ...mockResourceMetadata, + resource: 'https://malicious.com/mcp', + }), + }); + + await expect( + OAuthUtils.discoverOAuthConfig('https://example.com/mcp'), + ).rejects.toThrow(/does not match expected/); + }); + }); + describe('metadataToOAuthConfig', () => { it('should convert metadata to OAuth config', () => { const metadata: OAuthAuthorizationServerMetadata = { diff --git a/packages/core/src/mcp/oauth-utils.ts b/packages/core/src/mcp/oauth-utils.ts index 0f4bd0b24a..477374b596 100644 --- a/packages/core/src/mcp/oauth-utils.ts +++ b/packages/core/src/mcp/oauth-utils.ts @@ -8,6 +8,16 @@ import type { MCPOAuthConfig } from './oauth-provider.js'; import { getErrorMessage } from '../utils/errors.js'; import { debugLogger } from '../utils/debugLogger.js'; +/** + * Error thrown when the discovered resource metadata does not match the expected resource. + */ +export class ResourceMismatchError extends Error { + constructor(message: string) { + super(message); + this.name = 'ResourceMismatchError'; + } +} + /** * OAuth authorization server metadata as per RFC 8414. */ @@ -243,6 +253,18 @@ export class OAuthUtils { } } + if (resourceMetadata) { + // RFC 9728 Section 7.3: The client MUST ensure that the resource identifier URL + // it is using as the prefix for the metadata request exactly matches the value + // of the resource metadata parameter in the protected resource metadata document. + const expectedResource = this.buildResourceParameter(serverUrl); + if (resourceMetadata.resource !== expectedResource) { + throw new ResourceMismatchError( + `Protected resource ${resourceMetadata.resource} does not match expected ${expectedResource}`, + ); + } + } + if (resourceMetadata?.authorization_servers?.length) { // Use the first authorization server const authServerUrl = resourceMetadata.authorization_servers[0]; @@ -279,6 +301,9 @@ export class OAuthUtils { return null; } catch (error) { + if (error instanceof ResourceMismatchError) { + throw error; + } debugLogger.debug( `Failed to discover OAuth configuration: ${getErrorMessage(error)}`, ); @@ -305,10 +330,12 @@ export class OAuthUtils { * Discover OAuth configuration from WWW-Authenticate header. * * @param wwwAuthenticate The WWW-Authenticate header value + * @param mcpServerUrl Optional MCP server URL to validate against the resource metadata * @returns The discovered OAuth configuration or null if not available */ static async discoverOAuthFromWWWAuthenticate( wwwAuthenticate: string, + mcpServerUrl?: string, ): Promise { const resourceMetadataUri = this.parseWWWAuthenticateHeader(wwwAuthenticate); @@ -318,6 +345,17 @@ export class OAuthUtils { const resourceMetadata = await this.fetchProtectedResourceMetadata(resourceMetadataUri); + + if (resourceMetadata && mcpServerUrl) { + // Validate resource parameter per RFC 9728 Section 7.3 + const expectedResource = this.buildResourceParameter(mcpServerUrl); + if (resourceMetadata.resource !== expectedResource) { + throw new ResourceMismatchError( + `Protected resource ${resourceMetadata.resource} does not match expected ${expectedResource}`, + ); + } + } + if (!resourceMetadata?.authorization_servers?.length) { return null; }