From 1000a0625a3de5805287e83c51bd5e2ab4f89632 Mon Sep 17 00:00:00 2001 From: Amal Date: Fri, 17 Jul 2026 00:00:07 -0700 Subject: [PATCH 1/2] security: SSRF guardrails for server-side connector fetches Port the fork's SSRF hardening for MCP connector egress into the upstream layout: - Extract private/reserved IP classification into lib/privateIp.ts and fix IPv6 gaps: fe80::/10 link-local matching (the /^fe[89ab]:/ regex only matched the hextet "fe8:" and let fe80::1 through), hex-form IPv4-mapped addresses (::ffff:a00:1), NAT64 (64:ff9b::/96) and 6to4 (2002::/16) embedded IPv4 ranges. - Strip brackets from IPv6 literals in validateRemoteMcpUrl so [::1] et al. are classified by the private-IP guard instead of falling through to DNS lookup. - Route all OAuth egress (metadata fetch, discovery probes, dynamic client registration, token refresh) through guardedFetch so every outbound MCP request gets the same HTTPS-only / blocked-host / private-IP / no-redirect checks; the discovery probes were previously raw, unvalidated fetches. - Add SSRF regression tests (run atop the test-harness PR) and exclude test files from the tsc production build. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01CEguyEgXa9JjCciXCcVemC --- .../src/lib/mcp/__tests__/client.ssrf.test.ts | 124 ++++++++++++++++++ backend/src/lib/mcp/client.ts | 50 ++----- backend/src/lib/mcp/oauth.ts | 21 +-- backend/src/lib/privateIp.ts | 122 +++++++++++++++++ backend/tsconfig.json | 2 +- 5 files changed, 271 insertions(+), 48 deletions(-) create mode 100644 backend/src/lib/mcp/__tests__/client.ssrf.test.ts create mode 100644 backend/src/lib/privateIp.ts diff --git a/backend/src/lib/mcp/__tests__/client.ssrf.test.ts b/backend/src/lib/mcp/__tests__/client.ssrf.test.ts new file mode 100644 index 000000000..3bd58c325 --- /dev/null +++ b/backend/src/lib/mcp/__tests__/client.ssrf.test.ts @@ -0,0 +1,124 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +// Mock DNS resolution so the SSRF guard is exercised deterministically without +// touching the network. `lookupMock` is hoisted so the vi.mock factory can +// reference it. +const { lookupMock } = vi.hoisted(() => ({ lookupMock: vi.fn() })); +vi.mock("dns/promises", () => ({ + default: { lookup: lookupMock }, +})); + +import { guardedFetch, validateRemoteMcpUrl } from "../client"; + +function resolvesTo(...addresses: string[]) { + lookupMock.mockResolvedValue( + addresses.map((address) => ({ + address, + family: address.includes(":") ? 6 : 4, + })), + ); +} + +beforeEach(() => { + lookupMock.mockReset(); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("validateRemoteMcpUrl", () => { + it("rejects non-HTTPS URLs", async () => { + await expect(validateRemoteMcpUrl("http://example.com/")).rejects.toThrow( + /HTTPS/, + ); + }); + + it("rejects invalid URLs", async () => { + await expect(validateRemoteMcpUrl("not a url")).rejects.toThrow( + /valid URL/, + ); + }); + + it("rejects localhost and metadata hosts without a DNS lookup", async () => { + for (const host of [ + "https://localhost/", + "https://foo.localhost/", + "https://metadata.google.internal/", + "https://instance-data/", + ]) { + await expect(validateRemoteMcpUrl(host), host).rejects.toThrow( + /blocked host/, + ); + } + expect(lookupMock).not.toHaveBeenCalled(); + }); + + it("rejects private IPv4/IPv6 literals without a DNS lookup", async () => { + for (const host of [ + "https://127.0.0.1/", + "https://10.0.0.1/", + "https://169.254.169.254/", + "https://[::1]/", + "https://[fd00::1]/", + ]) { + await expect(validateRemoteMcpUrl(host), host).rejects.toThrow( + /blocked network address/, + ); + } + expect(lookupMock).not.toHaveBeenCalled(); + }); + + it("rejects a hostname that resolves to a private address", async () => { + resolvesTo("10.0.0.5"); + await expect( + validateRemoteMcpUrl("https://rebind.example.com/"), + ).rejects.toThrow(/blocked network address/); + }); + + it("rejects when ANY resolved address is private (mixed record set)", async () => { + resolvesTo("93.184.216.34", "192.168.1.1"); + await expect( + validateRemoteMcpUrl("https://mixed.example.com/"), + ).rejects.toThrow(/blocked network address/); + }); + + it("accepts a public host and strips credentials/hash", async () => { + resolvesTo("93.184.216.34"); + const out = await validateRemoteMcpUrl( + "https://user:secret@public.example.com/path?q=1#frag", + ); + expect(out).toBe("https://public.example.com/path?q=1"); + expect(out).not.toContain("secret"); + expect(out).not.toContain("frag"); + }); +}); + +describe("guardedFetch", () => { + it("throws and never calls fetch when the URL fails validation", async () => { + resolvesTo("10.0.0.5"); + const fetchSpy = vi.spyOn(globalThis, "fetch"); + await expect( + guardedFetch("https://rebind.example.com/"), + ).rejects.toThrow(/blocked network address/); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it("disables redirects for public hosts", async () => { + resolvesTo("93.184.216.34"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response("ok", { status: 200 })); + + const res = await guardedFetch("https://public.example.com/x", { + method: "GET", + }); + expect(res.status).toBe(200); + expect(fetchSpy).toHaveBeenCalledTimes(1); + + const init = fetchSpy.mock.calls[0][1] as RequestInit; + expect(init.redirect).toBe("manual"); + // Original request options are preserved. + expect(init.method).toBe("GET"); + }); +}); diff --git a/backend/src/lib/mcp/client.ts b/backend/src/lib/mcp/client.ts index 27b8cdf44..09523a8ee 100644 --- a/backend/src/lib/mcp/client.ts +++ b/backend/src/lib/mcp/client.ts @@ -1,6 +1,7 @@ import crypto from "crypto"; import dns from "dns/promises"; import net from "net"; +import { isBlockedIp } from "../privateIp"; import { BLOCKED_METADATA_HOSTS, HEADER_NAME_RE, @@ -223,41 +224,8 @@ export function toConnectorSummary( }; } -function isPrivateIpv4(ip: string) { - const parts = ip.split(".").map((part) => Number.parseInt(part, 10)); - if (parts.length !== 4 || parts.some((part) => !Number.isFinite(part))) { - return true; - } - const [a, b] = parts; - return ( - a === 0 || - a === 10 || - a === 127 || - (a === 100 && b >= 64 && b <= 127) || - (a === 169 && b === 254) || - (a === 172 && b >= 16 && b <= 31) || - (a === 192 && b === 168) || - (a === 192 && b === 0) || - (a === 198 && (b === 18 || b === 19)) || - a >= 224 - ); -} - -function isPrivateIpv6(ip: string) { - const normalized = ip.toLowerCase(); - if (normalized === "::1" || normalized === "::") return true; - if (normalized.startsWith("fc") || normalized.startsWith("fd")) return true; - if (/^fe[89ab]:/.test(normalized)) return true; - const ipv4Tail = normalized.match(/::ffff:(\d+\.\d+\.\d+\.\d+)$/); - return ipv4Tail ? isPrivateIpv4(ipv4Tail[1]) : false; -} - -function isBlockedIp(ip: string) { - const family = net.isIP(ip); - if (family === 4) return isPrivateIpv4(ip); - if (family === 6) return isPrivateIpv6(ip); - return true; -} +// Private/reserved IP classification lives in lib/privateIp.ts so every +// guarded egress check reuses the exact same ranges. export async function validateRemoteMcpUrl(rawUrl: string): Promise { let url: URL; @@ -282,9 +250,17 @@ export async function validateRemoteMcpUrl(rawUrl: string): Promise { throw new Error("MCP server URL points to a blocked host."); } - const literalFamily = net.isIP(hostname); + // URL.hostname wraps IPv6 literals in brackets ("[::1]"), which net.isIP + // does not recognize. Strip them so an IPv6 literal is classified by the + // private-IP guard rather than falling through to a DNS lookup that would + // treat the bracketed form as an (unresolvable) hostname. + const literalHost = + hostname.startsWith("[") && hostname.endsWith("]") + ? hostname.slice(1, -1) + : hostname; + const literalFamily = net.isIP(literalHost); const addresses = literalFamily - ? [{ address: hostname }] + ? [{ address: literalHost }] : await dns.lookup(hostname, { all: true, verbatim: true }); if (!addresses.length || addresses.some(({ address }) => isBlockedIp(address))) { throw new Error("MCP server URL resolves to a blocked network address."); diff --git a/backend/src/lib/mcp/oauth.ts b/backend/src/lib/mcp/oauth.ts index d03d597d7..8d007f440 100644 --- a/backend/src/lib/mcp/oauth.ts +++ b/backend/src/lib/mcp/oauth.ts @@ -45,8 +45,10 @@ function parseWwwAuthenticate(value: string | null): string | null { } async function fetchJson(url: string, init?: RequestInit) { - await validateRemoteMcpUrl(url); - const response = await fetch(url, { ...init, redirect: "manual" }); + // Route through the shared guarded egress helper so this call gets the same + // HTTPS-only / private-IP / no-redirect protections as the connector + // transport (closes the raw-fetch SSRF gap in OAuth discovery). + const response = await guardedFetch(url, init); if (!response.ok) { throw new Error(`Failed to fetch OAuth metadata (${response.status}).`); } @@ -58,12 +60,14 @@ async function fetchJson(url: string, init?: RequestInit) { } async function discoverProtectedResourceMetadataUrl(serverUrl: string) { + // The MCP server URL is attacker-influenced, so both discovery probes go + // through the shared guarded egress helper rather than raw fetch (previously + // an unvalidated SSRF sink). const attempts: Array<() => Promise> = [ - () => fetch(serverUrl, { method: "GET", redirect: "manual" }), + () => guardedFetch(serverUrl, { method: "GET" }), () => - fetch(serverUrl, { + guardedFetch(serverUrl, { method: "POST", - redirect: "manual", headers: { Accept: "application/json, text/event-stream", "Content-Type": "application/json", @@ -189,10 +193,8 @@ async function registerOAuthClient( redirectUri: string, ) { if (!metadata.registrationEndpoint) return null; - await validateRemoteMcpUrl(metadata.registrationEndpoint); - const response = await fetch(metadata.registrationEndpoint, { + const response = await guardedFetch(metadata.registrationEndpoint, { method: "POST", - redirect: "manual", headers: { Accept: "application/json", "Content-Type": "application/json", @@ -329,8 +331,7 @@ async function refreshOAuthAccessToken(row: OAuthTokenRow, db: Db) { }); if (clientSecret) body.set("client_secret", clientSecret); if (row.resource) body.set("resource", row.resource); - await validateRemoteMcpUrl(row.token_endpoint); - const response = await fetch(row.token_endpoint, { + const response = await guardedFetch(row.token_endpoint, { method: "POST", headers: { Accept: "application/json", diff --git a/backend/src/lib/privateIp.ts b/backend/src/lib/privateIp.ts new file mode 100644 index 000000000..6286527a1 --- /dev/null +++ b/backend/src/lib/privateIp.ts @@ -0,0 +1,122 @@ +import net from "net"; + +/** + * SSRF guard helpers: classify an IP literal as private/reserved/unsafe. + * Shared by the MCP connector egress checks so every caller rejects the same + * ranges. Conservative: anything unparseable or unrecognized is treated as + * blocked. + */ +export function isPrivateIpv4(ip: string): boolean { + const parts = ip.split(".").map((part) => Number.parseInt(part, 10)); + if (parts.length !== 4 || parts.some((part) => !Number.isFinite(part))) { + return true; + } + const [a, b] = parts; + return ( + a === 0 || + a === 10 || + a === 127 || + (a === 100 && b >= 64 && b <= 127) || + (a === 169 && b === 254) || + (a === 172 && b >= 16 && b <= 31) || + (a === 192 && b === 168) || + (a === 192 && b === 0) || + (a === 198 && (b === 18 || b === 19)) || + a >= 224 + ); +} + +/** + * Expand an IPv6 literal (possibly using `::` compression and/or a trailing + * dotted-quad IPv4 tail) into its eight 16-bit groups. Returns null if the + * input is not a well-formed IPv6 literal. Any embedded dotted IPv4 tail is + * folded into the final two hextets so callers can read the embedded address + * uniformly. + */ +function expandIpv6Groups(ip: string): number[] | null { + let s = ip.toLowerCase(); + const zone = s.indexOf("%"); + if (zone !== -1) s = s.slice(0, zone); + + // Fold a trailing dotted-quad IPv4 tail (e.g. `::ffff:1.2.3.4`) into two + // hex groups so the address is a pure list of hextets. + const dotted = s.match(/(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); + if (dotted) { + const octets = dotted.slice(1, 5).map((o) => Number.parseInt(o, 10)); + if (octets.some((o) => o > 255)) return null; + const hi = ((octets[0] << 8) | octets[1]).toString(16); + const lo = ((octets[2] << 8) | octets[3]).toString(16); + s = s.slice(0, dotted.index) + `${hi}:${lo}`; + } + + const halves = s.split("::"); + if (halves.length > 2) return null; + const head = halves[0] ? halves[0].split(":") : []; + const tail = halves.length === 2 && halves[1] ? halves[1].split(":") : []; + + let groups: string[]; + if (halves.length === 2) { + const fill = 8 - head.length - tail.length; + if (fill < 0) return null; + groups = [...head, ...Array(fill).fill("0"), ...tail]; + } else { + groups = head; + } + if (groups.length !== 8) return null; + + const nums = groups.map((g) => Number.parseInt(g || "0", 16)); + if (nums.some((n) => !Number.isInteger(n) || n < 0 || n > 0xffff)) { + return null; + } + return nums; +} + +function embeddedIpv4(hi: number, lo: number): string { + return `${(hi >> 8) & 0xff}.${hi & 0xff}.${(lo >> 8) & 0xff}.${lo & 0xff}`; +} + +export function isPrivateIpv6(ip: string): boolean { + const normalized = ip.toLowerCase(); + if (normalized === "::1" || normalized === "::") return true; + if (normalized.startsWith("fc") || normalized.startsWith("fd")) return true; + // Link-local fe80::/10 — the first hextet ranges fe80..febf (four hex + // digits). The narrower /^fe[89ab]:/ form was a bug: it only matched the + // unrelated hextet "fe8:" and let fe80::1 through. + if (/^fe[89ab][0-9a-f]:/.test(normalized)) return true; + + const groups = expandIpv6Groups(normalized); + if (!groups) return false; + + // IPv4-mapped ::ffff:0:0/96 — covers both dotted (`::ffff:1.2.3.4`) and + // hex (`::ffff:c0a8:0001`) forms. The address *is* the embedded IPv4. + if (groups.slice(0, 5).every((g) => g === 0) && groups[5] === 0xffff) { + return isPrivateIpv4(embeddedIpv4(groups[6], groups[7])); + } + // NAT64 well-known prefix 64:ff9b::/96 — last 32 bits are the target IPv4. + if ( + groups[0] === 0x64 && + groups[1] === 0xff9b && + groups[2] === 0 && + groups[3] === 0 && + groups[4] === 0 && + groups[5] === 0 + ) { + return isPrivateIpv4(embeddedIpv4(groups[6], groups[7])); + } + // 6to4 2002::/16 — the embedded IPv4 sits in the second and third hextets. + if (groups[0] === 0x2002) { + return isPrivateIpv4(embeddedIpv4(groups[1], groups[2])); + } + return false; +} + +/** + * True if `ip` is a private/reserved/unsafe address. Non-IP input returns true + * (fail closed) — callers should pass resolved IP literals. + */ +export function isBlockedIp(ip: string): boolean { + const family = net.isIP(ip); + if (family === 4) return isPrivateIpv4(ip); + if (family === 6) return isPrivateIpv6(ip); + return true; +} diff --git a/backend/tsconfig.json b/backend/tsconfig.json index a4b3abf67..bc27281c0 100644 --- a/backend/tsconfig.json +++ b/backend/tsconfig.json @@ -16,5 +16,5 @@ } }, "include": ["src/**/*"], - "exclude": ["node_modules", "dist"] + "exclude": ["node_modules", "dist", "src/**/*.test.ts", "src/**/__tests__/**"] } From 4d0ed91475888acebd0245ec98dc73941f215f0d Mon Sep 17 00:00:00 2001 From: Amal Date: Fri, 17 Jul 2026 00:12:47 -0700 Subject: [PATCH 2/2] security: pin MCP egress DNS at connect time (undici dispatcher) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restore the fork's pinnedGuardAgent verbatim: guardedFetch now routes through a per-request undici Agent whose connect-time DNS lookup runs the private-IP guard and returns only validated addresses, so the address we validate is the address we connect to — closing the DNS-rebinding/TOCTOU window between the pre-fetch validation lookup and the socket's own resolution. Adds the undici runtime dependency the fork ships for exactly this purpose ("undici": "^6.27.0"), and restores the dispatcher assertions in the SSRF test so it matches the fork's byte for byte. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01CEguyEgXa9JjCciXCcVemC --- backend/package-lock.json | 11 ++++ backend/package.json | 1 + .../src/lib/mcp/__tests__/client.ssrf.test.ts | 8 ++- backend/src/lib/mcp/client.ts | 51 ++++++++++++++++++- backend/src/lib/mcp/oauth.ts | 4 +- 5 files changed, 70 insertions(+), 5 deletions(-) diff --git a/backend/package-lock.json b/backend/package-lock.json index c7597a9ca..0d053f28a 100644 --- a/backend/package-lock.json +++ b/backend/package-lock.json @@ -7,6 +7,7 @@ "": { "name": "mike-backend", "version": "1.0.0", + "license": "AGPL-3.0-only", "dependencies": { "@anthropic-ai/sdk": "^0.90.0", "@aws-sdk/client-s3": "^3.787.0", @@ -28,6 +29,7 @@ "multer": "^1.4.5-lts.2", "pdfjs-dist": "^4.10.38", "resend": "^4.5.1", + "undici": "^6.27.0", "xlsx": "https://cdn.sheetjs.com/xlsx-0.20.3/xlsx-0.20.3.tgz", "zod": "^3.25.76" }, @@ -5383,6 +5385,15 @@ "integrity": "sha512-DXtD3ZtEQzc7M8m4cXotyHR+FAS18C64asBYY5vqZexfYryNNnDc02W4hKg3rdQuqOYas1jkseX0+nZXjTXnvQ==", "license": "MIT" }, + "node_modules/undici": { + "version": "6.27.0", + "resolved": "https://registry.npmjs.org/undici/-/undici-6.27.0.tgz", + "integrity": "sha512-YmfV3YnEDzXRC5lZ2jWtWWHKGUm1zIt8AhesR1tens+HTNv+YZlN/dp6G727LOvMJ8xjP9Be7Y2Sdr96LDm+pg==", + "license": "MIT", + "engines": { + "node": ">=18.17" + } + }, "node_modules/undici-types": { "version": "6.21.0", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-6.21.0.tgz", diff --git a/backend/package.json b/backend/package.json index 195a6acfc..2843d0ef1 100644 --- a/backend/package.json +++ b/backend/package.json @@ -28,6 +28,7 @@ "multer": "^1.4.5-lts.2", "pdfjs-dist": "^4.10.38", "resend": "^4.5.1", + "undici": "^6.27.0", "xlsx": "https://cdn.sheetjs.com/xlsx-0.20.3/xlsx-0.20.3.tgz", "zod": "^3.25.76" }, diff --git a/backend/src/lib/mcp/__tests__/client.ssrf.test.ts b/backend/src/lib/mcp/__tests__/client.ssrf.test.ts index 3bd58c325..babb35782 100644 --- a/backend/src/lib/mcp/__tests__/client.ssrf.test.ts +++ b/backend/src/lib/mcp/__tests__/client.ssrf.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { Agent } from "undici"; // Mock DNS resolution so the SSRF guard is exercised deterministically without // touching the network. `lookupMock` is hoisted so the vi.mock factory can @@ -104,7 +105,7 @@ describe("guardedFetch", () => { expect(fetchSpy).not.toHaveBeenCalled(); }); - it("disables redirects for public hosts", async () => { + it("pins the connection (undici dispatcher) and disables redirects for public hosts", async () => { resolvesTo("93.184.216.34"); const fetchSpy = vi .spyOn(globalThis, "fetch") @@ -116,8 +117,11 @@ describe("guardedFetch", () => { expect(res.status).toBe(200); expect(fetchSpy).toHaveBeenCalledTimes(1); - const init = fetchSpy.mock.calls[0][1] as RequestInit; + const init = fetchSpy.mock.calls[0][1] as RequestInit & { + dispatcher?: unknown; + }; expect(init.redirect).toBe("manual"); + expect(init.dispatcher).toBeInstanceOf(Agent); // Original request options are preserved. expect(init.method).toBe("GET"); }); diff --git a/backend/src/lib/mcp/client.ts b/backend/src/lib/mcp/client.ts index 09523a8ee..019666211 100644 --- a/backend/src/lib/mcp/client.ts +++ b/backend/src/lib/mcp/client.ts @@ -1,6 +1,7 @@ import crypto from "crypto"; import dns from "dns/promises"; import net from "net"; +import { Agent } from "undici"; import { isBlockedIp } from "../privateIp"; import { BLOCKED_METADATA_HOSTS, @@ -328,6 +329,50 @@ export function authConfigPatch(config: McpConnectorAuthConfig): Record { + dns.lookup(hostname, { all: true, verbatim: true }) + .then((addresses) => { + if ( + !addresses.length || + addresses.some(({ address }) => isBlockedIp(address)) + ) { + callback( + new Error( + "MCP server URL resolves to a blocked network address.", + ), + [], + ); + return; + } + callback(null, addresses); + }) + .catch((err: unknown) => + callback( + err instanceof Error ? err : new Error(String(err)), + [], + ), + ); + }, + }, + }); +} + +// The single guarded egress helper for every outbound MCP request (connector +// transport, OAuth discovery/registration/refresh). It rejects non-HTTPS, +// credentialed, metadata-host and private-IP-literal URLs up front, pins the +// 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. export async function guardedFetch( input: Parameters[0], init?: Parameters[1], @@ -339,7 +384,11 @@ export async function guardedFetch( ? input.toString() : input.url; await validateRemoteMcpUrl(url); - return fetch(input, { ...init, redirect: "manual" }); + return fetch(input, { + ...init, + redirect: "manual", + dispatcher: pinnedGuardAgent(), + } as RequestInit); } export function base64Url(buffer: Buffer) { diff --git a/backend/src/lib/mcp/oauth.ts b/backend/src/lib/mcp/oauth.ts index 8d007f440..96b16e404 100644 --- a/backend/src/lib/mcp/oauth.ts +++ b/backend/src/lib/mcp/oauth.ts @@ -46,8 +46,8 @@ function parseWwwAuthenticate(value: string | null): string | null { async function fetchJson(url: string, init?: RequestInit) { // Route through the shared guarded egress helper so this call gets the same - // HTTPS-only / private-IP / no-redirect protections as the connector - // transport (closes the raw-fetch SSRF gap in OAuth discovery). + // HTTPS-only / private-IP / connect-time-pinned / no-redirect protections as + // the connector transport (closes the raw-fetch SSRF gap in OAuth discovery). const response = await guardedFetch(url, init); if (!response.ok) { throw new Error(`Failed to fetch OAuth metadata (${response.status}).`);