fix(daemon): block SSRF in library ingest remote fetch (#5529)

* fix(daemon): block SSRF in library ingest remote fetch

POST /api/library/ingest fetches a client-supplied URL via
`fetchRemoteBytes`, which used a raw `fetch(url, { redirect: 'follow' })`
with no host validation. The sibling brand-asset path already routes
every external fetch through `assertPublicBrandUrl` (rejects
cloud-metadata 169.254.169.254, loopback, RFC1918/CGNAT, link-local, and
re-validates each redirect hop with redirect:'manual') — ingest just
missed it.

That let any local caller make the privileged daemon issue GET requests
to arbitrary internal services (127.0.0.1:*, 169.254.169.254, RFC1918)
and read the response bytes back out through the unauthenticated
`GET /api/library/assets/:id/raw` route — SSRF with full response
exfiltration (e.g. cloud-metadata IAM credentials). The ingest auth gate
is satisfied by a `chrome-extension://` Origin header, which a
non-browser local process (or a prompt-injected agent — this product
spawns external code-agent CLIs on untrusted content) sets trivially, so
no token is required. Same class as #5478.

Route `fetchRemoteBytes` through `fetchExternalBrandAsset` so the guard
and per-hop redirect re-validation apply. Drop-in; the size cap and mime
handling are unchanged.

Regression test drives a real daemon + production HTTP API against a
loopback canary standing in for an internal metadata service: pre-fix the
daemon fetched the canary and the secret was read back via /raw; post-fix
the internal URL is refused and never touched.

* fix(daemon): pin brand-asset SSRF guard to the connection (DNS rebinding)

Addresses the review blocker on the library-ingest SSRF fix:
`assertPublicBrandUrl` validated the host with its own DNS lookup, but
the subsequent `fetch` re-resolved the name independently. An
attacker-controlled hostname could answer a public address for the
validation and a private one for the actual connection (DNS rebinding /
TOCTOU), so the ingest route — which persists the response and exposes it
through /raw — stayed bypassable.

Pin the validation to the socket: `fetchExternalBrandAsset` now installs
a connection-time `lookup` (createValidatingLookup) on an undici Agent
dispatcher that refuses the connection when the resolved address is
non-public, using the same `isNonPublicHost` predicate as the pre-check
so the two block sets can't drift. The address we validate IS the address
the socket connects to, and the same dispatcher re-binds every redirect
hop. Same pattern the repo already uses in plugins/plugin-asset-cache.ts.
`assertPublicBrandUrl` stays as a cheap URL-level pre-check.

This hardens the shared brand-fetch guard, so both the brand-extraction
callers and the library-ingest path are covered. Adds a rebinding
regression test exercising the pinned connect-time lookup directly.

* fix(daemon): reuse a long-lived guarded dispatcher (do not close per-request)

Review follow-up: the per-request `Agent` was closed in a `finally` right
after `fetchExternalBrandAsset` returned the response. Closing an undici
Agent waits for its in-flight request, which only completes once the
caller consumes the body — so the successful path could not complete
(the caller reads `arrayBuffer()` after the function returns).

Use a single module-level `brandAssetDispatcher` reused across calls
(same lifetime pattern as plugins/plugin-asset-cache.ts) and never close
it per-request. The pinned validating lookup still refuses non-public
resolved addresses on every connection. Dropped the now-unused
per-call `lookupImpl` param; the rebinding test drives
`createValidatingLookup` directly.

---------

Co-authored-by: tomsen-ai <230283659+tomsen-ai@users.noreply.github.com>

apps/daemon/src/brands/safe-fetch.ts

@@ -16,11 +16,18 @@
 // public-to-public redirects (http->https, apex->www, CDN hops) still work while
 // a public host that 3xx's into private space is refused.
 //
-// Residual: DNS rebinding (a name that resolves public here but private when the
-// platform fetch re-resolves) is not defeated without a pinned dispatcher — the
-// same limitation the codebase's other check-then-fetch SSRF guards carry.
+// DNS rebinding (a name that resolves public during validation but private when
+// the platform fetch re-resolves for the socket) is defeated by pinning the
+// validation to the connection: `fetchExternalBrandAsset` installs a
+// connection-time `lookup` (see `createValidatingLookup`) that rejects the
+// socket if the resolved address is non-public, so the address we validate IS
+// the address we connect to — no separate pre-validation lookup to rebind
+// around. Same pattern as `plugins/plugin-asset-cache.ts`.
 
-import { promises as dnsPromises } from 'node:dns';
+import { promises as dnsPromises, lookup as dnsLookupCb } from 'node:dns';
+import type { LookupFunction } from 'node:net';
+
+import { Agent } from 'undici';
 
 import {
   isBlockedExternalApiHostname,
@@ -79,8 +86,62 @@ export async function assertPublicBrandUrl(url: string): Promise<void> {
   }
 }
 
+type DnsLookupCb = typeof dnsLookupCb;
+
+/**
+ * Wrap a `dns.lookup`-shaped resolver so the resolved address is rejected when
+ * it is non-public. Installed as the undici Agent's connection-time `lookup`, so
+ * the address we validate IS the one the socket connects to — closing the
+ * DNS-rebinding / TOCTOU gap that a separate pre-validation lookup leaves open
+ * (an attacker-controlled name answering public for the check and private for
+ * the connect). Uses the same `isNonPublicHost` predicate as
+ * `assertPublicBrandUrl` so the connect-time and pre-check block sets can't
+ * drift. Exported so the guard can be unit-tested without a live server.
+ */
+export function createValidatingLookup(lookupImpl: DnsLookupCb = dnsLookupCb): LookupFunction {
+  const validatingLookup = (
+    hostname: string,
+    options: unknown,
+    callback?: (err: Error | null, address?: unknown, family?: number) => void,
+  ): void => {
+    const cb = (typeof options === 'function' ? options : callback) as (
+      err: Error | null,
+      address?: unknown,
+      family?: number,
+    ) => void;
+    const opts = (typeof options === 'function' ? {} : (options ?? {})) as Record<string, unknown>;
+    (lookupImpl as unknown as (
+      h: string,
+      o: unknown,
+      c: (e: Error | null, a?: unknown, f?: number) => void,
+    ) => void)(hostname, opts, (err, address, family) => {
+      if (err) return cb(err);
+      const list = Array.isArray(address) ? address : [{ address, family }];
+      for (const entry of list) {
+        const addr = typeof entry === 'string' ? entry : (entry as { address: string }).address;
+        if (isNonPublicHost(String(addr))) {
+          return cb(new Error(`brand asset host resolves to a non-public address: ${addr}`));
+        }
+      }
+      return cb(null, address, family);
+    });
+  };
+  return validatingLookup as unknown as LookupFunction;
+}
+
 const MAX_BRAND_REDIRECTS = 5;
 
+// Pin SSRF validation to the actual outbound connection: the Agent resolves each
+// host once at connect time and refuses the socket if that exact address is
+// non-public. This is what defeats DNS rebinding — there is no separate
+// pre-validation resolve to race. A single long-lived dispatcher (like
+// plugins/plugin-asset-cache.ts) is reused across calls: it must NOT be closed
+// per-request, since closing an undici Agent waits for the in-flight request,
+// which only finishes once the caller has consumed the returned response body.
+const brandAssetDispatcher = new Agent({
+  connect: { lookup: createValidatingLookup() },
+});
+
 export async function fetchExternalBrandAsset(
   url: string,
   init: RequestInit = {},
@@ -88,9 +149,15 @@ export async function fetchExternalBrandAsset(
   let target = url;
   for (let hop = 0; ; hop += 1) {
     // Re-validate every hop's host (initial URL and each redirect target) before
-    // the request, so a public site can't 3xx us into private space.
+    // the request, so a public site can't 3xx us into private space. The real
+    // SSRF stop is the pinned dispatcher below; this is a cheap URL-level
+    // pre-check (protocol, literal private IPs, redirect target).
     await assertPublicBrandUrl(target);
-    const res = await fetch(target, { ...init, redirect: 'manual' });
+    const res = await fetch(target, {
+      ...init,
+      redirect: 'manual',
+      dispatcher: brandAssetDispatcher as unknown as NonNullable<RequestInit['dispatcher']>,
+    });
     const location =
       res.status >= 300 && res.status < 400 ? res.headers.get('location') : null;
     if (!location) return res;
@@ -99,7 +166,8 @@ export async function fetchExternalBrandAsset(
     if (hop >= MAX_BRAND_REDIRECTS) {
       throw new Error(`too many brand asset redirects (> ${MAX_BRAND_REDIRECTS})`);
     }
-    // Resolve a possibly-relative Location; the next loop re-validates it.
+    // Resolve a possibly-relative Location; the next loop re-validates it and the
+    // same pinned dispatcher re-binds the new connection.
     target = new URL(location, target).toString();
   }
 }

apps/daemon/src/routes/library.ts

@@ -45,6 +45,7 @@ import {
   writeFigmaSidecar,
 } from '../library.js';
 import { reconcileLibrary, type ReconcileLibraryResult } from '../library-sync.js';
+import { fetchExternalBrandAsset } from '../brands/safe-fetch.js';
 import { ensureProjectSubdir } from '../projects.js';
 import {
   confirmPairing,
@@ -107,7 +108,14 @@ function parseDataUrl(dataUrl: string): { bytes: Buffer; mime: string | undefine
 }
 
 async function fetchRemoteBytes(url: string): Promise<{ bytes: Buffer; mime: string | undefined }> {
-  const resp = await fetch(url, { redirect: 'follow' });
+  // Route the client-supplied URL through the same SSRF guard the brand-asset
+  // path uses (assertPublicBrandUrl): reject cloud-metadata (169.254.169.254),
+  // loopback, RFC1918/CGNAT, and link-local hosts, re-validating on every
+  // redirect hop (redirect:'manual'). Without this a caller could make the
+  // privileged daemon fetch an internal/loopback URL and read the response back
+  // via GET /api/library/assets/:id/raw — SSRF + response exfiltration. Sibling
+  // to the loopback-SSRF class in #5478.
+  const resp = await fetchExternalBrandAsset(url);
   if (!resp.ok) throw new Error(`remote fetch failed: ${resp.status}`);
   const declared = Number(resp.headers.get('content-length') ?? '0');
   if (declared && declared > MAX_REMOTE_BYTES) throw new Error('remote resource too large');

apps/daemon/tests/brand-safe-fetch.test.ts

@@ -1,6 +1,6 @@
 import { afterEach, describe, expect, it, vi } from 'vitest';
 
-import { fetchExternalBrandAsset } from '../src/brands/safe-fetch.js';
+import { createValidatingLookup, fetchExternalBrandAsset } from '../src/brands/safe-fetch.js';
 
 // The brand-extraction fetchers pull a user-supplied site URL and then follow
 // sub-resource URLs (CSS/@font-face/img/favicon) discovered inside the fetched
@@ -72,3 +72,46 @@ describe('fetchExternalBrandAsset redirect handling', () => {
     expect(fetchMock).toHaveBeenCalledTimes(1);
   });
 });
+
+// DNS rebinding: an attacker-controlled name can answer public for the
+// validation lookup and private for the socket's own re-resolution. The pre-URL
+// check alone can't stop that, so the guard is pinned to the connection via a
+// validating `lookup` (createValidatingLookup) — the address it validates IS the
+// one the socket connects to. This exercises that connect-time lookup directly.
+describe('createValidatingLookup (DNS-rebinding pin)', () => {
+  const runLookup = (
+    resolved: Array<{ address: string; family: number }> | { address: string; family: number },
+    opts: Record<string, unknown> = { all: true },
+  ) =>
+    new Promise<{ err: Error | null; address?: unknown }>((resolve) => {
+      const impl = ((_h: string, _o: unknown, cb: (e: Error | null, a?: unknown, f?: number) => void) => {
+        if (Array.isArray(resolved)) cb(null, resolved);
+        else cb(null, resolved.address, resolved.family);
+      }) as unknown as Parameters<typeof createValidatingLookup>[0];
+      const lookup = createValidatingLookup(impl) as unknown as (
+        h: string,
+        o: unknown,
+        cb: (e: Error | null, a?: unknown) => void,
+      ) => void;
+      lookup('rebind.attacker.example', opts, (err, address) => resolve({ err, address }));
+    });
+
+  it('rejects the connection when the host resolves to a private/metadata address', async () => {
+    // The rebind: validation may have seen a public IP, but the socket's own
+    // resolution lands on cloud metadata. The pinned lookup refuses it.
+    const meta = await runLookup([{ address: '169.254.169.254', family: 4 }]);
+    expect(meta.err).toBeInstanceOf(Error);
+
+    const loopback = await runLookup({ address: '127.0.0.1', family: 4 }, {});
+    expect(loopback.err).toBeInstanceOf(Error);
+
+    const rfc1918 = await runLookup([{ address: '10.0.0.7', family: 4 }]);
+    expect(rfc1918.err).toBeInstanceOf(Error);
+  });
+
+  it('passes a genuinely public resolution through unchanged', async () => {
+    const pub = await runLookup([{ address: '93.184.216.34', family: 4 }]);
+    expect(pub.err).toBeNull();
+    expect(pub.address).toEqual([{ address: '93.184.216.34', family: 4 }]);
+  });
+});

apps/daemon/tests/library-ingest-ssrf.test.ts

@@ -0,0 +1,146 @@
+// Regression: SSRF via POST /api/library/ingest (+ read-back exfil via /raw).
+//
+// Before the fix, `fetchRemoteBytes` (src/routes/library.ts) did a raw
+// `fetch(url, { redirect: 'follow' })` on the client-supplied `body.url` with
+// NO SSRF host validation, unlike the sibling brand path which routes every
+// external asset through `assertPublicBrandUrl` (rejects loopback / RFC1918 /
+// link-local / metadata IPs). Any local process — including a prompt-injected
+// agent, or anything that can set a `chrome-extension://` Origin header, which
+// a non-browser client does trivially — can make the privileged daemon issue
+// GET requests to arbitrary internal services (127.0.0.1:*, 169.254.169.254,
+// RFC1918), and then read the fetched bytes back out through the unauthenticated
+// `GET /api/library/assets/:id/raw` route. That is SSRF with full response
+// exfiltration.
+//
+// This spec asserts the SECURE invariant (a loopback/internal URL must be
+// refused and the internal service must never be touched). RED before the fix
+// (the daemon fetched the canary and exfil'd its secret); green after routing
+// fetchRemoteBytes through the SSRF guard.
+
+import type http from 'node:http';
+import { createServer } from 'node:http';
+import { mkdtemp, rm } from 'node:fs/promises';
+import os from 'node:os';
+import path from 'node:path';
+import { afterEach, beforeEach, describe, expect, it } from 'vitest';
+
+let daemon: http.Server | undefined;
+let daemonShutdown: (() => Promise<void> | void) | undefined;
+let baseUrl = '';
+let dataDir = '';
+
+// A stand-in "internal service" (think cloud metadata / a loopback admin API).
+// It records every request it receives and returns a secret body.
+let canary: http.Server | undefined;
+let canaryUrl = '';
+let canaryHits: string[] = [];
+const CANARY_SECRET = 'INTERNAL-SECRET-aws-creds-42';
+
+const PREV_DATA_DIR = process.env.OD_DATA_DIR;
+
+beforeEach(async () => {
+  canaryHits = [];
+  canary = createServer((req, res) => {
+    canaryHits.push(req.url ?? '');
+    res.setHeader('Content-Type', 'text/plain');
+    res.end(CANARY_SECRET);
+  });
+  await new Promise<void>((resolve) => canary!.listen(0, '127.0.0.1', () => resolve()));
+  const cAddr = canary.address() as { port: number };
+  canaryUrl = `http://127.0.0.1:${cAddr.port}/latest/meta-data/iam/security-credentials`;
+
+  dataDir = await mkdtemp(path.join(os.tmpdir(), 'od-ssrf-'));
+  process.env.OD_DATA_DIR = dataDir;
+
+  // Dynamic import AFTER OD_DATA_DIR is set: RUNTIME_DATA_DIR is resolved at
+  // module-eval time, so a static import would pin the real data dir.
+  const { startServer } = await import('../src/server.js');
+  const started = (await startServer({ port: 0, host: '127.0.0.1', returnServer: true })) as {
+    url: string;
+    server: http.Server;
+    shutdown?: () => Promise<void> | void;
+  };
+  baseUrl = started.url;
+  daemon = started.server;
+  daemonShutdown = started.shutdown;
+});
+
+afterEach(async () => {
+  // The daemon's graceful shutdown can idle on background timers; race it so
+  // afterEach always tears the HTTP listener down (no leaked in-proc server).
+  if (daemonShutdown) {
+    await Promise.race([
+      Promise.resolve(daemonShutdown()),
+      new Promise((r) => setTimeout(r, 2000)),
+    ]);
+  }
+  // undici keeps the fetch sockets alive; drop them so close() resolves.
+  daemon?.closeAllConnections?.();
+  canary?.closeAllConnections?.();
+  if (daemon) await new Promise<void>((r) => daemon!.close(() => r()));
+  if (canary) await new Promise<void>((r) => canary!.close(() => r()));
+  daemon = undefined;
+  canary = undefined;
+  daemonShutdown = undefined;
+  await rm(dataDir, { recursive: true, force: true }).catch(() => {});
+  if (PREV_DATA_DIR === undefined) delete process.env.OD_DATA_DIR;
+  else process.env.OD_DATA_DIR = PREV_DATA_DIR;
+}, 15000);
+
+describe('library ingest SSRF', () => {
+  it('must NOT fetch a loopback/internal URL (and must not exfil its bytes)', async () => {
+    // Present a browser-extension Origin. A real browser reserves this for an
+    // installed extension, but any local non-browser client sets it freely —
+    // this also selects the "clipper" branch, which skips the mime allowlist,
+    // so the fetched bytes are stored verbatim and become read-back-able.
+    const ingestResp = await fetch(`${baseUrl}/api/library/ingest`, {
+      method: 'POST',
+      headers: {
+        'Content-Type': 'application/json',
+        Origin: 'chrome-extension://ssrfssrfssrfssrfssrfssrfssrfssrf',
+      },
+      body: JSON.stringify({ url: canaryUrl }),
+    });
+
+    const ingestJson = (await ingestResp.json().catch(() => ({}))) as {
+      asset?: { id?: string };
+      error?: unknown;
+    };
+
+    // Read-back exfil: pull the stored bytes through the unauthenticated /raw route.
+    let exfil = '';
+    if (ingestJson.asset?.id) {
+      const rawResp = await fetch(
+        `${baseUrl}/api/library/assets/${ingestJson.asset.id}/raw`,
+      );
+      exfil = await rawResp.text().catch(() => '');
+    }
+
+    // Evidence dump (visible in test output when the assertion fails on main).
+    // eslint-disable-next-line no-console
+    console.log(
+      `[SSRF EVIDENCE] ingest status=${ingestResp.status} canaryHits=${JSON.stringify(
+        canaryHits,
+      )} assetId=${ingestJson.asset?.id ?? '(none)'} exfilMatchesSecret=${exfil === CANARY_SECRET}`,
+    );
+
+    // SECURE invariant #1: the daemon must never have touched the internal service.
+    expect(canaryHits).toEqual([]);
+    // SECURE invariant #2: even if it did, the internal secret must not be
+    // exfiltrable back to the caller.
+    expect(exfil).not.toBe(CANARY_SECRET);
+  });
+
+  // A/B causal: the drop-in fix is to route `fetchRemoteBytes` through the SAME
+  // guard the sibling brand path already uses. This proves that guard rejects
+  // the exact loopback/metadata URL the ingest route happily fetched above —
+  // so the vulnerability is a one-call omission, not a design gap.
+  it('the existing sibling guard (assertPublicBrandUrl) rejects the same URL', async () => {
+    const { assertPublicBrandUrl } = await import('../src/brands/safe-fetch.js');
+    await expect(assertPublicBrandUrl(canaryUrl)).rejects.toThrow(/non-public|blocked/i);
+    // And the real metadata IP is refused too.
+    await expect(
+      assertPublicBrandUrl('http://169.254.169.254/latest/meta-data/'),
+    ).rejects.toThrow(/non-public|blocked/i);
+  });
+});