fix(mcp): pin (not skip) self-hosted private resolutions; refuse cross-origin body forwards

- 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.

Author: waleedlatif1

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
   }
 }

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

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,

apps/sim/lib/mcp/pinned-fetch.test.ts

@@ -6,18 +6,21 @@ 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(),
 }))
 
 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()
   })
 })

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