fix(ssrf): pin the socket to the validated IP (HTTP tier, #207 phase 1)

package.json

@@ -94,7 +94,7 @@
     ]
   },
   "engines": {
-    "node": ">=20"
+    "node": ">=20.18.1"
   },
   "dependencies": {
     "@anthropic-ai/sdk": "^0.91.1",
@@ -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",

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>;
@@ -141,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,
@@ -152,8 +165,16 @@ async function fetchWithRedirects(
   const visited = new Set<string>();
   let currentUrl = originalUrl;
   let redirectCount = 0;
+  // 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[] = [];
 
+  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);
     }
@@ -176,6 +197,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.
+        pinnedAgent = createPinnedAgent(rhost, resolved.addresses);
+        agents.push(pinnedAgent);
       }
     }
 
@@ -210,6 +253,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';
@@ -238,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);
@@ -265,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);
     }
 
@@ -314,4 +375,17 @@ async function fetchWithRedirects(
       rawBuffer,
     };
   }
+  } finally {
+    for (const a of agents) {
+      try {
+        // 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 {
+        /* tearing down a pool must never mask the real result */
+      }
+    }
+  }
 }

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 });
+}

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(
@@ -417,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,
@@ -437,7 +451,7 @@ export async function guardResolvedHost(
       };
     }
   }
-  return { ok: true };
+  return { ok: true, addresses };
 }
 
 /**
@@ -475,5 +489,5 @@ export async function guardResolvedServeTarget(
       };
     }
   }
-  return { ok: true };
+  return { ok: true, addresses };
 }