diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index c8d7405c..bf878e87 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -37,6 +37,10 @@ jobs: working-directory: ./client run: npm test + - name: Run server tests + working-directory: ./server + run: npm test + - run: npm run build publish: diff --git a/package-lock.json b/package-lock.json index d70bf35a..5e28f227 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13273,10 +13273,12 @@ "devDependencies": { "@types/cors": "^2.8.19", "@types/express": "^5.0.0", + "@types/node": "^22.0.0", "@types/shell-quote": "^1.7.5", "@types/ws": "^8.5.12", "tsx": "^4.19.0", - "typescript": "^5.6.2" + "typescript": "^5.6.2", + "vitest": "^4.1.0" } } } diff --git a/package.json b/package.json index 42d3fb90..b7d44689 100644 --- a/package.json +++ b/package.json @@ -42,6 +42,7 @@ "start-client": "cd client && npm run preview", "test": "npm run prettier-check && cd client && npm test", "test-cli": "cd cli && npm run test", + "test-server": "cd server && npm run test", "test:e2e": "MCP_AUTO_OPEN_ENABLED=false npm run test:e2e --workspace=client", "prettier-fix": "prettier --write .", "prettier-check": "prettier --check .", diff --git a/server/src/__tests__/proxy-security.test.ts b/server/src/__tests__/proxy-security.test.ts index 3772f0bf..1e07eaed 100644 --- a/server/src/__tests__/proxy-security.test.ts +++ b/server/src/__tests__/proxy-security.test.ts @@ -10,6 +10,9 @@ import http from "node:http"; import https from "node:https"; +import type dnsTypes from "node:dns"; +import type { AddressInfo } from "node:net"; +import nodeFetch from "node-fetch"; import { vi, describe, it, expect, afterEach } from "vitest"; // Mock node:dns/promises before importing the module under test so that @@ -164,9 +167,7 @@ describe("assertSafeProxyTarget", () => { }); it("skips DNS lookup for literal IPv4 hosts", async () => { - const addrs = await assertSafeProxyTarget( - new URL("http://127.0.0.1/path"), - ); + const addrs = await assertSafeProxyTarget(new URL("http://127.0.0.1/path")); expect(addrs).toEqual(["127.0.0.1"]); expect(mockLookup).not.toHaveBeenCalled(); }); @@ -201,6 +202,26 @@ describe("assertSafeProxyTarget", () => { // createPinnedAgent // --------------------------------------------------------------------------- +/** The lookup hook `createPinnedAgent` installed on the agent. */ +type PinnedLookup = ( + hostname: string, + options: dnsTypes.LookupOptions, + callback: ( + err: NodeJS.ErrnoException | null, + address: string | dnsTypes.LookupAddress[], + family?: number, + ) => void, +) => void; + +function pinnedLookup(agent: http.Agent | https.Agent): PinnedLookup { + const { lookup } = (agent as http.Agent & { options: http.AgentOptions }) + .options; + if (typeof lookup !== "function") { + throw new Error("expected createPinnedAgent to install a lookup hook"); + } + return lookup as PinnedLookup; +} + describe("createPinnedAgent", () => { it("returns an http.Agent for http: protocol", () => { const agent = createPinnedAgent("http:", "127.0.0.1"); @@ -213,53 +234,82 @@ describe("createPinnedAgent", () => { expect(agent).toBeInstanceOf(https.Agent); }); - it("pinned lookup always returns the IPv4 address regardless of queried hostname", () => - new Promise((resolve) => { - const agent = createPinnedAgent("http:", "192.0.2.99"); - const lookup = (agent as http.Agent & { options: { lookup: Function } }) - .options.lookup; + it("pinned lookup returns the IPv4 address regardless of queried hostname", () => { + const lookup = pinnedLookup(createPinnedAgent("http:", "192.0.2.99")); - lookup( - "example.com", - {}, - (err: Error | null, address: string, family: number) => { - expect(err).toBeNull(); - expect(address).toBe("192.0.2.99"); - expect(family).toBe(4); - resolve(); - }, - ); - })); + const callback = vi.fn(); + lookup("example.com", {}, callback); - it("pinned lookup always returns the IPv6 address and family 6", () => - new Promise((resolve) => { - const agent = createPinnedAgent("http:", "2001:db8::1"); - const lookup = (agent as http.Agent & { options: { lookup: Function } }) - .options.lookup; + expect(callback).toHaveBeenCalledWith(null, "192.0.2.99", 4); + }); - lookup( - "example.com", - {}, - (err: Error | null, address: string, family: number) => { - expect(err).toBeNull(); - expect(address).toBe("2001:db8::1"); - expect(family).toBe(6); - resolve(); - }, - ); - })); + it("pinned lookup returns the IPv6 address and family 6", () => { + const lookup = pinnedLookup(createPinnedAgent("http:", "2001:db8::1")); + + const callback = vi.fn(); + lookup("example.com", {}, callback); + + expect(callback).toHaveBeenCalledWith(null, "2001:db8::1", 6); + }); + + // Node's net.connect runs with autoSelectFamily on (Node >= 20), so it calls + // the hook with { all: true } and requires the ARRAY callback shape. Handing + // it a scalar there fails every hostname request with ERR_INVALID_IP_ADDRESS. + it("pinned lookup returns the array shape when called with { all: true }", () => { + const lookup = pinnedLookup(createPinnedAgent("http:", "192.0.2.99")); + + const callback = vi.fn(); + lookup("example.com", { all: true }, callback); + + expect(callback).toHaveBeenCalledWith(null, [ + { address: "192.0.2.99", family: 4 }, + ]); + }); + + it("pinned lookup returns the array shape for IPv6 too", () => { + const lookup = pinnedLookup(createPinnedAgent("http:", "2001:db8::1")); + + const callback = vi.fn(); + lookup("example.com", { all: true }, callback); + + expect(callback).toHaveBeenCalledWith(null, [ + { address: "2001:db8::1", family: 6 }, + ]); + }); it("TOCTOU guarantee: lookup never invokes the OS resolver", () => { - const agent = createPinnedAgent("http:", "10.0.0.1"); - const lookup = (agent as http.Agent & { options: { lookup: Function } }) - .options.lookup; + const lookup = pinnedLookup(createPinnedAgent("http:", "10.0.0.1")); - const osDnsLookup = vi.fn(); - lookup("any-hostname.example", {}, osDnsLookup); + const callback = vi.fn(); + lookup("any-hostname.example", {}, callback); - // osDnsLookup was called as the callback, not as a resolver — - // the pinned implementation calls it synchronously with the fixed IP. - expect(osDnsLookup).toHaveBeenCalledTimes(1); - expect(osDnsLookup).toHaveBeenCalledWith(null, "10.0.0.1", 4); + // The callback is invoked synchronously with the fixed IP — no resolver. + expect(callback).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith(null, "10.0.0.1", 4); + }); + + // End-to-end over the production call path: node-fetch → http.Agent → + // net.connect. The hostname is deliberately unresolvable, so the request can + // only succeed if the connection went to the pinned IP, and it exercises the + // real { all: true } callback shape rather than a hand-rolled invocation. + it("routes a real request to the pinned IP for an unresolvable hostname", async () => { + const server = http.createServer((_req, res) => res.end("pinned-ok")); + await new Promise((resolve) => + server.listen(0, "127.0.0.1", () => resolve()), + ); + const { port } = server.address() as AddressInfo; + + try { + const agent = createPinnedAgent("http:", "127.0.0.1"); + const response = await nodeFetch( + `http://pinned-target.invalid:${port}/`, + { agent }, + ); + + expect(response.status).toBe(200); + await expect(response.text()).resolves.toBe("pinned-ok"); + } finally { + await new Promise((resolve) => server.close(() => resolve())); + } }); }); diff --git a/server/src/index.ts b/server/src/index.ts index b0b6774b..25a9309b 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -898,8 +898,7 @@ async function safeProxyFetch(initialUrl: string, init?: RequestInit) { headers, body, redirect: "manual", - // eslint-disable-next-line @typescript-eslint/no-explicit-any - agent: agent as any, + agent, }); if (response.status >= 300 && response.status < 400) { diff --git a/server/src/proxy-security.ts b/server/src/proxy-security.ts index d1336541..45b7510d 100644 --- a/server/src/proxy-security.ts +++ b/server/src/proxy-security.ts @@ -21,6 +21,7 @@ import http from "node:http"; import https from "node:https"; import { isIP } from "node:net"; +import type dns from "node:dns"; import { lookup as dnsLookup } from "node:dns/promises"; // --------------------------------------------------------------------------- @@ -89,9 +90,7 @@ export class ProxyTargetError extends Error {} * @throws {ProxyTargetError} if the host cannot be resolved or any resolved * address falls inside the block-list. */ -export async function assertSafeProxyTarget( - parsedUrl: URL, -): Promise { +export async function assertSafeProxyTarget(parsedUrl: URL): Promise { const host = parsedUrl.hostname.replace(/^\[|\]$/g, ""); // strip IPv6 brackets let addresses: string[]; @@ -141,12 +140,26 @@ export function createPinnedAgent( ): http.Agent | https.Agent { const family = isIP(pinnedIp) === 6 ? 6 : 4; - // Node.js lookup signature: (hostname, options, callback) + // Node.js lookup signature: (hostname, options, callback). + // + // The callback shape depends on `options.all`. Node's `net.connect` runs with + // `autoSelectFamily` enabled by default (Node >= 20), so it calls the hook + // with `{ all: true }` and expects an ARRAY of `{ address, family }` — handing + // it the scalar form there fails the connection with ERR_INVALID_IP_ADDRESS. + // Both shapes are supported so the agent works on either call path. const lookup = ( _hostname: string, - _opts: unknown, - callback: (err: Error | null, address: string, family: number) => void, + opts: dns.LookupOptions | undefined, + callback: ( + err: NodeJS.ErrnoException | null, + address: string | dns.LookupAddress[], + family?: number, + ) => void, ): void => { + if (opts?.all) { + callback(null, [{ address: pinnedIp, family }]); + return; + } callback(null, pinnedIp, family); };