From 2923735aa4322f49b27b069d5e0fedc3b4164c16 Mon Sep 17 00:00:00 2001 From: Andrew Barnes Date: Mon, 9 Mar 2026 14:02:44 -0400 Subject: [PATCH] fix: prevent bearer tokens from leaking to discovery endpoints Stop adding Authorization headers to requestInit.headers, which the SDK's createFetchWithInit merges into all HTTP requests including OAuth discovery and metadata endpoints (/.well-known/*). OAuth tokens are now handled exclusively by the authProvider via the transport's _commonHeaders() method, which only applies them to MCP server requests. Custom Authorization headers from the UI are applied through the fetch wrapper's mergeHeaders helper, which lets the SDK's _commonHeaders() take priority. Fixes #1092 --- .../hooks/__tests__/useConnection.test.tsx | 37 ++-- client/src/lib/hooks/useConnection.ts | 205 +++++++++++------- 2 files changed, 148 insertions(+), 94 deletions(-) diff --git a/client/src/lib/hooks/__tests__/useConnection.test.tsx b/client/src/lib/hooks/__tests__/useConnection.test.tsx index 4907a085b..a88545069 100644 --- a/client/src/lib/hooks/__tests__/useConnection.test.tsx +++ b/client/src/lib/hooks/__tests__/useConnection.test.tsx @@ -1087,7 +1087,7 @@ describe("useConnection", () => { ); }); - test("preserves server Authorization header when proxy auth is configured", async () => { + test("excludes server Authorization from requestInit when proxy auth is configured", async () => { const customHeaders: CustomHeaders = [ { name: "Authorization", @@ -1114,12 +1114,11 @@ describe("useConnection", () => { await result.current.connect(); }); - // Check that both headers are present and distinct + // Authorization should NOT be in requestInit to prevent leaking to + // discovery endpoints. The authProvider and custom fetch wrapper handle + // adding it only to MCP server requests. const headers = mockSSETransport.options?.requestInit?.headers; - expect(headers).toHaveProperty( - "Authorization", - "Bearer server-auth-token", - ); + expect(headers).not.toHaveProperty("Authorization"); expect(headers).toHaveProperty( "X-MCP-Proxy-Auth", "Bearer test-proxy-token", @@ -1223,7 +1222,10 @@ describe("useConnection", () => { expect(mockSSETransport.options?.requestInit?.headers).toBeDefined(); const headers = mockSSETransport.options?.requestInit?.headers; - expect(headers).toHaveProperty("Authorization", "Bearer token123"); + // Authorization should NOT be in requestInit headers to prevent + // leaking to discovery/metadata endpoints. The authProvider handles + // OAuth tokens, and custom Authorization is applied via the fetch wrapper. + expect(headers).not.toHaveProperty("Authorization"); expect(headers).toHaveProperty("X-Tenant-ID", "acme-inc"); expect(headers).toHaveProperty("X-Environment", "staging"); expect(headers).toHaveProperty( @@ -1253,7 +1255,8 @@ describe("useConnection", () => { }); const headers = mockSSETransport.options?.requestInit?.headers; - expect(headers).toHaveProperty("Authorization", "Bearer token123"); + // Authorization excluded from requestInit to prevent leaking to discovery endpoints + expect(headers).not.toHaveProperty("Authorization"); expect(headers).toHaveProperty("X-Enabled", "should-appear"); expect(headers).not.toHaveProperty("X-Disabled"); }); @@ -1283,7 +1286,7 @@ describe("useConnection", () => { ); }); - test("uses OAuth token when no custom headers or legacy auth provided", async () => { + test("uses OAuth token via authProvider, not requestInit headers", async () => { const propsWithoutAuth = { ...defaultProps, }; @@ -1294,8 +1297,12 @@ describe("useConnection", () => { await result.current.connect(); }); + // OAuth tokens should NOT be in requestInit headers to prevent + // leaking to discovery/metadata endpoints (see #1092). + // The authProvider handles adding Authorization dynamically + // via the transport's _commonHeaders() method. const headers = mockSSETransport.options?.requestInit?.headers; - expect(headers).toHaveProperty("Authorization", "Bearer mock-token"); + expect(headers).not.toHaveProperty("Authorization"); }); test("warns of enabled empty Bearer token", async () => { @@ -1322,8 +1329,9 @@ describe("useConnection", () => { const headers = mockSSETransport.options?.requestInit?.headers; - expect(headers).toHaveProperty("Authorization", "Bearer"); - // Should not have the x-custom-auth-headers since Authorization is standard + // Empty Authorization headers are filtered out and never added to requestInit. + // The authProvider handles OAuth tokens dynamically instead. + expect(headers).not.toHaveProperty("Authorization"); expect(headers).not.toHaveProperty("x-custom-auth-headers"); // Should show toast notification for empty Authorization header @@ -1352,8 +1360,11 @@ describe("useConnection", () => { await result.current.connect(); }); + // Authorization excluded from requestInit to prevent leaking to discovery + // endpoints. Custom Authorization is applied via the fetch wrapper only + // for MCP server requests. const headers = mockSSETransport.options?.requestInit?.headers; - expect(headers).toHaveProperty("Authorization", "Bearer custom-token"); + expect(headers).not.toHaveProperty("Authorization"); }); }); diff --git a/client/src/lib/hooks/useConnection.ts b/client/src/lib/hooks/useConnection.ts index e14d1037f..95d2554ba 100644 --- a/client/src/lib/hooks/useConnection.ts +++ b/client/src/lib/hooks/useConnection.ts @@ -520,41 +520,40 @@ export function useConnection({ }); } - const needsOAuthToken = !finalHeaders.some( - (header) => - header.enabled && - header.name.trim().toLowerCase() === "authorization", + // Do NOT add OAuth tokens to requestInit headers. The authProvider + // handles bearer tokens dynamically via the transport's _commonHeaders(), + // which only applies them to MCP server requests. Adding tokens here + // would leak them to OAuth discovery/metadata endpoints (/.well-known/*) + // via the SDK's createFetchWithInit wrapper. See: #1092 + + // Remove empty auth headers from custom headers + finalHeaders = finalHeaders.filter( + (header) => !isEmptyAuthHeader(header), ); - if (needsOAuthToken) { - const oauthToken = (await serverAuthProvider.tokens())?.access_token; - if (oauthToken) { - // Add the OAuth token - finalHeaders = [ - // Remove any existing Authorization headers with empty tokens - ...finalHeaders.filter((header) => !isEmptyAuthHeader(header)), - { - name: "Authorization", - value: `Bearer ${oauthToken}`, - enabled: true, - }, - ]; - } - } - - // Process all enabled custom headers + // Process all enabled custom headers, separating Authorization from others. + // Authorization headers are excluded from requestInit to prevent them from + // leaking to discovery/metadata endpoints during OAuth flows. const customHeaderNames: string[] = []; + let customAuthorizationHeader: string | undefined; finalHeaders.forEach((header) => { if (header.enabled && header.name.trim() && header.value.trim()) { const headerName = header.name.trim(); const headerValue = header.value.trim(); + // Skip Authorization headers - they must not be in requestInit + // because the SDK passes requestInit headers to discovery endpoints. + // The authProvider handles OAuth tokens, and custom Authorization + // headers are applied only to MCP server requests below. + if (headerName.toLowerCase() === "authorization") { + customAuthorizationHeader = headerValue; + return; + } + headers[headerName] = headerValue; // Track custom header names for server processing - if (headerName.toLowerCase() !== "authorization") { - customHeaderNames.push(headerName); - } + customHeaderNames.push(headerName); } }); @@ -570,6 +569,33 @@ export function useConnection({ let serverUrl: URL; + // Helper to merge request headers with custom headers from the UI, + // while ensuring Authorization from _commonHeaders() takes priority. + // Custom Authorization headers (from the UI) are only added when + // _commonHeaders() doesn't provide one (i.e., no OAuth token). + const mergeHeaders = ( + initHeaders?: HeadersInit, + baseHeaders?: Record, + ): Record => { + const base = baseHeaders ? { ...baseHeaders } : {}; + const init = initHeaders + ? initHeaders instanceof Headers + ? Object.fromEntries(initHeaders.entries()) + : Array.isArray(initHeaders) + ? Object.fromEntries(initHeaders) + : { ...initHeaders } + : {}; + const merged = { ...base, ...init }; + // Add custom Authorization only if not already set by authProvider + if ( + customAuthorizationHeader && + !Object.keys(merged).some((k) => k.toLowerCase() === "authorization") + ) { + merged["Authorization"] = customAuthorizationHeader; + } + return merged; + }; + // Determine connection URL based on the connection type if (connectionType === "direct" && transportType !== "stdio") { // Direct connection - use the provided URL directly (not available for STDIO) @@ -589,9 +615,13 @@ export function useConnection({ url: string | URL | globalThis.Request, init?: RequestInit, ) => { + // Merge custom headers with SDK-provided headers (from + // _commonHeaders), letting the SDK's Authorization take + // priority over any custom Authorization header. + const merged = mergeHeaders(init?.headers, requestHeaders); const response = await fetch(url, { ...init, - headers: requestHeaders, + headers: merged, }); // Capture protocol-related headers from response @@ -614,9 +644,13 @@ export function useConnection({ requestHeaders["Accept"] = "text/event-stream, application/json"; requestHeaders["Content-Type"] = "application/json"; + // Merge custom headers with SDK-provided headers (from + // _commonHeaders), letting the SDK's Authorization take + // priority over any custom Authorization header. + const merged = mergeHeaders(init?.headers, requestHeaders); const response = await fetch(url, { - headers: requestHeaders, ...init, + headers: merged, }); // Capture protocol-related headers from response @@ -663,22 +697,25 @@ export function useConnection({ proxyFullAddress, ); } - transportOptions = { - authProvider: serverAuthProvider, - eventSourceInit: { - fetch: ( - url: string | URL | globalThis.Request, - init?: RequestInit, - ) => - fetch(url, { - ...init, - headers: { ...headers, ...proxyHeaders }, - }), - }, - requestInit: { - headers: { ...headers, ...proxyHeaders }, - }, - }; + { + const baseHeaders = { ...headers, ...proxyHeaders }; + transportOptions = { + authProvider: serverAuthProvider, + eventSourceInit: { + fetch: ( + url: string | URL | globalThis.Request, + init?: RequestInit, + ) => + fetch(url, { + ...init, + headers: mergeHeaders(init?.headers, baseHeaders), + }), + }, + requestInit: { + headers: baseHeaders, + }, + }; + } break; } @@ -694,51 +731,57 @@ export function useConnection({ proxyFullAddressSSE, ); } - transportOptions = { - authProvider: serverAuthProvider, - eventSourceInit: { - fetch: ( - url: string | URL | globalThis.Request, - init?: RequestInit, - ) => - fetch(url, { - ...init, - headers: { ...headers, ...proxyHeaders }, - }), - }, - requestInit: { - headers: { ...headers, ...proxyHeaders }, - }, - }; + { + const baseHeaders = { ...headers, ...proxyHeaders }; + transportOptions = { + authProvider: serverAuthProvider, + eventSourceInit: { + fetch: ( + url: string | URL | globalThis.Request, + init?: RequestInit, + ) => + fetch(url, { + ...init, + headers: mergeHeaders(init?.headers, baseHeaders), + }), + }, + requestInit: { + headers: baseHeaders, + }, + }; + } break; } case "streamable-http": mcpProxyServerUrl = new URL(`${getMCPProxyAddress(config)}/mcp`); mcpProxyServerUrl.searchParams.append("url", sseUrl); - transportOptions = { - authProvider: serverAuthProvider, - eventSourceInit: { - fetch: ( - url: string | URL | globalThis.Request, - init?: RequestInit, - ) => - fetch(url, { - ...init, - headers: { ...headers, ...proxyHeaders }, - }), - }, - requestInit: { - headers: { ...headers, ...proxyHeaders }, - }, - // TODO these should be configurable... - reconnectionOptions: { - maxReconnectionDelay: 30000, - initialReconnectionDelay: 1000, - reconnectionDelayGrowFactor: 1.5, - maxRetries: 2, - }, - }; + { + const baseHeaders = { ...headers, ...proxyHeaders }; + transportOptions = { + authProvider: serverAuthProvider, + eventSourceInit: { + fetch: ( + url: string | URL | globalThis.Request, + init?: RequestInit, + ) => + fetch(url, { + ...init, + headers: mergeHeaders(init?.headers, baseHeaders), + }), + }, + requestInit: { + headers: baseHeaders, + }, + // TODO these should be configurable... + reconnectionOptions: { + maxReconnectionDelay: 30000, + initialReconnectionDelay: 1000, + reconnectionDelayGrowFactor: 1.5, + maxRetries: 2, + }, + }; + } break; } serverUrl = mcpProxyServerUrl as URL;