Skip to content

Commit 3458220

Browse files
authored
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
1 parent e9898ea commit 3458220

7 files changed

Lines changed: 521 additions & 54 deletions

File tree

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

Lines changed: 172 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,8 @@ export async function validateUrlWithDNS(
106106

107107
try {
108108
// Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs
109-
// on IPv4-only egress (e.g. AWS NAT gateways). See validateMcpServerSsrf.
109+
// on IPv4-only egress (e.g. AWS NAT gateways) — still-pinned consumers (providers, SSO,
110+
// A2A) depend on this ordering.
110111
const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true })
111112
const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0]
112113

@@ -422,6 +423,176 @@ export function createPinnedLookup(resolvedIP: string): LookupFunction {
422423
}
423424
}
424425

426+
/**
427+
* DNS lookup that resolves normally and validates EVERY resolved address against
428+
* the SSRF policy at socket-connect time (the LibreChat `getSSRFConnect` pattern).
429+
* Private/reserved/loopback records are filtered out; if nothing publicly routable
430+
* remains the connect fails. Because the check runs on each dial — including
431+
* redirects and reconnects — there is no validated-then-trusted window for a DNS
432+
* rebind to slip through, and unlike single-IP pinning the connector keeps the
433+
* full public address set, so the OS/undici can fall back across addresses.
434+
* IPv4 is ordered first (`verbatim: false`) — our egress is IPv4-only.
435+
*/
436+
export function createSsrfGuardedLookup(): LookupFunction {
437+
return (hostname, options, callback) => {
438+
dns
439+
.lookup(hostname, { all: true, verbatim: false })
440+
.then((addresses) => {
441+
const usable = addresses.filter((entry) => !isPrivateOrReservedIP(entry.address))
442+
if (usable.length === 0) {
443+
callback(
444+
new Error(`Blocked by SSRF policy: ${hostname} has no publicly routable address`),
445+
'',
446+
4
447+
)
448+
return
449+
}
450+
if (options.all) callback(null, usable)
451+
else callback(null, usable[0].address, usable[0].family)
452+
})
453+
.catch((error) => callback(toError(error), '', 4))
454+
}
455+
}
456+
457+
const MAX_GUARDED_REDIRECTS = 5
458+
459+
/**
460+
* Rejects a redirect hop whose target is a private/reserved IP LITERAL. Node's
461+
* `net.connect` bypasses the custom `lookup` for numeric hosts (`isIP(host)`
462+
* short-circuits), so the connect-time guard never sees IP-literal dials —
463+
* a 3xx to `http://169.254.169.254/` would otherwise connect directly. Hostname
464+
* targets are covered by {@link createSsrfGuardedLookup} at connect time.
465+
*/
466+
function assertGuardedRedirectTarget(url: URL): void {
467+
if (url.protocol !== 'http:' && url.protocol !== 'https:') {
468+
throw new Error(`Blocked by SSRF policy: redirect to unsupported protocol ${url.protocol}`)
469+
}
470+
const host =
471+
url.hostname.startsWith('[') && url.hostname.endsWith(']')
472+
? url.hostname.slice(1, -1)
473+
: url.hostname
474+
if (ipaddr.isValid(host) && isPrivateOrReservedIP(host)) {
475+
throw new Error('Blocked by SSRF policy: redirect to a private or reserved address')
476+
}
477+
}
478+
479+
/**
480+
* Manual, revalidating redirect follower used by the guarded fetch. Auto-follow
481+
* is unsafe here on two counts the connect-time lookup cannot cover: IP-literal
482+
* redirect targets bypass the lookup entirely (validated per hop instead), and
483+
* undici retains CUSTOM request headers across cross-origin redirects (it strips
484+
* only Authorization/Cookie) — so caller headers are dropped on any cross-origin
485+
* hop. Exported for tests.
486+
*/
487+
export async function followRedirectsGuarded(
488+
rawFetch: (url: string, init: UndiciRequestInit) => Promise<Response>,
489+
input: string,
490+
init: UndiciRequestInit
491+
): Promise<Response> {
492+
let currentUrl = new URL(input)
493+
// The initial URL gets the same IP-literal check as redirect hops, so the exported
494+
// guard is self-contained even when a caller skips its own up-front validation.
495+
assertGuardedRedirectTarget(currentUrl)
496+
let method = (init.method ?? 'GET').toUpperCase()
497+
let body = init.body
498+
let headers = init.headers
499+
for (let hop = 0; ; hop++) {
500+
const response = await rawFetch(currentUrl.href, {
501+
...init,
502+
method,
503+
body,
504+
headers,
505+
redirect: 'manual',
506+
})
507+
const status = response.status
508+
const location = response.headers.get('location')
509+
if (![301, 302, 303, 307, 308].includes(status) || !location) return response
510+
// Cancel the redirect body up front so the throw paths below (hop cap, blocked
511+
// target) can't leave a socket checked out on the long-lived Agent.
512+
await response.body?.cancel().catch(() => {})
513+
if (hop >= MAX_GUARDED_REDIRECTS) {
514+
throw new Error(`Blocked by SSRF policy: more than ${MAX_GUARDED_REDIRECTS} redirects`)
515+
}
516+
const nextUrl = new URL(location, currentUrl)
517+
assertGuardedRedirectTarget(nextUrl)
518+
// Per the fetch spec: 303 (and 301/302 on POST) switch to a bodyless GET, dropping
519+
// the entity headers that described the removed body (a retained Content-Length /
520+
// Content-Type on a bodyless GET is malformed and undici rejects it).
521+
if (status === 303 || ((status === 301 || status === 302) && method === 'POST')) {
522+
method = 'GET'
523+
body = undefined
524+
if (headers !== undefined) {
525+
const sanitized = new Headers(headers as HeadersInit)
526+
sanitized.delete('content-length')
527+
sanitized.delete('content-type')
528+
sanitized.delete('content-encoding')
529+
sanitized.delete('transfer-encoding')
530+
// double-cast-allowed: Headers is a valid undici HeadersInit at runtime but the DOM/undici types differ
531+
headers = sanitized as unknown as UndiciRequestInit['headers']
532+
}
533+
}
534+
if (nextUrl.origin !== currentUrl.origin) {
535+
headers = undefined
536+
// 307/308 preserve method+body; forwarding a body cross-origin can hand OAuth
537+
// client secrets / tokens to an open-redirect target now that redirects really
538+
// dial the new origin. No legitimate MCP/OAuth flow does this — refuse it.
539+
if (body !== undefined && body !== null) {
540+
throw new Error(
541+
'Blocked by SSRF policy: cross-origin redirect would forward a request body'
542+
)
543+
}
544+
}
545+
currentUrl = nextUrl
546+
}
547+
}
548+
549+
/**
550+
* SSRF-guarded `fetch` + its `Agent` for outbound requests to user-controlled
551+
* hosts: DNS resolves normally, and every socket connect validates the chosen
552+
* addresses via {@link createSsrfGuardedLookup}; redirects are followed manually
553+
* with per-hop validation (see {@link followRedirectsGuarded}) so IP-literal
554+
* targets can't bypass the lookup and custom headers never cross origins. See
555+
* {@link createPinnedFetchWithDispatcher} for the `maxResponseSize` semantics.
556+
*/
557+
export function createSsrfGuardedFetchWithDispatcher(options?: { maxResponseSize?: number }): {
558+
fetch: typeof fetch
559+
dispatcher: Agent
560+
} {
561+
const dispatcher = new Agent({
562+
allowH2: false,
563+
connect: { lookup: createSsrfGuardedLookup() },
564+
...(options?.maxResponseSize !== undefined ? { maxResponseSize: options.maxResponseSize } : {}),
565+
})
566+
567+
const rawFetch = async (url: string, init: UndiciRequestInit): Promise<Response> => {
568+
const response = await undiciFetch(url, { ...init, dispatcher })
569+
// double-cast-allowed: undici Response and DOM Response are structurally compatible at runtime
570+
return response as unknown as Response
571+
}
572+
573+
const guarded = async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => {
574+
const target = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url
575+
// A Request input carries its own method/headers/body/signal; lift them into the
576+
// init (explicit init fields win, per fetch semantics) so the manual redirect
577+
// follower doesn't silently downgrade a guarded POST Request to a bare GET.
578+
let effectiveInit: RequestInit = init ?? {}
579+
if (typeof Request !== 'undefined' && input instanceof Request) {
580+
const bodyAllowed = input.method !== 'GET' && input.method !== 'HEAD'
581+
effectiveInit = {
582+
method: input.method,
583+
headers: input.headers,
584+
body: bodyAllowed ? await input.clone().arrayBuffer() : undefined,
585+
signal: input.signal,
586+
...init,
587+
}
588+
}
589+
// double-cast-allowed: DOM RequestInit and undici RequestInit are structurally compatible at runtime but the TS types differ
590+
return followRedirectsGuarded(rawFetch, target, effectiveInit as unknown as UndiciRequestInit)
591+
}
592+
593+
return { fetch: guarded, dispatcher }
594+
}
595+
425596
/**
426597
* Builds a standard `fetch`-compatible function that pins every outbound
427598
* connection to `resolvedIP`, preventing DNS-rebinding (TOCTOU) between URL

0 commit comments

Comments
 (0)