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.

Author: waleedlatif1

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<Response>,
+  input: string,
+  init: UndiciRequestInit
+): Promise<Response> {
+  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<Response> => {
+    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<Response> => {
+    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

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')
+  })
+})

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 })),
 }))
 
 /**

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<void>
+  private closeGuardedTransport?: () => Promise<void>
 
   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<void> {
-    const close = this.closePinnedTransport
+    const close = this.closeGuardedTransport
     if (!close) return
-    this.closePinnedTransport = undefined
+    this.closeGuardedTransport = undefined
     try {
       await close()
     } catch (error) {