From a97074e749db9703d4fac71fa55d4924f30fc284 Mon Sep 17 00:00:00 2001 From: terpstra-209 <261108509+terpstra-209@users.noreply.github.com> Date: Sat, 15 Aug 2026 22:32:20 -0700 Subject: [PATCH] fix(mcp): follow redirects with per-hop SSRF re-validation guardedFetch set redirect: "manual" and returned the 3xx as-is, so any MCP server whose OAuth discovery endpoints redirect was unusable. The MCP SDK continues its well-known URL search only on 4xx and treats anything else as fatal, so a single 302 aborted discovery even when a later candidate URL would have returned valid metadata. Follow redirects here rather than delegating to the runtime, so every hop is re-checked by validateRemoteMcpUrl (HTTPS, private-IP, DNS re-resolution). This is stricter than redirect: "follow", which would let a public URL bounce to an address the guard never saw. Bounded: GET/HEAD only (no POST body replay), 5 hops max, and the Authorization header is dropped when the origin changes. Co-Authored-By: Claude Opus 5 --- .../src/lib/mcp/__tests__/client.ssrf.test.ts | 118 ++++++++++++++++++ backend/src/lib/mcp/client.ts | 58 ++++++++- 2 files changed, 174 insertions(+), 2 deletions(-) diff --git a/backend/src/lib/mcp/__tests__/client.ssrf.test.ts b/backend/src/lib/mcp/__tests__/client.ssrf.test.ts index 454f274a89..b937f0f6a2 100644 --- a/backend/src/lib/mcp/__tests__/client.ssrf.test.ts +++ b/backend/src/lib/mcp/__tests__/client.ssrf.test.ts @@ -143,3 +143,121 @@ describe("guardedFetch", () => { expect(nextInit.dispatcher).toBe(init.dispatcher); }); }); + +describe("guardedFetch redirect following", () => { + function redirectTo(location: string, status = 302) { + return new Response(null, { status, headers: { location } }); + } + + it("follows a redirect and returns the final response", async () => { + resolvesTo("93.184.216.34"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValueOnce( + redirectTo("https://public.example.com/mcp/.well-known/x"), + ) + .mockResolvedValueOnce(new Response("{}", { status: 200 })); + + const res = await guardedFetch( + "https://public.example.com/.well-known/x/mcp", + ); + + expect(res.status).toBe(200); + expect(fetchSpy).toHaveBeenCalledTimes(2); + expect(fetchSpy.mock.calls[1][0]).toBe( + "https://public.example.com/mcp/.well-known/x", + ); + }); + + it("resolves a relative Location against the current URL", async () => { + resolvesTo("93.184.216.34"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValueOnce(redirectTo("/elsewhere", 307)) + .mockResolvedValueOnce(new Response("{}", { status: 200 })); + + await guardedFetch("https://public.example.com/a/b"); + + expect(fetchSpy.mock.calls[1][0]).toBe( + "https://public.example.com/elsewhere", + ); + }); + + it("re-validates each hop and refuses a redirect to a private address", async () => { + lookupMock.mockImplementation(async (hostname: string) => + hostname === "public.example.com" + ? [{ address: "93.184.216.34", family: 4 }] + : [{ address: "169.254.169.254", family: 4 }], + ); + vi.spyOn(globalThis, "fetch").mockResolvedValueOnce( + redirectTo("https://internal.example.com/latest/meta-data/"), + ); + + await expect( + guardedFetch("https://public.example.com/mcp"), + ).rejects.toThrow(/blocked network address/); + }); + + it("drops the Authorization header when the origin changes", async () => { + resolvesTo("93.184.216.34"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValueOnce(redirectTo("https://other.example.com/x")) + .mockResolvedValueOnce(new Response("{}", { status: 200 })); + + await guardedFetch("https://public.example.com/mcp", { + headers: { authorization: "Bearer secret", accept: "application/json" }, + }); + + const headers = new Headers( + (fetchSpy.mock.calls[1][1] as RequestInit).headers, + ); + expect(headers.get("authorization")).toBeNull(); + expect(headers.get("accept")).toBe("application/json"); + }); + + it("keeps the Authorization header on a same-origin redirect", async () => { + resolvesTo("93.184.216.34"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValueOnce(redirectTo("https://public.example.com/x")) + .mockResolvedValueOnce(new Response("{}", { status: 200 })); + + await guardedFetch("https://public.example.com/mcp", { + headers: { authorization: "Bearer secret" }, + }); + + const headers = new Headers( + (fetchSpy.mock.calls[1][1] as RequestInit).headers, + ); + expect(headers.get("authorization")).toBe("Bearer secret"); + }); + + it("does not follow redirects for non-GET requests", async () => { + resolvesTo("93.184.216.34"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(redirectTo("https://public.example.com/x")); + + const res = await guardedFetch("https://public.example.com/mcp", { + method: "POST", + body: "{}", + }); + + expect(res.status).toBe(302); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("stops after the redirect cap instead of looping forever", async () => { + resolvesTo("93.184.216.34"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(redirectTo("https://public.example.com/loop")); + + const res = await guardedFetch("https://public.example.com/loop"); + + expect(res.status).toBe(302); + // 1 initial + MAX_MCP_REDIRECTS follows. + expect(fetchSpy).toHaveBeenCalledTimes(6); + }); +}); diff --git a/backend/src/lib/mcp/client.ts b/backend/src/lib/mcp/client.ts index ed2a800b7a..a763bf093c 100644 --- a/backend/src/lib/mcp/client.ts +++ b/backend/src/lib/mcp/client.ts @@ -371,22 +371,76 @@ const guardedAgent = new Agent({ // connection to a connect-time-validated address, and refuses to auto-follow // redirects (`redirect: "manual"`) so a 3xx to an internal host cannot smuggle // egress past the guard. +// Redirects are followed here rather than by the runtime so that every hop is +// re-checked by validateRemoteMcpUrl — `redirect: "follow"` would let a public +// URL bounce us to a private address the guard never saw. Refusing outright is +// not an option either: RFC 8414 well-known discovery paths are commonly served +// as redirects, and the MCP SDK treats any non-4xx as fatal, so a single 302 +// aborts discovery even when a later candidate URL would have worked. +const MAX_MCP_REDIRECTS = 5; + export async function guardedFetch( input: Parameters[0], init?: Parameters[1], ) { - const url = + const isRequest = typeof input === "object" && input instanceof Request; + let url = typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; await validateRemoteMcpUrl(url); - return fetch(input, { + let response = await fetch(input, { ...init, redirect: "manual", dispatcher: guardedAgent, } as RequestInit); + + const method = ( + init?.method ?? + (isRequest ? input.method : null) ?? + "GET" + ).toUpperCase(); + // Only bodyless methods are followed. Replaying a POST body across a + // redirect is not something any MCP flow needs, and skipping it avoids + // having to reason about 307/308 body semantics. + if (method !== "GET" && method !== "HEAD") return response; + + const baseHeaders = new Headers( + (init?.headers as HeadersInit | undefined) ?? + (isRequest ? input.headers : undefined), + ); + + for (let hop = 0; hop < MAX_MCP_REDIRECTS; hop++) { + if (response.status < 300 || response.status > 399) return response; + const location = response.headers.get("location"); + if (!location) return response; + + let target: string; + try { + target = new URL(location, url).toString(); + } catch { + return response; + } + await response.body?.cancel().catch(() => undefined); + + const validated = await validateRemoteMcpUrl(target); + const headers = new Headers(baseHeaders); + // Never carry credentials to a different origin. + if (new URL(validated).origin !== new URL(url).origin) { + headers.delete("authorization"); + } + url = validated; + response = await fetch(validated, { + ...init, + method, + headers, + redirect: "manual", + dispatcher: guardedAgent, + } as RequestInit); + } + return response; } export function base64Url(buffer: Buffer) {