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.

Author: waleedlatif1

apps/sim/lib/core/security/input-validation.server.ts

@@ -14,7 +14,6 @@ import {
   Agent,
   type Dispatcher,
   type RequestInit as UndiciRequestInit,
-  interceptors as undiciInterceptors,
   request as undiciRequest,
 } from 'undici'
 import { isHosted, isPrivateDatabaseHostsAllowed } from '@/lib/core/config/env-flags'
@@ -587,7 +586,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(() => {})
@@ -880,6 +885,33 @@ 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,
+        ...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 +935,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,21 +995,40 @@ 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. Unlike the guarded builder, the
-  // pinned fetch is handed straight to provider/A2A SDKs with no `followRedirectsGuarded`
-  // wrapper, so redirects are followed here via undici's redirect interceptor. Every hop still
-  // dispatches through the pinned `Agent` (its `connect.lookup` forces `resolvedIP`), so a
-  // redirect can't escape to another address — matching the old fetch path's guarantee.
-  const redirecting = dispatcher.compose(
-    undiciInterceptors.redirect({ maxRedirections: DEFAULT_MAX_REDIRECTS })
-  )
-  const pinned = (input: RequestInfo | URL, init?: RequestInit): Promise<Response> =>
-    undiciRequestAsResponse(input, init ?? {}, redirecting)
-
-  // Return the base `Agent` (not the composed dispatcher) so callers `destroy()` the socket
-  // owner on close; the interceptor is stateless and re-dispatches through it.
+  // {@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> => {
+    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 = 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
+    }
+    return followRedirectsGuarded(rawFetch, target, undiciInit)
+  }
+
   return { fetch: pinned, dispatcher }
 }
 

apps/sim/lib/core/security/pinned-fetch.server.test.ts

@@ -4,49 +4,27 @@
 import { Readable } from 'node:stream'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
-const {
-  mockAgent,
-  mockUndiciRequest,
-  mockRedirectInterceptor,
-  capturedAgentOptions,
-  capturedRedirectOptions,
-} = vi.hoisted(() => {
+const { mockAgent, mockUndiciRequest, capturedAgentOptions } = vi.hoisted(() => {
   const capturedAgentOptions: unknown[] = []
-  const capturedRedirectOptions: unknown[] = []
   class MockAgent {
     constructor(options: unknown) {
       capturedAgentOptions.push(options)
     }
-    // The pinned builder follows redirects by composing this Agent with the redirect
-    // interceptor; the composed dispatcher is what reaches undici.request.
-    compose(interceptor: unknown) {
-      return { __composed: true, base: this, interceptor }
-    }
     close() {
       return Promise.resolve()
     }
     destroy() {
       return Promise.resolve()
     }
   }
-  const mockRedirectInterceptor = vi.fn((options: unknown) => {
-    capturedRedirectOptions.push(options)
-    return { __redirectInterceptor: true }
-  })
   return {
     mockAgent: MockAgent,
     mockUndiciRequest: vi.fn(),
-    mockRedirectInterceptor,
     capturedAgentOptions,
-    capturedRedirectOptions,
   }
 })
 
-vi.mock('undici', () => ({
-  Agent: mockAgent,
-  request: mockUndiciRequest,
-  interceptors: { redirect: mockRedirectInterceptor },
-}))
+vi.mock('undici', () => ({ Agent: mockAgent, request: mockUndiciRequest }))
 
 declare module '@/lib/core/security/input-validation.server?pinned-fetch-test' {
   // biome-ignore lint/suspicious/noExportsInTest: ambient re-declaration for the query-suffixed specifier
@@ -73,7 +51,6 @@ describe('createPinnedFetch', () => {
   beforeEach(() => {
     vi.clearAllMocks()
     capturedAgentOptions.length = 0
-    capturedRedirectOptions.length = 0
     mockUndiciRequest.mockResolvedValue(undiciReply(200, {}, byteStream('ok')))
   })
 
@@ -113,13 +90,7 @@ describe('createPinnedFetch', () => {
     expect(resolved).toEqual({ address: '2606:4700:4700::1111', family: 6 })
   })
 
-  it('follows redirects through the pinned Agent (interceptor composed with the app max)', () => {
-    createPinnedFetch('203.0.113.10')
-    expect(mockRedirectInterceptor).toHaveBeenCalledTimes(1)
-    expect(capturedRedirectOptions[0]).toEqual({ maxRedirections: 5 })
-  })
-
-  it('dispatches through the composed (redirect-following) dispatcher, preserving init', async () => {
+  it('dispatches through the pinned Agent, preserving init', async () => {
     const pinned = createPinnedFetch('203.0.113.10')
     const controller = new AbortController()
 
@@ -133,22 +104,54 @@ describe('createPinnedFetch', () => {
     expect(mockUndiciRequest).toHaveBeenCalledTimes(1)
     const [url, options] = mockUndiciRequest.mock.calls[0]
     expect(url).toBe('https://myresource.openai.azure.com/openai/v1/responses')
-    expect((options.dispatcher as { __composed?: boolean }).__composed).toBe(true)
-    expect((options.dispatcher as { base: unknown }).base).toBeInstanceOf(mockAgent)
+    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 dispatching through the pinned 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')
+
+    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('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')
-    await pinned('https://example.com')
-    const options = mockUndiciRequest.mock.calls[0][1] as { dispatcher: { __composed?: boolean } }
-    expect(options.dispatcher.__composed).toBe(true)
+
+    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('reuses one composed dispatcher across all calls of a single instance', async () => {
+  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')