From c1e071b7a0fc54149be088f2a21cfdedee4217ce Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 22 May 2026 20:39:16 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20fix:=20Harden=20MCP=20O?= =?UTF-8?q?Auth=20Request=20Handling=20(#13264)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Harden MCP OAuth request handling * fix: Bound MCP OAuth dispatcher cache * fix: Harden OAuth DNS lookup handling --- packages/api/src/auth/agent.spec.ts | 83 ++++++++++++++++- packages/api/src/auth/agent.ts | 54 ++++++++--- .../MCPConnectionAgentLifecycle.test.ts | 2 + .../mcp/__tests__/MCPConnectionSSRF.test.ts | 1 + .../MCPOAuthClientRegistrationReuse.test.ts | 2 +- .../MCPOAuthConnectionEvents.test.ts | 2 + .../src/mcp/__tests__/MCPOAuthFlow.test.ts | 1 + .../__tests__/MCPOAuthRaceCondition.test.ts | 2 + .../__tests__/dbSourced.integration.test.ts | 2 + .../api/src/mcp/__tests__/handler.test.ts | 55 ++++++----- .../mcp/oauth/detectOAuth.fallback.test.ts | 2 + .../api/src/mcp/oauth/detectOAuth.test.ts | 7 ++ packages/api/src/mcp/oauth/detectOAuth.ts | 62 +++++++++---- packages/api/src/mcp/oauth/handler.ts | 75 ++++++++++----- .../mcp/oauth/hardenedFetch.behavior.test.ts | 91 ++++++++++++++++++ .../api/src/mcp/oauth/hardenedFetch.test.ts | 82 +++++++++++++++++ packages/api/src/mcp/oauth/hardenedFetch.ts | 92 +++++++++++++++++++ packages/api/src/mcp/oauth/types.ts | 4 + packages/api/src/mcp/oauth/url.ts | 6 ++ .../src/mcp/registry/MCPServerInspector.ts | 6 +- .../MCPReinitRecovery.integration.test.ts | 2 + 21 files changed, 555 insertions(+), 78 deletions(-) create mode 100644 packages/api/src/mcp/oauth/hardenedFetch.behavior.test.ts create mode 100644 packages/api/src/mcp/oauth/hardenedFetch.test.ts create mode 100644 packages/api/src/mcp/oauth/hardenedFetch.ts create mode 100644 packages/api/src/mcp/oauth/url.ts diff --git a/packages/api/src/auth/agent.spec.ts b/packages/api/src/auth/agent.spec.ts index 0764f7aa2f..f74542c6c8 100644 --- a/packages/api/src/auth/agent.spec.ts +++ b/packages/api/src/auth/agent.spec.ts @@ -11,7 +11,11 @@ import http from 'node:http'; import type { LookupFunction } from 'node:net'; import { createSSRFSafeAgents, createSSRFSafeUndiciConnect } from './agent'; -type LookupCallback = (err: NodeJS.ErrnoException | null, address: string, family: number) => void; +type LookupCallback = ( + err: NodeJS.ErrnoException | null, + address: string | dns.LookupAddress[], + family?: number, +) => void; const mockedDnsLookup = dns.lookup as jest.MockedFunction; const httpAgentPrototype = http.Agent.prototype as unknown as { @@ -28,6 +32,16 @@ function mockDnsResult(address: string, family: number): void { }) as never); } +function mockDnsAllResult(addresses: dns.LookupAddress[]): void { + mockedDnsLookup.mockImplementation((( + _hostname: string, + _options: unknown, + callback: LookupCallback, + ) => { + callback(null, addresses); + }) as never); +} + function mockDnsError(err: NodeJS.ErrnoException): void { mockedDnsLookup.mockImplementation((( _hostname: string, @@ -123,6 +137,73 @@ describe('createSSRFSafeUndiciConnect', () => { expect(result.address).toBe('93.184.216.34'); }); + it('lookup should block private IPs when DNS returns all addresses', async () => { + mockDnsAllResult([{ address: '127.0.0.1', family: 4 }]); + const connect = createSSRFSafeUndiciConnect(); + + const result = await new Promise<{ err: NodeJS.ErrnoException | null }>((resolve) => { + connect.lookup('localhost', { all: true }, (err) => { + resolve({ err }); + }); + }); + + expect(result.err).toBeTruthy(); + expect(result.err!.code).toBe('ESSRF'); + }); + + it('lookup should allow public IPs when DNS returns all addresses', async () => { + const addresses = [{ address: '93.184.216.34', family: 4 }]; + mockDnsAllResult(addresses); + const connect = createSSRFSafeUndiciConnect(); + + const result = await new Promise<{ + err: NodeJS.ErrnoException | null; + address: string | dns.LookupAddress[]; + }>((resolve) => { + connect.lookup('example.com', { all: true }, (err, address) => { + resolve({ err, address }); + }); + }); + + expect(result.err).toBeNull(); + expect(result.address).toEqual(addresses); + }); + + it('lookup should block mixed public and private all-address results', async () => { + mockDnsAllResult([ + { address: '93.184.216.34', family: 4 }, + { address: '10.0.0.1', family: 4 }, + ]); + const connect = createSSRFSafeUndiciConnect(); + + const result = await new Promise<{ err: NodeJS.ErrnoException | null }>((resolve) => { + connect.lookup('rebinding.example.com', { all: true }, (err) => { + resolve({ err }); + }); + }); + + expect(result.err).toBeTruthy(); + expect(result.err!.code).toBe('ESSRF'); + }); + + it('lookup should honor allowedAddresses when DNS returns all addresses', async () => { + const addresses = [{ address: '10.0.0.5', family: 4 }]; + mockDnsAllResult(addresses); + const connect = createSSRFSafeUndiciConnect(['10.0.0.5:11434'], '11434'); + + const result = await new Promise<{ + err: NodeJS.ErrnoException | null; + address: string | dns.LookupAddress[]; + }>((resolve) => { + connect.lookup('private.example.com', { all: true }, (err, address) => { + resolve({ err, address }); + }); + }); + + expect(result.err).toBeNull(); + expect(result.address).toEqual(addresses); + }); + it('lookup should forward DNS errors', async () => { const dnsError = Object.assign(new Error('ENOTFOUND'), { code: 'ENOTFOUND', diff --git a/packages/api/src/auth/agent.ts b/packages/api/src/auth/agent.ts index 8337a7d071..7222d8de64 100644 --- a/packages/api/src/auth/agent.ts +++ b/packages/api/src/auth/agent.ts @@ -9,6 +9,37 @@ import { } from './allowedAddresses'; import { isPrivateIP } from './ip'; +type LookupResult = string | dns.LookupAddress[]; + +function createSSRFLookupError(hostname: string, address: string): NodeJS.ErrnoException { + return Object.assign( + new Error(`SSRF protection: ${hostname} resolved to blocked address ${address}`), + { code: 'ESSRF' }, + ) as NodeJS.ErrnoException; +} + +function getBlockedLookupAddress( + lookupResult: LookupResult, + hostnameAllowed: boolean, + exemptSet: Set | null, + normalizedPort: string, +): string | null { + if (hostnameAllowed) { + return null; + } + + const addresses = Array.isArray(lookupResult) + ? lookupResult.map(({ address }) => address) + : [lookupResult]; + + return ( + addresses.find( + (address) => + isPrivateIP(address) && !isAddressInAllowedSet(address, exemptSet, normalizedPort), + ) ?? null + ); +} + /** * Builds a DNS lookup wrapper that blocks resolution to private/reserved IP * addresses. When `allowedAddresses` is provided, hostname/IP + port pairs @@ -34,20 +65,19 @@ function buildSSRFSafeLookup( callback(err, '', 0); return; } - if ( - !hostnameAllowed && - typeof address === 'string' && - isPrivateIP(address) && - !isAddressInAllowedSet(address, exemptSet, normalizedPort) - ) { - const ssrfError = Object.assign( - new Error(`SSRF protection: ${hostname} resolved to blocked address ${address}`), - { code: 'ESSRF' }, - ) as NodeJS.ErrnoException; - callback(ssrfError, address, family as number); + + const blockedAddress = getBlockedLookupAddress( + address, + hostnameAllowed, + exemptSet, + normalizedPort, + ); + if (blockedAddress) { + callback(createSSRFLookupError(hostname, blockedAddress), blockedAddress, family); return; } - callback(null, address as string, family as number); + + callback(null, address, family); }); }; } diff --git a/packages/api/src/mcp/__tests__/MCPConnectionAgentLifecycle.test.ts b/packages/api/src/mcp/__tests__/MCPConnectionAgentLifecycle.test.ts index 99cfa2363a..95274f699f 100644 --- a/packages/api/src/mcp/__tests__/MCPConnectionAgentLifecycle.test.ts +++ b/packages/api/src/mcp/__tests__/MCPConnectionAgentLifecycle.test.ts @@ -39,6 +39,8 @@ jest.mock('@librechat/data-schemas', () => ({ jest.mock('~/auth', () => ({ createSSRFSafeUndiciConnect: jest.fn(() => undefined), + isOAuthUrlAllowed: jest.fn(() => false), + isSSRFTarget: jest.fn(() => false), resolveHostnameSSRF: jest.fn(async () => false), })); diff --git a/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts b/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts index aaec527e46..93375cbd68 100644 --- a/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts +++ b/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts @@ -56,6 +56,7 @@ jest.mock('~/auth', () => ({ callback(null, '127.0.0.1', 4); }, })), + isOAuthUrlAllowed: jest.fn(() => false), isSSRFTarget: jest.fn(() => false), resolveHostnameSSRF: jest.fn(async () => false), })); diff --git a/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts b/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts index 75cf4147b2..68723ea099 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthClientRegistrationReuse.test.ts @@ -45,7 +45,7 @@ jest.mock('@librechat/data-schemas', () => ({ jest.mock('~/auth', () => ({ createSSRFSafeUndiciConnect: jest.fn(() => undefined), resolveHostnameSSRF: jest.fn(async () => false), - isSSRFTarget: jest.fn(async () => false), + isSSRFTarget: jest.fn(() => false), isOAuthUrlAllowed: jest.fn(() => true), })); diff --git a/packages/api/src/mcp/__tests__/MCPOAuthConnectionEvents.test.ts b/packages/api/src/mcp/__tests__/MCPOAuthConnectionEvents.test.ts index 79470337a7..08b28dd817 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthConnectionEvents.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthConnectionEvents.test.ts @@ -25,6 +25,8 @@ jest.mock('@librechat/data-schemas', () => ({ jest.mock('~/auth', () => ({ createSSRFSafeUndiciConnect: jest.fn(() => undefined), + isOAuthUrlAllowed: jest.fn(() => false), + isSSRFTarget: jest.fn(() => false), resolveHostnameSSRF: jest.fn(async () => false), })); diff --git a/packages/api/src/mcp/__tests__/MCPOAuthFlow.test.ts b/packages/api/src/mcp/__tests__/MCPOAuthFlow.test.ts index cbd29d3571..545540ee57 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthFlow.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthFlow.test.ts @@ -30,6 +30,7 @@ jest.mock('@librechat/data-schemas', () => ({ /** Bypass SSRF validation — these tests use real local HTTP servers. */ jest.mock('~/auth', () => ({ ...jest.requireActual('~/auth'), + createSSRFSafeUndiciConnect: jest.fn(() => undefined), isSSRFTarget: jest.fn(() => false), resolveHostnameSSRF: jest.fn(async () => false), })); diff --git a/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts b/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts index bf907422f5..ae3d055ba3 100644 --- a/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts +++ b/packages/api/src/mcp/__tests__/MCPOAuthRaceCondition.test.ts @@ -31,6 +31,8 @@ jest.mock('@librechat/data-schemas', () => ({ jest.mock('~/auth', () => ({ createSSRFSafeUndiciConnect: jest.fn(() => undefined), + isOAuthUrlAllowed: jest.fn(() => false), + isSSRFTarget: jest.fn(() => false), resolveHostnameSSRF: jest.fn(async () => false), })); diff --git a/packages/api/src/mcp/__tests__/dbSourced.integration.test.ts b/packages/api/src/mcp/__tests__/dbSourced.integration.test.ts index 5866fa1a08..79241f1d6b 100644 --- a/packages/api/src/mcp/__tests__/dbSourced.integration.test.ts +++ b/packages/api/src/mcp/__tests__/dbSourced.integration.test.ts @@ -37,6 +37,8 @@ jest.mock('@librechat/data-schemas', () => ({ jest.mock('~/auth', () => ({ createSSRFSafeUndiciConnect: jest.fn(() => undefined), + isOAuthUrlAllowed: jest.fn(() => false), + isSSRFTarget: jest.fn(() => false), resolveHostnameSSRF: jest.fn(async () => false), })); diff --git a/packages/api/src/mcp/__tests__/handler.test.ts b/packages/api/src/mcp/__tests__/handler.test.ts index 6e0c1961bf..f69ccd52a5 100644 --- a/packages/api/src/mcp/__tests__/handler.test.ts +++ b/packages/api/src/mcp/__tests__/handler.test.ts @@ -836,14 +836,17 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { await MCPOAuthHandler.revokeOAuthToken(mockServerName, mockToken, 'access', metadata); - expect(mockFetch).toHaveBeenCalledWith(new URL('https://auth.example.com/oauth/revoke'), { - method: 'POST', - body: 'token=test-token-12345&token_type_hint=access_token', - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - Authorization: `Basic ${Buffer.from('test-client-id:test-client-secret').toString('base64')}`, - }, - }); + expect(mockFetch).toHaveBeenCalledWith( + new URL('https://auth.example.com/oauth/revoke'), + expect.objectContaining({ + method: 'POST', + body: 'token=test-token-12345&token_type_hint=access_token', + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + Authorization: `Basic ${Buffer.from('test-client-id:test-client-secret').toString('base64')}`, + }, + }), + ); }); it('should successfully revoke a refresh token with client_secret_basic auth', async () => { @@ -862,14 +865,17 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { await MCPOAuthHandler.revokeOAuthToken(mockServerName, mockToken, 'refresh', metadata); - expect(mockFetch).toHaveBeenCalledWith(new URL('https://auth.example.com/oauth/revoke'), { - method: 'POST', - body: 'token=test-token-12345&token_type_hint=refresh_token', - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - Authorization: `Basic ${Buffer.from('test-client-id:test-client-secret').toString('base64')}`, - }, - }); + expect(mockFetch).toHaveBeenCalledWith( + new URL('https://auth.example.com/oauth/revoke'), + expect.objectContaining({ + method: 'POST', + body: 'token=test-token-12345&token_type_hint=refresh_token', + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + Authorization: `Basic ${Buffer.from('test-client-id:test-client-secret').toString('base64')}`, + }, + }), + ); }); it('should successfully revoke an access token with client_secret_post auth', async () => { @@ -888,13 +894,16 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { await MCPOAuthHandler.revokeOAuthToken(mockServerName, mockToken, 'access', metadata); - expect(mockFetch).toHaveBeenCalledWith(new URL('https://auth.example.com/oauth/revoke'), { - method: 'POST', - body: 'token=test-token-12345&token_type_hint=access_token&client_secret=test-client-secret&client_id=test-client-id', - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - }, - }); + expect(mockFetch).toHaveBeenCalledWith( + new URL('https://auth.example.com/oauth/revoke'), + expect.objectContaining({ + method: 'POST', + body: 'token=test-token-12345&token_type_hint=access_token&client_secret=test-client-secret&client_id=test-client-id', + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + }, + }), + ); }); it('should fallback to /revoke endpoint when revocationEndpoint is not provided', async () => { diff --git a/packages/api/src/mcp/oauth/detectOAuth.fallback.test.ts b/packages/api/src/mcp/oauth/detectOAuth.fallback.test.ts index 324fca8df6..4a9e20699d 100644 --- a/packages/api/src/mcp/oauth/detectOAuth.fallback.test.ts +++ b/packages/api/src/mcp/oauth/detectOAuth.fallback.test.ts @@ -6,6 +6,8 @@ jest.mock('@modelcontextprotocol/sdk/client/auth.js', () => ({ })); jest.mock('~/auth', () => ({ + createSSRFSafeUndiciConnect: jest.fn(() => ({ lookup: jest.fn() })), + isOAuthUrlAllowed: jest.fn(() => false), isSSRFTarget: jest.fn(() => false), resolveHostnameSSRF: jest.fn(async () => false), })); diff --git a/packages/api/src/mcp/oauth/detectOAuth.test.ts b/packages/api/src/mcp/oauth/detectOAuth.test.ts index cfcbf9a692..c65d9416b0 100644 --- a/packages/api/src/mcp/oauth/detectOAuth.test.ts +++ b/packages/api/src/mcp/oauth/detectOAuth.test.ts @@ -8,6 +8,8 @@ jest.mock('@modelcontextprotocol/sdk/client/auth.js', () => ({ })); jest.mock('~/auth', () => ({ + createSSRFSafeUndiciConnect: jest.fn(() => ({ lookup: jest.fn() })), + isOAuthUrlAllowed: jest.fn(() => false), isSSRFTarget: jest.fn(() => false), resolveHostnameSSRF: jest.fn(async () => false), })); @@ -233,6 +235,7 @@ describe('detectOAuthRequirement', () => { expect(mockDiscoverOAuthProtectedResourceMetadata).toHaveBeenCalledWith( 'https://mcp.example.com', expect.objectContaining({ resourceMetadataUrl: new URL(metadataUrl) }), + expect.any(Function), ); }); @@ -284,6 +287,7 @@ describe('detectOAuthRequirement', () => { expect(mockDiscoverOAuthProtectedResourceMetadata).toHaveBeenCalledWith( 'https://mcp.example.com/mcp', expect.objectContaining({ resourceMetadataUrl: new URL(metadataUrl) }), + expect.any(Function), ); }); }); @@ -315,6 +319,7 @@ describe('detectOAuthRequirement', () => { expect(mockDiscoverOAuthProtectedResourceMetadata).toHaveBeenCalledWith( 'https://mcp.example.com', expect.objectContaining({ resourceMetadataUrl: undefined }), + expect.any(Function), ); expect(result.requiresOAuth).toBe(true); expect(result.method).toBe('protected-resource-metadata'); @@ -343,6 +348,7 @@ describe('detectOAuthRequirement', () => { expect(mockDiscoverOAuthProtectedResourceMetadata).toHaveBeenCalledWith( 'https://mcp.example.com', expect.objectContaining({ resourceMetadataUrl: undefined }), + expect.any(Function), ); expect(result.requiresOAuth).toBe(true); expect(result.method).toBe('401-challenge-metadata'); @@ -371,6 +377,7 @@ describe('detectOAuthRequirement', () => { expect(mockDiscoverOAuthProtectedResourceMetadata).toHaveBeenCalledWith( 'https://mcp.example.com', expect.objectContaining({ resourceMetadataUrl: undefined }), + expect.any(Function), ); }); }); diff --git a/packages/api/src/mcp/oauth/detectOAuth.ts b/packages/api/src/mcp/oauth/detectOAuth.ts index 33391b181d..52bfe82862 100644 --- a/packages/api/src/mcp/oauth/detectOAuth.ts +++ b/packages/api/src/mcp/oauth/detectOAuth.ts @@ -6,8 +6,11 @@ // Manual testing ensures the OAuth detection still works against real MCP servers. import { discoverOAuthProtectedResourceMetadata } from '@modelcontextprotocol/sdk/client/auth.js'; -import { isSSRFTarget, resolveHostnameSSRF } from '~/auth'; +import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport'; +import { isSSRFTarget, resolveHostnameSSRF, isOAuthUrlAllowed } from '~/auth'; import { probeResourceMetadataHint } from './resourceHint'; +import { createHardenedOAuthFetch } from './hardenedFetch'; +import { getOAuthUrlPort } from './url'; import { mcpConfig } from '../mcpConfig'; export interface OAuthDetectionResult { @@ -32,8 +35,13 @@ export interface OAuthDetectionResult { * * @param serverUrl - The MCP server URL to check for OAuth requirements */ -export async function detectOAuthRequirement(serverUrl: string): Promise { - const hint = await probeResourceMetadataHint(serverUrl); +export async function detectOAuthRequirement( + serverUrl: string, + allowedDomains?: string[] | null, + allowedAddresses?: string[] | null, +): Promise { + const fetchFn = createHardenedOAuthFetch({ allowedDomains, allowedAddresses }); + const hint = await probeResourceMetadataHint(serverUrl, fetchFn); /** * The `resource_metadata` URL is attacker-controlled (it's echoed from the MCP @@ -42,10 +50,10 @@ export async function detectOAuthRequirement(serverUrl: string): Promise { try { - const resourceMetadata = await discoverOAuthProtectedResourceMetadata(serverUrl, { - resourceMetadataUrl, - }); + const resourceMetadata = await discoverOAuthProtectedResourceMetadata( + serverUrl, + { + resourceMetadataUrl, + }, + fetchFn, + ); if (!resourceMetadata?.authorization_servers?.length) return null; @@ -118,15 +131,25 @@ async function checkProtectedResourceMetadata( /** * SSRF-guards an attacker-controlled `resource_metadata` hint before the SDK follows it. - * `detectOAuthRequirement` runs without admin-scoped `allowedDomains`, so the rejection - * policy here is stricter than the handler's: any private/loopback/metadata-service - * target is dropped, regardless of origin relative to the MCP server. On rejection the - * caller continues with path-aware discovery (safe, since it targets the server itself). + * Honors the same allowedDomains/allowedAddresses policy used by the OAuth handler: + * trusted admin allowlist matches bypass the private-address block; otherwise hints + * are rejected when they target restricted hostnames or resolve to private addresses. + * On rejection the caller continues with path-aware discovery. */ -async function validateHintUrl(hintUrl: URL): Promise { +async function validateHintUrl( + hintUrl: URL, + allowedDomains?: string[] | null, + allowedAddresses?: string[] | null, +): Promise { try { - if (isSSRFTarget(hintUrl.hostname)) return undefined; - if (await resolveHostnameSSRF(hintUrl.hostname)) return undefined; + if (isOAuthUrlAllowed(hintUrl.href, allowedDomains, allowedAddresses)) return hintUrl; + + const port = getOAuthUrlPort(hintUrl); + const allowedDomainsActive = Array.isArray(allowedDomains) && allowedDomains.length > 0; + const effectiveAddresses = allowedDomainsActive ? null : allowedAddresses; + + if (isSSRFTarget(hintUrl.hostname, effectiveAddresses, port)) return undefined; + if (await resolveHostnameSSRF(hintUrl.hostname, effectiveAddresses, port)) return undefined; return hintUrl; } catch { // If validation itself fails (e.g. DNS lookup threw), be conservative and drop the hint. @@ -135,9 +158,12 @@ async function validateHintUrl(hintUrl: URL): Promise { } // Fallback: only called when probing threw. Caller already gates on `OAUTH_ON_AUTH_ERROR`. -async function checkAuthErrorFallback(serverUrl: string): Promise { +async function checkAuthErrorFallback( + serverUrl: string, + fetchFn: FetchLike, +): Promise { try { - const response = await fetch(serverUrl, { + const response = await fetchFn(serverUrl, { method: 'HEAD', signal: AbortSignal.timeout(mcpConfig.OAUTH_DETECTION_TIMEOUT), }); diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index 519a9a20bf..0e7a645b03 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -1,6 +1,6 @@ import { randomBytes } from 'crypto'; import { logger } from '@librechat/data-schemas'; -import { FetchLike } from '@modelcontextprotocol/sdk/shared/transport'; +import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport'; import { OAuthMetadataSchema } from '@modelcontextprotocol/sdk/shared/auth.js'; import { checkResourceAllowed, @@ -32,18 +32,13 @@ import { import { isSSRFTarget, resolveHostnameSSRF, isOAuthUrlAllowed } from '~/auth'; import { probeResourceMetadataHint } from './resourceHint'; import { MCPTokenStorage } from './tokens'; +import { createHardenedOAuthFetch } from './hardenedFetch'; +import { getOAuthUrlPort } from './url'; import { sanitizeUrlForLogging } from '~/mcp/utils'; /** Type for the OAuth metadata from the SDK */ type SDKOAuthMetadata = Parameters[1]['metadata']; -function getOAuthUrlPort(url: URL): string { - if (url.port) return url.port; - if (url.protocol === 'http:') return '80'; - if (url.protocol === 'https:') return '443'; - return ''; -} - export class MCPOAuthHandler { private static readonly FLOW_TYPE = 'mcp_oauth'; private static readonly FLOW_TTL = 10 * 60 * 1000; // 10 minutes @@ -54,7 +49,11 @@ export class MCPOAuthHandler { private static createOAuthFetch( headers: Record, clientInfo?: OAuthClientInformation, + allowedDomains?: string[] | null, + allowedAddresses?: string[] | null, ): FetchLike { + const hardenedFetch = createHardenedOAuthFetch({ allowedDomains, allowedAddresses }); + return async (url: string | URL, init?: RequestInit): Promise => { const newHeaders = new Headers(init?.headers ?? {}); for (const [key, value] of Object.entries(headers)) { @@ -118,13 +117,13 @@ export class MCPOAuthHandler { } } - return fetch(url, { + return hardenedFetch(url, { ...init, body: params.toString(), headers: newHeaders, }); } - return fetch(url, { + return hardenedFetch(url, { ...init, headers: newHeaders, }); @@ -151,7 +150,12 @@ export class MCPOAuthHandler { let authServerUrl = new URL(serverUrl); let resourceMetadata: OAuthProtectedResourceMetadata | undefined; - const fetchFn = this.createOAuthFetch(oauthHeaders); + const fetchFn = this.createOAuthFetch( + oauthHeaders, + undefined, + allowedDomains, + allowedAddresses, + ); /** * RFC 9728 §5.1: when the server's 401 `WWW-Authenticate` header advertises a @@ -356,6 +360,8 @@ export class MCPOAuthHandler { resourceMetadata?: OAuthProtectedResourceMetadata, redirectUri?: string, tokenExchangeMethod?: TokenExchangeMethodEnum, + allowedDomains?: string[] | null, + allowedAddresses?: string[] | null, ): Promise { logger.debug( `[MCPOAuth] Starting client registration for ${sanitizeUrlForLogging(serverUrl)}, server metadata:`, @@ -417,7 +423,7 @@ export class MCPOAuthHandler { const clientInfo = await registerClient(serverUrl, { metadata: metadata as unknown as SDKOAuthMetadata, clientMetadata, - fetchFn: this.createOAuthFetch(oauthHeaders), + fetchFn: this.createOAuthFetch(oauthHeaders, undefined, allowedDomains, allowedAddresses), }); const forcedAuthMethod = getForcedTokenEndpointAuthMethod(tokenExchangeMethod); @@ -556,6 +562,8 @@ export class MCPOAuthHandler { codeVerifier, clientInfo, metadata, + ...(allowedDomains !== undefined && { allowedDomains }), + ...(allowedAddresses !== undefined && { allowedAddresses }), ...(Object.keys(oauthHeaders).length > 0 && { oauthHeaders }), }; @@ -637,6 +645,8 @@ export class MCPOAuthHandler { resourceMetadata, redirectUri, config?.token_exchange_method, + allowedDomains, + allowedAddresses, ); logger.debug(`[MCPOAuth] Client registered with ID: ${clientInfo.client_id}`); } @@ -711,6 +721,8 @@ export class MCPOAuthHandler { clientInfo, metadata, resourceMetadata, + ...(allowedDomains !== undefined && { allowedDomains }), + ...(allowedAddresses !== undefined && { allowedAddresses }), ...(Object.keys(oauthHeaders).length > 0 && { oauthHeaders }), ...(reusedStoredClient && { reusedStoredClient }), }; @@ -739,9 +751,9 @@ export class MCPOAuthHandler { /** * Completes the OAuth flow by exchanging the authorization code for tokens. * - * `allowedDomains` is intentionally absent: all URLs used here (serverUrl, - * token_endpoint) originate from {@link MCPOAuthFlowMetadata} that was - * SSRF-validated during {@link initiateOAuthFlow}. No new URL resolution occurs. + * The token exchange reuses the SSRF policy captured during + * {@link initiateOAuthFlow} and enforces it again at connect time. This closes + * DNS rebinding gaps between the preflight validation and the callback request. */ static async completeOAuthFlow( flowId: string, @@ -789,7 +801,12 @@ export class MCPOAuthHandler { codeVerifier: metadata.codeVerifier, authorizationCode, resource, - fetchFn: this.createOAuthFetch(oauthHeaders, metadata.clientInfo), + fetchFn: this.createOAuthFetch( + oauthHeaders, + metadata.clientInfo, + metadata.allowedDomains, + metadata.allowedAddresses, + ), }); logger.debug('[MCPOAuth] Token exchange successful', { @@ -1070,7 +1087,12 @@ export class MCPOAuthHandler { } else { /** Auto-discover OAuth configuration for refresh */ const serverUrl = new URL(metadata.serverUrl); - const fetchFn = this.createOAuthFetch(oauthHeaders); + const fetchFn = this.createOAuthFetch( + oauthHeaders, + undefined, + allowedDomains, + allowedAddresses, + ); const oauthMetadata = await this.discoverWithOriginFallback(serverUrl, fetchFn); if (!oauthMetadata) { @@ -1149,7 +1171,8 @@ export class MCPOAuthHandler { has_auth_header: !!headers['Authorization'], }); - const response = await fetch(tokenUrl, { + const oauthFetch = createHardenedOAuthFetch({ allowedDomains, allowedAddresses }); + const response = await oauthFetch(tokenUrl, { method: 'POST', headers, body, @@ -1232,7 +1255,8 @@ export class MCPOAuthHandler { body.append('client_id', config.client_id); } - const response = await fetch(tokenUrl, { + const oauthFetch = createHardenedOAuthFetch({ allowedDomains, allowedAddresses }); + const response = await oauthFetch(tokenUrl, { method: 'POST', headers, body, @@ -1256,7 +1280,12 @@ export class MCPOAuthHandler { /** Auto-discover OAuth configuration for refresh */ const serverUrl = new URL(metadata.serverUrl); - const fetchFn = this.createOAuthFetch(oauthHeaders); + const fetchFn = this.createOAuthFetch( + oauthHeaders, + undefined, + allowedDomains, + allowedAddresses, + ); const oauthMetadata = await this.discoverWithOriginFallback(serverUrl, fetchFn); let tokenUrl: URL; @@ -1286,7 +1315,8 @@ export class MCPOAuthHandler { ...oauthHeaders, }; - const response = await fetch(tokenUrl, { + const oauthFetch = createHardenedOAuthFetch({ allowedDomains, allowedAddresses }); + const response = await oauthFetch(tokenUrl, { method: 'POST', headers, body, @@ -1361,7 +1391,8 @@ export class MCPOAuthHandler { logger.info( `[MCPOAuth] Revoking tokens for ${serverName} via ${sanitizeUrlForLogging(revokeUrl.toString())}`, ); - const response = await fetch(revokeUrl, { + const oauthFetch = createHardenedOAuthFetch({ allowedDomains, allowedAddresses }); + const response = await oauthFetch(revokeUrl, { method: 'POST', body: body.toString(), headers, diff --git a/packages/api/src/mcp/oauth/hardenedFetch.behavior.test.ts b/packages/api/src/mcp/oauth/hardenedFetch.behavior.test.ts new file mode 100644 index 0000000000..206616fdd0 --- /dev/null +++ b/packages/api/src/mcp/oauth/hardenedFetch.behavior.test.ts @@ -0,0 +1,91 @@ +import http from 'node:http'; +import type { AddressInfo, Socket } from 'node:net'; +import { createHardenedOAuthFetch, resetHardenedOAuthFetchDispatchers } from './hardenedFetch'; + +type TestServer = { + port: number; + requestCount: () => number; + close: () => Promise; +}; + +async function createLocalServer(): Promise { + let requestCount = 0; + const sockets = new Set(); + const server = http.createServer((_req, res) => { + requestCount += 1; + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ ok: true })); + }); + + server.on('connection', (socket) => { + sockets.add(socket); + socket.once('close', () => sockets.delete(socket)); + }); + + await new Promise((resolve) => server.listen(0, 'localhost', resolve)); + const address = server.address() as AddressInfo; + + return { + port: address.port, + requestCount: () => requestCount, + close: () => + new Promise((resolve) => { + for (const socket of sockets) { + socket.destroy(); + } + sockets.clear(); + server.close(() => resolve()); + }), + }; +} + +describe('createHardenedOAuthFetch request policy', () => { + let server: TestServer; + + beforeEach(async () => { + server = await createLocalServer(); + }); + + afterEach(async () => { + resetHardenedOAuthFetchDispatchers(); + await server.close(); + }); + + it('blocks local OAuth requests unless the endpoint is explicitly trusted', async () => { + const oauthFetch = createHardenedOAuthFetch(); + + await expect( + oauthFetch(`http://localhost:${server.port}/token`, { + signal: AbortSignal.timeout(1000), + }), + ).rejects.toThrow(); + + expect(server.requestCount()).toBe(0); + }); + + it('allows explicitly trusted local OAuth endpoints', async () => { + const oauthFetch = createHardenedOAuthFetch({ allowedDomains: ['localhost'] }); + + const response = await oauthFetch(`http://localhost:${server.port}/token`, { + signal: AbortSignal.timeout(1000), + }); + + await expect(response.json()).resolves.toEqual({ ok: true }); + expect(server.requestCount()).toBe(1); + }); + + it('does not use address exemptions when domain policy is active but unmatched', async () => { + const oauthFetch = createHardenedOAuthFetch({ + allowedDomains: ['trusted.example.com'], + allowedAddresses: [`localhost:${server.port}`], + }); + + await expect( + oauthFetch(`http://localhost:${server.port}/token`, { + signal: AbortSignal.timeout(1000), + }), + ).rejects.toThrow(); + + expect(server.requestCount()).toBe(0); + }); +}); diff --git a/packages/api/src/mcp/oauth/hardenedFetch.test.ts b/packages/api/src/mcp/oauth/hardenedFetch.test.ts new file mode 100644 index 0000000000..5e9f5bb082 --- /dev/null +++ b/packages/api/src/mcp/oauth/hardenedFetch.test.ts @@ -0,0 +1,82 @@ +import { createSSRFSafeUndiciConnect, isOAuthUrlAllowed } from '~/auth'; +import { createHardenedOAuthFetch, resetHardenedOAuthFetchDispatchers } from './hardenedFetch'; + +jest.mock('~/auth', () => ({ + createSSRFSafeUndiciConnect: jest.fn(() => ({ lookup: jest.fn() })), + isOAuthUrlAllowed: jest.fn(() => false), +})); + +const mockCreateSSRFSafeUndiciConnect = createSSRFSafeUndiciConnect as jest.MockedFunction< + typeof createSSRFSafeUndiciConnect +>; +const mockIsOAuthUrlAllowed = isOAuthUrlAllowed as jest.MockedFunction; + +describe('createHardenedOAuthFetch', () => { + const originalFetch = global.fetch; + const mockFetch = jest.fn() as unknown as jest.MockedFunction; + + beforeEach(() => { + jest.clearAllMocks(); + global.fetch = mockFetch; + mockFetch.mockResolvedValue({ ok: true } as Response); + mockIsOAuthUrlAllowed.mockReturnValue(false); + }); + + afterEach(() => { + resetHardenedOAuthFetchDispatchers(); + }); + + afterAll(() => { + global.fetch = originalFetch; + }); + + it('attaches an SSRF-safe dispatcher at connect time', async () => { + await createHardenedOAuthFetch()('https://auth.example.com:9443/token', { + method: 'POST', + }); + + expect(mockCreateSSRFSafeUndiciConnect).toHaveBeenCalledWith(undefined, '9443'); + expect(mockFetch).toHaveBeenCalledWith( + 'https://auth.example.com:9443/token', + expect.objectContaining({ + method: 'POST', + dispatcher: expect.any(Object), + }), + ); + }); + + it('does not apply allowedAddresses when allowedDomains is active but unmatched', async () => { + await createHardenedOAuthFetch({ + allowedDomains: ['https://trusted.example.com'], + allowedAddresses: ['10.0.0.5:9444'], + })('https://untrusted.example.com:9444/token'); + + expect(mockCreateSSRFSafeUndiciConnect).toHaveBeenCalledWith(null, '9444'); + expect(mockFetch.mock.calls[0][1]).toEqual( + expect.objectContaining({ dispatcher: expect.any(Object) }), + ); + }); + + it('preserves admin-trusted allowedDomains bypass behavior', async () => { + mockIsOAuthUrlAllowed.mockReturnValueOnce(true); + + await createHardenedOAuthFetch({ + allowedDomains: ['https://auth.example.com'], + })('https://auth.example.com/token', { method: 'GET' }); + + expect(mockCreateSSRFSafeUndiciConnect).not.toHaveBeenCalled(); + expect(mockFetch.mock.calls[0][1]).not.toHaveProperty('dispatcher'); + }); + + it('normalizes allowedAddresses before caching dispatchers', async () => { + await createHardenedOAuthFetch({ + allowedAddresses: ['10.0.0.5:9443', '192.168.1.5:9443'], + })('https://auth.example.com:9443/token'); + + await createHardenedOAuthFetch({ + allowedAddresses: ['192.168.1.5:9443', '10.0.0.5:9443'], + })('https://auth.example.com:9443/token'); + + expect(mockCreateSSRFSafeUndiciConnect).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/api/src/mcp/oauth/hardenedFetch.ts b/packages/api/src/mcp/oauth/hardenedFetch.ts new file mode 100644 index 0000000000..18988af0c7 --- /dev/null +++ b/packages/api/src/mcp/oauth/hardenedFetch.ts @@ -0,0 +1,92 @@ +import { Agent } from 'undici'; +import type { Dispatcher } from 'undici'; +import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport'; +import { createSSRFSafeUndiciConnect, isOAuthUrlAllowed } from '~/auth'; +import { getOAuthUrlPort } from './url'; + +type FetchInitWithDispatcher = RequestInit & { + dispatcher?: Dispatcher; +}; + +const MAX_OAUTH_DISPATCHERS = 64; +const oauthDispatchers = new Map(); + +function shouldBypassSSRFDispatcher(url: string | URL, allowedDomains?: string[] | null): boolean { + if (!Array.isArray(allowedDomains) || allowedDomains.length === 0) { + return false; + } + + return isOAuthUrlAllowed(url.toString(), allowedDomains, null); +} + +function getDispatcherCacheKey(port: string, allowedAddresses?: string[] | null): string { + const normalizedAddresses = Array.isArray(allowedAddresses) + ? [...new Set(allowedAddresses)].sort().join('\n') + : ''; + return `${port}\0${normalizedAddresses}`; +} + +function evictOldestDispatcher(): void { + const oldestKey = oauthDispatchers.keys().next().value as string | undefined; + if (!oldestKey) { + return; + } + + const dispatcher = oauthDispatchers.get(oldestKey); + oauthDispatchers.delete(oldestKey); + dispatcher?.destroy(); +} + +function getOAuthDispatcher( + url: string | URL, + allowedDomains?: string[] | null, + allowedAddresses?: string[] | null, +): Agent | undefined { + if (shouldBypassSSRFDispatcher(url, allowedDomains)) { + return undefined; + } + + const parsedUrl = url instanceof URL ? url : new URL(url); + const port = getOAuthUrlPort(parsedUrl); + const effectiveAddresses = + Array.isArray(allowedDomains) && allowedDomains.length > 0 ? null : allowedAddresses; + const cacheKey = getDispatcherCacheKey(port, effectiveAddresses); + const cached = oauthDispatchers.get(cacheKey); + if (cached) { + oauthDispatchers.delete(cacheKey); + oauthDispatchers.set(cacheKey, cached); + return cached; + } + + if (oauthDispatchers.size >= MAX_OAUTH_DISPATCHERS) { + evictOldestDispatcher(); + } + + const dispatcher = new Agent({ + connect: createSSRFSafeUndiciConnect(effectiveAddresses, port), + }); + oauthDispatchers.set(cacheKey, dispatcher); + return dispatcher; +} + +export function createHardenedOAuthFetch({ + allowedDomains, + allowedAddresses, +}: { + allowedDomains?: string[] | null; + allowedAddresses?: string[] | null; +} = {}): FetchLike { + return async (url: string | URL, init?: RequestInit): Promise => { + const dispatcher = getOAuthDispatcher(url, allowedDomains, allowedAddresses); + const fetchInit: FetchInitWithDispatcher = + dispatcher != null ? { ...init, dispatcher } : { ...init }; + return fetch(url, fetchInit); + }; +} + +export function resetHardenedOAuthFetchDispatchers(): void { + for (const dispatcher of oauthDispatchers.values()) { + dispatcher.destroy(); + } + oauthDispatchers.clear(); +} diff --git a/packages/api/src/mcp/oauth/types.ts b/packages/api/src/mcp/oauth/types.ts index 20db2bc2a7..7e9d2d24b2 100644 --- a/packages/api/src/mcp/oauth/types.ts +++ b/packages/api/src/mcp/oauth/types.ts @@ -91,6 +91,10 @@ export interface MCPOAuthFlowMetadata extends FlowMetadata { authorizationUrl?: string; /** Custom headers for OAuth token exchange, persisted at flow initiation for the callback. */ oauthHeaders?: Record; + /** Domain allowlist captured at flow initiation for callback-time SSRF enforcement. */ + allowedDomains?: string[] | null; + /** Address exemptions captured at flow initiation for callback-time SSRF enforcement. */ + allowedAddresses?: string[] | null; /** True when the flow reused a stored client registration from a prior successful OAuth flow */ reusedStoredClient?: boolean; /** Tenant context captured at flow initiation for callback replay (SameSite cookies unavailable on cross-origin redirects) */ diff --git a/packages/api/src/mcp/oauth/url.ts b/packages/api/src/mcp/oauth/url.ts new file mode 100644 index 0000000000..6556507ed5 --- /dev/null +++ b/packages/api/src/mcp/oauth/url.ts @@ -0,0 +1,6 @@ +export function getOAuthUrlPort(url: URL): string { + if (url.port) return url.port; + if (url.protocol === 'http:') return '80'; + if (url.protocol === 'https:') return '443'; + return ''; +} diff --git a/packages/api/src/mcp/registry/MCPServerInspector.ts b/packages/api/src/mcp/registry/MCPServerInspector.ts index e4877e352b..fde1c7d521 100644 --- a/packages/api/src/mcp/registry/MCPServerInspector.ts +++ b/packages/api/src/mcp/registry/MCPServerInspector.ts @@ -106,7 +106,11 @@ export class MCPServerInspector { return; } - const result = await detectOAuthRequirement(this.config.url); + const result = await detectOAuthRequirement( + this.config.url, + this.allowedDomains, + this.allowedAddresses, + ); this.config.requiresOAuth = result.requiresOAuth; this.config.oauthMetadata = result.metadata; } diff --git a/packages/api/src/mcp/registry/__tests__/MCPReinitRecovery.integration.test.ts b/packages/api/src/mcp/registry/__tests__/MCPReinitRecovery.integration.test.ts index 9545486fde..00a485cd85 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPReinitRecovery.integration.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPReinitRecovery.integration.test.ts @@ -46,6 +46,8 @@ jest.mock('@librechat/data-schemas', () => ({ jest.mock('~/auth', () => ({ createSSRFSafeUndiciConnect: jest.fn(() => undefined), + isOAuthUrlAllowed: jest.fn(() => false), + isSSRFTarget: jest.fn(() => false), resolveHostnameSSRF: jest.fn(async () => false), }));