diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index f8532021313..09e758fa008 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -106,7 +106,8 @@ export async function validateUrlWithDNS( try { // Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs - // on IPv4-only egress (e.g. AWS NAT gateways). See validateMcpServerSsrf. + // on IPv4-only egress (e.g. AWS NAT gateways) — still-pinned consumers (providers, SSO, + // A2A) depend on this ordering. const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true }) const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0] @@ -422,6 +423,176 @@ export function createPinnedLookup(resolvedIP: string): LookupFunction { } } +/** + * DNS lookup that resolves normally and validates EVERY resolved address against + * the SSRF policy at socket-connect time (the LibreChat `getSSRFConnect` pattern). + * Private/reserved/loopback records are filtered out; if nothing publicly routable + * remains the connect fails. Because the check runs on each dial — including + * redirects and reconnects — there is no validated-then-trusted window for a DNS + * rebind to slip through, and unlike single-IP pinning the connector keeps the + * full public address set, so the OS/undici can fall back across addresses. + * IPv4 is ordered first (`verbatim: false`) — our egress is IPv4-only. + */ +export function createSsrfGuardedLookup(): LookupFunction { + return (hostname, options, callback) => { + dns + .lookup(hostname, { all: true, verbatim: false }) + .then((addresses) => { + const usable = addresses.filter((entry) => !isPrivateOrReservedIP(entry.address)) + if (usable.length === 0) { + callback( + new Error(`Blocked by SSRF policy: ${hostname} has no publicly routable address`), + '', + 4 + ) + return + } + if (options.all) callback(null, usable) + else callback(null, usable[0].address, usable[0].family) + }) + .catch((error) => callback(toError(error), '', 4)) + } +} + +const MAX_GUARDED_REDIRECTS = 5 + +/** + * Rejects a redirect hop whose target is a private/reserved IP LITERAL. Node's + * `net.connect` bypasses the custom `lookup` for numeric hosts (`isIP(host)` + * short-circuits), so the connect-time guard never sees IP-literal dials — + * a 3xx to `http://169.254.169.254/` would otherwise connect directly. Hostname + * targets are covered by {@link createSsrfGuardedLookup} at connect time. + */ +function assertGuardedRedirectTarget(url: URL): void { + if (url.protocol !== 'http:' && url.protocol !== 'https:') { + throw new Error(`Blocked by SSRF policy: redirect to unsupported protocol ${url.protocol}`) + } + const host = + url.hostname.startsWith('[') && url.hostname.endsWith(']') + ? url.hostname.slice(1, -1) + : url.hostname + if (ipaddr.isValid(host) && isPrivateOrReservedIP(host)) { + throw new Error('Blocked by SSRF policy: redirect to a private or reserved address') + } +} + +/** + * Manual, revalidating redirect follower used by the guarded fetch. Auto-follow + * is unsafe here on two counts the connect-time lookup cannot cover: IP-literal + * redirect targets bypass the lookup entirely (validated per hop instead), and + * undici retains CUSTOM request headers across cross-origin redirects (it strips + * only Authorization/Cookie) — so caller headers are dropped on any cross-origin + * hop. Exported for tests. + */ +export async function followRedirectsGuarded( + rawFetch: (url: string, init: UndiciRequestInit) => Promise, + input: string, + init: UndiciRequestInit +): Promise { + let currentUrl = new URL(input) + // The initial URL gets the same IP-literal check as redirect hops, so the exported + // guard is self-contained even when a caller skips its own up-front validation. + assertGuardedRedirectTarget(currentUrl) + let method = (init.method ?? 'GET').toUpperCase() + let body = init.body + let headers = init.headers + for (let hop = 0; ; hop++) { + const response = await rawFetch(currentUrl.href, { + ...init, + method, + body, + headers, + redirect: 'manual', + }) + const status = response.status + const location = response.headers.get('location') + if (![301, 302, 303, 307, 308].includes(status) || !location) return response + // Cancel the redirect body up front so the throw paths below (hop cap, blocked + // target) can't leave a socket checked out on the long-lived Agent. + await response.body?.cancel().catch(() => {}) + if (hop >= MAX_GUARDED_REDIRECTS) { + throw new Error(`Blocked by SSRF policy: more than ${MAX_GUARDED_REDIRECTS} redirects`) + } + const nextUrl = new URL(location, currentUrl) + assertGuardedRedirectTarget(nextUrl) + // Per the fetch spec: 303 (and 301/302 on POST) switch to a bodyless GET, dropping + // the entity headers that described the removed body (a retained Content-Length / + // Content-Type on a bodyless GET is malformed and undici rejects it). + if (status === 303 || ((status === 301 || status === 302) && method === 'POST')) { + method = 'GET' + body = undefined + if (headers !== undefined) { + const sanitized = new Headers(headers as HeadersInit) + sanitized.delete('content-length') + sanitized.delete('content-type') + sanitized.delete('content-encoding') + sanitized.delete('transfer-encoding') + // double-cast-allowed: Headers is a valid undici HeadersInit at runtime but the DOM/undici types differ + headers = sanitized as unknown as UndiciRequestInit['headers'] + } + } + if (nextUrl.origin !== currentUrl.origin) { + headers = undefined + // 307/308 preserve method+body; forwarding a body cross-origin can hand OAuth + // client secrets / tokens to an open-redirect target now that redirects really + // dial the new origin. No legitimate MCP/OAuth flow does this — refuse it. + if (body !== undefined && body !== null) { + throw new Error( + 'Blocked by SSRF policy: cross-origin redirect would forward a request body' + ) + } + } + currentUrl = nextUrl + } +} + +/** + * SSRF-guarded `fetch` + its `Agent` for outbound requests to user-controlled + * hosts: DNS resolves normally, and every socket connect validates the chosen + * addresses via {@link createSsrfGuardedLookup}; redirects are followed manually + * with per-hop validation (see {@link followRedirectsGuarded}) so IP-literal + * targets can't bypass the lookup and custom headers never cross origins. See + * {@link createPinnedFetchWithDispatcher} for the `maxResponseSize` semantics. + */ +export function createSsrfGuardedFetchWithDispatcher(options?: { maxResponseSize?: number }): { + fetch: typeof fetch + dispatcher: Agent +} { + const dispatcher = new Agent({ + allowH2: false, + connect: { lookup: createSsrfGuardedLookup() }, + ...(options?.maxResponseSize !== undefined ? { maxResponseSize: options.maxResponseSize } : {}), + }) + + const rawFetch = async (url: string, init: UndiciRequestInit): Promise => { + const response = await undiciFetch(url, { ...init, dispatcher }) + // double-cast-allowed: undici Response and DOM Response are structurally compatible at runtime + return response as unknown as Response + } + + const guarded = async (input: RequestInfo | URL, init?: RequestInit): Promise => { + const target = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url + // A Request input carries its own method/headers/body/signal; lift them into the + // init (explicit init fields win, per fetch semantics) so the manual redirect + // follower doesn't silently downgrade a guarded POST Request to a bare GET. + let effectiveInit: RequestInit = init ?? {} + if (typeof Request !== 'undefined' && input instanceof Request) { + const bodyAllowed = input.method !== 'GET' && input.method !== 'HEAD' + effectiveInit = { + method: input.method, + headers: input.headers, + body: bodyAllowed ? await input.clone().arrayBuffer() : undefined, + signal: input.signal, + ...init, + } + } + // double-cast-allowed: DOM RequestInit and undici RequestInit are structurally compatible at runtime but the TS types differ + return followRedirectsGuarded(rawFetch, target, effectiveInit as unknown as UndiciRequestInit) + } + + return { fetch: guarded, dispatcher } +} + /** * Builds a standard `fetch`-compatible function that pins every outbound * connection to `resolvedIP`, preventing DNS-rebinding (TOCTOU) between URL diff --git a/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts b/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts new file mode 100644 index 00000000000..b504017c435 --- /dev/null +++ b/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts @@ -0,0 +1,232 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockDnsLookup } = vi.hoisted(() => ({ mockDnsLookup: vi.fn() })) + +vi.mock('dns/promises', () => ({ default: { lookup: mockDnsLookup } })) + +import { createSsrfGuardedLookup } from '@/lib/core/security/input-validation.server' + +type LookupResult = { address: string; family: number } + +function runLookup( + hostname: string, + options: { all?: boolean } = {} +): Promise<{ err: Error | null; address?: string | LookupResult[]; family?: number }> { + return new Promise((resolve) => { + const lookup = createSsrfGuardedLookup() + type LookupCb = (err: Error | null, address?: string | LookupResult[], family?: number) => void + // double-cast-allowed: net.LookupFunction's overloaded callback shapes collapse to this in practice + ;(lookup as unknown as (h: string, o: object, cb: LookupCb) => void)( + hostname, + options, + (err: Error | null, address?: string | LookupResult[], family?: number) => + resolve({ err, address, family }) + ) + }) +} + +describe('createSsrfGuardedLookup', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('passes through a public address', async () => { + mockDnsLookup.mockResolvedValue([{ address: '93.184.216.34', family: 4 }]) + const r = await runLookup('example.com') + expect(r.err).toBeNull() + expect(r.address).toBe('93.184.216.34') + expect(r.family).toBe(4) + }) + + it.each([ + ['loopback', '127.0.0.1'], + ['RFC1918 10.x', '10.0.0.5'], + ['RFC1918 192.168.x', '192.168.1.1'], + ['link-local metadata', '169.254.169.254'], + ['IPv6 loopback', '::1'], + ['IPv4-mapped private', '::ffff:10.0.0.1'], + ])('fails the connect when the host resolves only to %s', async (_label, ip) => { + mockDnsLookup.mockResolvedValue([{ address: ip, family: ip.includes(':') ? 6 : 4 }]) + const r = await runLookup('rebind.attacker.example') + expect(r.err?.message).toMatch(/Blocked by SSRF policy/) + }) + + it('filters private records out of a mixed answer and connects only to public ones', async () => { + // A rebinding server can interleave private records with public ones; only the + // public set may ever reach the socket. + mockDnsLookup.mockResolvedValue([ + { address: '10.1.2.3', family: 4 }, + { address: '93.184.216.34', family: 4 }, + { address: '169.254.169.254', family: 4 }, + ]) + const r = await runLookup('mixed.example', { all: true }) + expect(r.err).toBeNull() + expect(r.address).toEqual([{ address: '93.184.216.34', family: 4 }]) + }) + + it('re-validates on every call (no validated-then-trusted window)', async () => { + // First resolution is public; the rebind flips to private — the second connect fails. + mockDnsLookup.mockResolvedValueOnce([{ address: '93.184.216.34', family: 4 }]) + mockDnsLookup.mockResolvedValueOnce([{ address: '169.254.169.254', family: 4 }]) + + const first = await runLookup('rebind.example') + const second = await runLookup('rebind.example') + + expect(first.err).toBeNull() + expect(second.err?.message).toMatch(/Blocked by SSRF policy/) + }) + + it('propagates DNS resolution failures', async () => { + mockDnsLookup.mockRejectedValue(new Error('ENOTFOUND')) + const r = await runLookup('nope.invalid') + expect(r.err?.message).toBe('ENOTFOUND') + }) + + it('returns the full public set for options.all (fallback across addresses)', async () => { + mockDnsLookup.mockResolvedValue([ + { address: '104.21.22.105', family: 4 }, + { address: '172.67.204.95', family: 4 }, + ]) + const r = await runLookup('multi.example', { all: true }) + expect(r.err).toBeNull() + expect(r.address).toHaveLength(2) + }) +}) + +import { followRedirectsGuarded } from '@/lib/core/security/input-validation.server' + +function redirectTo(location: string, status = 302): Response { + return new Response(null, { status, headers: { location } }) +} + +describe('followRedirectsGuarded', () => { + it('returns a non-redirect response as-is', async () => { + const raw = vi.fn(async () => new Response('ok', { status: 200 })) + const res = await followRedirectsGuarded(raw, 'https://a.example/x', {}) + expect(res.status).toBe(200) + expect(raw).toHaveBeenCalledTimes(1) + }) + + it('follows a same-origin redirect and keeps the caller headers', async () => { + const raw = vi + .fn() + .mockResolvedValueOnce(redirectTo('https://a.example/y')) + .mockResolvedValueOnce(new Response('ok', { status: 200 })) + const res = await followRedirectsGuarded(raw, 'https://a.example/x', { + headers: { 'x-api-key': 'secret' }, + }) + expect(res.status).toBe(200) + expect(raw.mock.calls[1][0]).toBe('https://a.example/y') + expect(raw.mock.calls[1][1].headers).toEqual({ 'x-api-key': 'secret' }) + }) + + it('drops custom headers on a cross-origin redirect', async () => { + const raw = vi + .fn() + .mockResolvedValueOnce(redirectTo('https://b.example/harvest')) + .mockResolvedValueOnce(new Response('ok', { status: 200 })) + await followRedirectsGuarded(raw, 'https://a.example/x', { + headers: { 'x-api-key': 'secret' }, + }) + expect(raw.mock.calls[1][1].headers).toBeUndefined() + }) + + it('blocks a redirect to a private IP literal (metadata endpoint)', async () => { + const raw = vi.fn(async () => redirectTo('http://169.254.169.254/latest/meta-data/')) + await expect(followRedirectsGuarded(raw, 'https://a.example/x', {})).rejects.toThrow( + /Blocked by SSRF policy/ + ) + expect(raw).toHaveBeenCalledTimes(1) + }) + + it('blocks a redirect to a bracketed private IPv6 literal', async () => { + const raw = vi.fn(async () => redirectTo('http://[::1]/admin')) + await expect(followRedirectsGuarded(raw, 'https://a.example/x', {})).rejects.toThrow( + /Blocked by SSRF policy/ + ) + }) + + it('blocks non-http(s) redirect protocols', async () => { + const raw = vi.fn(async () => redirectTo('file:///etc/passwd')) + await expect(followRedirectsGuarded(raw, 'https://a.example/x', {})).rejects.toThrow( + /unsupported protocol/ + ) + }) + + it('caps the number of hops', async () => { + const raw = vi.fn(async () => redirectTo('https://a.example/loop')) + await expect(followRedirectsGuarded(raw, 'https://a.example/x', {})).rejects.toThrow( + /more than \d+ redirects/ + ) + }) + + it('switches POST to a bodyless GET on 303', async () => { + const raw = vi + .fn() + .mockResolvedValueOnce(redirectTo('https://a.example/next', 303)) + .mockResolvedValueOnce(new Response('ok', { status: 200 })) + await followRedirectsGuarded(raw, 'https://a.example/x', { method: 'POST', body: 'data' }) + expect(raw.mock.calls[1][1].method).toBe('GET') + expect(raw.mock.calls[1][1].body).toBeUndefined() + }) + + it('preserves method and body on 307', async () => { + const raw = vi + .fn() + .mockResolvedValueOnce(redirectTo('https://a.example/next', 307)) + .mockResolvedValueOnce(new Response('ok', { status: 200 })) + await followRedirectsGuarded(raw, 'https://a.example/x', { method: 'POST', body: 'data' }) + expect(raw.mock.calls[1][1].method).toBe('POST') + expect(raw.mock.calls[1][1].body).toBe('data') + }) +}) + +describe('followRedirectsGuarded — hardening', () => { + it('blocks a private IP-literal as the INITIAL url (guard is self-contained)', async () => { + const raw = vi.fn(async () => new Response('ok')) + await expect( + followRedirectsGuarded(raw, 'http://169.254.169.254/latest/meta-data/', {}) + ).rejects.toThrow(/Blocked by SSRF policy/) + expect(raw).not.toHaveBeenCalled() + }) + + it('drops entity headers when a 303 switches POST to a bodyless GET', async () => { + const raw = vi + .fn() + .mockResolvedValueOnce(redirectTo('https://a.example/next', 303)) + .mockResolvedValueOnce(new Response('ok', { status: 200 })) + await followRedirectsGuarded(raw, 'https://a.example/x', { + method: 'POST', + body: '{"a":1}', + headers: { 'content-type': 'application/json', 'content-length': '7', 'x-keep': 'yes' }, + }) + const hopHeaders = new Headers(raw.mock.calls[1][1].headers) + expect(hopHeaders.get('content-type')).toBeNull() + expect(hopHeaders.get('content-length')).toBeNull() + expect(hopHeaders.get('x-keep')).toBe('yes') + }) +}) + +describe('followRedirectsGuarded — cross-origin body protection', () => { + it('refuses a cross-origin 307 that would forward a request body', async () => { + const raw = vi.fn(async () => redirectTo('https://b.example/steal', 307)) + await expect( + followRedirectsGuarded(raw, 'https://a.example/token', { + method: 'POST', + body: 'client_secret=shh', + }) + ).rejects.toThrow(/cross-origin redirect would forward a request body/) + }) + + it('allows a bodyless cross-origin redirect', async () => { + const raw = vi + .fn() + .mockResolvedValueOnce(redirectTo('https://b.example/next', 302)) + .mockResolvedValueOnce(new Response('ok', { status: 200 })) + const res = await followRedirectsGuarded(raw, 'https://a.example/x', {}) + expect(res.status).toBe(200) + }) +}) diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index dda637bbe88..f5e6b85298c 100644 --- a/apps/sim/lib/mcp/client.test.ts +++ b/apps/sim/lib/mcp/client.test.ts @@ -21,7 +21,7 @@ vi.mock('@sim/logger', () => ({ })) vi.mock('@/lib/mcp/pinned-fetch', () => ({ - createPinnedMcpFetch: vi.fn(() => ({ fetch: vi.fn(), close: mockPinnedClose })), + createGuardedMcpFetch: vi.fn(() => ({ fetch: vi.fn(), close: mockPinnedClose })), })) /** @@ -276,7 +276,7 @@ describe('McpClient notification handler', () => { const client = new McpClient({ config: createConfig(), securityPolicy: { requireConsent: false, auditLevel: 'basic' }, - resolvedIP: '203.0.113.10', + resolvedIP: '93.184.216.34', }) // A failed connect discards the client without a disconnect(), so the Agent @@ -290,7 +290,7 @@ describe('McpClient notification handler', () => { const client = new McpClient({ config: createConfig(), securityPolicy: { requireConsent: false, auditLevel: 'basic' }, - resolvedIP: '203.0.113.10', + resolvedIP: '93.184.216.34', }) await client.connect() @@ -304,7 +304,7 @@ describe('McpClient notification handler', () => { const client = new McpClient({ config: createConfig(), securityPolicy: { requireConsent: false, auditLevel: 'basic' }, - resolvedIP: '203.0.113.10', + resolvedIP: '93.184.216.34', }) await expect(client.connect()).rejects.toThrow() diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 80931413320..7e7590fc7a2 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -11,9 +11,10 @@ import { import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { getMaxExecutionTimeout } from '@/lib/core/execution-limits' +import { isPrivateOrReservedIP } from '@/lib/core/security/input-validation.server' import { getMcpSafeErrorDiagnostics } from '@/lib/mcp/error-diagnostics' import { McpOauthRedirectRequired } from '@/lib/mcp/oauth' -import { createPinnedMcpFetch } from '@/lib/mcp/pinned-fetch' +import { createGuardedMcpFetch, createPinnedPrivateMcpFetch } from '@/lib/mcp/pinned-fetch' import { type McpClientOptions, McpConnectionError, @@ -73,7 +74,7 @@ export class McpClient { private onToolsChanged?: McpToolsChangedCallback private authProvider?: McpClientOptions['authProvider'] private isConnected = false - private closePinnedTransport?: () => Promise + private closeGuardedTransport?: () => Promise constructor(options: McpClientOptions) { this.config = options.config @@ -96,12 +97,21 @@ export class McpClient { throw new McpError('OAuth MCP server requires an authProvider') } const useOauth = this.config.authType === 'oauth' - const pinned = resolvedIP ? createPinnedMcpFetch(resolvedIP) : undefined - this.closePinnedTransport = pinned?.close + // `resolvedIP` non-null signals the SSRF policy is active for this server (it is null in + // allowlist mode / localhost-on-self-hosted); the guard validates addresses per-connect. + // A private/loopback resolvedIP only reaches here on self-hosted (where the policy + // permits it) — the guarded lookup would filter it, so that case keeps the legacy pin + // to the validated address (old behavior + its anti-rebinding property). + const guarded = resolvedIP + ? isPrivateOrReservedIP(resolvedIP) + ? createPinnedPrivateMcpFetch(resolvedIP) + : createGuardedMcpFetch() + : undefined + this.closeGuardedTransport = guarded?.close this.transport = new StreamableHTTPClientTransport(new URL(this.config.url), { authProvider: useOauth ? this.authProvider : undefined, requestInit: { headers: this.config.headers }, - ...(pinned ? { fetch: pinned.fetch } : {}), + ...(guarded ? { fetch: guarded.fetch } : {}), }) this.client = new Client( @@ -228,16 +238,16 @@ export class McpClient { } /** - * Tears down the pinned transport's HTTP/2 Agent, releasing its sockets. Must run + * Tears down the guarded transport's Agent, releasing its sockets. Must run * on every terminal path — successful disconnect, and failed or cancelled connect — * since a failed `connect()` discards this client without a `disconnect()` call. * Idempotent: the handle is cleared before use so repeat calls (a failed connect * followed by the caller's `disconnect()`) never destroy the same Agent twice. */ private async closeTransportAgent(): Promise { - const close = this.closePinnedTransport + const close = this.closeGuardedTransport if (!close) return - this.closePinnedTransport = undefined + this.closeGuardedTransport = undefined try { await close() } catch (error) { diff --git a/apps/sim/lib/mcp/oauth/revoke.test.ts b/apps/sim/lib/mcp/oauth/revoke.test.ts index beb3a9dd0ab..1e69c99f8d8 100644 --- a/apps/sim/lib/mcp/oauth/revoke.test.ts +++ b/apps/sim/lib/mcp/oauth/revoke.test.ts @@ -31,7 +31,9 @@ const { })) vi.mock('@/lib/core/security/input-validation.server', () => ({ - createPinnedFetchWithDispatcher: vi.fn(() => ({ + isPrivateOrReservedIP: (ip: string) => + ip.startsWith('127.') || ip.startsWith('10.') || ip === '::1', + createSsrfGuardedFetchWithDispatcher: vi.fn(() => ({ fetch: mockUndiciFetch, dispatcher: { destroy: vi.fn(() => Promise.resolve()) }, })), diff --git a/apps/sim/lib/mcp/pinned-fetch.test.ts b/apps/sim/lib/mcp/pinned-fetch.test.ts index fc873cfffdd..608499f17b1 100644 --- a/apps/sim/lib/mcp/pinned-fetch.test.ts +++ b/apps/sim/lib/mcp/pinned-fetch.test.ts @@ -5,11 +5,13 @@ import { sleep } from '@sim/utils/helpers' import { beforeEach, describe, expect, it, vi } from 'vitest' const { + mockCreateGuardedFetchWithDispatcher, mockCreatePinnedFetchWithDispatcher, mockValidateMcpServerSsrf, sentinelFetch, mockDestroy, } = vi.hoisted(() => ({ + mockCreateGuardedFetchWithDispatcher: vi.fn(), mockCreatePinnedFetchWithDispatcher: vi.fn(), mockValidateMcpServerSsrf: vi.fn(), sentinelFetch: vi.fn(), @@ -17,36 +19,36 @@ const { })) vi.mock('@/lib/core/security/input-validation.server', () => ({ + createSsrfGuardedFetchWithDispatcher: mockCreateGuardedFetchWithDispatcher, createPinnedFetchWithDispatcher: mockCreatePinnedFetchWithDispatcher, + isPrivateOrReservedIP: (ip: string) => + ip.startsWith('127.') || ip.startsWith('10.') || ip === '::1', })) vi.mock('@/lib/mcp/domain-check', () => ({ validateMcpServerSsrf: mockValidateMcpServerSsrf, })) -import { createPinnedMcpFetch, createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' +import { createGuardedMcpFetch, createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' -/** The per-request pinned Agent is always built with a DoS-backstop response cap. */ +/** The per-request guarded Agent is always built with a DoS-backstop response cap. */ const withResponseCap = expect.objectContaining({ maxResponseSize: expect.any(Number) }) -describe('createPinnedMcpFetch', () => { +describe('createGuardedMcpFetch', () => { beforeEach(() => { vi.clearAllMocks() mockDestroy.mockResolvedValue(undefined) - mockCreatePinnedFetchWithDispatcher.mockReturnValue({ + mockCreateGuardedFetchWithDispatcher.mockReturnValue({ fetch: sentinelFetch, dispatcher: { destroy: mockDestroy }, }) }) - it('builds the transport on HTTP/1.1 — never opts into allowH2 (undici h2 stalls)', () => { - const { close } = createPinnedMcpFetch('203.0.113.10') + it('builds the transport on the guarded connector with no response cap (streaming)', () => { + const { close } = createGuardedMcpFetch() - // Called with the IP only: no `allowH2`, so the Agent stays on undici's h1.1 default. - expect(mockCreatePinnedFetchWithDispatcher).toHaveBeenCalledWith('203.0.113.10') - const options = mockCreatePinnedFetchWithDispatcher.mock.calls[0][1] as - | { allowH2?: boolean } - | undefined - expect(options?.allowH2).toBeUndefined() + // No options: no `allowH2` opt-in (h1.1 default) and no maxResponseSize — + // the long-lived transport must stream unbounded SSE. + expect(mockCreateGuardedFetchWithDispatcher).toHaveBeenCalledWith() // close() tears down the pooled sockets (incl. the long-lived SSE) on disconnect. void close() @@ -58,24 +60,21 @@ describe('createSsrfGuardedMcpFetch', () => { beforeEach(() => { vi.clearAllMocks() mockDestroy.mockResolvedValue(undefined) - mockCreatePinnedFetchWithDispatcher.mockReturnValue({ + mockCreateGuardedFetchWithDispatcher.mockReturnValue({ fetch: sentinelFetch, dispatcher: { destroy: mockDestroy }, }) sentinelFetch.mockImplementation(async () => new Response('ok')) }) - it('validates each request URL and pins to the resolved IP', async () => { + it('validates each request URL and issues it over the guarded connector', async () => { mockValidateMcpServerSsrf.mockResolvedValue('203.0.113.10') const fetchLike = createSsrfGuardedMcpFetch() await fetchLike('https://attacker.example/revoke', { method: 'POST' }) expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith('https://attacker.example/revoke') - // The pinned Agent is always built with the DoS-backstop response-size cap. - expect(mockCreatePinnedFetchWithDispatcher).toHaveBeenCalledWith( - '203.0.113.10', - withResponseCap - ) + // The guarded Agent is always built with the DoS-backstop response-size cap. + expect(mockCreateGuardedFetchWithDispatcher).toHaveBeenCalledWith(withResponseCap) expect(sentinelFetch).toHaveBeenCalledWith( 'https://attacker.example/revoke', expect.objectContaining({ method: 'POST', signal: expect.any(AbortSignal) }) @@ -258,7 +257,7 @@ describe('createSsrfGuardedMcpFetch', () => { await expect(fetchLike('https://slow-dns.example/token')).rejects.toThrow(/timed out after 5ms/) // Never got past validation, so no request was issued and no Agent was created. - expect(mockCreatePinnedFetchWithDispatcher).not.toHaveBeenCalled() + expect(mockCreateGuardedFetchWithDispatcher).not.toHaveBeenCalled() expect(sentinelFetch).not.toHaveBeenCalled() expect(mockDestroy).not.toHaveBeenCalled() }) @@ -274,7 +273,7 @@ describe('createSsrfGuardedMcpFetch', () => { await expect( fetchLike('https://slow.example/token', { signal: controller.signal }) ).rejects.toThrow('pre-aborted') - expect(mockCreatePinnedFetchWithDispatcher).not.toHaveBeenCalled() + expect(mockCreateGuardedFetchWithDispatcher).not.toHaveBeenCalled() // Let the swallowed validation rejection settle so a leak would surface here. await sleep(0) }) @@ -288,7 +287,7 @@ describe('createSsrfGuardedMcpFetch', () => { controller.abort(new Error('caller cancelled')) await expect(pending).rejects.toThrow('caller cancelled') - expect(mockCreatePinnedFetchWithDispatcher).not.toHaveBeenCalled() + expect(mockCreateGuardedFetchWithDispatcher).not.toHaveBeenCalled() }) it('propagates a caller-initiated abort unchanged (composed with the deadline)', async () => { @@ -320,7 +319,7 @@ describe('createSsrfGuardedMcpFetch', () => { await expect( fetchLike('http://169.254.169.254/latest/meta-data/', { method: 'POST' }) ).rejects.toThrow('blocked') - expect(mockCreatePinnedFetchWithDispatcher).not.toHaveBeenCalled() + expect(mockCreateGuardedFetchWithDispatcher).not.toHaveBeenCalled() expect(sentinelFetch).not.toHaveBeenCalled() }) @@ -330,10 +329,7 @@ describe('createSsrfGuardedMcpFetch', () => { await fetchLike(new URL('https://attacker.example/discover')) expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith('https://attacker.example/discover') - expect(mockCreatePinnedFetchWithDispatcher).toHaveBeenCalledWith( - '203.0.113.10', - withResponseCap - ) + expect(mockCreateGuardedFetchWithDispatcher).toHaveBeenCalledWith(withResponseCap) }) it('falls back to global fetch when validation returns no IP', async () => { @@ -345,7 +341,7 @@ describe('createSsrfGuardedMcpFetch', () => { const fetchLike = createSsrfGuardedMcpFetch() await fetchLike('https://allowed.internal/mcp') - expect(mockCreatePinnedFetchWithDispatcher).not.toHaveBeenCalled() + expect(mockCreateGuardedFetchWithDispatcher).not.toHaveBeenCalled() expect(globalFetch).toHaveBeenCalledTimes(1) // No pinned Agent was created, so there is nothing to tear down. expect(mockDestroy).not.toHaveBeenCalled() @@ -354,3 +350,24 @@ describe('createSsrfGuardedMcpFetch', () => { } }) }) + +describe('self-hosted private-resolution carve-out', () => { + it('keeps the legacy pin for a loopback-resolving host (guarded lookup would filter it)', async () => { + // Self-hosted DNS alias -> 127.0.0.1: policy allows it. The guarded lookup would + // strand the connect and an unguarded fallback would reopen rebinding — so this case + // pins to the validated address, preserving the old behavior and its security property. + mockValidateMcpServerSsrf.mockResolvedValue('127.0.0.1') + mockCreatePinnedFetchWithDispatcher.mockReturnValue({ + fetch: sentinelFetch, + dispatcher: { destroy: mockDestroy }, + }) + sentinelFetch.mockImplementation(async () => new Response('ok')) + const fetchLike = createSsrfGuardedMcpFetch() + await fetchLike('https://my-local-alias/mcp') + expect(mockCreatePinnedFetchWithDispatcher).toHaveBeenCalledWith( + '127.0.0.1', + expect.objectContaining({ maxResponseSize: expect.any(Number) }) + ) + expect(mockCreateGuardedFetchWithDispatcher).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/mcp/pinned-fetch.ts b/apps/sim/lib/mcp/pinned-fetch.ts index 162f568c2af..3a83b783cc1 100644 --- a/apps/sim/lib/mcp/pinned-fetch.ts +++ b/apps/sim/lib/mcp/pinned-fetch.ts @@ -1,31 +1,55 @@ import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport.js' import { createLogger } from '@sim/logger' import type { Agent } from 'undici' -import { createPinnedFetchWithDispatcher } from '@/lib/core/security/input-validation.server' +import { + createPinnedFetchWithDispatcher, + createSsrfGuardedFetchWithDispatcher, + isPrivateOrReservedIP, +} from '@/lib/core/security/input-validation.server' import { validateMcpServerSsrf } from '@/lib/mcp/domain-check' import { McpError } from '@/lib/mcp/types' const logger = createLogger('McpOauthFetch') const transportLogger = createLogger('McpTransportFetch') -/** Pinned fetch for the live MCP transport, plus a handle to release its sockets. */ -export interface PinnedMcpFetch { +/** SSRF-guarded fetch for the live MCP transport, plus a handle to release its sockets. */ +export interface GuardedMcpFetch { fetch: typeof fetch /** Tears down the Agent's pooled sockets; call when the MCP client disconnects. */ close: () => Promise } /** - * Pinned fetch for the long-lived MCP transport (one Agent reused per connection). + * SSRF-guarded fetch for the long-lived MCP transport (one Agent reused per connection). + * + * DNS resolves normally and EVERY socket connect validates the resolved addresses + * against the private/reserved blocklist (validate-at-connect, the LibreChat + * pattern), and redirects are followed manually with per-hop validation — an + * IP-literal redirect target (which bypasses any connect-time lookup) is checked + * explicitly, and custom headers are dropped on cross-origin hops. This replaces + * the previous single-IP pin, which no reference MCP client uses and which welded + * every request to one address with no fallback. * * Runs HTTP/1.1: we do not opt into undici's experimental `allowH2`, whose h2 path stalls * with headers-but-no-body on reused POST sessions (nodejs/undici #2311, #3433, #4143) — * the streamable-HTTP `initialize` hang behind a CDN. Both official MCP SDKs use h1.1, and * the transport has no concurrency to gain from h2. `close` tears down pooled sockets - * (incl. the SSE connection) on disconnect; IP-pinning is protocol-independent. + * (incl. the SSE connection) on disconnect. + */ +/** + * Legacy single-IP pin, kept ONLY for self-hosted private/loopback resolutions + * (a DNS alias the policy explicitly permits): the guarded lookup would filter + * the address and strand the connect, while an unguarded fallback would reopen + * rebinding/redirect escape. Pinning to the validated address preserves the old + * behavior and its security property for exactly this carve-out. */ -export function createPinnedMcpFetch(resolvedIP: string): PinnedMcpFetch { +export function createPinnedPrivateMcpFetch(resolvedIP: string): GuardedMcpFetch { const { fetch: pinnedFetch, dispatcher } = createPinnedFetchWithDispatcher(resolvedIP) + return { fetch: pinnedFetch, close: () => dispatcher.destroy() } +} + +export function createGuardedMcpFetch(): GuardedMcpFetch { + const { fetch: guardedFetch, dispatcher } = createSsrfGuardedFetchWithDispatcher() // Per-request phase logging: a stalled transport request (e.g. a first `initialize` that hangs // to the client timeout) shows whether it stalls BEFORE response headers ("request" with no // "response headers" = connect/request stall) or AFTER ("response headers" then the SDK's @@ -44,7 +68,7 @@ export function createPinnedMcpFetch(resolvedIP: string): PinnedMcpFetch { const startedAt = Date.now() transportLogger.info('MCP transport request', { host, method }) try { - const response = await pinnedFetch(input, init) + const response = await guardedFetch(input, init) transportLogger.info('MCP transport response headers', { host, method, @@ -180,11 +204,13 @@ function releaseStreamOnSettle( * Builds a `FetchLike` for one-shot MCP OAuth calls (discovery, dynamic client * registration, token exchange/refresh, RFC 7009 revocation). Each hop's URL comes from * attacker-controllable authorization-server metadata, so every request is re-validated - * against the SSRF policy and pinned to the resolved IP. + * against the SSRF policy up front (fail fast, friendly errors) AND at socket-connect + * time via the guarded lookup, with redirects followed manually under per-hop + * validation (IP-literal targets included) and cross-origin header stripping. * * The SDK provides none of the safety itself (no timeout on OAuth legs, lazy body reads), * so the guard owns it: the `timeoutMs` deadline covers validation + request + the buffered - * body read; the per-request pinned Agent is `destroy()`ed on every path; and the returned + * body read; the per-request guarded Agent is `destroy()`ed on every path; and the returned * `Response` is a detached in-memory copy. Only our own deadline is relabeled to `McpError`. * * @param timeoutMs Per-request deadline in ms (defaults to 30s; override for tests). @@ -204,16 +230,25 @@ export function createSsrfGuardedMcpFetch(timeoutMs: number = OAUTH_FETCH_TIMEOU try { logger.info('OAuth guarded fetch: validating', { host }) const resolvedIP = await withDeadline(validateMcpServerSsrf(target), signal) - logger.info('OAuth guarded fetch: requesting', { host, pinned: Boolean(resolvedIP) }) + logger.info('OAuth guarded fetch: requesting', { host, guarded: Boolean(resolvedIP) }) let response: Response - if (resolvedIP) { + if (resolvedIP && isPrivateOrReservedIP(resolvedIP)) { + // Self-hosted private/loopback resolution (policy-permitted): the guarded lookup + // would filter the address, and an unguarded fallback would reopen rebinding — + // keep the legacy pin to the validated address for exactly this case. const pinned = createPinnedFetchWithDispatcher(resolvedIP, { maxResponseSize: MAX_OAUTH_RESPONSE_BYTES, }) dispatcher = pinned.dispatcher response = await withDeadline(pinned.fetch(url, { ...init, signal }), signal) + } else if (resolvedIP) { + const guarded = createSsrfGuardedFetchWithDispatcher({ + maxResponseSize: MAX_OAUTH_RESPONSE_BYTES, + }) + dispatcher = guarded.dispatcher + response = await withDeadline(guarded.fetch(url, { ...init, signal }), signal) } else { - // No pin (self-hosted allowlist) — global fetch over the shared dispatcher. + // No guard (self-hosted allowlist / localhost carve-out) — global fetch as before. response = await withDeadline(globalThis.fetch(url, { ...init, signal }), signal) } // The probe's `initialize` can stream (text/event-stream); hand it back live so the