Skip to content

Commit a518963

Browse files
committed
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.
1 parent f6bd96b commit a518963

2 files changed

Lines changed: 59 additions & 3 deletions

File tree

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

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,10 @@ export async function validateUrlWithDNS(
105105
}
106106

107107
try {
108-
const { address } = await dns.lookup(cleanHostname, { verbatim: true })
108+
// 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.
110+
const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true })
111+
const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0]
109112

110113
const resolvedIsLoopback =
111114
ipaddr.isValid(address) &&
@@ -486,6 +489,9 @@ export async function followRedirectsGuarded(
486489
init: UndiciRequestInit
487490
): Promise<Response> {
488491
let currentUrl = new URL(input)
492+
// The initial URL gets the same IP-literal check as redirect hops, so the exported
493+
// guard is self-contained even when a caller skips its own up-front validation.
494+
assertGuardedRedirectTarget(currentUrl)
489495
let method = (init.method ?? 'GET').toUpperCase()
490496
let body = init.body
491497
let headers = init.headers
@@ -506,10 +512,20 @@ export async function followRedirectsGuarded(
506512
const nextUrl = new URL(location, currentUrl)
507513
assertGuardedRedirectTarget(nextUrl)
508514
await response.body?.cancel().catch(() => {})
509-
// Per the fetch spec: 303 (and 301/302 on POST) switch to a bodyless GET.
515+
// Per the fetch spec: 303 (and 301/302 on POST) switch to a bodyless GET, dropping
516+
// the entity headers that described the removed body (a retained Content-Length /
517+
// Content-Type on a bodyless GET is malformed and undici rejects it).
510518
if (status === 303 || ((status === 301 || status === 302) && method === 'POST')) {
511519
method = 'GET'
512520
body = undefined
521+
if (headers !== undefined) {
522+
const sanitized = new Headers(headers as HeadersInit)
523+
sanitized.delete('content-length')
524+
sanitized.delete('content-type')
525+
sanitized.delete('content-encoding')
526+
sanitized.delete('transfer-encoding')
527+
headers = sanitized as unknown as UndiciRequestInit['headers']
528+
}
513529
}
514530
if (nextUrl.origin !== currentUrl.origin) headers = undefined
515531
currentUrl = nextUrl
@@ -542,8 +558,22 @@ export function createSsrfGuardedFetchWithDispatcher(options?: { maxResponseSize
542558

543559
const guarded = async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => {
544560
const target = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url
561+
// A Request input carries its own method/headers/body/signal; lift them into the
562+
// init (explicit init fields win, per fetch semantics) so the manual redirect
563+
// follower doesn't silently downgrade a guarded POST Request to a bare GET.
564+
let effectiveInit: RequestInit = init ?? {}
565+
if (typeof Request !== 'undefined' && input instanceof Request) {
566+
const bodyAllowed = input.method !== 'GET' && input.method !== 'HEAD'
567+
effectiveInit = {
568+
method: input.method,
569+
headers: input.headers,
570+
body: bodyAllowed ? await input.clone().arrayBuffer() : undefined,
571+
signal: input.signal,
572+
...init,
573+
}
574+
}
545575
// double-cast-allowed: DOM RequestInit and undici RequestInit are structurally compatible at runtime but the TS types differ
546-
return followRedirectsGuarded(rawFetch, target, (init ?? {}) as unknown as UndiciRequestInit)
576+
return followRedirectsGuarded(rawFetch, target, effectiveInit as unknown as UndiciRequestInit)
547577
}
548578

549579
return { fetch: guarded, dispatcher }

apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,3 +183,29 @@ describe('followRedirectsGuarded', () => {
183183
expect(raw.mock.calls[1][1].body).toBe('data')
184184
})
185185
})
186+
187+
describe('followRedirectsGuarded — hardening', () => {
188+
it('blocks a private IP-literal as the INITIAL url (guard is self-contained)', async () => {
189+
const raw = vi.fn(async () => new Response('ok'))
190+
await expect(
191+
followRedirectsGuarded(raw, 'http://169.254.169.254/latest/meta-data/', {})
192+
).rejects.toThrow(/Blocked by SSRF policy/)
193+
expect(raw).not.toHaveBeenCalled()
194+
})
195+
196+
it('drops entity headers when a 303 switches POST to a bodyless GET', async () => {
197+
const raw = vi
198+
.fn()
199+
.mockResolvedValueOnce(redirectTo('https://a.example/next', 303))
200+
.mockResolvedValueOnce(new Response('ok', { status: 200 }))
201+
await followRedirectsGuarded(raw, 'https://a.example/x', {
202+
method: 'POST',
203+
body: '{"a":1}',
204+
headers: { 'content-type': 'application/json', 'content-length': '7', 'x-keep': 'yes' },
205+
})
206+
const hopHeaders = new Headers(raw.mock.calls[1][1].headers)
207+
expect(hopHeaders.get('content-type')).toBeNull()
208+
expect(hopHeaders.get('content-length')).toBeNull()
209+
expect(hopHeaders.get('x-keep')).toBe('yes')
210+
})
211+
})

0 commit comments

Comments
 (0)