From deea679f3a3baad121bdd61f667c538c7434a702 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 21 Jul 2026 20:35:38 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=90=20fix:=20Withhold=20MCP=20OAuth=20?= =?UTF-8?q?Headers=20From=20Untrusted=20Preconfigured=20Discovery=20(#1437?= =?UTF-8?q?9)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../api/src/mcp/__tests__/handler.test.ts | 75 ++++++++++++++++++- packages/api/src/mcp/oauth/handler.ts | 10 +-- 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/packages/api/src/mcp/__tests__/handler.test.ts b/packages/api/src/mcp/__tests__/handler.test.ts index 8ab6999775..d73a66a87b 100644 --- a/packages/api/src/mcp/__tests__/handler.test.ts +++ b/packages/api/src/mcp/__tests__/handler.test.ts @@ -291,6 +291,79 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { ); }); + it('should not send custom headers during pre-configured metadata discovery', async () => { + const discoveryRequests: Array<{ url: string; method: string; headers: Headers }> = []; + const originalFetch = global.fetch; + global.fetch = (async (url, init) => { + discoveryRequests.push({ + url: url.toString(), + method: init?.method ?? 'GET', + headers: new Headers(init?.headers), + }); + return new Response('{}'); + }) as typeof fetch; + + mockProbeResourceMetadataHint.mockImplementationOnce(async (url, fetchFn) => { + await fetchFn?.(url, { method: 'HEAD' }); + await fetchFn?.(url, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: '{}', + }); + return { bearerChallenge: false, headAuthChallenge: false }; + }); + mockDiscoverOAuthProtectedResourceMetadata.mockImplementationOnce(async (_, __, fetchFn) => { + await fetchFn?.('https://example.com/.well-known/oauth-protected-resource'); + return { + resource: mockServerUrl, + authorization_servers: ['https://auth.example.com'], + }; + }); + mockDiscoverAuthorizationServerMetadata.mockImplementationOnce(async (_, options) => { + await options?.fetchFn?.('https://auth.example.com/.well-known/oauth-authorization-server'); + return { + issuer: 'https://auth.example.com', + authorization_endpoint: baseConfig.authorization_url, + token_endpoint: baseConfig.token_url, + token_endpoint_auth_methods_supported: ['client_secret_post'], + response_types_supported: ['code'], + } as AuthorizationServerMetadata; + }); + + try { + await MCPOAuthHandler.initiateOAuthFlow( + mockServerName, + mockServerUrl, + mockUserId, + { + Authorization: 'Bearer admin-runtime-token', + 'X-API-Key': 'gateway-api-key-secret', + }, + baseConfig, + ['example.com', 'auth.example.com'], + ); + } finally { + global.fetch = originalFetch; + } + + expect(discoveryRequests.map(({ url, method }) => ({ url, method }))).toEqual([ + { url: mockServerUrl, method: 'HEAD' }, + { url: mockServerUrl, method: 'POST' }, + { + url: 'https://example.com/.well-known/oauth-protected-resource', + method: 'GET', + }, + { + url: 'https://auth.example.com/.well-known/oauth-authorization-server', + method: 'GET', + }, + ]); + for (const { headers } of discoveryRequests) { + expect(headers.get('Authorization')).toBeNull(); + expect(headers.get('X-API-Key')).toBeNull(); + } + }); + it('should use default values when OAuth metadata fields are not configured', async () => { await MCPOAuthHandler.initiateOAuthFlow( mockServerName, @@ -1422,7 +1495,7 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { expect(headers.get('foo')).toBe('bar'); }); - it('passes headers to discovery operations', async () => { + it('passes headers to auto-discovery operations', async () => { mockDiscoverOAuthProtectedResourceMetadata.mockImplementation(async (_, __, fetchFn) => { await fetchFn?.('http://example.com/.well-known/oauth-protected-resource', {}); return { diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index f26d508456..d0c55c662a 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -236,9 +236,9 @@ export class MCPOAuthHandler { * other way round — or a split deployment can serve stale/wrong metadata at the * path-aware endpoint and strand the flow at a defunct authorization server. * - * Reuse `fetchFn` so admin-configured `oauthHeaders` (e.g. a gateway API key - * required to reach the MCP endpoint at all) are attached to the probe — without - * them, the probe would 401 for the wrong reason and never see the real challenge. + * Reuse the caller's `fetchFn` so discovery shares its hardened transport and timeout. + * Auto-discovery callers may attach gateway headers, while pre-configured discovery + * deliberately uses a headerless fetch because these URLs are not trusted yet. */ const hint = await probeResourceMetadataHint(serverUrl, fetchFn); /** @@ -368,7 +368,6 @@ export class MCPOAuthHandler { serverUrl: string, authorizationUrl: string, discoverCapabilities: boolean, - oauthHeaders: Record, allowedDomains?: string[] | null, allowedAddresses?: string[] | null, ): Promise { @@ -385,7 +384,7 @@ export class MCPOAuthHandler { }, PRECONFIGURED_DISCOVERY_TIMEOUT_MS); const fetchFn = this.createOAuthFetch( - oauthHeaders, + {}, undefined, allowedDomains, allowedAddresses, @@ -652,7 +651,6 @@ export class MCPOAuthHandler { serverUrl, config.authorization_url, shouldDiscoverCapabilities, - oauthHeaders, allowedDomains, allowedAddresses, );