fix(mcp): stream pinned transport under Bun (providers + self-hosted-private MCP) (#5901)

* fix(mcp): stream pinned transport via undici.request + redirect interceptor Extends the Bun undici-streaming fix to createPinnedFetchWithDispatcher (providers, A2A, self-hosted-private MCP over SSE). It now routes through undiciRequestAsResponse like the guarded builder, so streaming bodies deliver under Bun. Unlike the guarded path it has no followRedirectsGuarded wrapper (it's handed straight to provider SDKs), so redirects are followed via undici's redirect interceptor composed onto the pinned Agent — every hop still dispatches through the pinned connect.lookup (resolvedIP), so a redirect can't escape to another address, matching the old fetch guarantee. secureFetchWithPinnedIP (raw Node http, tools path) is untouched. * fix(mcp): honor redirect mode + drop cross-origin credentials on pinned fetch Replaces the always-on redirect interceptor with redirect-mode-aware handling: - redirect:'manual' returns the 3xx without following (detectMcpAuthType inspects it) - redirect:'error' throws on a 3xx - default 'follow' uses followRedirectsGuarded, which drops ALL headers on a cross-origin hop (so a redirect can't disclose a provider api-key to another origin — Greptile P1) and stamps the final response.url + redirected flag. Extracts the shared Request-lift helper used by both guarded and pinned builders. * fix(mcp): don't block private IP-literal URLs on the pinned fetch path Routing the pinned fetch through followRedirectsGuarded added an initial assertGuardedRedirectTarget check the old undici.fetch path never ran, which would block a self-hosted MCP configured with a private IP-literal URL (e.g. http://10.0.0.5:3000/mcp) — its own transport. The pinned path's callers already validate the target and the private carve-out intentionally pins to a private IP, so skip the initial-target check (validateInitialTarget: false) while still validating every redirect hop. Adds a regression test. * fix(mcp): carry redirect mode from a Request input in liftFetchArgs liftFetchArgs copied method/headers/body/signal from a Request but omitted redirect, so a Request({ redirect: 'manual' }) on the pinned path defaulted to 'follow' and was transparently followed. Copy input.redirect (explicit init still wins). Adds a Request-input redirect-mode test. * fix(mcp): permit the pinned IP as a redirect target (initial + hops), block other private IPs Consolidates the pinned-path redirect policy into one mechanism. followRedirectsGuarded took validateInitialTarget to skip the initial private-IP check, but per-hop checks still blocked a self-hosted MCP redirecting to its own pinned private IP (e.g. a trailing-slash 301 to http://10.0.0.5/mcp/). Replace it with allowRedirectToIp: the pinned fetch permits exactly its own validated IP as a target — initial URL and any hop that stays on it — while every OTHER private target (e.g. the 169.254.169.254 metadata IP) stays blocked. Tests cover the same-IP hop (followed) and the metadata-IP escape (still refused).
2026-09-24 15:45:35 +08:00 · 2026-07-23 15:27:16 -07:00
parent a5e1030e9e
commit 6f90d8976d
2 changed files with 218 additions and 67 deletions
@@ -14,7 +14,6 @@ import {
  Agent,
  type Dispatcher,
  type RequestInit as UndiciRequestInit,
-  fetch as undiciFetch,
  request as undiciRequest,
 } from 'undici'
 import { isHosted, isPrivateDatabaseHostsAllowed } from '@/lib/core/config/env-flags'
@@ -544,7 +543,7 @@ const MAX_GUARDED_REDIRECTS = 5
 * 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 {
+function assertGuardedRedirectTarget(url: URL, allowedPinnedIp?: string): void {
  if (url.protocol !== 'http:' && url.protocol !== 'https:') {
    throw new Error(`Blocked by SSRF policy: redirect to unsupported protocol ${url.protocol}`)
  }
@@ -553,6 +552,16 @@ function assertGuardedRedirectTarget(url: URL): void {
      ? url.hostname.slice(1, -1)
      : url.hostname
  if (ipaddr.isValid(host) && isPrivateOrReservedIP(host)) {
+    // The pinned-private carve-out permits exactly its own validated IP as a target (a
+    // self-hosted MCP on a private IP, or a same-host redirect that stays on it) — but nothing
+    // else private (a redirect to e.g. the cloud metadata IP is still blocked).
+    if (
+      allowedPinnedIp &&
+      ipaddr.isValid(allowedPinnedIp) &&
+      ipaddr.process(host).toString() === ipaddr.process(allowedPinnedIp).toString()
+    ) {
+      return
+    }
    throw new Error('Blocked by SSRF policy: redirect to a private or reserved address')
  }
 }
@@ -568,12 +577,15 @@ function assertGuardedRedirectTarget(url: URL): void {
 export async function followRedirectsGuarded(
  rawFetch: (url: string, init: UndiciRequestInit) => Promise<Response>,
  input: string,
-  init: UndiciRequestInit
+  init: UndiciRequestInit,
+  options?: { allowRedirectToIp?: string }
 ): Promise<Response> {
  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)
+  // 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. `allowRedirectToIp`
+  // (the pinned-private MCP carve-out's validated IP) permits that one private target — both the
+  // initial URL and any hop that stays on it — while everything else private stays blocked.
+  assertGuardedRedirectTarget(currentUrl, options?.allowRedirectToIp)
  let method = (init.method ?? 'GET').toUpperCase()
  let body = init.body
  let headers = init.headers
@@ -587,7 +599,13 @@ 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
+    if (![301, 302, 303, 307, 308].includes(status) || !location) {
+      // `response.url` is already the final hop's URL (set per-request by the raw fetch); flag
+      // `redirected` too when at least one hop was followed, matching fetch semantics.
+      if (hop > 0)
+        Object.defineProperty(response, 'redirected', { value: true, configurable: true })
+      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(() => {})
@@ -595,7 +613,7 @@ export async function followRedirectsGuarded(
      throw new Error(`Blocked by SSRF policy: more than ${MAX_GUARDED_REDIRECTS} redirects`)
    }
    const nextUrl = new URL(location, currentUrl)
-    assertGuardedRedirectTarget(nextUrl)
+    assertGuardedRedirectTarget(nextUrl, options?.allowRedirectToIp)
    // 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).
@@ -781,7 +799,7 @@ function nodeReadableToWebStream(nodeStream: Readable): ReadableStream<Uint8Arra
 async function undiciRequestAsResponse(
  input: RequestInfo | URL,
  init: RequestInit,
-  dispatcher: Agent
+  dispatcher: Dispatcher
 ): Promise<Response> {
  let url: string
  let effectiveInit = init as UndiciRequestInit
@@ -880,6 +898,36 @@ async function undiciRequestAsResponse(
  }
 }

+/**
+ * Normalizes a `fetch(input, init)` call into a URL string + init. A `Request` input carries
+ * its own method/headers/body/signal; lift them into the init (explicit init fields win, per
+ * fetch semantics) so a manual redirect follower can't silently downgrade a POST Request to a
+ * bare GET or lose its headers.
+ */
+async function liftFetchArgs(
+  input: RequestInfo | URL,
+  init?: RequestInit
+): Promise<{ target: string; effectiveInit: RequestInit }> {
+  const target = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url
+  if (typeof Request !== 'undefined' && input instanceof Request) {
+    const bodyAllowed = input.method !== 'GET' && input.method !== 'HEAD'
+    return {
+      target,
+      effectiveInit: {
+        method: input.method,
+        headers: input.headers,
+        body: bodyAllowed ? await input.clone().arrayBuffer() : undefined,
+        signal: input.signal,
+        // Carry the Request's redirect mode so the pinned fetch honors `manual`/`error`
+        // instead of defaulting a `Request({ redirect: 'manual' })` to `follow`.
+        redirect: input.redirect,
+        ...init,
+      },
+    }
+  }
+  return { target, effectiveInit: init ?? {} }
+}
+
 /**
 * SSRF-guarded `fetch` + its `Agent` for outbound requests to user-controlled
 * hosts: DNS resolves normally, and every socket connect validates the chosen
@@ -903,21 +951,7 @@ export function createSsrfGuardedFetchWithDispatcher(options?: { maxResponseSize
    undiciRequestAsResponse(url, init as unknown as RequestInit, dispatcher)

  const guarded = async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => {
-    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,
-      }
-    }
+    const { target, effectiveInit } = await liftFetchArgs(input, 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)
  }
@@ -977,14 +1011,42 @@ export function createPinnedFetchWithDispatcher(
    ...(options?.maxResponseSize !== undefined ? { maxResponseSize: options.maxResponseSize } : {}),
  })

+  const rawFetch = (url: string, init: UndiciRequestInit): Promise<Response> =>
+    // double-cast-allowed: DOM RequestInit and undici RequestInit differ in TS but match at runtime
+    undiciRequestAsResponse(url, init as unknown as RequestInit, dispatcher)
+
+  // Requests go through `undici.request` (not `undici.fetch`) because fetch's streaming
+  // `response.body` never delivers under the Bun runtime the server runs on — the same bug
+  // {@link createSsrfGuardedFetchWithDispatcher} works around. Redirects are handled here (not
+  // by a caller's wrapper — the pinned fetch is passed straight to provider/A2A SDKs), honoring
+  // the request's `redirect` mode: `manual`/`error` must NOT transparently follow (e.g.
+  // `detectMcpAuthType` inspects the 3xx to classify auth). The default `follow` uses
+  // {@link followRedirectsGuarded}, which drops headers on cross-origin hops (so a redirect
+  // can't disclose a provider `api-key` to another origin) and stamps the final `response.url`.
+  // Every hop still dispatches through the pinned `Agent` (its `connect.lookup` forces
+  // `resolvedIP`), so a redirect can't escape to another address.
  const pinned = async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => {
-    // double-cast-allowed: DOM RequestInfo/URL and undici fetch input types differ but are structurally compatible at runtime (Node's global fetch IS undici)
-    const undiciInput = input as unknown as Parameters<typeof undiciFetch>[0]
+    const { target, effectiveInit } = await liftFetchArgs(input, init)
+    const mode = effectiveInit.redirect ?? 'follow'
    // double-cast-allowed: DOM RequestInit and undici RequestInit are structurally compatible at runtime but the TS types differ
-    const undiciInit: UndiciRequestInit = { ...(init as unknown as UndiciRequestInit), dispatcher }
-    const response = await undiciFetch(undiciInput, undiciInit)
-    // double-cast-allowed: undici Response and DOM Response are structurally compatible at runtime
-    return response as unknown as Response
+    const undiciInit = effectiveInit as unknown as UndiciRequestInit
+    if (mode === 'manual') {
+      return rawFetch(target, undiciInit)
+    }
+    if (mode === 'error') {
+      const response = await rawFetch(target, undiciInit)
+      const location = response.headers.get('location')
+      if (response.status >= 300 && response.status < 400 && location) {
+        await response.body?.cancel().catch(() => {})
+        throw new TypeError('Pinned fetch received an unexpected redirect (redirect: "error")')
+      }
+      return response
+    }
+    // Permit this pinned IP as a redirect/initial target even when it's private (the
+    // self-hosted MCP carve-out on a private/loopback IP, and same-host redirects that stay on
+    // it) — otherwise the guarded policy would block a self-hosted server reaching itself. Any
+    // OTHER private target (e.g. a redirect to the cloud metadata IP) is still blocked.
+    return followRedirectsGuarded(rawFetch, target, undiciInit, { allowRedirectToIp: resolvedIP })
  }

  return { fetch: pinned, dispatcher }
@@ -1,39 +1,33 @@
 /**
 * @vitest-environment node
 */
+import { Readable } from 'node:stream'
 import { beforeEach, describe, expect, it, vi } from 'vitest'

-const { mockAgent, mockUndiciFetch, capturedAgentOptions, agentCloses } = vi.hoisted(() => {
+const { mockAgent, mockUndiciRequest, capturedAgentOptions } = vi.hoisted(() => {
  const capturedAgentOptions: unknown[] = []
-  const agentCloses: unknown[] = []
  class MockAgent {
    constructor(options: unknown) {
      capturedAgentOptions.push(options)
    }
    close() {
-      agentCloses.push(this)
+      return Promise.resolve()
+    }
+    destroy() {
      return Promise.resolve()
    }
  }
  return {
    mockAgent: MockAgent,
-    mockUndiciFetch: vi.fn(),
+    mockUndiciRequest: vi.fn(),
    capturedAgentOptions,
-    agentCloses,
  }
 })

-vi.mock('undici', () => ({ Agent: mockAgent, fetch: mockUndiciFetch }))
-/**
- * Query-suffixed import gives this file a private instance of the module under
- * test. Under `isolate: false` the worker's module graph is shared across test
- * files, so the plain specifier may already be cached with the real `undici`
- * binding (mocks never reach an already-evaluated module) — and evaluating it
- * here under this file's mocks would poison it for later files. The suffixed id
- * is unique to this file, so it always evaluates fresh with the mocks above.
- */
+vi.mock('undici', () => ({ Agent: mockAgent, request: mockUndiciRequest }))
+
 declare module '@/lib/core/security/input-validation.server?pinned-fetch-test' {
-  // biome-ignore lint/suspicious/noExportsInTest: ambient type re-declaration for the query-suffixed specifier, not a runtime export
+  // biome-ignore lint/suspicious/noExportsInTest: ambient re-declaration for the query-suffixed specifier
  export * from '@/lib/core/security/input-validation.server'
 }

@@ -42,12 +36,22 @@ import { createPinnedFetch } from '@/lib/core/security/input-validation.server?p
 type LookupCallback = (err: Error | null, address: string, family: number) => void
 type PinnedLookup = (hostname: string, options: { all?: boolean }, callback: LookupCallback) => void

+function byteStream(text: string): Readable {
+  const stream = new Readable({ read() {} })
+  stream.push(Buffer.from(text))
+  stream.push(null)
+  return stream
+}
+
+function undiciReply(statusCode: number, headers: Record<string, string>, body: Readable) {
+  return { statusCode, headers, body, trailers: {}, opaque: null, context: {} }
+}
+
 describe('createPinnedFetch', () => {
  beforeEach(() => {
    vi.clearAllMocks()
    capturedAgentOptions.length = 0
-    agentCloses.length = 0
-    mockUndiciFetch.mockResolvedValue(new Response('ok'))
+    mockUndiciRequest.mockResolvedValue(undiciReply(200, {}, byteStream('ok')))
  })

  it('builds an undici Agent whose pinned lookup always resolves to the validated IP', async () => {
@@ -86,7 +90,7 @@ describe('createPinnedFetch', () => {
    expect(resolved).toEqual({ address: '2606:4700:4700::1111', family: 6 })
  })

-  it('forwards the pinned dispatcher on every call while preserving init options', async () => {
+  it('dispatches through the pinned Agent, preserving init', async () => {
    const pinned = createPinnedFetch('203.0.113.10')
    const controller = new AbortController()

@@ -97,32 +101,117 @@ describe('createPinnedFetch', () => {
      signal: controller.signal,
    })

-    expect(mockUndiciFetch).toHaveBeenCalledTimes(1)
-    const [url, init] = mockUndiciFetch.mock.calls[0]
+    expect(mockUndiciRequest).toHaveBeenCalledTimes(1)
+    const [url, options] = mockUndiciRequest.mock.calls[0]
    expect(url).toBe('https://myresource.openai.azure.com/openai/v1/responses')
-    const typedInit = init as RequestInit & { dispatcher?: unknown }
-    expect(typedInit.dispatcher).toBeInstanceOf(mockAgent)
-    expect(typedInit.method).toBe('POST')
-    expect(typedInit.headers).toEqual({ 'api-key': 'secret' })
-    expect(typedInit.body).toBe('{}')
-    expect(typedInit.signal).toBe(controller.signal)
+    expect(options.dispatcher).toBeInstanceOf(mockAgent)
+    expect(options.method).toBe('POST')
+    expect(options.headers).toEqual({ 'api-key': 'secret' })
+    expect(options.body).toBe('{}')
+    expect(options.signal).toBe(controller.signal)
  })

-  it('handles an undefined init by still attaching the dispatcher', async () => {
+  it('honors redirect: "manual" — returns the 3xx without following (auth-type probe)', async () => {
+    mockUndiciRequest.mockResolvedValueOnce(
+      undiciReply(302, { location: 'https://login.example.com/' }, byteStream(''))
+    )
    const pinned = createPinnedFetch('203.0.113.10')
-    await pinned('https://example.com')
-    const init = mockUndiciFetch.mock.calls[0][1] as { dispatcher?: unknown }
-    expect(init.dispatcher).toBeInstanceOf(mockAgent)
+
+    const response = await pinned('https://mcp.example.com/', { redirect: 'manual' })
+
+    expect(mockUndiciRequest).toHaveBeenCalledTimes(1)
+    expect(response.status).toBe(302)
+    expect(response.headers.get('location')).toBe('https://login.example.com/')
  })

-  it('reuses one captured dispatcher across all calls of a single instance', async () => {
+  it('honors redirect mode carried on a Request input (not just init)', async () => {
+    mockUndiciRequest.mockResolvedValueOnce(
+      undiciReply(302, { location: 'https://login.example.com/' }, byteStream(''))
+    )
+    const pinned = createPinnedFetch('203.0.113.10')
+
+    const response = await pinned(new Request('https://mcp.example.com/', { redirect: 'manual' }))
+
+    expect(mockUndiciRequest).toHaveBeenCalledTimes(1)
+    expect(response.status).toBe(302)
+  })
+
+  it('follows redirects by default and DROPS headers on a cross-origin hop (no api-key leak)', async () => {
+    mockUndiciRequest
+      .mockResolvedValueOnce(
+        undiciReply(307, { location: 'https://other-origin.example/final' }, byteStream(''))
+      )
+      .mockResolvedValueOnce(undiciReply(200, {}, byteStream('done')))
+    const pinned = createPinnedFetch('203.0.113.10')
+
+    const response = await pinned('https://azure.example.com/v1/responses', {
+      method: 'GET',
+      headers: { 'api-key': 'secret' },
+    })
+
+    expect(mockUndiciRequest).toHaveBeenCalledTimes(2)
+    // Second (cross-origin) hop must not carry the provider credential — no headers forwarded.
+    const secondHopHeaders = (mockUndiciRequest.mock.calls[1][1].headers ?? {}) as Record<
+      string,
+      string
+    >
+    expect(secondHopHeaders['api-key']).toBeUndefined()
+    expect(Object.keys(secondHopHeaders)).toHaveLength(0)
+    expect(response.status).toBe(200)
+    expect(response.url).toBe('https://other-origin.example/final')
+    expect(response.redirected).toBe(true)
+    expect(await response.text()).toBe('done')
+  })
+
+  it('does NOT block a private IP-literal URL (self-hosted-private MCP carve-out)', async () => {
+    mockUndiciRequest.mockResolvedValueOnce(undiciReply(200, {}, byteStream('mcp')))
+    const pinned = createPinnedFetch('10.0.0.5')
+
+    // A self-hosted MCP configured with a private IP-literal URL must still connect — the old
+    // undici.fetch path never ran the SSRF initial-target check that would otherwise block it.
+    const response = await pinned('http://10.0.0.5:3000/mcp', { method: 'POST', body: '{}' })
+
+    expect(mockUndiciRequest).toHaveBeenCalledTimes(1)
+    expect(response.status).toBe(200)
+    expect(await response.text()).toBe('mcp')
+  })
+
+  it('follows a redirect that stays on the pinned private IP (self-hosted MCP alias)', async () => {
+    mockUndiciRequest
+      .mockResolvedValueOnce(
+        undiciReply(301, { location: 'http://10.0.0.5:3000/mcp/' }, byteStream(''))
+      )
+      .mockResolvedValueOnce(undiciReply(200, {}, byteStream('mcp')))
+    const pinned = createPinnedFetch('10.0.0.5')
+
+    const response = await pinned('http://10.0.0.5:3000/mcp', { method: 'GET' })
+
+    expect(mockUndiciRequest).toHaveBeenCalledTimes(2)
+    expect(response.status).toBe(200)
+    expect(await response.text()).toBe('mcp')
+  })
+
+  it('STILL blocks a redirect to a different private IP (no metadata-IP escape)', async () => {
+    mockUndiciRequest.mockResolvedValueOnce(
+      undiciReply(302, { location: 'http://169.254.169.254/latest/meta-data/' }, byteStream(''))
+    )
+    const pinned = createPinnedFetch('10.0.0.5')
+
+    await expect(pinned('http://10.0.0.5:3000/mcp', { method: 'GET' })).rejects.toThrow(
+      /private or reserved/
+    )
+    // The initial request happened; the redirect to the metadata IP was refused.
+    expect(mockUndiciRequest).toHaveBeenCalledTimes(1)
+  })
+
+  it('reuses one dispatcher across all calls of a single instance', async () => {
    const pinned = createPinnedFetch('203.0.113.10')
    await pinned('https://example.com/a')
    await pinned('https://example.com/b')

    expect(capturedAgentOptions).toHaveLength(1)
-    const d1 = (mockUndiciFetch.mock.calls[0][1] as { dispatcher: unknown }).dispatcher
-    const d2 = (mockUndiciFetch.mock.calls[1][1] as { dispatcher: unknown }).dispatcher
+    const d1 = (mockUndiciRequest.mock.calls[0][1] as { dispatcher: unknown }).dispatcher
+    const d2 = (mockUndiciRequest.mock.calls[1][1] as { dispatcher: unknown }).dispatcher
    expect(d1).toBe(d2)
  })

@@ -133,13 +222,13 @@ describe('createPinnedFetch', () => {
    await b('https://example.com/b')

    expect(capturedAgentOptions).toHaveLength(2)
-    const d1 = (mockUndiciFetch.mock.calls[0][1] as { dispatcher: unknown }).dispatcher
-    const d2 = (mockUndiciFetch.mock.calls[1][1] as { dispatcher: unknown }).dispatcher
+    const d1 = (mockUndiciRequest.mock.calls[0][1] as { dispatcher: unknown }).dispatcher
+    const d2 = (mockUndiciRequest.mock.calls[1][1] as { dispatcher: unknown }).dispatcher
    expect(d1).not.toBe(d2)
  })

-  it('returns the response produced by undici fetch', async () => {
-    mockUndiciFetch.mockResolvedValueOnce(new Response('pong', { status: 201 }))
+  it('returns a streaming Response built from the undici.request body', async () => {
+    mockUndiciRequest.mockResolvedValueOnce(undiciReply(201, {}, byteStream('pong')))
    const pinned = createPinnedFetch('203.0.113.10')
    const response = await pinned('https://example.com')
    expect(response.status).toBe(201)