Skip to content

fix: prevent bearer tokens from leaking to discovery endpoints #1135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Bortlesboat wants to merge 1 commit into
base: main
Choose a base branch
from
+148 −94
Open

fix: prevent bearer tokens from leaking to discovery endpoints #1135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 24 additions & 13 deletions client/src/lib/hooks/__tests__/useConnection.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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");
});
Expand Down Expand Up @@ -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,
};
Expand All @@ -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 () => {
Expand All @@ -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
Expand Down Expand Up @@ -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");
});
});

Expand Down
205 changes: 124 additions & 81 deletions client/src/lib/hooks/useConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
});

Expand All @@ -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<string, string>,
): Record<string, string> => {
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)
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
Expand Down