From 10a1622c4fda89ca900334c123b1fd1ba16f5ee1 Mon Sep 17 00:00:00 2001
From: Conner Webber <39662153+spartan8806@users.noreply.github.com>
Date: Fri, 21 Aug 2026 19:19:01 -0500
Subject: [PATCH 1/6] fix(ssrf): pin the socket to the validated IP (HTTP tier)

Closes the DNS-rebinding half of #207 for the HTTP client tier. #206/#210 added a
resolve-and-validate re-check at every fetch seam, which closes the static-record
bypass, but the socket then resolved DNS a second time at connect. An attacker
controlling the authoritative resolver could answer with a public address for the
check and a private one for the connect, inside the TTL window.

guardResolvedHost / guardResolvedServeTarget now return the addresses that passed
validation, and the HTTP tier builds an undici Agent whose connect lookup returns
exactly those. There is no second resolution left to race.

A lookup hook rather than rewriting the URL to the IP: rewriting would also avoid
re-resolution but breaks TLS, since the certificate is checked against the name in
the URL. A lookup hook changes only which address the socket dials, so SNI, Host
and certificate verification all still use the real hostname.

The result type is widened additively ({ ok: true } gains an optional addresses),
so every existing caller that only checks .ok is unaffected. The other tiers are
untouched and keep the #206 re-check as their floor.

Tests:
- pinned-dispatcher.test.ts  the hook in isolation, including a resolver that
  flips to 169.254.169.254 on the second lookup, plus the must-not-do control
  that a different hostname is never pinned
- pinned-rebind.test.ts      rebinding reproduced over a real socket: two servers
  on one port, 127.0.0.1 validated and 127.0.0.2 as the attacker. Without pinning
  the request lands on the attacker; with pinning it lands on the validated host
  and the attacker gets nothing
- pinned-e2e.test.ts         a request by hostname through the real client, since
  every existing http-client test fetches an IP literal and so skips this path

undici is promoted from a transitive dependency to a direct one; it was already in
the tree at 7.28.0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 package.json                          |   3 +-
 src/fetch/http-client.ts              |  29 +++++++
 src/fetch/pinned-dispatcher.ts        | 106 ++++++++++++++++++++++++++
 src/watch/ssrf.ts                     |  22 ++++--
 tests/fetch/pinned-dispatcher.test.ts |  97 +++++++++++++++++++++++
 tests/fetch/pinned-e2e.test.ts        |  49 ++++++++++++
 tests/fetch/pinned-rebind.test.ts     |  96 +++++++++++++++++++++++
 7 files changed, 394 insertions(+), 8 deletions(-)
 create mode 100755 src/fetch/pinned-dispatcher.ts
 create mode 100755 tests/fetch/pinned-dispatcher.test.ts
 create mode 100755 tests/fetch/pinned-e2e.test.ts
 create mode 100755 tests/fetch/pinned-rebind.test.ts

diff --git a/package.json b/package.json
index 9d704f6a5..84c456478 100644
--- a/package.json
+++ b/package.json
@@ -124,7 +124,8 @@
     "react": "^18.3.1",
     "sqlite-vec": "^0.1.9",
     "tinyld": "^1.3.4",
-    "turndown": "^7.2.4"
+    "turndown": "^7.2.4",
+    "undici": "^7.28.0"
   },
   "devDependencies": {
     "@seriousme/openapi-schema-validator": "2.9.0",
diff --git a/src/fetch/http-client.ts b/src/fetch/http-client.ts
index d77a9610e..1cbdee960 100644
--- a/src/fetch/http-client.ts
+++ b/src/fetch/http-client.ts
@@ -2,6 +2,8 @@ import { getConfig } from '../config.js';
 import { createLogger } from '../logger.js';
 import { anySignal } from '../util/abort.js';
 import { guardFetchUrl, guardResolvedHost } from '../watch/ssrf.js';
+import { createPinnedAgent } from './pinned-dispatcher.js';
+import type { Agent } from 'undici';
 
 export interface HttpFetchOptions {
   headers?: Record<string, string>;
@@ -152,7 +154,13 @@ async function fetchWithRedirects(
   const visited = new Set<string>();
   let currentUrl = originalUrl;
   let redirectCount = 0;
+  // One Agent per hop, each holding a connection pool. They are closed in the finally below once
+  // the body has been consumed — an Agent left open is a socket leak, and the body is read inside
+  // this function, so closing on the way out is safe.
+  const agents: Agent[] = [];
+  let pinnedAgent: Agent | undefined;
 
+  try {
   while (true) {
     if (visited.has(currentUrl)) {
       throw new HttpFetchError(`Redirect loop detected at ${currentUrl}`, false);
@@ -176,6 +184,15 @@ async function fetchWithRedirects(
         if (!resolved.ok) {
           throw new HttpFetchError(resolved.reason, false);
         }
+        // PIN the socket to what we just validated. Without this the connection resolves DNS a
+        // second time, and an attacker controlling the resolver can answer with a public IP for
+        // the check above and a private one for the connect (DNS rebinding). Pinning removes the
+        // second resolution, so there is no window to race. Literal IPs need no pinning — there
+        // is no name to re-resolve.
+        if (resolved.addresses?.length) {
+          pinnedAgent = createPinnedAgent(rhost, resolved.addresses);
+          agents.push(pinnedAgent);
+        }
       }
     }
 
@@ -210,6 +227,9 @@ async function fetchWithRedirects(
         headers: mergedHeaders,
         redirect: 'manual',
         signal,
+        // `dispatcher` is undici's extension to RequestInit; Node's global fetch honours it but
+        // the DOM typings do not declare it, hence the cast.
+        ...(pinnedAgent ? ({ dispatcher: pinnedAgent } as Record<string, unknown>) : {}),
       });
     } catch (err) {
       const isTimeout = err instanceof Error && err.name === 'TimeoutError';
@@ -314,4 +334,13 @@ async function fetchWithRedirects(
       rawBuffer,
     };
   }
+  } finally {
+    for (const a of agents) {
+      try {
+        await a.close();
+      } catch {
+        /* closing a pool must never mask the real result */
+      }
+    }
+  }
 }
diff --git a/src/fetch/pinned-dispatcher.ts b/src/fetch/pinned-dispatcher.ts
new file mode 100755
index 000000000..4e5951389
--- /dev/null
+++ b/src/fetch/pinned-dispatcher.ts
@@ -0,0 +1,106 @@
+import { Agent, type buildConnector } from 'undici';
+import { lookup as nodeLookup } from 'node:dns';
+
+/**
+ * Pin a connection to the addresses SSRF validation already cleared.
+ *
+ * `guardResolvedHost` resolves a hostname and checks every returned address, but the socket then
+ * resolves DNS *again* at connect time. An attacker who controls the authoritative resolver can
+ * answer with a public IP for our check and a private one for the connect, inside the TTL window —
+ * classic DNS rebinding, and the resolved-IP re-check cannot see it because it has already run.
+ *
+ * The fix is to stop the second resolution happening at all: hand the socket a `lookup` that
+ * returns the exact addresses we validated, so there is no window to race.
+ *
+ * WHY A LOOKUP HOOK RATHER THAN REWRITING THE URL TO THE IP
+ * --------------------------------------------------------
+ * Rewriting `https://example.com/x` to `https://93.184.216.34/x` would also avoid re-resolution,
+ * and it would break TLS: the certificate is checked against the name in the URL, so the request
+ * would either fail verification or have to disable it. A `lookup` hook changes only which address
+ * the socket dials. The SNI name, the Host header and certificate verification all still use the
+ * real hostname, so pinning costs nothing in transport security.
+ */
+
+/** The `dns.lookup` shape `net.connect` accepts. Both callback arities are in play — see below. */
+export type PinnedLookup = (
+  hostname: string,
+  options: { family?: number; all?: boolean; hints?: number },
+  callback: (
+    err: NodeJS.ErrnoException | null,
+    address: string | { address: string; family: number }[],
+    family?: number,
+  ) => void,
+) => void;
+
+export interface ValidatedAddress {
+  address: string;
+  family: number;
+}
+
+/**
+ * Build the lookup hook. Exported separately from the Agent so it can be unit-tested without
+ * opening a socket.
+ *
+ * Two behaviours worth stating, because both are security-relevant:
+ *
+ *  - **A different hostname is never pinned.** If the socket asks for a host we did not validate,
+ *    we fall through to real DNS rather than handing back this host's addresses. Returning them
+ *    would send a request for host B to host A's IP, which is a worse bug than the one being
+ *    fixed. In practice the caller builds a fresh hook per validated host, so this is a guard
+ *    against future misuse rather than a path we expect to hit.
+ *  - **An empty address set is never pinned.** Callers must not construct this with `[]`; if they
+ *    do, we fall back to real DNS instead of failing the connection in a way that looks like a
+ *    network error.
+ */
+export function createPinnedLookup(
+  hostname: string,
+  addresses: ValidatedAddress[],
+  realLookup: typeof nodeLookup = nodeLookup,
+): PinnedLookup {
+  return (host, options, callback) => {
+    if (host !== hostname || addresses.length === 0) {
+      // Not ours to pin — defer to the real resolver.
+      (realLookup as unknown as PinnedLookup)(host, options, callback);
+      return;
+    }
+
+    // Honour an explicit family request; `family: 0` / undefined means "either".
+    const wanted = options?.family;
+    const matching =
+      wanted === 4 || wanted === 6 ? addresses.filter((a) => a.family === wanted) : addresses;
+
+    if (matching.length === 0) {
+      // We hold addresses, but none in the family the socket asked for. Report it as a resolution
+      // failure rather than silently widening to a family the caller rejected.
+      const err: NodeJS.ErrnoException = new Error(
+        `no validated IPv${wanted} address for ${host}`,
+      );
+      err.code = 'ENOTFOUND';
+      callback(err, '');
+      return;
+    }
+
+    // `all: true` wants the array form; otherwise the (address, family) arity.
+    if (options?.all) {
+      callback(null, matching.map((a) => ({ address: a.address, family: a.family })));
+      return;
+    }
+    callback(null, matching[0].address, matching[0].family);
+  };
+}
+
+/**
+ * An undici Agent whose sockets dial only the validated addresses.
+ *
+ * The caller owns the returned Agent and must `close()` (or `destroy()`) it once the response body
+ * has been consumed — an Agent holds a connection pool, so one per request that is never closed is
+ * a socket leak.
+ */
+export function createPinnedAgent(hostname: string, addresses: ValidatedAddress[]): Agent {
+  // `BuildOptions` is a union whose TCP member carries `lookup`; one cast at the boundary keeps
+  // that from leaking into the rest of the module, where the hook stays fully typed.
+  const connect = {
+    lookup: createPinnedLookup(hostname, addresses),
+  } as unknown as buildConnector.BuildOptions;
+  return new Agent({ connect });
+}
diff --git a/src/watch/ssrf.ts b/src/watch/ssrf.ts
index dc0960413..9da79d451 100644
--- a/src/watch/ssrf.ts
+++ b/src/watch/ssrf.ts
@@ -373,7 +373,14 @@ export type LookupAll = (
   callback: (err: NodeJS.ErrnoException | null, addresses: { address: string; family: number }[]) => void,
 ) => void;
 
-export type ResolveGuardResult = { ok: true } | SsrfRejection;
+/**
+ * `addresses` carries the records that passed validation, so a caller can PIN the socket to
+ * them (see `src/fetch/pinned-dispatcher.ts`). It is absent when the host did not resolve —
+ * there is nothing to pin to, and that is not a bypass.
+ */
+export type ResolveGuardResult =
+  | { ok: true; addresses?: { address: string; family: number }[] }
+  | SsrfRejection;
 
 /**
  * Fetch-time SSRF re-check. `guardFetchUrl` validates only the LITERAL hostname,
@@ -384,10 +391,11 @@ export type ResolveGuardResult = { ok: true } | SsrfRejection;
  * resolved-IP policy is identical to the literal-IP policy — no drift.
  *
  * Call this right before the actual fetch (and on each redirect hop) for any
- * non-IP-literal host. For full rebinding (TOCTOU) safety the connection should
- * additionally be pinned to the validated address — a `lookup` hook / custom
- * dispatcher — which callers can layer on; this guard closes the static-record
- * bypass (the metadata-credential-theft case) on its own.
+ * non-IP-literal host. On success it returns the validated `addresses`; pass them to
+ * `createPinnedAgent` (src/fetch/pinned-dispatcher.ts) so the socket dials one of those
+ * addresses instead of re-resolving, which is what closes the rebinding (TOCTOU) window.
+ * Without pinning this guard still closes the static-record bypass (the
+ * metadata-credential-theft case) on its own.
  */
 /** Resolve every A/AAAA record for a host, or null on NXDOMAIN / timeout / empty. */
 async function resolveAll(
@@ -437,7 +445,7 @@ export async function guardResolvedHost(
       };
     }
   }
-  return { ok: true };
+  return { ok: true, addresses };
 }
 
 /**
@@ -475,5 +483,5 @@ export async function guardResolvedServeTarget(
       };
     }
   }
-  return { ok: true };
+  return { ok: true, addresses };
 }
diff --git a/tests/fetch/pinned-dispatcher.test.ts b/tests/fetch/pinned-dispatcher.test.ts
new file mode 100755
index 000000000..d3fe97a15
--- /dev/null
+++ b/tests/fetch/pinned-dispatcher.test.ts
@@ -0,0 +1,97 @@
+import { describe, expect, it } from 'vitest';
+import { createPinnedLookup, type ValidatedAddress } from '../../src/fetch/pinned-dispatcher.js';
+
+/**
+ * The lookup hook is tested directly rather than through a socket: it is the whole security
+ * boundary, it is pure, and testing it here means the rebinding case can be expressed exactly —
+ * "the second resolution returns something different" — which is impossible to stage reliably
+ * against a real resolver.
+ */
+
+/** A real-DNS stand-in that returns whatever the attacker would answer on the SECOND lookup. */
+function rebindingResolver(addr: string, family = 4) {
+  return ((host: string, options: { all?: boolean }, cb: (...a: unknown[]) => void) => {
+    if (options?.all) cb(null, [{ address: addr, family }]);
+    else cb(null, addr, family);
+  }) as never;
+}
+
+function callLookup(
+  fn: ReturnType<typeof createPinnedLookup>,
+  host: string,
+  options: { family?: number; all?: boolean } = {},
+): Promise<{ err: NodeJS.ErrnoException | null; address: unknown; family?: number }> {
+  return new Promise((resolve) => {
+    fn(host, options, (err, address, family) => resolve({ err, address, family }));
+  });
+}
+
+const VALIDATED: ValidatedAddress[] = [{ address: '93.184.216.34', family: 4 }];
+
+describe('createPinnedLookup (DNS-rebinding pin, issue #207)', () => {
+  it('returns the validated address instead of re-resolving — the rebinding case', async () => {
+    // The attacker's resolver would now answer with the metadata IP. The pin must never ask it.
+    const fn = createPinnedLookup(
+      'rebind.evil.example',
+      VALIDATED,
+      rebindingResolver('169.254.169.254'),
+    );
+    const r = await callLookup(fn, 'rebind.evil.example');
+    expect(r.err).toBeNull();
+    expect(r.address).toBe('93.184.216.34');
+    expect(r.family).toBe(4);
+  });
+
+  it('returns the array shape when the socket asks for all: true', async () => {
+    const fn = createPinnedLookup('example.com', VALIDATED, rebindingResolver('169.254.169.254'));
+    const r = await callLookup(fn, 'example.com', { all: true });
+    expect(r.err).toBeNull();
+    expect(r.address).toEqual([{ address: '93.184.216.34', family: 4 }]);
+  });
+
+  it('does NOT pin a different hostname — it defers to real DNS', async () => {
+    // Handing host B the addresses validated for host A would be a worse bug than the one we are
+    // fixing, so this is the must-not-do control.
+    const fn = createPinnedLookup('example.com', VALIDATED, rebindingResolver('203.0.113.9'));
+    const r = await callLookup(fn, 'other.example');
+    expect(r.err).toBeNull();
+    expect(r.address).toBe('203.0.113.9');
+  });
+
+  it('falls back to real DNS when the validated set is empty rather than pinning to nothing', async () => {
+    const fn = createPinnedLookup('example.com', [], rebindingResolver('203.0.113.9'));
+    const r = await callLookup(fn, 'example.com');
+    expect(r.err).toBeNull();
+    expect(r.address).toBe('203.0.113.9');
+  });
+
+  it('honours an explicit family request', async () => {
+    const both: ValidatedAddress[] = [
+      { address: '93.184.216.34', family: 4 },
+      { address: '2606:2800:220:1:248:1893:25c8:1946', family: 6 },
+    ];
+    const fn = createPinnedLookup('example.com', both, rebindingResolver('169.254.169.254'));
+    const v6 = await callLookup(fn, 'example.com', { family: 6 });
+    expect(v6.address).toBe('2606:2800:220:1:248:1893:25c8:1946');
+    const v4 = await callLookup(fn, 'example.com', { family: 4 });
+    expect(v4.address).toBe('93.184.216.34');
+  });
+
+  it('fails the lookup when no validated address matches the requested family', async () => {
+    // Widening to a family the caller excluded would be silently ignoring the request.
+    const fn = createPinnedLookup('example.com', VALIDATED, rebindingResolver('::1', 6));
+    const r = await callLookup(fn, 'example.com', { family: 6 });
+    expect(r.err).toBeTruthy();
+    expect(r.err?.code).toBe('ENOTFOUND');
+  });
+
+  it('passes every validated address through when the socket wants all of them', async () => {
+    const many: ValidatedAddress[] = [
+      { address: '93.184.216.34', family: 4 },
+      { address: '93.184.216.35', family: 4 },
+    ];
+    const fn = createPinnedLookup('example.com', many, rebindingResolver('169.254.169.254'));
+    const r = await callLookup(fn, 'example.com', { all: true });
+    expect(r.address).toEqual(many);
+  });
+});
diff --git a/tests/fetch/pinned-e2e.test.ts b/tests/fetch/pinned-e2e.test.ts
new file mode 100755
index 000000000..7d6cf3a54
--- /dev/null
+++ b/tests/fetch/pinned-e2e.test.ts
@@ -0,0 +1,49 @@
+import { createServer, type Server } from 'node:http';
+import { afterAll, beforeAll, describe, expect, it } from 'vitest';
+import { httpFetch } from '../../src/fetch/http-client.js';
+
+/**
+ * The unit tests for `createPinnedLookup` prove the hook returns the right address. They do NOT
+ * prove the hook is actually wired into a real request, or that wiring it does not break one —
+ * and every other http-client test fetches an IP literal (`http://127.0.0.1:PORT/`), which skips
+ * the pinned path completely.
+ *
+ * So this fetches by NAME. `localhost` resolves to loopback, which the fetch policy allows for the
+ * local-dev promise, so the guard passes, returns its validated addresses, and the request goes
+ * out through a pinned Agent. If pinning were broken — wrong callback arity, wrong family, a
+ * dispatcher that never connects — this test hangs or fails while every IP-literal test keeps
+ * passing.
+ */
+describe('http tier: a request by hostname goes through the pinned dispatcher (issue #207)', () => {
+  let server: Server;
+  let port: number;
+  let hits = 0;
+
+  beforeAll(async () => {
+    server = createServer((_req, res) => {
+      hits++;
+      res.writeHead(200, { 'content-type': 'text/html' });
+      res.end('<html><body>pinned ok</body></html>');
+    });
+    await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve));
+    port = (server.address() as { port: number }).port;
+  });
+
+  afterAll(async () => {
+    await new Promise<void>((resolve) => server.close(() => resolve()));
+  });
+
+  it('fetches successfully by name, not by IP literal', async () => {
+    const res = await httpFetch(`http://localhost:${port}/`);
+    expect(res.statusCode).toBe(200);
+    expect(res.html).toContain('pinned ok');
+    expect(hits).toBeGreaterThan(0);
+  });
+
+  it('still fetches by IP literal, where no pinning applies', async () => {
+    const before = hits;
+    const res = await httpFetch(`http://127.0.0.1:${port}/`);
+    expect(res.statusCode).toBe(200);
+    expect(hits).toBe(before + 1);
+  });
+});
diff --git a/tests/fetch/pinned-rebind.test.ts b/tests/fetch/pinned-rebind.test.ts
new file mode 100755
index 000000000..0e6853ffc
--- /dev/null
+++ b/tests/fetch/pinned-rebind.test.ts
@@ -0,0 +1,96 @@
+import { createServer, type Server } from 'node:http';
+import { Agent, request } from 'undici';
+import { afterAll, beforeAll, describe, expect, it } from 'vitest';
+import { createPinnedLookup } from '../../src/fetch/pinned-dispatcher.js';
+
+/**
+ * The discriminating test: does pinning actually change which host the socket dials?
+ *
+ * Two servers on the same port, different loopback addresses — 127.0.0.1 is the address SSRF
+ * validation cleared, 127.0.0.2 stands in for the address an attacker's resolver flips to inside
+ * the TTL window. A "rebinding" lookup always answers 127.0.0.2.
+ *
+ *   - through a plain Agent using that lookup, the request lands on the ATTACKER server
+ *   - through a pinned Agent, it lands on the VALIDATED one
+ *
+ * That is DNS rebinding reproduced over a real socket, and it is the assertion that fails if the
+ * pin is ever removed or wired wrong. The `expect(attacker.hits)` line is the control: without it
+ * a broken pin that simply never connected would look the same as a working one.
+ */
+describe('pinning changes which address the socket dials (issue #207)', () => {
+  const VALIDATED_IP = '127.0.0.1';
+  const ATTACKER_IP = '127.0.0.2';
+  let validated: Server;
+  let attacker: Server;
+  let port = 0;
+  const hits = { validated: 0, attacker: 0 };
+  let bothBound = false;
+
+  beforeAll(async () => {
+    validated = createServer((_q, r) => {
+      hits.validated++;
+      r.writeHead(200, { 'content-type': 'text/plain' });
+      r.end('validated');
+    });
+    attacker = createServer((_q, r) => {
+      hits.attacker++;
+      r.writeHead(200, { 'content-type': 'text/plain' });
+      r.end('attacker');
+    });
+    await new Promise<void>((res) => validated.listen(0, VALIDATED_IP, res));
+    port = (validated.address() as { port: number }).port;
+    // 127.0.0.2 is bindable on Linux and Windows; if a platform refuses it, skip rather than
+    // report a pass we did not actually get.
+    bothBound = await new Promise<boolean>((res) => {
+      attacker.once('error', () => res(false));
+      attacker.listen(port, ATTACKER_IP, () => res(true));
+    });
+  });
+
+  afterAll(async () => {
+    await new Promise<void>((res) => validated.close(() => res()));
+    if (bothBound) await new Promise<void>((res) => attacker.close(() => res()));
+  });
+
+  /** A resolver that always answers with the attacker's address — the second, hostile resolution. */
+  const rebindingLookup = ((
+    _host: string,
+    options: { all?: boolean },
+    cb: (...a: unknown[]) => void,
+  ) => {
+    if (options?.all) cb(null, [{ address: ATTACKER_IP, family: 4 }]);
+    else cb(null, ATTACKER_IP, 4);
+  }) as never;
+
+  it('WITHOUT pinning the rebinding resolver wins — the attacker server is reached', async (ctx) => {
+    // Skip VISIBLY where 127.0.0.2 will not bind. A silent `return` here would report a pass
+    // having asserted nothing, which is the failure mode this whole test exists to prevent.
+    if (!bothBound) ctx.skip();
+    const agent = new Agent({ connect: { lookup: rebindingLookup } as never });
+    const res = await request(`http://pinned.test:${port}/`, { dispatcher: agent });
+    const body = await res.body.text();
+    expect(body).toBe('attacker');
+    expect(hits.attacker).toBeGreaterThan(0);
+    await agent.close();
+  });
+
+  it('WITH pinning the validated address wins, even though DNS would say otherwise', async (ctx) => {
+    if (!bothBound) ctx.skip();
+    const before = { ...hits };
+    const agent = new Agent({
+      connect: {
+        lookup: createPinnedLookup(
+          'pinned.test',
+          [{ address: VALIDATED_IP, family: 4 }],
+          rebindingLookup,
+        ),
+      } as never,
+    });
+    const res = await request(`http://pinned.test:${port}/`, { dispatcher: agent });
+    const body = await res.body.text();
+    expect(body).toBe('validated');
+    expect(hits.validated).toBe(before.validated + 1);
+    expect(hits.attacker).toBe(before.attacker); // the attacker got nothing
+    await agent.close();
+  });
+});

From e02edf1a52dcd3e917875bc6fb56dcdf9eab561c Mon Sep 17 00:00:00 2001
From: Conner Webber <39662153+spartan8806@users.noreply.github.com>
Date: Fri, 21 Aug 2026 19:45:33 -0500
Subject: [PATCH 2/6] docs: document the two functions this diff touches

CodeRabbit scopes docstring coverage to functions the diff touches and flagged 75%
against an 80% threshold.

fetchWithRedirects gets the explanation the loop deserves: why redirect: 'manual'
is deliberate (an automatic redirect would connect to the next host without the
SSRF checks seeing it), and why consuming the body here is what makes closing the
per-hop dispatchers in the finally safe.

callLookup notes that the hook is callback-shaped with two arities and that both
are asserted per test rather than normalised away, since picking the wrong arity
is itself a way the pin could break.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 src/fetch/http-client.ts              | 11 +++++++++++
 tests/fetch/pinned-dispatcher.test.ts |  7 +++++++
 2 files changed, 18 insertions(+)

diff --git a/src/fetch/http-client.ts b/src/fetch/http-client.ts
index 1cbdee960..5ad14f4c0 100644
--- a/src/fetch/http-client.ts
+++ b/src/fetch/http-client.ts
@@ -143,6 +143,17 @@ class HttpFetchError extends Error {
   }
 }
 
+/**
+ * Drive one fetch to completion, following redirects manually so every hop can be re-guarded.
+ *
+ * `redirect: 'manual'` is deliberate: an automatic redirect would connect to the next host without
+ * the SSRF checks below ever seeing it. Each hop therefore re-runs the literal-URL guard, the
+ * resolved-IP guard, and — since #207 — pins the socket to the addresses that guard just cleared,
+ * so the connection cannot be re-resolved onto a different address after validation.
+ *
+ * The response body is fully consumed here (text, or buffered for PDFs), which is what makes it
+ * safe to close the per-hop dispatchers in the `finally`.
+ */
 async function fetchWithRedirects(
   originalUrl: string,
   options: HttpFetchOptions,
diff --git a/tests/fetch/pinned-dispatcher.test.ts b/tests/fetch/pinned-dispatcher.test.ts
index d3fe97a15..9533d24a1 100755
--- a/tests/fetch/pinned-dispatcher.test.ts
+++ b/tests/fetch/pinned-dispatcher.test.ts
@@ -16,6 +16,13 @@ function rebindingResolver(addr: string, family = 4) {
   }) as never;
 }
 
+/**
+ * Promisify one call to the lookup hook.
+ *
+ * The hook is callback-shaped with two arities — `(err, address, family)` normally and
+ * `(err, addresses[])` under `all: true` — so both are captured and asserted on per test rather
+ * than normalised away here, since picking the wrong arity is itself a way the pin could break.
+ */
 function callLookup(
   fn: ReturnType<typeof createPinnedLookup>,
   host: string,

From 159ccbaf85a055ce25cadbb1411de29ce07780a4 Mon Sep 17 00:00:00 2001
From: Conner Webber <39662153+spartan8806@users.noreply.github.com>
Date: Fri, 21 Aug 2026 20:20:30 -0500
Subject: [PATCH 3/6] fix(ssrf): fail closed when there is nothing to pin to
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Raised by CodeRabbit on the PR and confirmed against the code before acting on it.

guardResolvedHost reports ok with NO addresses when the host did not resolve, and
the old reasoning was that an unresolvable host has no IP to connect to, so it is
not a bypass. That only holds if both lookups get the same answer. They are two
separate DNS queries, so an attacker controlling the authority can answer the
validation query with NXDOMAIN or an empty set and the connect query with a
private address. The caller then skipped pinning and let fetch re-resolve, which
reinstates exactly the rebinding path this branch exists to close.

The HTTP tier now refuses to connect when validation produced no addresses, and
the guardResolvedHost contract says outright that connection callers must fail
closed rather than describing the unresolved case as safe.

Marked retryable: at crawl scale a transient resolver blip is far more common
than an attack and the retry budget is bounded. One line to flip if you would
rather fail fast on bad hostnames.

pinned-failclosed.test.ts covers it, with a must-pass control alongside — a change
that refused every hostname would satisfy the refusal assertion on its own and
look correct.

Verified no regression rather than assuming: the wider suite has 21 pre-existing
failures on pristine main (repl/shell, TUI, plugins, research, skills — nothing
touching fetch or SSRF) and the same 21 with this branch applied. A 22nd in one
run was VerifyScreen.test.tsx, which passes 5/5 in isolation and did not recur.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 src/fetch/http-client.ts              | 21 +++++++++++---
 src/watch/ssrf.ts                     | 10 +++++--
 tests/fetch/pinned-failclosed.test.ts | 42 +++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 6 deletions(-)
 create mode 100755 tests/fetch/pinned-failclosed.test.ts

diff --git a/src/fetch/http-client.ts b/src/fetch/http-client.ts
index 5ad14f4c0..1c9912755 100644
--- a/src/fetch/http-client.ts
+++ b/src/fetch/http-client.ts
@@ -195,15 +195,28 @@ async function fetchWithRedirects(
         if (!resolved.ok) {
           throw new HttpFetchError(resolved.reason, false);
         }
+        // FAIL CLOSED. `guardResolvedHost` reports ok with no addresses when the host did not
+        // resolve, and the old reasoning was that there is then no IP to connect to. That holds
+        // only if both lookups get the same answer. They are two separate queries, so an attacker
+        // who controls the authority can answer the validation query with NXDOMAIN or an empty
+        // set and the connect query with a private address — which would sail through here
+        // unpinned and reinstate the rebinding path this change exists to close.
+        //
+        // Retryable, because at crawl scale a transient resolver blip is far more common than an
+        // attack, and the retry budget is bounded. Flip to `false` for fail-fast on bad hostnames.
+        if (!resolved.addresses?.length) {
+          throw new HttpFetchError(
+            `Could not resolve ${rhost} to a validated address; refusing to connect without pinning`,
+            true,
+          );
+        }
         // PIN the socket to what we just validated. Without this the connection resolves DNS a
         // second time, and an attacker controlling the resolver can answer with a public IP for
         // the check above and a private one for the connect (DNS rebinding). Pinning removes the
         // second resolution, so there is no window to race. Literal IPs need no pinning — there
         // is no name to re-resolve.
-        if (resolved.addresses?.length) {
-          pinnedAgent = createPinnedAgent(rhost, resolved.addresses);
-          agents.push(pinnedAgent);
-        }
+        pinnedAgent = createPinnedAgent(rhost, resolved.addresses);
+        agents.push(pinnedAgent);
       }
     }
 
diff --git a/src/watch/ssrf.ts b/src/watch/ssrf.ts
index 9da79d451..af979d72e 100644
--- a/src/watch/ssrf.ts
+++ b/src/watch/ssrf.ts
@@ -425,8 +425,14 @@ function isResolvedLoopback(address: string): boolean {
  * Fetch-time SSRF re-check for the plain fetch/crawl policy. Resolves the host and runs EVERY
  * resolved address back through `guardFetchUrl` (as an `http://<ip>/` literal), so the resolved-IP
  * policy is identical to the literal-IP one (metadata/RFC-1918 blocked, loopback allowed for the
- * local-dev promise). A host that does not resolve is NOT a bypass — there is no IP to connect to,
- * so we fall through (`ok: true`) and let the real fetch surface the natural DNS error.
+ * local-dev promise).
+ *
+ * A host that does not resolve returns `ok: true` with NO `addresses`. That is not an allow —
+ * it means there was nothing to validate, and therefore nothing to pin to. **Callers that go on to
+ * open a connection MUST fail closed in that case.** The lookup done here and the lookup done by
+ * the socket are two separate queries, so an attacker controlling the authority can answer this
+ * one with NXDOMAIN or an empty set and the connect one with a private address; proceeding
+ * unpinned would hand them exactly the rebinding window pinning is meant to remove.
  */
 export async function guardResolvedHost(
   hostname: string,
diff --git a/tests/fetch/pinned-failclosed.test.ts b/tests/fetch/pinned-failclosed.test.ts
new file mode 100755
index 000000000..043679df9
--- /dev/null
+++ b/tests/fetch/pinned-failclosed.test.ts
@@ -0,0 +1,42 @@
+import { createServer, type Server } from 'node:http';
+import { afterAll, beforeAll, describe, expect, it } from 'vitest';
+import { httpFetch } from '../../src/fetch/http-client.js';
+
+/**
+ * The must-reject control for the pin.
+ *
+ * `guardResolvedHost` reports ok with no addresses when a host does not resolve. Before this
+ * change the caller shrugged and let `fetch` re-resolve, on the reasoning that an unresolvable
+ * host has no IP to connect to. That only holds if both lookups get the same answer, and they are
+ * two separate queries — an attacker controlling the authority can answer the validation query
+ * with nothing and the connect query with 169.254.169.254.
+ *
+ * `.invalid` is reserved by RFC 6761 and guaranteed never to resolve, so it stands in for the
+ * "validation produced nothing" case without needing a hostile resolver.
+ */
+describe('fail closed when there is nothing to pin to', () => {
+  let server: Server;
+  let port: number;
+
+  beforeAll(async () => {
+    server = createServer((_q, r) => r.end('should not be reached'));
+    await new Promise<void>((res) => server.listen(0, '127.0.0.1', () => res()));
+    port = (server.address() as { port: number }).port;
+  });
+
+  afterAll(async () => {
+    await new Promise<void>((res) => server.close(() => res()));
+  });
+
+  it('refuses a hostname that produced no validated addresses', async () => {
+    await expect(httpFetch('http://nothing-resolves-here.invalid/')).rejects.toThrow(
+      /refusing to connect without pinning/i,
+    );
+  }, 30000);
+
+  it('MUST-PASS control: a hostname that does resolve is still fetched normally', async () => {
+    // Without this, a change that refused *everything* would pass the test above and look correct.
+    const res = await httpFetch(`http://localhost:${port}/`);
+    expect(res.statusCode).toBe(200);
+  }, 30000);
+});

From f406923a7605bf030285ab410bff20a7bbafb959 Mon Sep 17 00:00:00 2001
From: Conner Webber <39662153+spartan8806@users.noreply.github.com>
Date: Fri, 21 Aug 2026 20:30:04 -0500
Subject: [PATCH 4/6] fix(ssrf): scope the pinned dispatcher to a single
 redirect hop

Raised by CodeRabbit and confirmed: pinnedAgent was declared outside the redirect
loop and the !isIpLiteral branch is the only place it is assigned, so a named hop
followed by an IP-literal hop reused the previous hop's Agent. The agents array
stays outside the loop; it exists only for cleanup at the end.

On impact, honestly: this was contained rather than exploitable. createPinnedLookup
defers to real DNS whenever the host it is asked for is not the host it was built
for, so the inherited Agent behaved like a plain one. That fallthrough was written
as a guard against exactly this kind of future misuse and it did its job. Still
wrong, and a hop should not depend on the previous hop's dispatcher being harmless.

pinned-redirect.test.ts covers both mixed orders. It is a regression guard rather
than a discriminating test and says so in the file: the buggy version passes it
too, for the fallthrough reason above. Nothing else in the suite exercised a mixed
named/literal chain.

No regression: tests/watch + tests/unit is 21 failures on pristine main and 21
with this branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 src/fetch/http-client.ts            | 10 +++--
 tests/fetch/pinned-redirect.test.ts | 60 +++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 4 deletions(-)
 create mode 100755 tests/fetch/pinned-redirect.test.ts

diff --git a/src/fetch/http-client.ts b/src/fetch/http-client.ts
index 1c9912755..1b74fa3ed 100644
--- a/src/fetch/http-client.ts
+++ b/src/fetch/http-client.ts
@@ -165,14 +165,16 @@ async function fetchWithRedirects(
   const visited = new Set<string>();
   let currentUrl = originalUrl;
   let redirectCount = 0;
-  // One Agent per hop, each holding a connection pool. They are closed in the finally below once
-  // the body has been consumed — an Agent left open is a socket leak, and the body is read inside
-  // this function, so closing on the way out is safe.
+  // Every Agent created across the redirect chain, kept only so the finally below can close them
+  // once the body has been consumed. An Agent left open is a socket leak, and the body is read
+  // inside this function, so closing on the way out is safe.
   const agents: Agent[] = [];
-  let pinnedAgent: Agent | undefined;
 
   try {
   while (true) {
+    // Per HOP, not per request: an IP-literal hop takes no pin, and leaving the previous hop's
+    // Agent in scope would send that request through a dispatcher built for a different host.
+    let pinnedAgent: Agent | undefined;
     if (visited.has(currentUrl)) {
       throw new HttpFetchError(`Redirect loop detected at ${currentUrl}`, false);
     }
diff --git a/tests/fetch/pinned-redirect.test.ts b/tests/fetch/pinned-redirect.test.ts
new file mode 100755
index 000000000..f716fa7ba
--- /dev/null
+++ b/tests/fetch/pinned-redirect.test.ts
@@ -0,0 +1,60 @@
+import { createServer, type Server } from 'node:http';
+import { afterAll, beforeAll, describe, expect, it } from 'vitest';
+import { httpFetch } from '../../src/fetch/http-client.js';
+
+/**
+ * Redirect chains that mix a named hop with an IP-literal hop.
+ *
+ * A named hop takes a pinned dispatcher; an IP-literal hop takes none. Before the per-hop fix the
+ * dispatcher was declared outside the loop, so the literal hop inherited the previous hop's Agent.
+ *
+ * Be straight about what this test is: a REGRESSION GUARD, not a discriminating test. The buggy
+ * version passes it too, because `createPinnedLookup` defers to real DNS whenever the host it is
+ * asked for is not the host it was built for — so the inherited Agent behaved like a plain one and
+ * produced the right answer by accident. That fallthrough is what kept this a correctness problem
+ * rather than a routing one. The test is here so the mixed chain stays exercised, since nothing
+ * else in the suite covers it.
+ */
+describe('redirect chains mixing named and IP-literal hops', () => {
+  let server: Server;
+  let port: number;
+  const seen: string[] = [];
+
+  beforeAll(async () => {
+    server = createServer((req, res) => {
+      seen.push(req.url ?? '');
+      if (req.url === '/to-literal') {
+        res.writeHead(302, { location: `http://127.0.0.1:${port}/done` });
+        res.end();
+        return;
+      }
+      if (req.url === '/to-name') {
+        res.writeHead(302, { location: `http://localhost:${port}/done` });
+        res.end();
+        return;
+      }
+      res.writeHead(200, { 'content-type': 'text/html' });
+      res.end('<html><body>arrived</body></html>');
+    });
+    await new Promise<void>((res) => server.listen(0, '127.0.0.1', () => res()));
+    port = (server.address() as { port: number }).port;
+  });
+
+  afterAll(async () => {
+    await new Promise<void>((res) => server.close(() => res()));
+  });
+
+  it('name -> IP literal: the literal hop must not inherit the named hop dispatcher', async () => {
+    const res = await httpFetch(`http://localhost:${port}/to-literal`);
+    expect(res.statusCode).toBe(200);
+    expect(res.html).toContain('arrived');
+    expect(res.finalUrl).toBe(`http://127.0.0.1:${port}/done`);
+  }, 30000);
+
+  it('IP literal -> name: the named hop still gets its own pin', async () => {
+    const res = await httpFetch(`http://127.0.0.1:${port}/to-name`);
+    expect(res.statusCode).toBe(200);
+    expect(res.html).toContain('arrived');
+    expect(res.finalUrl).toBe(`http://localhost:${port}/done`);
+  }, 30000);
+});

From 2850a5d51b5a9ae9d06a873e5bd8008d5c787066 Mon Sep 17 00:00:00 2001
From: Conner Webber <39662153+spartan8806@users.noreply.github.com>
Date: Fri, 21 Aug 2026 20:52:07 -0500
Subject: [PATCH 5/6] fix(ssrf): release the response body on paths that
 abandon it
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Raised by CodeRabbit. Real, and a regression from this branch — but measured rather
than assumed, and it is a stall rather than a hang.

The redirect branch reads Location and continues without touching the body. That
was untidy before; it became costly once the per-hop Agents started being closed on
the way out, because Agent.close() waits for in-flight requests and an unread body
keeps one in flight. That wait sits in the finally, outside the AbortSignal that
bounds the request.

Measured with a 302 whose body never ends: httpFetch returned in 2038ms against
timeoutMs=2000. So it is bounded by the per-hop signal, not unbounded — but a
server can impose a full timeoutMs on every redirect, silently.

- cancel the body at the top of the redirect branch, above the no-location and
  too-many-redirects throws so those release it too
- same for the retryable-status branch, which also throws without reading
- cleanup now uses destroy() rather than close(). By then the body has been read in
  full or deliberately cancelled, so there is nothing worth waiting for, and a path
  missed in future cannot cost latency

pinned-redirect-body.test.ts discriminates: 2038ms before, 97ms after, asserted
against timeoutMs so it fails if the stall returns.

No regression: 21 failures on pristine main, 21 with this branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 src/fetch/http-client.ts                 | 23 ++++++++-
 tests/fetch/pinned-redirect-body.test.ts | 65 ++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 2 deletions(-)
 create mode 100755 tests/fetch/pinned-redirect-body.test.ts

diff --git a/src/fetch/http-client.ts b/src/fetch/http-client.ts
index 1b74fa3ed..86cca9b02 100644
--- a/src/fetch/http-client.ts
+++ b/src/fetch/http-client.ts
@@ -284,6 +284,15 @@ async function fetchWithRedirects(
     }
 
     if (REDIRECT_STATUSES.has(response.status)) {
+      // Let go of the body before doing anything else with this hop. We never read a redirect
+      // body, and an unread one keeps the request in flight, which stalls the Agent cleanup in
+      // the `finally` for a full timeoutMs. Placed above the error branches too, so the
+      // no-location and too-many-redirects throws release it as well.
+      try {
+        await response.body?.cancel();
+      } catch {
+        /* already closed, or never had a body */
+      }
       const location = response.headers.get('location');
       if (!location) {
         throw new HttpFetchError(`Redirect with no location header at ${currentUrl}`, false);
@@ -311,6 +320,12 @@ async function fetchWithRedirects(
     }
 
     if (RETRYABLE_STATUSES.has(response.status)) {
+      // Same reasoning as the redirect branch: this throws without reading the body.
+      try {
+        await response.body?.cancel();
+      } catch {
+        /* already closed, or never had a body */
+      }
       throw new HttpFetchError(`HTTP ${response.status} from ${currentUrl}`, true);
     }
 
@@ -363,9 +378,13 @@ async function fetchWithRedirects(
   } finally {
     for (const a of agents) {
       try {
-        await a.close();
+        // destroy(), not close(): close() waits for in-flight requests, so a body we failed to
+        // release anywhere above would stall this cleanup for a full timeoutMs. By the time we
+        // reach here the body has either been read in full or deliberately cancelled, so there is
+        // nothing left worth waiting for, and this way a missed path cannot cost latency.
+        await a.destroy();
       } catch {
-        /* closing a pool must never mask the real result */
+        /* tearing down a pool must never mask the real result */
       }
     }
   }
diff --git a/tests/fetch/pinned-redirect-body.test.ts b/tests/fetch/pinned-redirect-body.test.ts
new file mode 100755
index 000000000..7208a3d30
--- /dev/null
+++ b/tests/fetch/pinned-redirect-body.test.ts
@@ -0,0 +1,65 @@
+import { createServer, type Server } from 'node:http';
+import { afterAll, beforeAll, describe, expect, it } from 'vitest';
+import { httpFetch } from '../../src/fetch/http-client.js';
+
+/**
+ * A redirect whose body never ends must not be able to hang the fetch.
+ *
+ * The redirect path reads `location` and continues without consuming the response body. That was
+ * untidy before this branch; it became a hang once the per-hop Agents started being closed on the
+ * way out, because `Agent.close()` waits for in-flight requests and an undrained body keeps one in
+ * flight forever. The wait happens in the `finally`, outside the AbortSignal that bounds the
+ * request, so `timeoutMs` cannot rescue it.
+ *
+ * The server here sends a 302 with a valid Location and then writes forever without ending. If the
+ * body is not cancelled before the hop advances, this test times out instead of asserting.
+ */
+describe('a never-ending redirect body must not hang the fetch', () => {
+  let server: Server;
+  let port: number;
+  let stop: (() => void) | undefined;
+
+  beforeAll(async () => {
+    server = createServer((req, res) => {
+      if (req.url === '/redir') {
+        res.writeHead(302, {
+          location: `http://127.0.0.1:${port}/done`,
+          'content-type': 'text/plain',
+        });
+        // Never call res.end(). Keep the body open so the request stays in flight.
+        const t = setInterval(() => {
+          try {
+            res.write('.'.repeat(1024));
+          } catch {
+            /* client went away */
+          }
+        }, 10);
+        stop = () => clearInterval(t);
+        res.on('close', () => clearInterval(t));
+        return;
+      }
+      res.writeHead(200, { 'content-type': 'text/html' });
+      res.end('<html><body>done</body></html>');
+    });
+    await new Promise<void>((res) => server.listen(0, '127.0.0.1', () => res()));
+    port = (server.address() as { port: number }).port;
+  });
+
+  afterAll(async () => {
+    stop?.();
+    await new Promise<void>((res) => server.close(() => res()));
+  });
+
+  it('completes rather than hanging on the undrained redirect body', async () => {
+    const started = Date.now();
+    const res = await httpFetch(`http://localhost:${port}/redir`, { timeoutMs: 2000 });
+    const elapsed = Date.now() - started;
+    expect(res.statusCode).toBe(200);
+    expect(res.html).toContain('done');
+    // With the body cancelled the hop costs nothing. If it is left undrained the call is held up
+    // until the per-hop AbortSignal fires, so elapsed tracks timeoutMs instead.
+    console.log(`   elapsed ${elapsed}ms for timeoutMs=2000`);
+    expect(elapsed).toBeLessThan(1500);
+  }, 20000);
+});
+

From 666906eb72a3b22a7836f8372b686165ba5f2f08 Mon Sep 17 00:00:00 2001
From: Conner Webber <39662153+spartan8806@users.noreply.github.com>
Date: Sat, 22 Aug 2026 17:27:13 -0500
Subject: [PATCH 6/6] test: make the redirect-body test actually test the
 cancel
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

CodeRabbit was right and I verified it by deleting the fix: with response.body
.cancel() removed and only Agent.destroy() left, the old test still passed, in
202ms. destroy() does not wait for in-flight requests, so an elapsed-time
assertion alone proves the teardown works, not that the body was released.

/done is now gated on the redirect response's close event, so the second hop
cannot complete until the first hop's body is actually let go. Measured both
ways: fails at 2041ms without the cancel, passes at 95ms with it.

Worth having because the two halves of the fix are separable — someone could
reasonably decide destroy() alone is enough and drop the cancel, losing the
graceful connection release, with nothing to catch it.

fix(deps): declare the Node floor undici actually requires, and sync the lock

Also CodeRabbit, also correct. Two real inconsistencies I introduced:

- undici 7.28.0 declares engines node >=20.18.1 while this project declared
  >=20, so the two disagreed
- package.json listed undici but the lockfile root entry did not, so the
  manifest and the lock were out of sync

engines bumped to >=20.18.1 and the lock regenerated (--package-lock-only, 6
lines).

⚠ THIS RAISES THE RUNTIME NODE FLOOR AND THAT IS THE MAINTAINER'S CALL. undici
was previously DEV-ONLY here — dev: true in the lock, pulled in by
@yao-pkg/pkg-fetch, a devDependency — so it imposed nothing on users at runtime.
Making it a runtime dependency moves the real floor from 20.0.0 to 20.18.1.
Flagged on the PR with the alternative (node:https + a lookup option, no new
dependency, but a rewrite of the request path) so it can be rejected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 package-lock.json                        |  6 +--
 package.json                             |  2 +-
 tests/fetch/pinned-redirect-body.test.ts | 65 ++++++++++++++++--------
 3 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/package-lock.json b/package-lock.json
index 755e3cb87..01575bd55 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -36,7 +36,8 @@
         "react": "^18.3.1",
         "sqlite-vec": "^0.1.9",
         "tinyld": "^1.3.4",
-        "turndown": "^7.2.4"
+        "turndown": "^7.2.4",
+        "undici": "^7.28.0"
       },
       "bin": {
         "wigolo": "dist/index.js"
@@ -57,7 +58,7 @@
         "vitest": "^4.1.4"
       },
       "engines": {
-        "node": ">=20"
+        "node": ">=20.18.1"
       },
       "optionalDependencies": {
         "@napi-rs/keyring": "^1.3.0",
@@ -9842,7 +9843,6 @@
       "version": "7.28.0",
       "resolved": "https://registry.npmjs.org/undici/-/undici-7.28.0.tgz",
       "integrity": "sha512-cRZYrTDwWznlnRiPjggAGxZXanty6M8RV1ff8Wm4LWXBp7/IG8v5DnOm74DtUBp9OONpK75YlPnIjQqX0dBDtA==",
-      "dev": true,
       "license": "MIT",
       "engines": {
         "node": ">=20.18.1"
diff --git a/package.json b/package.json
index 84c456478..d72e02f43 100644
--- a/package.json
+++ b/package.json
@@ -94,7 +94,7 @@
     ]
   },
   "engines": {
-    "node": ">=20"
+    "node": ">=20.18.1"
   },
   "dependencies": {
     "@anthropic-ai/sdk": "^0.91.1",
diff --git a/tests/fetch/pinned-redirect-body.test.ts b/tests/fetch/pinned-redirect-body.test.ts
index 7208a3d30..97c8d3783 100755
--- a/tests/fetch/pinned-redirect-body.test.ts
+++ b/tests/fetch/pinned-redirect-body.test.ts
@@ -1,65 +1,86 @@
-import { createServer, type Server } from 'node:http';
+import { createServer, type Server } from 'node:http';
 import { afterAll, beforeAll, describe, expect, it } from 'vitest';
 import { httpFetch } from '../../src/fetch/http-client.js';
 
 /**
- * A redirect whose body never ends must not be able to hang the fetch.
+ * A redirect whose body never ends must not be able to hold up the fetch.
  *
  * The redirect path reads `location` and continues without consuming the response body. That was
- * untidy before this branch; it became a hang once the per-hop Agents started being closed on the
- * way out, because `Agent.close()` waits for in-flight requests and an undrained body keeps one in
- * flight forever. The wait happens in the `finally`, outside the AbortSignal that bounds the
- * request, so `timeoutMs` cannot rescue it.
+ * untidy before this branch; it became costly once the per-hop Agents started being torn down on
+ * the way out, because an unread body keeps the request in flight. Measured at the time: 2038ms
+ * against `timeoutMs=2000`, i.e. a full timeout added to every such redirect.
  *
- * The server here sends a 302 with a valid Location and then writes forever without ending. If the
- * body is not cancelled before the hop advances, this test times out instead of asserting.
+ * WHY /done IS GATED ON THE REDIRECT SOCKET CLOSING
+ * -------------------------------------------------
+ * An elapsed-time assertion alone does NOT test what it looks like it tests. The fix has two
+ * halves — `response.body.cancel()` on the redirect path, and `Agent.destroy()` rather than
+ * `close()` in the cleanup — and `destroy()` does not wait for in-flight requests. So with the
+ * cancel deleted and only `destroy()` left, the call still returns immediately and a naive timing
+ * test passes. Verified by deleting it: the test passed in 202ms with no cancel anywhere.
+ *
+ * Gating `/done` on the redirect response's `close` event makes the second hop *depend* on the
+ * first hop's body actually being released. With the cancel in place the socket closes at once and
+ * this finishes in milliseconds. Without it, nothing closes until the per-hop AbortSignal fires,
+ * so `/done` cannot answer and elapsed climbs to `timeoutMs` — the assertion fails, by design.
  */
-describe('a never-ending redirect body must not hang the fetch', () => {
+describe('a never-ending redirect body must not hold up the fetch', () => {
   let server: Server;
   let port: number;
-  let stop: (() => void) | undefined;
+  let stopWriting: (() => void) | undefined;
+  let onRedirectClosed: (() => void) | undefined;
+  let redirectClosed: Promise<void>;
 
   beforeAll(async () => {
+    redirectClosed = new Promise<void>((resolve) => {
+      onRedirectClosed = resolve;
+    });
+
     server = createServer((req, res) => {
       if (req.url === '/redir') {
         res.writeHead(302, {
           location: `http://127.0.0.1:${port}/done`,
           'content-type': 'text/plain',
         });
-        // Never call res.end(). Keep the body open so the request stays in flight.
-        const t = setInterval(() => {
+        // Never end this response. It stays in flight until the client lets go of the body.
+        const timer = setInterval(() => {
           try {
             res.write('.'.repeat(1024));
           } catch {
-            /* client went away */
+            /* peer gone */
           }
         }, 10);
-        stop = () => clearInterval(t);
-        res.on('close', () => clearInterval(t));
+        stopWriting = () => clearInterval(timer);
+        res.on('close', () => {
+          clearInterval(timer);
+          onRedirectClosed?.();
+        });
         return;
       }
-      res.writeHead(200, { 'content-type': 'text/html' });
-      res.end('<html><body>done</body></html>');
+
+      // The second hop only answers once the first hop's body has actually been released.
+      void redirectClosed.then(() => {
+        res.writeHead(200, { 'content-type': 'text/html' });
+        res.end('<html><body>done</body></html>');
+      });
     });
+
     await new Promise<void>((res) => server.listen(0, '127.0.0.1', () => res()));
     port = (server.address() as { port: number }).port;
   });
 
   afterAll(async () => {
-    stop?.();
+    stopWriting?.();
+    onRedirectClosed?.();
     await new Promise<void>((res) => server.close(() => res()));
   });
 
-  it('completes rather than hanging on the undrained redirect body', async () => {
+  it('releases the redirect body instead of leaving it in flight', async () => {
     const started = Date.now();
     const res = await httpFetch(`http://localhost:${port}/redir`, { timeoutMs: 2000 });
     const elapsed = Date.now() - started;
     expect(res.statusCode).toBe(200);
     expect(res.html).toContain('done');
-    // With the body cancelled the hop costs nothing. If it is left undrained the call is held up
-    // until the per-hop AbortSignal fires, so elapsed tracks timeoutMs instead.
     console.log(`   elapsed ${elapsed}ms for timeoutMs=2000`);
     expect(elapsed).toBeLessThan(1500);
   }, 20000);
 });
-