fix(server): make the pinned lookup honor Node's all:true callback shape
Addresses the round-1 Copilot review.
- createPinnedAgent: Node's net.connect runs with autoSelectFamily on
(Node >= 20), so it calls the lookup hook with `{ all: true }` and
requires an ARRAY of `{ address, family }`. The scalar-only callback
failed every hostname request with ERR_INVALID_IP_ADDRESS, so /fetch
could only reach literal-IP targets. Both callback shapes are now
supported. Reproduced against Node 22/26 before and after.
- Tests: cover the `{ all: true }` shape for IPv4 and IPv6, and add an
end-to-end request through the agent over the production path
(node-fetch -> http.Agent -> net.connect) using an unresolvable
hostname, so the request can only succeed via the pinned IP. Verified
these three fail without the fix. Replaced the `Function`-typed casts
with a typed pinnedLookup() helper.
- CI: run the new server suite (.github/workflows/main.yml) and add a
root `test-server` script, so this regression coverage is enforced.
- package-lock.json: sync the server workspace manifest mirror with the
added @types/node and vitest devDependencies.
- index.ts: drop the `agent as any` cast and its eslint-disable — the
node-fetch RequestInit already types `agent`, and tsc is clean without
the suppression.
- Formatting: prettier-check now passes (it failed on the two new files).
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
9140789869
commit
0e1bc1d402
6 changed files with 125 additions and 56 deletions
4
.github/workflows/main.yml
vendored
4
.github/workflows/main.yml
vendored
|
|
@ -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:
|
||||
|
|
|
|||
4
package-lock.json
generated
4
package-lock.json
generated
|
|
@ -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"
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 .",
|
||||
|
|
|
|||
|
|
@ -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<void>((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<void>((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<void>((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<void>((resolve) => server.close(() => resolve()));
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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<string[]> {
|
||||
export async function assertSafeProxyTarget(parsedUrl: URL): Promise<string[]> {
|
||||
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);
|
||||
};
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue