fix(server): pin all validated addresses; keep tests out of the build
Addresses the round-2 Copilot review (both suppressed comments). - createPinnedAgent now takes the whole validated address list instead of a single IP, so Node's address-family fallback still works: a dual-stack host whose first address is unreachable (IPv6 on an IPv4-only network) connects via a later one. The set net.connect may choose from is exactly the prevalidated set, so the TOCTOU guarantee is unchanged. Empty input throws ProxyTargetError rather than pinning undefined. - Tests: dual-stack pass-through, the empty-list guard, and an end-to-end request where the first pinned address is a black hole (2001:db8::1) and the connection must complete via the second. - server/tsconfig.json excludes src/**/__tests__/** so the suite is no longer emitted into build/ and published with the package (verified: build/__tests__ is gone). tsconfig.test.json keeps the suite type-checked, and `npm test` runs it before vitest. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
0e1bc1d402
commit
923c58edec
6 changed files with 91 additions and 20 deletions
|
|
@ -25,7 +25,7 @@
|
|||
"start": "node build/index.js",
|
||||
"dev": "tsx watch --clear-screen=false src/index.ts",
|
||||
"dev:windows": "tsx watch --clear-screen=false src/index.ts < NUL",
|
||||
"test": "vitest run",
|
||||
"test": "tsc -p tsconfig.test.json && vitest run",
|
||||
"test:watch": "vitest"
|
||||
},
|
||||
"devDependencies": {
|
||||
|
|
|
|||
|
|
@ -224,18 +224,18 @@ function pinnedLookup(agent: http.Agent | https.Agent): PinnedLookup {
|
|||
|
||||
describe("createPinnedAgent", () => {
|
||||
it("returns an http.Agent for http: protocol", () => {
|
||||
const agent = createPinnedAgent("http:", "127.0.0.1");
|
||||
const agent = createPinnedAgent("http:", ["127.0.0.1"]);
|
||||
expect(agent).toBeInstanceOf(http.Agent);
|
||||
expect(agent).not.toBeInstanceOf(https.Agent);
|
||||
});
|
||||
|
||||
it("returns an https.Agent for https: protocol", () => {
|
||||
const agent = createPinnedAgent("https:", "127.0.0.1");
|
||||
const agent = createPinnedAgent("https:", ["127.0.0.1"]);
|
||||
expect(agent).toBeInstanceOf(https.Agent);
|
||||
});
|
||||
|
||||
it("pinned lookup returns the IPv4 address regardless of queried hostname", () => {
|
||||
const lookup = pinnedLookup(createPinnedAgent("http:", "192.0.2.99"));
|
||||
const lookup = pinnedLookup(createPinnedAgent("http:", ["192.0.2.99"]));
|
||||
|
||||
const callback = vi.fn();
|
||||
lookup("example.com", {}, callback);
|
||||
|
|
@ -244,7 +244,7 @@ describe("createPinnedAgent", () => {
|
|||
});
|
||||
|
||||
it("pinned lookup returns the IPv6 address and family 6", () => {
|
||||
const lookup = pinnedLookup(createPinnedAgent("http:", "2001:db8::1"));
|
||||
const lookup = pinnedLookup(createPinnedAgent("http:", ["2001:db8::1"]));
|
||||
|
||||
const callback = vi.fn();
|
||||
lookup("example.com", {}, callback);
|
||||
|
|
@ -256,7 +256,7 @@ describe("createPinnedAgent", () => {
|
|||
// 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 lookup = pinnedLookup(createPinnedAgent("http:", ["192.0.2.99"]));
|
||||
|
||||
const callback = vi.fn();
|
||||
lookup("example.com", { all: true }, callback);
|
||||
|
|
@ -267,7 +267,7 @@ describe("createPinnedAgent", () => {
|
|||
});
|
||||
|
||||
it("pinned lookup returns the array shape for IPv6 too", () => {
|
||||
const lookup = pinnedLookup(createPinnedAgent("http:", "2001:db8::1"));
|
||||
const lookup = pinnedLookup(createPinnedAgent("http:", ["2001:db8::1"]));
|
||||
|
||||
const callback = vi.fn();
|
||||
lookup("example.com", { all: true }, callback);
|
||||
|
|
@ -277,8 +277,52 @@ describe("createPinnedAgent", () => {
|
|||
]);
|
||||
});
|
||||
|
||||
// Every validated address is handed to the hook, so Node keeps its normal
|
||||
// address-family fallback for a dual-stack host whose first address is
|
||||
// unreachable — while still only ever seeing prevalidated addresses.
|
||||
it("passes every validated address through for dual-stack fallback", () => {
|
||||
const lookup = pinnedLookup(
|
||||
createPinnedAgent("http:", ["2001:db8::1", "192.0.2.7"]),
|
||||
);
|
||||
|
||||
const callback = vi.fn();
|
||||
lookup("dual.example", { all: true }, callback);
|
||||
|
||||
expect(callback).toHaveBeenCalledWith(null, [
|
||||
{ address: "2001:db8::1", family: 6 },
|
||||
{ address: "192.0.2.7", family: 4 },
|
||||
]);
|
||||
});
|
||||
|
||||
it("throws when given no validated address", () => {
|
||||
expect(() => createPinnedAgent("http:", [])).toThrow(ProxyTargetError);
|
||||
});
|
||||
|
||||
// A dual-stack host whose first (IPv6) address is dead: the connection must
|
||||
// still complete via the second, validated address.
|
||||
it("falls back to a later validated address when the first is unreachable", async () => {
|
||||
const server = http.createServer((_req, res) => res.end("fallback-ok"));
|
||||
await new Promise<void>((resolve) =>
|
||||
server.listen(0, "127.0.0.1", () => resolve()),
|
||||
);
|
||||
const { port } = server.address() as AddressInfo;
|
||||
|
||||
try {
|
||||
// 2001:db8::1 is the documentation range — nothing answers there.
|
||||
const agent = createPinnedAgent("http:", ["2001:db8::1", "127.0.0.1"]);
|
||||
const response = await nodeFetch(`http://dual.invalid:${port}/`, {
|
||||
agent,
|
||||
});
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
await expect(response.text()).resolves.toBe("fallback-ok");
|
||||
} finally {
|
||||
await new Promise<void>((resolve) => server.close(() => resolve()));
|
||||
}
|
||||
});
|
||||
|
||||
it("TOCTOU guarantee: lookup never invokes the OS resolver", () => {
|
||||
const lookup = pinnedLookup(createPinnedAgent("http:", "10.0.0.1"));
|
||||
const lookup = pinnedLookup(createPinnedAgent("http:", ["10.0.0.1"]));
|
||||
|
||||
const callback = vi.fn();
|
||||
lookup("any-hostname.example", {}, callback);
|
||||
|
|
@ -300,7 +344,7 @@ describe("createPinnedAgent", () => {
|
|||
const { port } = server.address() as AddressInfo;
|
||||
|
||||
try {
|
||||
const agent = createPinnedAgent("http:", "127.0.0.1");
|
||||
const agent = createPinnedAgent("http:", ["127.0.0.1"]);
|
||||
const response = await nodeFetch(
|
||||
`http://pinned-target.invalid:${port}/`,
|
||||
{ agent },
|
||||
|
|
|
|||
|
|
@ -888,10 +888,12 @@ async function safeProxyFetch(initialUrl: string, init?: RequestInit) {
|
|||
throw new ProxyTargetError("Only http/https URLs are allowed");
|
||||
}
|
||||
|
||||
// Resolve DNS once, validate every address, and pin the connection to the
|
||||
// first validated IP so node-fetch never does a second DNS lookup.
|
||||
// Resolve DNS once, validate every address, and pin the connection to that
|
||||
// validated set so node-fetch never does a second DNS lookup. All of them
|
||||
// are passed through, so Node's address-family fallback still works while
|
||||
// the reachable set stays limited to what was just validated.
|
||||
const validatedAddresses = await assertSafeProxyTarget(currentUrl);
|
||||
const agent = createPinnedAgent(currentUrl.protocol, validatedAddresses[0]);
|
||||
const agent = createPinnedAgent(currentUrl.protocol, validatedAddresses);
|
||||
|
||||
const response = await fetch(currentUrl.toString(), {
|
||||
method,
|
||||
|
|
|
|||
|
|
@ -125,20 +125,33 @@ export async function assertSafeProxyTarget(parsedUrl: URL): Promise<string[]> {
|
|||
|
||||
/**
|
||||
* Creates an `http.Agent` or `https.Agent` whose internal `lookup` hook always
|
||||
* returns `pinnedIp`, bypassing the OS resolver entirely.
|
||||
* returns `pinnedIps`, bypassing the OS resolver entirely.
|
||||
*
|
||||
* Pass this agent to `node-fetch` on every hop of `safeProxyFetch` so the TCP
|
||||
* connection always goes to the IP that was validated by `assertSafeProxyTarget`
|
||||
* connection always goes to an IP that was validated by `assertSafeProxyTarget`
|
||||
* — even if the domain's DNS record changes mid-flight.
|
||||
*
|
||||
* Every address `assertSafeProxyTarget` returned is handed to the hook, not
|
||||
* just the first, so Node keeps its normal address-family fallback: a
|
||||
* dual-stack host whose first address is unreachable (IPv6 on an IPv4-only
|
||||
* network, say) still connects via a later one. The set it may choose from is
|
||||
* exactly the prevalidated set, so the TOCTOU guarantee is unchanged.
|
||||
*
|
||||
* @param protocol - `"http:"` or `"https:"` (from `URL.protocol`)
|
||||
* @param pinnedIp - A validated IPv4 or IPv6 address string
|
||||
* @param pinnedIps - Validated IPv4/IPv6 address strings, in preference order
|
||||
*/
|
||||
export function createPinnedAgent(
|
||||
protocol: string,
|
||||
pinnedIp: string,
|
||||
pinnedIps: string[],
|
||||
): http.Agent | https.Agent {
|
||||
const family = isIP(pinnedIp) === 6 ? 6 : 4;
|
||||
if (pinnedIps.length === 0) {
|
||||
throw new ProxyTargetError("No validated address to connect to");
|
||||
}
|
||||
|
||||
const resolved: dns.LookupAddress[] = pinnedIps.map((address) => ({
|
||||
address,
|
||||
family: isIP(address) === 6 ? 6 : 4,
|
||||
}));
|
||||
|
||||
// Node.js lookup signature: (hostname, options, callback).
|
||||
//
|
||||
|
|
@ -157,10 +170,10 @@ export function createPinnedAgent(
|
|||
) => void,
|
||||
): void => {
|
||||
if (opts?.all) {
|
||||
callback(null, [{ address: pinnedIp, family }]);
|
||||
callback(null, resolved);
|
||||
return;
|
||||
}
|
||||
callback(null, pinnedIp, family);
|
||||
callback(null, resolved[0].address, resolved[0].family);
|
||||
};
|
||||
|
||||
if (protocol === "https:") {
|
||||
|
|
|
|||
|
|
@ -12,5 +12,5 @@
|
|||
"resolveJsonModule": true
|
||||
},
|
||||
"include": ["src/**/*"],
|
||||
"exclude": ["node_modules", "packages", "**/*.spec.ts"]
|
||||
"exclude": ["node_modules", "packages", "**/*.spec.ts", "src/**/__tests__/**"]
|
||||
}
|
||||
|
|
|
|||
12
server/tsconfig.test.json
Normal file
12
server/tsconfig.test.json
Normal file
|
|
@ -0,0 +1,12 @@
|
|||
// The build config excludes __tests__ so the suite never lands in the
|
||||
// published `build/` output. This config exists only so the tests are still
|
||||
// type-checked; it emits nothing and is run by `npm test`.
|
||||
{
|
||||
"extends": "./tsconfig.json",
|
||||
"compilerOptions": {
|
||||
"noEmit": true,
|
||||
"types": ["node"]
|
||||
},
|
||||
"include": ["src/**/*"],
|
||||
"exclude": ["node_modules", "packages"]
|
||||
}
|
||||
Loading…
Reference in a new issue