From f6bd96b3688923377dd9050e2b271a7c288a54fd Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 15:54:43 -0700 Subject: [PATCH 1/6] fix(mcp): replace single-IP pinning with validate-at-connect SSRF guard Swaps the MCP transport + OAuth guard from pin-one-resolved-IP to the standard pattern (LibreChat getSSRFConnect): DNS resolves normally and EVERY socket connect filters the resolved addresses against the private/reserved blocklist, closing the validate-then-trust window a rebind could race and removing the non-standard mechanism implicated in the headers-then-no-body stalls (no reference MCP client pins IPs; a pinned attempt welded to a bad flow cannot escape, while retries rotate via resolver round-robin and succeed). Adversarially reviewed before shipping; both findings closed here: - Redirects are now followed manually with per-hop validation: an IP-literal redirect target (which bypasses ANY connect-time lookup - Node skips the custom lookup for numeric hosts) is checked explicitly, closing a metadata- endpoint redirect hole the old pin also had. - Custom request headers are dropped on cross-origin hops, so a redirect to a second attacker host cannot harvest configured auth headers. Policy gating unchanged: the guard activates exactly where the pin did (validateMcpServerSsrf non-null; allowlist mode / localhost-on-self-hosted stay unguarded). Non-MCP consumers of the pinned fetch are untouched. 397 tests incl. new rebinding, mixed-answer, IP-literal-redirect, and hop-cap coverage. --- .../core/security/input-validation.server.ts | 135 ++++++++++++- .../core/security/ssrf-guarded-lookup.test.ts | 185 ++++++++++++++++++ apps/sim/lib/mcp/client.test.ts | 2 +- apps/sim/lib/mcp/client.ts | 16 +- apps/sim/lib/mcp/oauth/revoke.test.ts | 2 +- apps/sim/lib/mcp/pinned-fetch.test.ts | 53 +++-- apps/sim/lib/mcp/pinned-fetch.ts | 40 ++-- 7 files changed, 374 insertions(+), 59 deletions(-) create mode 100644 apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index f8532021313..54b898bad49 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -105,10 +105,7 @@ 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. - const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true }) - const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0] + const { address } = await dns.lookup(cleanHostname, { verbatim: true }) const resolvedIsLoopback = ipaddr.isValid(address) && @@ -422,6 +419,136 @@ 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) + 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 + 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) + await response.body?.cancel().catch(() => {}) + // Per the fetch spec: 303 (and 301/302 on POST) switch to a bodyless GET. + if (status === 303 || ((status === 301 || status === 302) && method === 'POST')) { + method = 'GET' + body = undefined + } + if (nextUrl.origin !== currentUrl.origin) headers = undefined + 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 + // double-cast-allowed: DOM RequestInit and undici RequestInit are structurally compatible at runtime but the TS types differ + return followRedirectsGuarded(rawFetch, target, (init ?? {}) 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..69aa4aedc4b --- /dev/null +++ b/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts @@ -0,0 +1,185 @@ +/** + * @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') + }) +}) diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index dda637bbe88..a723e5a6f10 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 })), })) /** diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 80931413320..8ce043d7bfd 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -13,7 +13,7 @@ import { getErrorMessage } from '@sim/utils/errors' import { getMaxExecutionTimeout } from '@/lib/core/execution-limits' import { getMcpSafeErrorDiagnostics } from '@/lib/mcp/error-diagnostics' import { McpOauthRedirectRequired } from '@/lib/mcp/oauth' -import { createPinnedMcpFetch } from '@/lib/mcp/pinned-fetch' +import { createGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' import { type McpClientOptions, McpConnectionError, @@ -73,7 +73,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 +96,14 @@ 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. + const guarded = 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( @@ -235,9 +237,9 @@ export class McpClient { * 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..09dca357301 100644 --- a/apps/sim/lib/mcp/oauth/revoke.test.ts +++ b/apps/sim/lib/mcp/oauth/revoke.test.ts @@ -31,7 +31,7 @@ const { })) vi.mock('@/lib/core/security/input-validation.server', () => ({ - createPinnedFetchWithDispatcher: vi.fn(() => ({ + 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..63ec1eba7dd 100644 --- a/apps/sim/lib/mcp/pinned-fetch.test.ts +++ b/apps/sim/lib/mcp/pinned-fetch.test.ts @@ -5,48 +5,45 @@ import { sleep } from '@sim/utils/helpers' import { beforeEach, describe, expect, it, vi } from 'vitest' const { - mockCreatePinnedFetchWithDispatcher, + mockCreateGuardedFetchWithDispatcher, mockValidateMcpServerSsrf, sentinelFetch, mockDestroy, } = vi.hoisted(() => ({ - mockCreatePinnedFetchWithDispatcher: vi.fn(), + mockCreateGuardedFetchWithDispatcher: vi.fn(), mockValidateMcpServerSsrf: vi.fn(), sentinelFetch: vi.fn(), mockDestroy: vi.fn(), })) vi.mock('@/lib/core/security/input-validation.server', () => ({ - createPinnedFetchWithDispatcher: mockCreatePinnedFetchWithDispatcher, + createSsrfGuardedFetchWithDispatcher: mockCreateGuardedFetchWithDispatcher, })) 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 +55,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 +252,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 +268,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 +282,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 +314,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 +324,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 +336,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() diff --git a/apps/sim/lib/mcp/pinned-fetch.ts b/apps/sim/lib/mcp/pinned-fetch.ts index 162f568c2af..4f3c9bb1e57 100644 --- a/apps/sim/lib/mcp/pinned-fetch.ts +++ b/apps/sim/lib/mcp/pinned-fetch.ts @@ -1,31 +1,39 @@ 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 { createSsrfGuardedFetchWithDispatcher } 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. */ -export function createPinnedMcpFetch(resolvedIP: string): PinnedMcpFetch { - const { fetch: pinnedFetch, dispatcher } = createPinnedFetchWithDispatcher(resolvedIP) +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 +52,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 +188,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 +214,16 @@ 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) { - const pinned = createPinnedFetchWithDispatcher(resolvedIP, { + const guarded = createSsrfGuardedFetchWithDispatcher({ maxResponseSize: MAX_OAUTH_RESPONSE_BYTES, }) - dispatcher = pinned.dispatcher - response = await withDeadline(pinned.fetch(url, { ...init, signal }), signal) + 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 From a518963f55f5116a17a76346b6cf28ed289808ee Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 16:11:16 -0700 Subject: [PATCH 2/6] fix(mcp): restore IPv4 preference and harden the guarded redirect follower - Restore validateUrlWithDNS's prefer-IPv4 resolution (a stale working-tree hunk accidentally reverted #5798 for the still-pinned non-MCP consumers). - Validate the INITIAL url's IP-literal in followRedirectsGuarded, not just redirect hops, so the exported guard is self-contained. - Drop entity headers (content-length/type/encoding) when a 301/302/303 switches a POST to a bodyless GET, which undici would otherwise reject. - Lift method/headers/body/signal from a Request input instead of silently downgrading a guarded POST Request to a bare GET. --- .../core/security/input-validation.server.ts | 36 +++++++++++++++++-- .../core/security/ssrf-guarded-lookup.test.ts | 26 ++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index 54b898bad49..a25789206c7 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -105,7 +105,10 @@ export async function validateUrlWithDNS( } try { - const { address } = await dns.lookup(cleanHostname, { verbatim: true }) + // Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs + // on IPv4-only egress (e.g. AWS NAT gateways). See validateMcpServerSsrf. + const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true }) + const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0] const resolvedIsLoopback = ipaddr.isValid(address) && @@ -486,6 +489,9 @@ export async function followRedirectsGuarded( 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 @@ -506,10 +512,20 @@ export async function followRedirectsGuarded( const nextUrl = new URL(location, currentUrl) assertGuardedRedirectTarget(nextUrl) await response.body?.cancel().catch(() => {}) - // Per the fetch spec: 303 (and 301/302 on POST) switch to a bodyless GET. + // 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') + headers = sanitized as unknown as UndiciRequestInit['headers'] + } } if (nextUrl.origin !== currentUrl.origin) headers = undefined currentUrl = nextUrl @@ -542,8 +558,22 @@ export function createSsrfGuardedFetchWithDispatcher(options?: { maxResponseSize 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, (init ?? {}) as unknown as UndiciRequestInit) + return followRedirectsGuarded(rawFetch, target, effectiveInit as unknown as UndiciRequestInit) } return { fetch: guarded, dispatcher } diff --git a/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts b/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts index 69aa4aedc4b..598d01953fa 100644 --- a/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts +++ b/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts @@ -183,3 +183,29 @@ describe('followRedirectsGuarded', () => { 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') + }) +}) From b5adb5197aa381c0541814c8ee2f6ad3e2e14792 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 16:26:16 -0700 Subject: [PATCH 3/6] fix(mcp): annotate headers double-cast and cancel redirect body before throw paths - Add the missing double-cast-allowed annotation on the sanitized-headers cast (strict boundary audit). - Cancel the redirect response body before the hop-cap / blocked-target throws so those paths can't leave a socket checked out on the long-lived Agent. --- apps/sim/lib/core/security/input-validation.server.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index a25789206c7..cbcb20d3c7d 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -506,12 +506,14 @@ export async function followRedirectsGuarded( 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) - await response.body?.cancel().catch(() => {}) // 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). @@ -524,6 +526,7 @@ export async function followRedirectsGuarded( 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'] } } From ccde12e54c50fee891e00d904ff7b4da3df189ac Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 16:37:37 -0700 Subject: [PATCH 4/6] fix(mcp): keep self-hosted private-resolving hosts unguarded (old-pin parity) A DNS alias resolving to loopback/private is only reachable on self-hosted, where the policy explicitly permits it; the guarded lookup would filter the address and strand the connect where the old pin connected. Both MCP gates (transport + OAuth guard) now route private/loopback resolutions over the unguarded path, same as the localhost carve-out. Test IPs moved off RFC-5737 TEST-NET (which is correctly classified reserved). --- apps/sim/lib/mcp/client.test.ts | 6 +++--- apps/sim/lib/mcp/client.ts | 7 ++++++- apps/sim/lib/mcp/oauth/revoke.test.ts | 2 ++ apps/sim/lib/mcp/pinned-fetch.test.ts | 21 +++++++++++++++++++++ apps/sim/lib/mcp/pinned-fetch.ts | 10 ++++++++-- 5 files changed, 40 insertions(+), 6 deletions(-) diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index a723e5a6f10..f5e6b85298c 100644 --- a/apps/sim/lib/mcp/client.test.ts +++ b/apps/sim/lib/mcp/client.test.ts @@ -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 8ce043d7bfd..aee4197402b 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -11,6 +11,7 @@ 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 { createGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' @@ -98,7 +99,11 @@ export class McpClient { const useOauth = this.config.authType === 'oauth' // `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. - const guarded = resolvedIP ? createGuardedMcpFetch() : undefined + // A private/loopback resolvedIP only reaches here on self-hosted (where the policy + // permits it) — the guarded lookup would filter it, so those connect unguarded like the + // localhost carve-out. + const guarded = + resolvedIP && !isPrivateOrReservedIP(resolvedIP) ? createGuardedMcpFetch() : undefined this.closeGuardedTransport = guarded?.close this.transport = new StreamableHTTPClientTransport(new URL(this.config.url), { authProvider: useOauth ? this.authProvider : undefined, diff --git a/apps/sim/lib/mcp/oauth/revoke.test.ts b/apps/sim/lib/mcp/oauth/revoke.test.ts index 09dca357301..1e69c99f8d8 100644 --- a/apps/sim/lib/mcp/oauth/revoke.test.ts +++ b/apps/sim/lib/mcp/oauth/revoke.test.ts @@ -31,6 +31,8 @@ const { })) vi.mock('@/lib/core/security/input-validation.server', () => ({ + 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 63ec1eba7dd..13f71eb54b0 100644 --- a/apps/sim/lib/mcp/pinned-fetch.test.ts +++ b/apps/sim/lib/mcp/pinned-fetch.test.ts @@ -18,6 +18,8 @@ const { vi.mock('@/lib/core/security/input-validation.server', () => ({ createSsrfGuardedFetchWithDispatcher: mockCreateGuardedFetchWithDispatcher, + isPrivateOrReservedIP: (ip: string) => + ip.startsWith('127.') || ip.startsWith('10.') || ip === '::1', })) vi.mock('@/lib/mcp/domain-check', () => ({ validateMcpServerSsrf: mockValidateMcpServerSsrf, @@ -345,3 +347,22 @@ describe('createSsrfGuardedMcpFetch', () => { } }) }) + +describe('self-hosted private-resolution carve-out', () => { + it('routes a loopback-resolving host over global fetch (guarded lookup would filter it)', async () => { + // Self-hosted DNS alias -> 127.0.0.1: policy allows it, so the guard must not + // strand the connect. Falls back to global fetch, same as the allowlist path. + mockValidateMcpServerSsrf.mockResolvedValue('127.0.0.1') + const globalFetch = vi + .spyOn(globalThis, 'fetch') + .mockImplementation(async () => new Response('ok')) + try { + const fetchLike = createSsrfGuardedMcpFetch() + await fetchLike('https://my-local-alias/mcp') + expect(mockCreateGuardedFetchWithDispatcher).not.toHaveBeenCalled() + expect(globalFetch).toHaveBeenCalledTimes(1) + } finally { + globalFetch.mockRestore() + } + }) +}) diff --git a/apps/sim/lib/mcp/pinned-fetch.ts b/apps/sim/lib/mcp/pinned-fetch.ts index 4f3c9bb1e57..85756f85e11 100644 --- a/apps/sim/lib/mcp/pinned-fetch.ts +++ b/apps/sim/lib/mcp/pinned-fetch.ts @@ -1,7 +1,10 @@ import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport.js' import { createLogger } from '@sim/logger' import type { Agent } from 'undici' -import { createSsrfGuardedFetchWithDispatcher } from '@/lib/core/security/input-validation.server' +import { + createSsrfGuardedFetchWithDispatcher, + isPrivateOrReservedIP, +} from '@/lib/core/security/input-validation.server' import { validateMcpServerSsrf } from '@/lib/mcp/domain-check' import { McpError } from '@/lib/mcp/types' @@ -216,7 +219,10 @@ export function createSsrfGuardedMcpFetch(timeoutMs: number = OAUTH_FETCH_TIMEOU const resolvedIP = await withDeadline(validateMcpServerSsrf(target), signal) logger.info('OAuth guarded fetch: requesting', { host, guarded: Boolean(resolvedIP) }) let response: Response - if (resolvedIP) { + // A private/loopback resolvedIP only occurs on self-hosted where the policy permits + // it — the guarded lookup would filter it out, so those go over global fetch like the + // allowlist carve-out below. + if (resolvedIP && !isPrivateOrReservedIP(resolvedIP)) { const guarded = createSsrfGuardedFetchWithDispatcher({ maxResponseSize: MAX_OAUTH_RESPONSE_BYTES, }) From 8ada731f33cd1c52b6b777f6f1905e506dacbe0d Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 16:46:00 -0700 Subject: [PATCH 5/6] fix(mcp): pin (not skip) self-hosted private resolutions; refuse cross-origin body forwards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The private/loopback carve-out now keeps the LEGACY PIN to the validated address instead of falling back to unguarded fetch — preserving both the old behavior and its anti-rebinding property for self-hosted DNS aliases. - Cross-origin redirect hops now also refuse to forward a request body (307/308 preserve method+body; post-pin those redirects really dial the new origin, so an open redirect could exfiltrate OAuth client secrets). Bodyless cross-origin redirects still follow. Tests for both. --- .../core/security/input-validation.server.ts | 12 ++++++- .../core/security/ssrf-guarded-lookup.test.ts | 21 ++++++++++++ apps/sim/lib/mcp/client.ts | 13 +++++--- apps/sim/lib/mcp/pinned-fetch.test.ts | 33 +++++++++++-------- apps/sim/lib/mcp/pinned-fetch.ts | 27 ++++++++++++--- 5 files changed, 82 insertions(+), 24 deletions(-) diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index cbcb20d3c7d..3ac3c6ea731 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -530,7 +530,17 @@ export async function followRedirectsGuarded( headers = sanitized as unknown as UndiciRequestInit['headers'] } } - if (nextUrl.origin !== currentUrl.origin) headers = undefined + 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 } } diff --git a/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts b/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts index 598d01953fa..b504017c435 100644 --- a/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts +++ b/apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts @@ -209,3 +209,24 @@ describe('followRedirectsGuarded — hardening', () => { 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.ts b/apps/sim/lib/mcp/client.ts index aee4197402b..9cb281888fc 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -14,7 +14,7 @@ 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 { createGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' +import { createGuardedMcpFetch, createPinnedPrivateMcpFetch } from '@/lib/mcp/pinned-fetch' import { type McpClientOptions, McpConnectionError, @@ -100,10 +100,13 @@ export class McpClient { // `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 those connect unguarded like the - // localhost carve-out. - const guarded = - resolvedIP && !isPrivateOrReservedIP(resolvedIP) ? createGuardedMcpFetch() : undefined + // 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, diff --git a/apps/sim/lib/mcp/pinned-fetch.test.ts b/apps/sim/lib/mcp/pinned-fetch.test.ts index 13f71eb54b0..608499f17b1 100644 --- a/apps/sim/lib/mcp/pinned-fetch.test.ts +++ b/apps/sim/lib/mcp/pinned-fetch.test.ts @@ -6,11 +6,13 @@ 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(), mockDestroy: vi.fn(), @@ -18,6 +20,7 @@ const { vi.mock('@/lib/core/security/input-validation.server', () => ({ createSsrfGuardedFetchWithDispatcher: mockCreateGuardedFetchWithDispatcher, + createPinnedFetchWithDispatcher: mockCreatePinnedFetchWithDispatcher, isPrivateOrReservedIP: (ip: string) => ip.startsWith('127.') || ip.startsWith('10.') || ip === '::1', })) @@ -349,20 +352,22 @@ describe('createSsrfGuardedMcpFetch', () => { }) describe('self-hosted private-resolution carve-out', () => { - it('routes a loopback-resolving host over global fetch (guarded lookup would filter it)', async () => { - // Self-hosted DNS alias -> 127.0.0.1: policy allows it, so the guard must not - // strand the connect. Falls back to global fetch, same as the allowlist path. + 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') - const globalFetch = vi - .spyOn(globalThis, 'fetch') - .mockImplementation(async () => new Response('ok')) - try { - const fetchLike = createSsrfGuardedMcpFetch() - await fetchLike('https://my-local-alias/mcp') - expect(mockCreateGuardedFetchWithDispatcher).not.toHaveBeenCalled() - expect(globalFetch).toHaveBeenCalledTimes(1) - } finally { - globalFetch.mockRestore() - } + 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 85756f85e11..3a83b783cc1 100644 --- a/apps/sim/lib/mcp/pinned-fetch.ts +++ b/apps/sim/lib/mcp/pinned-fetch.ts @@ -2,6 +2,7 @@ import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport.js' import { createLogger } from '@sim/logger' import type { Agent } from 'undici' import { + createPinnedFetchWithDispatcher, createSsrfGuardedFetchWithDispatcher, isPrivateOrReservedIP, } from '@/lib/core/security/input-validation.server' @@ -35,6 +36,18 @@ export interface GuardedMcpFetch { * the transport has no concurrency to gain from h2. `close` tears down pooled sockets * (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 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 @@ -219,10 +232,16 @@ export function createSsrfGuardedMcpFetch(timeoutMs: number = OAUTH_FETCH_TIMEOU const resolvedIP = await withDeadline(validateMcpServerSsrf(target), signal) logger.info('OAuth guarded fetch: requesting', { host, guarded: Boolean(resolvedIP) }) let response: Response - // A private/loopback resolvedIP only occurs on self-hosted where the policy permits - // it — the guarded lookup would filter it out, so those go over global fetch like the - // allowlist carve-out below. - if (resolvedIP && !isPrivateOrReservedIP(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, }) From 616abae40424cb1b6b10a220770fad1a08044018 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 16:52:16 -0700 Subject: [PATCH 6/6] chore(mcp): true up stale pinned-era doc comments --- apps/sim/lib/core/security/input-validation.server.ts | 3 ++- apps/sim/lib/mcp/client.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index 3ac3c6ea731..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] diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 9cb281888fc..7e7590fc7a2 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -238,7 +238,7 @@ 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