fix(mcp): replace single-IP pinning with validate-at-connect SSRF guard (#5823)

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

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

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

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

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

* chore(mcp): true up stale pinned-era doc comments
This commit is contained in:
Waleed
2026-07-21 17:15:04 -07:00
committed by GitHub
parent e9898ea24d
commit 3458220ec9
7 changed files with 521 additions and 54 deletions
@@ -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]
@@ -422,6 +423,176 @@ 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)
// 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
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
// 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)
// 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')
// double-cast-allowed: Headers is a valid undici HeadersInit at runtime but the DOM/undici types differ
headers = sanitized as unknown as UndiciRequestInit['headers']
}
}
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
}
}
/**
* 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
// 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, effectiveInit 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
@@ -0,0 +1,232 @@
/**
* @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')
})
})
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')
})
})
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)
})
})
+4 -4
View File
@@ -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 })),
}))
/**
@@ -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()
+18 -8
View File
@@ -11,9 +11,10 @@ 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 { createPinnedMcpFetch } from '@/lib/mcp/pinned-fetch'
import { createGuardedMcpFetch, createPinnedPrivateMcpFetch } from '@/lib/mcp/pinned-fetch'
import {
type McpClientOptions,
McpConnectionError,
@@ -73,7 +74,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 +97,21 @@ 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.
// A private/loopback resolvedIP only reaches here on self-hosted (where the policy
// 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,
requestInit: { headers: this.config.headers },
...(pinned ? { fetch: pinned.fetch } : {}),
...(guarded ? { fetch: guarded.fetch } : {}),
})
this.client = new Client(
@@ -228,16 +238,16 @@ 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
* 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) {
+3 -1
View File
@@ -31,7 +31,9 @@ const {
}))
vi.mock('@/lib/core/security/input-validation.server', () => ({
createPinnedFetchWithDispatcher: vi.fn(() => ({
isPrivateOrReservedIP: (ip: string) =>
ip.startsWith('127.') || ip.startsWith('10.') || ip === '::1',
createSsrfGuardedFetchWithDispatcher: vi.fn(() => ({
fetch: mockUndiciFetch,
dispatcher: { destroy: vi.fn(() => Promise.resolve()) },
})),
+45 -28
View File
@@ -5,11 +5,13 @@ import { sleep } from '@sim/utils/helpers'
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(),
@@ -17,36 +19,36 @@ const {
}))
vi.mock('@/lib/core/security/input-validation.server', () => ({
createSsrfGuardedFetchWithDispatcher: mockCreateGuardedFetchWithDispatcher,
createPinnedFetchWithDispatcher: mockCreatePinnedFetchWithDispatcher,
isPrivateOrReservedIP: (ip: string) =>
ip.startsWith('127.') || ip.startsWith('10.') || ip === '::1',
}))
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 +60,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 +257,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 +273,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 +287,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 +319,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 +329,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 +341,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()
@@ -354,3 +350,24 @@ describe('createSsrfGuardedMcpFetch', () => {
}
})
})
describe('self-hosted private-resolution carve-out', () => {
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')
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()
})
})
+47 -12
View File
@@ -1,31 +1,55 @@
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 {
createPinnedFetchWithDispatcher,
createSsrfGuardedFetchWithDispatcher,
isPrivateOrReservedIP,
} 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<void>
}
/**
* 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 {
/**
* 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
// 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 +68,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 +204,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 +230,25 @@ 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) {
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,
})
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