fix(ui,scraping): a crafted icon executed at import, and a session cookie went to the wrong host

CHANGELOG.md

@@ -252,6 +252,56 @@ Semver applies from 1.0.0. A breaking change to a documented API needs a major 
 
 ### Fixed
 
+- **SECURITY — the icon generator wrote network-fetched data into TypeScript source unescaped, so a
+  crafted glyph executed at import.** `build-icons.ts` emitted `${key}: '${value}'` where `value`
+  came from `icon-nodes.json` fetched over the network. `iconElements` validated tag names,
+  attribute *names* and the `fill` value — never the value of `d`, `points` or `cx`. A value that
+  closes the string, the object and the array element and reopens all three produces a glyph module
+  that runs arbitrary code **in every app that imports that icon**. **Proven exploitable**: the
+  regression test transpiles and evaluates the generated module, and the payload set a global before
+  the fix. `JSON.stringify` is the fix; a `SAFE_ATTR_VALUE` allowlist is defence in depth, added only
+  because it demonstrably refuses none of the 1767 committed glyphs. The sibling `icon-glyph.ts`
+  already stated the principle — *"data reaching an attribute sink unchecked is how `onload=` gets
+  into the DOM"* — and this was a **code** sink, which is worse.
+
+- **SECURITY — a scraped session's cookies were sent to hosts they do not belong to.** The match was
+  `host.endsWith(cookie.domain)`, with no dot boundary in either direction: a host-only cookie for
+  `bank.test` was sent to **`evilbank.test`** *and* to `sub.bank.test`. The CDP jar is every domain
+  the session ever touched, so an SSO hop's cookies rode along. Cookie scoping is now one function
+  implementing RFC 6265 §5.1.3 and §5.1.4 — domain **and** path, both boundaries — failing closed on
+  an unparseable URL, and a `secure` cookie no longer reaches an `http:` URL. Path scoping was a
+  third leak the audit had not named.
+
+- **SECURITY — two tenants shared one authenticated scrape session.** A declared `auth.key` replaced
+  the whole session key instead of discriminating within the tenant, so `orgOf(ctx)` was never
+  consulted and the value was never sanitised. `sessionKeyFor`'s third parameter is named
+  `discriminator` and had no caller anywhere — it does now.
+
+- **A disabled item made the rest of a `Menu` or `Tabs` unreachable by keyboard.** A disabled control
+  cannot take focus, so the roving reducer returned its index forever and arrow keys stopped dead. If
+  the disabled item was first, nothing in the group was tabbable at all. A `Toolbar` separately stole
+  arrow keys from a text field inside it — its own documented use — swallowing the caret move.
+
+- **`ToastRegion` was not a live region**, which is precisely the failure its own header says the
+  region/child split exists to prevent: each toast created a fresh live region with its content
+  already in it, which most screen readers do not announce.
+
+- **A file dropped on `Dropzone` never reached the form.** `onSelect` fired, `input.files` stayed
+  empty, and `required` blocked the submit — while `name`/`required` are props and native form
+  participation is the advertised contract.
+
+- **Seven more instances of the caught-value totality class**, in `@ultimat3/ai` and `@ultimat3/mcp`:
+  a tool result that could not be serialised took down the tool loop, a hostile provider rejection
+  escaped the gateway's retry classifier, and the MCP server's error renderer read four fields off a
+  value the framework did not build. A tool whose *output* is unserialisable now reports that the
+  tool **ran**, never that it failed — so the model does not re-buy the side effects.
+
+- Also: a refused scrape credential was walked back to the login form when `reuse: false`;
+  `robots: 'obey'` was unenforced on the offline HTTP leg every test exercises, so a `Disallow:`ed
+  endpoint replayed green and failed in production; a browser process leaked per failed attempt, and
+  the throws that caused it reached the job retry classifier as bare `Error`s with no code; and
+  restored `localStorage` was written on `about:blank` where it can never reach the site's origin.
+
 - **The outbox relay's claim locked nothing, so two relays could publish one batch and a job could
   run twice.** `SQL_OUTBOX_CLAIM` ends in `for update skip locked`, but the relay issues it on a
   **pooled** connection with no transaction — a bare statement runs in an implicit transaction that

packages/scraping/CLAUDE.md

@@ -51,6 +51,10 @@ and a value cannot leak. `driver-parity.test.ts` runs the real driver's code pat
 | the fake never drifts from the real driver | `driver-parity.test.ts` runs one suite against `fake`, `fixture` and the puppeteer path, and pins the one honest divergence (no layout engine offline, so no box and no hit-target) |
 | an unrecorded request throws | `html-target.ts` and `http-recorded.ts` — an offline driver that fell through to the network would make a green suite secretly live |
 | `allowHosts` is enforced, never advisory | `intercept.ts` is the single decision, asked by every driver AND by the HTTP leg, before a byte leaves |
+| robots is enforced on BOTH legs | `http-recorded.ts` takes the gate too (`http-recorded.test.ts`) — the offline leg is the one every test runs, so a `Disallow:`ed endpoint that only the live leg refuses is a rule no suite can see |
+| a session cookie reaches one host | `cookie-scope.ts`, RFC 6265 §5.1.3/§5.1.4, pinned in `cookie-scope.test.ts`. The jar is `browser.cookies()` — every domain the session touched — so the boundary is a dot in both directions: `evilbank.test` is not `bank.test`, and a host-only cookie is not a subdomain's |
+| a launched browser is never orphaned | `driver-cdp.ts`'s `opened()` rolls back with `browser.close()` on any throw between the launch and the `WedgeGuard` (`driver-cdp.test.ts`) — `runScrape`'s `finally` cannot close a session `open()` never returned |
+| restored `localStorage` lands on its ORIGIN | `cdp-target.ts` defers the storage half to the first navigation that reaches `session.origin` (`cdp-target.test.ts`); `restore()` runs on `about:blank`, which has no storage to write to and is not the site |
 | Chrome is never needed for `bun test` | `fakeBrowser`/`fakePage` run on Bun's own `HTMLRewriter` |
 
 ## Logging
@@ -66,7 +70,12 @@ never a value.
 - `secrets:` on the definition holds **names**; values are resolved in the worker, per attempt.
 - A `Secret` typed into a page **taints** it: `screenshot()` and `pdf()` then refuse
   (`X_SCRAPE_SECRET_EXPOSED`). Pixels cannot be redacted after the fact; `page.html()` can, and is.
-- A session is credential material: tenant-scoped key, never logged, never an artifact.
+- A session is credential material: tenant-scoped key, never logged, never an artifact. The key is
+  ALWAYS `sessionKeyFor({ scrape, tenant, discriminator })` — `auth.key` supplies the
+  discriminator and never the whole key, or two tenants naming one account share one authenticated
+  session, and a key that is also a storage path goes unsanitised (`scrape-run.test.ts`).
+- The refusal tombstone is read BEFORE `reuse` is honoured: `reuse: false` means "do not restore
+  this session", never "present the rejected credential again" (`auth.ts`).
 - `X_SCRAPE_AUTH_FAILED` is registered `terminal`, so `executeJob` dead-letters it on the attempt
   that threw it (`packages/jobs/src/retry-classification.ts` — `nextRetryForError`). It ALSO writes
   a refusal into the session record, which is a different distance: `restorableSession()` reads it

packages/scraping/README.md

@@ -74,8 +74,15 @@ browser's cookies, headers and proxy, the same `allowHosts`, the same robots gat
 limit, the same cancellation. `page.session()` exposes the handoff so an author can see what
 carried over.
 
+The jar is scoped per request, RFC 6265: a cookie stored for `bank.test` is sent to `bank.test`
+and to nothing else — not `evilbank.test`, not `sub.bank.test` — and only a domain-scoped
+`.bank.test` reaches subdomains. `SessionSnapshot.headers` is the one field the real driver cannot
+fill (CDP exposes no read for it, and `driver-parity.test.ts` pins that): a token the HTTP leg must
+carry goes on the request, `http.request(url, { headers })`.
+
 Both legs replay from **one** fixture directory (`fixtureBrowser(dir)`), so a hybrid run — browser
-login, session handoff, HTTP bulk fetch — is tested end to end.
+login, session handoff, HTTP bulk fetch — is tested end to end. Both legs apply the same robots
+gate, offline included.
 
 ## What it owns
 

packages/scraping/src/auth.test.ts

@@ -121,6 +121,16 @@ describe('unit · validate decides, and an invalid session is burned before the
 });
 
 describe('unit · a refused credential is never presented twice', () => {
+  test('reuse: false still refuses — the tombstone is read BEFORE the reuse decision', async () => {
+    const store = memorySessionStore();
+    // `reuse: false` means "do not restore this session", never "present the rejected credential
+    // again": a second wrong password is what locks the account, and the record is the only thing
+    // that knows it was wrong.
+    const plan = planFor<unknown>({ login: () => Promise.resolve(), store, reuse: false });
+    await markRefused(plan);
+    expect(await codeOf(restorableSession(plan))).toBe('X_SCRAPE_AUTH_FAILED');
+  });
+
   test('the refusal is written down, and the NEXT attempt fails before reaching a login form', async () => {
     const store = memorySessionStore();
     const plan = planFor<unknown>({ login: () => Promise.resolve(), store });

packages/scraping/src/auth.ts

@@ -64,7 +64,12 @@ export interface ScrapeAuth<I> {
   validate?(context: AuthContext<I>): Promise<boolean>;
   /** Where sessions live. Omitted means no reuse at all — every run logs in. */
   readonly store?: ScrapeSessionStore | undefined;
-  /** Distinguishes two sessions for one scrape and one tenant — a second account, say. */
+  /**
+   * The DISCRIMINATOR inside this tenant's key space — a second account, say. It is one segment of
+   * `sessionKeyFor({ scrape, tenant, discriminator })` and never the whole key: a value that
+   * replaced the key would put two tenants declaring the same account name on one authenticated
+   * session. Sanitised like every other segment, so it cannot escape the key space either.
+   */
   key?(input: I): string;
   /** Reuse a stored session. `false` forces a fresh login every run. Defaults to `true`. */
   readonly reuse?: boolean | undefined;
@@ -106,10 +111,14 @@ export async function restorableSession<I>(
   plan: AuthPlanInput<I>,
 ): Promise<SessionState | undefined> {
   const store = plan.auth?.store;
-  if (store === undefined || plan.auth?.reuse === false) return undefined;
+  if (store === undefined) return undefined;
   const found = await store.load(plan.key);
   if (found === undefined) return undefined;
+  // The tombstone is read BEFORE `reuse` is honoured. `reuse: false` says "do not restore this
+  // session"; it does not say "present the rejected credential again", and reading it first meant
+  // a `reuse: false` scrape walked a refused password back to the login form on every requeue.
   if (found.refusedAt !== undefined) throw authFailed(plan.scrape, `refused at ${found.refusedAt}`);
+  if (plan.auth?.reuse === false) return undefined;
   const maxAge = plan.auth?.maxAge;
   if (maxAge !== undefined) {
     const age = plan.clock.now().getTime() - new Date(found.savedAt).getTime();

packages/scraping/src/cdp-port.ts

@@ -15,6 +15,12 @@
 export interface CdpRequestLike {
   url(): string;
   resourceType(): string;
+  /**
+   * OPTIONAL, and read defensively: this is the shape of somebody else's event payload, so a
+   * launcher that predates the method (or a provider SDK that never had it) still satisfies the
+   * port and its requests are recorded as `GET` rather than crashing the interception handler.
+   */
+  method?(): string;
   abort(): Promise<void>;
   continue(): Promise<void>;
 }

packages/scraping/src/cdp-target.test.ts

@@ -0,0 +1,216 @@
+// The real driver's target, over a hand-built CDP page. Three things only this file can see:
+// WHEN a restored session's `localStorage` is written, what a network entry says the method was,
+// and what a console line says its level was — all three are read straight off the library's own
+// event payloads, so the offline drivers cannot pin any of them.
+
+import { describe, expect, test } from 'bun:test';
+import type { CdpBrowserLike, CdpPageLike } from './cdp-port';
+import { cdpTarget } from './cdp-target';
+import { testClock } from './clock';
+import type { SessionSnapshot } from './session-state';
+
+interface Recorder {
+  readonly page: CdpPageLike;
+  readonly browser: CdpBrowserLike;
+  /** Every `evaluate` expression and `goto`, in order, so ORDER is what the test asserts on. */
+  readonly calls: readonly string[];
+  emit(event: string, payload: unknown): void;
+}
+
+const recorder = (start = 'about:blank'): Recorder => {
+  const calls: string[] = [];
+  const handlers = new Map<string, ((payload: unknown) => void)[]>();
+  let url = start;
+  const page: CdpPageLike = {
+    url: () => url,
+    goto: (next: string) => {
+      calls.push(`goto ${next}`);
+      url = next;
+      return Promise.resolve(undefined);
+    },
+    content: () => Promise.resolve(''),
+    evaluate: (expression: string) => {
+      calls.push(`evaluate ${expression}`);
+      return Promise.resolve(undefined);
+    },
+    click: () => Promise.resolve(),
+    type: () => Promise.resolve(),
+    select: () => Promise.resolve([]),
+    screenshot: () => Promise.resolve(new Uint8Array()),
+    pdf: () => Promise.resolve(new Uint8Array()),
+    setRequestInterception: () => Promise.resolve(),
+    on: (event: string, handler: (payload: unknown) => void) => {
+      const listeners = handlers.get(event) ?? [];
+      listeners.push(handler);
+      handlers.set(event, listeners);
+      return undefined;
+    },
+    frames: () => [],
+    close: () => Promise.resolve(),
+  };
+  return {
+    page,
+    browser: {
+      newPage: () => Promise.resolve(page),
+      setCookie: () => Promise.resolve(),
+      close: () => Promise.resolve(),
+      process: () => null,
+    },
+    calls,
+    emit: (event, payload) => {
+      for (const handler of handlers.get(event) ?? []) handler(payload);
+    },
+  };
+};
+
+const SESSION: SessionSnapshot = {
+  cookies: [
+    { name: 'sid', value: 'x', domain: 'shop.test', path: '/', httpOnly: true, secure: true },
+  ],
+  headers: {},
+  storage: { token: 'bearer-abc' },
+  userAgent: 'agent',
+  origin: 'https://shop.test',
+};
+
+const open = (start?: string) => {
+  const rec = recorder(start);
+  return {
+    rec,
+    target: cdpTarget({
+      page: rec.page,
+      browser: rec.browser,
+      rules: { allowHosts: ['*'] },
+      clock: testClock(),
+    }),
+  };
+};
+
+const storageWrites = (calls: readonly string[]): readonly string[] =>
+  calls.filter((call) => call.includes('setItem'));
+
+describe('unit · restored localStorage lands on the session ORIGIN, never on about:blank', () => {
+  test('restore() before the first navigation writes no storage — an opaque origin has none', async () => {
+    const { rec, target } = open();
+    await (await target).restore(SESSION);
+    expect(storageWrites(rec.calls)).toEqual([]);
+  });
+
+  test('it lands on the first navigation to the origin the session belongs to', async () => {
+    const { rec, target } = open();
+    const page = await target;
+    await page.restore(SESSION);
+    await page.goto('https://shop.test/orders', { timeoutMs: 1_000 });
+    expect(storageWrites(rec.calls)).toHaveLength(1);
+    expect(rec.calls.indexOf('goto https://shop.test/orders')).toBeLessThan(
+      rec.calls.findIndex((call) => call.includes('setItem')),
+    );
+  });
+
+  test('and never on another origin — a bearer token is not handed to a site it is not for', async () => {
+    const { rec, target } = open();
+    const page = await target;
+    await page.restore(SESSION);
+    await page.goto('https://other.test/', { timeoutMs: 1_000 });
+    expect(storageWrites(rec.calls)).toEqual([]);
+    // Still pending: the run that finally reaches the site gets its session.
+    await page.goto('https://shop.test/orders', { timeoutMs: 1_000 });
+    expect(storageWrites(rec.calls)).toHaveLength(1);
+  });
+
+  test('it is written once, not on every navigation', async () => {
+    const { rec, target } = open();
+    const page = await target;
+    await page.restore(SESSION);
+    await page.goto('https://shop.test/a', { timeoutMs: 1_000 });
+    await page.goto('https://shop.test/b', { timeoutMs: 1_000 });
+    expect(storageWrites(rec.calls)).toHaveLength(1);
+  });
+});
+
+describe('unit · a network entry says what the request actually was', () => {
+  // RECEIVER-DEPENDENT on purpose. puppeteer's `HTTPRequest.method()` reads the request's own
+  // internals, so a fake that closed over a constant would answer the same whether the framework
+  // called `request.method()` or handed the bare function to a helper — and the second one is
+  // `undefined` against the real library. `DETACHED` is what a lost `this` looks like here.
+  const request = (url: string, method: string | undefined) => {
+    const base = {
+      url: () => url,
+      resourceType: () => 'fetch',
+      abort: () => Promise.resolve(),
+      continue: () => Promise.resolve(),
+    };
+    return method === undefined
+      ? base
+      : {
+          ...base,
+          verb: method,
+          method(this: { readonly verb?: string } | undefined): string {
+            return typeof this?.verb === 'string' ? this.verb : 'DETACHED';
+          },
+        };
+  };
+
+  test('a POST is recorded as a POST — page.network() is what X_SCRAPE_HTTP_FAILED points at', async () => {
+    const { rec, target } = open();
+    const page = await target;
+    rec.emit('request', request('https://shop.test/api', 'POST'));
+    expect(page.network.entries().map((entry) => entry.method)).toEqual(['POST']);
+  });
+
+  test('a refused request keeps its method too — a blocked POST is not a blocked GET', async () => {
+    const rec = recorder();
+    const page = await cdpTarget({
+      page: rec.page,
+      browser: rec.browser,
+      rules: { allowHosts: ['shop.test'] },
+      clock: testClock(),
+    });
+    rec.emit('request', request('https://evil.test/api', 'PUT'));
+    expect(page.network.entries().map((entry) => [entry.method, entry.refused])).toEqual([
+      ['PUT', 'host'],
+    ]);
+  });
+
+  test('a launcher whose request has no method() still records one', async () => {
+    const { rec, target } = open();
+    const page = await target;
+    rec.emit('request', request('https://shop.test/api', undefined));
+    expect(page.network.entries().map((entry) => entry.method)).toEqual(['GET']);
+  });
+});
+
+describe('unit · a console line keeps its level', () => {
+  // Same rule as the request fake: `ConsoleMessage.type()` and `.text()` read `this`, so these
+  // answer out of the payload rather than out of a closure.
+  const message = (level: string) => ({
+    level,
+    type(this: { readonly level?: string } | undefined): string {
+      return typeof this?.level === 'string' ? this.level : 'DETACHED';
+    },
+    text(this: { readonly level?: string } | undefined): string {
+      return typeof this?.level === 'string' ? `a ${this.level}` : 'DETACHED';
+    },
+  });
+
+  test('the four levels below log are reachable on the real driver', async () => {
+    const { rec, target } = open();
+    const page = await target;
+    for (const type of ['error', 'warning', 'info', 'debug', 'table'])
+      rec.emit('console', message(type));
+    expect(page.console.entries().map((line) => line.level)).toEqual([
+      'error',
+      'warn',
+      'info',
+      'debug',
+      'log',
+    ]);
+  });
+
+  test('and its text — an accessor is called THROUGH the message, not bare', async () => {
+    const { rec, target } = open();
+    const page = await target;
+    rec.emit('console', message('warning'));
+    expect(page.console.entries().map((line) => line.text)).toEqual(['a warning']);
+  });
+});