diff --git a/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts b/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts index f365f166e4..d25bb70c68 100644 --- a/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts +++ b/packages/api/src/mcp/__tests__/MCPConnectionSSRF.test.ts @@ -24,9 +24,10 @@ import type { } from 'undici'; import type { Socket } from 'net'; import { MCPConnection } from '~/mcp/connection'; -import { resolveHostnameSSRF } from '~/auth'; +import { createSSRFSafeUndiciConnect, resolveHostnameSSRF } from '~/auth'; type CustomFetch = (input: UndiciRequestInfo, init?: UndiciRequestInit) => Promise; +type LookupCallback = (err: NodeJS.ErrnoException | null, address: string, family: number) => void; jest.mock('@librechat/data-schemas', () => ({ logger: { @@ -38,7 +39,18 @@ jest.mock('@librechat/data-schemas', () => ({ })); jest.mock('~/auth', () => ({ - createSSRFSafeUndiciConnect: jest.fn(() => undefined), + createSSRFSafeUndiciConnect: jest.fn(() => ({ + lookup: (_hostname: string, optionsOrCallback: unknown, maybeCallback?: LookupCallback) => { + const callback = + typeof optionsOrCallback === 'function' + ? (optionsOrCallback as LookupCallback) + : maybeCallback; + if (!callback) { + throw new Error('lookup callback missing'); + } + callback(null, '127.0.0.1', 4); + }, + })), resolveHostnameSSRF: jest.fn(async () => false), })); @@ -49,6 +61,64 @@ jest.mock('~/mcp/mcpConfig', () => ({ const mockedResolveHostnameSSRF = resolveHostnameSSRF as jest.MockedFunction< typeof resolveHostnameSSRF >; +const mockedCreateSSRFSafeUndiciConnect = createSSRFSafeUndiciConnect as jest.MockedFunction< + typeof createSSRFSafeUndiciConnect +>; + +function getLookupCallback( + optionsOrCallback: unknown, + maybeCallback?: LookupCallback, +): LookupCallback { + if (typeof optionsOrCallback === 'function') { + return optionsOrCallback as LookupCallback; + } + if (maybeCallback) { + return maybeCallback; + } + throw new Error('lookup callback missing'); +} + +function createRebindBlockingLookup(): jest.Mock { + return jest.fn((hostname: string, optionsOrCallback: unknown, maybeCallback?: LookupCallback) => { + const callback = getLookupCallback(optionsOrCallback, maybeCallback); + if (hostname !== 'rebind.test') { + callback(null, '127.0.0.1', 4); + return; + } + const err = Object.assign( + new Error('SSRF protection: rebind.test resolved to blocked address 127.0.0.1'), + { code: 'ESSRF' }, + ) as NodeJS.ErrnoException; + callback(err, '127.0.0.1', 4); + }); +} + +function collectErrorMessages(error: unknown): string[] { + const messages: string[] = []; + let current: unknown = error; + while (current instanceof Error) { + messages.push(current.message); + current = (current as { cause?: unknown }).cause; + } + return messages; +} + +async function expectRebindSSRFRejection(promise: Promise): Promise { + let caught: unknown; + let didReject = false; + try { + await promise; + } catch (error) { + didReject = true; + caught = error; + } + if (!didReject) { + throw new Error('Expected promise to reject with SSRF error, but it resolved'); + } + expect( + collectErrorMessages(caught).some((message) => /SSRF protection: rebind\.test/.test(message)), + ).toBe(true); +} async function safeDisconnect(conn: MCPConnection | null): Promise { if (!conn) { @@ -586,6 +656,66 @@ describe('MCP SSRF protection – 307/308 redirect to private IP', () => { await capture.close(); } }); + + it('should block DNS rebinding between redirect pre-check and connect-time lookup', async () => { + const capture = await createHeaderCaptureServer(); + try { + const rebindUrl = new URL(capture.url); + rebindUrl.hostname = 'rebind.test'; + server = await createCrossOriginRedirectingServer(rebindUrl.href, 308); + mockedResolveHostnameSSRF.mockResolvedValueOnce(false); + mockedCreateSSRFSafeUndiciConnect.mockClear(); + const lookup = createRebindBlockingLookup(); + mockedCreateSSRFSafeUndiciConnect.mockReturnValueOnce({ + lookup, + } as ReturnType); + + conn = new MCPConnection({ + serverName: 'redirect-rebinding-block', + serverConfig: { type: 'streamable-http', url: server.url }, + useSSRFProtection: false, + }); + + await expectRebindSSRFRejection(conn.connect()); + expect(server.redirectHit).toBe(true); + expect(mockedCreateSSRFSafeUndiciConnect).toHaveBeenCalledTimes(1); + expect(lookup).toHaveBeenCalled(); + expect(lookup.mock.calls[0]?.[0]).toBe('rebind.test'); + expect(capture.receivedHeaders).toHaveLength(0); + } finally { + await capture.close(); + } + }); + + it('should keep using the original SSRF-safe dispatcher when protection is already on', async () => { + const capture = await createHeaderCaptureServer(); + try { + const rebindUrl = new URL(capture.url); + rebindUrl.hostname = 'rebind.test'; + server = await createCrossOriginRedirectingServer(rebindUrl.href, 308); + mockedResolveHostnameSSRF.mockResolvedValueOnce(false); + mockedCreateSSRFSafeUndiciConnect.mockClear(); + const lookup = createRebindBlockingLookup(); + mockedCreateSSRFSafeUndiciConnect.mockReturnValueOnce({ + lookup, + } as ReturnType); + + conn = new MCPConnection({ + serverName: 'redirect-rebinding-block-protection-on', + serverConfig: { type: 'streamable-http', url: server.url }, + useSSRFProtection: true, + }); + + await expectRebindSSRFRejection(conn.connect()); + expect(server.redirectHit).toBe(true); + expect(mockedCreateSSRFSafeUndiciConnect).toHaveBeenCalledTimes(1); + expect(lookup).toHaveBeenCalled(); + expect(lookup.mock.calls.some(([hostname]) => hostname === 'rebind.test')).toBe(true); + expect(capture.receivedHeaders).toHaveLength(0); + } finally { + await capture.close(); + } + }); }); describe('MCP SSRF protection – cross-origin credential stripping on redirect', () => { diff --git a/packages/api/src/mcp/connection.ts b/packages/api/src/mcp/connection.ts index 904a24f155..2a68a83c70 100644 --- a/packages/api/src/mcp/connection.ts +++ b/packages/api/src/mcp/connection.ts @@ -570,6 +570,9 @@ export class MCPConnection extends EventEmitter { ): (input: UndiciRequestInfo, init?: UndiciRequestInit) => Promise { const ssrfConnect = this.useSSRFProtection ? createSSRFSafeUndiciConnect() : undefined; const connectOpts = ssrfConnect != null ? { connect: ssrfConnect } : {}; + /** Capture only the fields needed by the fetch closure; see factory note above. */ + const useSSRFProtection = this.useSSRFProtection; + const agents = this.agents; const effectiveTimeout = timeout || DEFAULT_TIMEOUT; const postAgent = new Agent({ bodyTimeout: effectiveTimeout, @@ -588,6 +591,33 @@ export class MCPConnection extends EventEmitter { this.agents.push(getAgent); } + let safeRedirectPostAgent: Agent | undefined; + let safeRedirectGetAgent: Agent | undefined; + /** + * Allowlist mode keeps the original MCP URL admin-approved, but redirect + * targets are server-controlled. These agents add connect-time DNS checks + * for those cross-origin hops so DNS rebinding cannot beat the standalone + * resolveHostnameSSRF pre-check. + */ + const createSafeRedirectAgent = (bodyTimeout: number): Agent => { + const redirectSSRFConnect = createSSRFSafeUndiciConnect(); + const agent = new Agent({ + bodyTimeout, + headersTimeout: effectiveTimeout, + connect: redirectSSRFConnect, + }); + agents.push(agent); + return agent; + }; + const getSafeRedirectDispatcher = (isGetRequest: boolean): Agent => { + if (!isGetRequest || sseBodyTimeout == null) { + safeRedirectPostAgent ??= createSafeRedirectAgent(effectiveTimeout); + return safeRedirectPostAgent; + } + safeRedirectGetAgent ??= createSafeRedirectAgent(sseBodyTimeout); + return safeRedirectGetAgent; + }; + return async function customFetch( input: UndiciRequestInfo, init?: UndiciRequestInit, @@ -635,12 +665,13 @@ export class MCPConnection extends EventEmitter { } const targetUrl = new URL(location, currentUrlString); + const isCrossOriginRedirect = targetUrl.origin !== originalOrigin; /** - * Defense-in-depth SSRF check on every redirect hop. The dispatcher's - * connect-time `lookup` only fires when `useSSRFProtection` is true; - * allowlist deployments disable it, and the allowlist only covers the - * originally-configured URL — not server-controlled redirect targets. + * Keep the standalone check for immediate literal/current-DNS blocks. + * Cross-origin allowlist redirects also switch to a connect-time + * SSRF-safe dispatcher below so DNS rebinding cannot change the + * address between this check and the socket connection. */ if (await resolveHostnameSSRF(targetUrl.hostname)) { logger.warn( @@ -651,7 +682,7 @@ export class MCPConnection extends EventEmitter { await response.body?.cancel().catch(() => undefined); - if (targetUrl.origin !== originalOrigin && currentInit.headers != null) { + if (isCrossOriginRedirect && currentInit.headers != null) { currentInit = { ...currentInit, headers: stripCrossOriginHeaders( @@ -661,6 +692,19 @@ export class MCPConnection extends EventEmitter { }; } + if (!useSSRFProtection && isCrossOriginRedirect) { + /** + * Once a server-controlled cross-origin hop is seen, keep the safe + * dispatcher for the rest of this redirect chain. Restoring the + * original dispatcher on a later hop back to the original origin + * would re-open the allowlist-mode rebinding gap. + */ + currentInit = { + ...currentInit, + dispatcher: getSafeRedirectDispatcher(isGet), + }; + } + currentUrlString = targetUrl.href; } };