From 791874f8c81b4cfbb557ce82f631fa471278ad46 Mon Sep 17 00:00:00 2001 From: edwinhu Date: Thu, 16 Jul 2026 15:28:23 -0400 Subject: [PATCH 1/5] fix: read session cookies from the browser target, not an arbitrary page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit session-refresh.ts exists because the extension's own targets never answer CDP. It reads cookies to sidestep that, on the premise that "page targets respond to CDP reliably" — but that premise does not hold, and the fallback bet on it: const target = pages.find(t => t.url.includes("superhuman.com")) ?? pages[0]; Measured against a live browser with six page targets, Network.getCookies hung indefinitely on one while the other five answered in milliseconds. Which target hangs drifts: a tab that hung on one probe answered three hours later, and a different tab had started hanging by then. It is a busy renderer, not a property of any site — so no page target can be assumed responsive, and pages[0] is an arbitrary tab. With no Superhuman tab open, that fallback attaches to whatever is first. If that renderer is wedged the read never returns — and a hang is not a rejection, so the try/catch cannot turn it into the documented null. The caller just stops. On-demand refresh then fails exactly as it did before this module existed, which is the attachment-download 401 it was written to prevent. Read from the browser-level target via Storage.getCookies instead: no renderer to wedge, and no Superhuman tab (or any tab) need be open. Bound the read too, so a future stall degrades to null rather than hanging. Two details worth flagging for review: - Attach via CDP.Version()'s webSocketDebuggerUrl. `target: "browser"` does not work — chrome-remote-interface treats a target string as an id and looks it up in CDP.List(), which never contains the browser. Using CDP.Version() rather than fetch() also keeps discovery on the library's transport, so the existing "no network call without a browser" test still holds; an earlier fetch()-based draft broke it, correctly. - Storage.getCookies takes no `urls` filter and returns the whole store, so scope with RFC 6265 host-matching against the two hosts the backend calls authenticate against. A plain *.superhuman.com filter is wrong: it also picks up media.superhuman.com's own device-id and sends duplicate names with different values. Verified live: the header is the same 5 name=value pairs as the old page-target path (set-identical; order differs, which RFC 6265 does not constrain and the backend accepts), read in 26ms, and isSessionRefreshHealthy() — a real CSRF exchange against accounts.superhuman.com — returns true. A dead port still returns null. Tests 432 pass / 6 fail, matching main exactly (the 6 are pre-existing live E2E --attach failures). tsc --noEmit clean. The new page-target assertion is mutation-verified: reverting the discovery to CDP.List page enumeration fails it. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01UZvrft29zMGgks5abHVtVK --- src/__tests__/session-refresh.test.ts | 21 ++++++ src/session-refresh.ts | 103 +++++++++++++++++++++----- 2 files changed, 106 insertions(+), 18 deletions(-) diff --git a/src/__tests__/session-refresh.test.ts b/src/__tests__/session-refresh.test.ts index 99c88e3..2396f7d 100644 --- a/src/__tests__/session-refresh.test.ts +++ b/src/__tests__/session-refresh.test.ts @@ -18,6 +18,27 @@ test("refreshViaSessionCookies returns null (never throws) with no browser", asy .toBeNull(); }); +test("readSessionCookieHeader never attaches to a page target", async () => { + // The point of the browser-level read: a page target routes through a + // renderer, and a busy renderer never answers — measured live, one of six + // page targets hung indefinitely on Network.getCookies while five answered + // in milliseconds, and which one hangs drifts over time. Attaching to any + // page at all reintroduces that gamble, so assert we never enumerate them. + const CDP = (await import("chrome-remote-interface")).default as any; + const realList = CDP.List; + let listed = false; + CDP.List = async (...args: unknown[]) => { + listed = true; + return realList(...args); + }; + try { + await readSessionCookieHeader(DEAD_PORT); + expect(listed).toBe(false); + } finally { + CDP.List = realList; + } +}); + test("refreshViaSessionCookies short-circuits before any network call", async () => { // Without cookies it must not reach accounts.superhuman.com at all. const realFetch = globalThis.fetch; diff --git a/src/session-refresh.ts b/src/session-refresh.ts index 3f8b277..f6fb2bf 100644 --- a/src/session-refresh.ts +++ b/src/session-refresh.ts @@ -17,8 +17,12 @@ * token. Anything hitting a live API with the cached OAuth token — notably * `attachment download` — then 401'd until the user manually re-ran * `superhuman account auth`. This path needs no CDP scripting of extension - * contexts: it only reads cookies (which page targets serve fine) and then - * talks HTTP to Superhuman's backend. + * contexts: it only reads cookies, and then talks HTTP to Superhuman's + * backend. + * + * The cookie read goes to the browser-level target, not a page — page targets + * are subject to the same never-answers failure when their renderer is busy + * (see readSessionCookieHeader). * * The flow (reverse-engineered from the extension bundle, * `background/background_page.js` → `Credential.refreshSession()`): @@ -45,7 +49,6 @@ import { getCDPHost, getCDPPort } from "./superhuman-api"; import type { TokenInfo } from "./token-api"; const ACCOUNTS_HOST = "https://accounts.superhuman.com"; -const COOKIE_URLS = [ACCOUNTS_HOST, "https://mail.superhuman.com"]; /** Backend response shape for sessions.getTokens (fields we consume). */ interface GetTokensResponse { @@ -58,13 +61,58 @@ interface GetTokensResponse { }; } +/** Upper bound on the cookie read. Override with CDP_TIMEOUT_MS. */ +const CDP_TIMEOUT_MS = parseInt(process.env.CDP_TIMEOUT_MS || "10000", 10); + +/** Hosts whose cookies authenticate the backend calls below. */ +const COOKIE_HOSTS = ["accounts.superhuman.com", "mail.superhuman.com"]; + +/** + * Does a cookie apply to `host`, per RFC 6265 domain-matching? + * + * A leading dot means the cookie covers the domain and its subdomains; + * otherwise it is host-only and must match exactly. Storage.getCookies returns + * the entire store with no `urls` filter, so this reproduces the scoping that + * Network.getCookies({ urls }) used to do for us — without it we would also + * pick up same-named cookies from other superhuman.com subdomains (media.* + * carries its own device-id) and send duplicates with the wrong values. + */ +function cookieAppliesToHost(domain: string, host: string): boolean { + const d = domain.toLowerCase(); + const h = host.toLowerCase(); + if (d.startsWith(".")) { + const base = d.slice(1); + return h === base || h.endsWith(`.${base}`); + } + return d === h; +} + /** * Read Superhuman's session cookies out of the live browser via CDP and * format them as a `Cookie` request header. * - * Uses a page target: `Network.getCookies` reads the shared browser cookie - * store (the `urls` argument selects the cookies, not the page's origin), and - * unlike the extension's own targets, page targets respond to CDP reliably. + * Uses the BROWSER-level target (`Storage.getCookies`), not a page target. + * + * The extension's own targets refuse CDP attachment (see the module header), + * which is why this path reads cookies at all. But page targets are not a safe + * fallback either: a CDP command routed through a page goes through its + * renderer, and a busy renderer simply never answers. Measured against a live + * browser with six page targets, `Network.getCookies` hung indefinitely on one + * of them while the other five answered in milliseconds — and *which* target + * hangs drifts over time (a tab that hung on one probe answered three hours + * later, while a different tab had started hanging). It is not a property of a + * particular site, so no page target can be assumed responsive. + * + * That made the old `?? pages[0]` fallback a coin flip: with no Superhuman tab + * open it attached to an arbitrary tab, and if that tab's renderer was wedged + * the read never returned. A hang is not a rejection, so the surrounding + * try/catch could not convert it into the documented `null` — the caller just + * stopped, and on-demand refresh silently failed exactly as it did before this + * module existed. + * + * The browser target has no renderer, so nothing can wedge it, and it needs no + * Superhuman tab (or any tab) to be open. The read is still bounded, so a + * future stall degrades to `null` rather than hanging. * * Returns null when no browser is reachable or no Superhuman cookies exist. */ @@ -72,26 +120,34 @@ export async function readSessionCookieHeader( port = getCDPPort() ): Promise { const host = getCDPHost(); - let targets: any[]; + + // Attach to the browser endpoint by its websocket URL. Passing target:"browser" + // does not work: chrome-remote-interface treats a target string as a target id + // and looks it up in CDP.List(), which never contains the browser itself. + // CDP.Version() (not fetch) keeps discovery on the library's own transport, + // so callers that assert "no network call without a browser" still hold. + let browserWsUrl: string; try { - targets = await CDP.List({ host, port }); + const ver = (await CDP.Version({ host, port })) as { webSocketDebuggerUrl?: string }; + if (!ver.webSocketDebuggerUrl) return null; + browserWsUrl = ver.webSocketDebuggerUrl; } catch { return null; } - // Prefer a Superhuman tab, but any page target can read the cookie store. - const pages = targets.filter((t: any) => t.type === "page"); - const target = - pages.find((t: any) => t.url.includes("superhuman.com")) ?? pages[0]; - if (!target) return null; - let client: CDP.Client | null = null; try { - client = await CDP({ target: target.id, host, port }); - await client.Network.enable(); - const { cookies } = await client.Network.getCookies({ urls: COOKIE_URLS }); + client = await CDP({ target: browserWsUrl, host, port }); + const { cookies } = await withTimeout( + client.Storage.getCookies({}), + "Storage.getCookies" + ); if (!cookies?.length) return null; - return cookies.map((c) => `${c.name}=${c.value}`).join("; "); + const relevant = cookies.filter((c) => + COOKIE_HOSTS.some((h) => cookieAppliesToHost(c.domain, h)) + ); + if (!relevant.length) return null; + return relevant.map((c) => `${c.name}=${c.value}`).join("; "); } catch { return null; } finally { @@ -105,6 +161,17 @@ export async function readSessionCookieHeader( } } +/** Reject rather than wait forever — an unbounded CDP wait is the bug above. */ +function withTimeout(p: Promise, what: string, ms = CDP_TIMEOUT_MS): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error(`${what} timed out after ${ms}ms`)), ms); + p.then( + (v) => { clearTimeout(timer); resolve(v); }, + (e) => { clearTimeout(timer); reject(e); } + ); + }); +} + /** * Health probe: is the browser's Superhuman session usable for refresh? * From 2e63024bb7cb164fc21b7b8fa0477de0329a9a6d Mon Sep 17 00:00:00 2001 From: edwinhu Date: Thu, 16 Jul 2026 16:06:35 -0400 Subject: [PATCH 2/5] fix: fall back to page targets so the Electron deployment cannot regress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut of this PR replaced the page-target read with a browser-level one outright. That was verified only against Chromium on Linux, by passing port 9222 explicitly. The desktop deployment is Electron on 9252 (getCDPPort's default), and nothing in this repo had ever attached browser-level before — CDP.Version and Storage.getCookies both had zero uses. So the change swapped a read that demonstrably works in Electron for one that is unverified there, on what is the primary deployment. Fixing Linux by regressing macOS is not a trade worth making on an untested assumption. Keep the browser target as the preferred route — it has no renderer, so it cannot wedge, and needs no Superhuman tab open — but fall back to page targets when it is unavailable for any reason (no webSocketDebuggerUrl, no Storage domain, error, or timeout). Worst case on Electron is now the previous behaviour rather than a break. The fallback is also better than what it replaces: it orders Superhuman tabs first and then ADVANCES to the next candidate when one does not answer within the timeout, instead of betting everything on `?? pages[0]`. A wedged renderer costs a timeout, not the refresh. Verified live on Linux: browser-level returns 5 cookies in 28ms; forcing CDP.Version to throw (simulating an Electron that does not serve Storage on its browser endpoint) falls through to the page path and returns 5 cookies in 25ms — the same set, confirmed by comparing sorted name=value pairs. Both agree. isSessionRefreshHealthy (a real CSRF exchange against accounts.superhuman.com) returns true, and a dead port still returns null. The page-target test now asserts ORDER (browser attempted first) rather than "never touches pages", since pages are a legitimate fallback now. Tests 433 pass / 6 fail — the +1 over main is this test; the 6 are pre-existing live E2E --attach failures. tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01UZvrft29zMGgks5abHVtVK --- src/__tests__/session-refresh.test.ts | 25 ++-- src/session-refresh.ts | 168 +++++++++++++++++++------- 2 files changed, 143 insertions(+), 50 deletions(-) diff --git a/src/__tests__/session-refresh.test.ts b/src/__tests__/session-refresh.test.ts index 2396f7d..a83b6d3 100644 --- a/src/__tests__/session-refresh.test.ts +++ b/src/__tests__/session-refresh.test.ts @@ -18,23 +18,30 @@ test("refreshViaSessionCookies returns null (never throws) with no browser", asy .toBeNull(); }); -test("readSessionCookieHeader never attaches to a page target", async () => { - // The point of the browser-level read: a page target routes through a - // renderer, and a busy renderer never answers — measured live, one of six - // page targets hung indefinitely on Network.getCookies while five answered - // in milliseconds, and which one hangs drifts over time. Attaching to any - // page at all reintroduces that gamble, so assert we never enumerate them. +test("readSessionCookieHeader prefers the browser target over page targets", async () => { + // Page targets route through a renderer, and a busy renderer never answers — + // measured live, one of six page targets hung indefinitely on + // Network.getCookies while five answered in milliseconds, and which one hangs + // drifts over time. So the browser target must be tried FIRST; page targets + // exist only as a fallback for deployments that may not serve Storage there. const CDP = (await import("chrome-remote-interface")).default as any; + const realVersion = CDP.Version; const realList = CDP.List; - let listed = false; + const order: string[] = []; + CDP.Version = async (...args: unknown[]) => { + order.push("browser"); + return realVersion(...args); + }; CDP.List = async (...args: unknown[]) => { - listed = true; + order.push("pages"); return realList(...args); }; try { await readSessionCookieHeader(DEAD_PORT); - expect(listed).toBe(false); + // Both may be attempted against a dead port; what matters is the order. + expect(order[0]).toBe("browser"); } finally { + CDP.Version = realVersion; CDP.List = realList; } }); diff --git a/src/session-refresh.ts b/src/session-refresh.ts index f6fb2bf..5dbaa41 100644 --- a/src/session-refresh.ts +++ b/src/session-refresh.ts @@ -49,6 +49,8 @@ import { getCDPHost, getCDPPort } from "./superhuman-api"; import type { TokenInfo } from "./token-api"; const ACCOUNTS_HOST = "https://accounts.superhuman.com"; +/** URLs whose cookies the page-target fallback asks for. */ +const COOKIE_URLS = [ACCOUNTS_HOST, "https://mail.superhuman.com"]; /** Backend response shape for sessions.getTokens (fields we consume). */ interface GetTokensResponse { @@ -87,45 +89,41 @@ function cookieAppliesToHost(domain: string, host: string): boolean { return d === h; } +/** Shape both cookie reads return; only name/value/domain are used. */ +interface RawCookie { + name: string; + value: string; + domain: string; +} + +/** Build the Cookie header from a raw cookie list, or null if none apply. */ +function toCookieHeader(cookies: RawCookie[] | undefined): string | null { + if (!cookies?.length) return null; + const relevant = cookies.filter((c) => + COOKIE_HOSTS.some((h) => cookieAppliesToHost(c.domain, h)) + ); + if (!relevant.length) return null; + return relevant.map((c) => `${c.name}=${c.value}`).join("; "); +} + /** - * Read Superhuman's session cookies out of the live browser via CDP and - * format them as a `Cookie` request header. + * Read cookies from the BROWSER-level target via Storage.getCookies. * - * Uses the BROWSER-level target (`Storage.getCookies`), not a page target. + * The browser endpoint has no renderer, so nothing can wedge it, and it needs + * no Superhuman tab — or any tab — to be open. This is the preferred route. * - * The extension's own targets refuse CDP attachment (see the module header), - * which is why this path reads cookies at all. But page targets are not a safe - * fallback either: a CDP command routed through a page goes through its - * renderer, and a busy renderer simply never answers. Measured against a live - * browser with six page targets, `Network.getCookies` hung indefinitely on one - * of them while the other five answered in milliseconds — and *which* target - * hangs drifts over time (a tab that hung on one probe answered three hours - * later, while a different tab had started hanging). It is not a property of a - * particular site, so no page target can be assumed responsive. - * - * That made the old `?? pages[0]` fallback a coin flip: with no Superhuman tab - * open it attached to an arbitrary tab, and if that tab's renderer was wedged - * the read never returned. A hang is not a rejection, so the surrounding - * try/catch could not convert it into the documented `null` — the caller just - * stopped, and on-demand refresh silently failed exactly as it did before this - * module existed. - * - * The browser target has no renderer, so nothing can wedge it, and it needs no - * Superhuman tab (or any tab) to be open. The read is still bounded, so a - * future stall degrades to `null` rather than hanging. - * - * Returns null when no browser is reachable or no Superhuman cookies exist. + * Returns null (never throws) if the endpoint is unreachable or the browser + * does not serve Storage there, so the caller can fall back. */ -export async function readSessionCookieHeader( - port = getCDPPort() -): Promise { - const host = getCDPHost(); - - // Attach to the browser endpoint by its websocket URL. Passing target:"browser" - // does not work: chrome-remote-interface treats a target string as a target id - // and looks it up in CDP.List(), which never contains the browser itself. - // CDP.Version() (not fetch) keeps discovery on the library's own transport, - // so callers that assert "no network call without a browser" still hold. +async function readCookiesFromBrowserTarget( + host: string, + port: number +): Promise { + // Attach by websocket URL. target:"browser" does NOT work — chrome-remote- + // interface treats a target string as a target id and looks it up in + // CDP.List(), which never contains the browser itself. CDP.Version() (not + // fetch) keeps discovery on the library's transport, so callers asserting + // "no network call without a browser" still hold. let browserWsUrl: string; try { const ver = (await CDP.Version({ host, port })) as { webSocketDebuggerUrl?: string }; @@ -138,16 +136,12 @@ export async function readSessionCookieHeader( let client: CDP.Client | null = null; try { client = await CDP({ target: browserWsUrl, host, port }); + if (!client.Storage?.getCookies) return null; const { cookies } = await withTimeout( client.Storage.getCookies({}), "Storage.getCookies" ); - if (!cookies?.length) return null; - const relevant = cookies.filter((c) => - COOKIE_HOSTS.some((h) => cookieAppliesToHost(c.domain, h)) - ); - if (!relevant.length) return null; - return relevant.map((c) => `${c.name}=${c.value}`).join("; "); + return cookies ?? null; } catch { return null; } finally { @@ -161,6 +155,98 @@ export async function readSessionCookieHeader( } } +/** + * Fallback: read cookies via a page target's Network.getCookies. + * + * Only reached when the browser target is unavailable. Tries Superhuman tabs + * first, then other pages, and — crucially — moves on to the next candidate + * when one does not answer within the timeout, rather than betting everything + * on a single tab. + */ +async function readCookiesFromPageTargets( + host: string, + port: number +): Promise { + let targets: any[]; + try { + targets = await CDP.List({ host, port }); + } catch { + return null; + } + + const pages = targets.filter((t: any) => t.type === "page"); + // Superhuman tabs first: most likely to be responsive and on the right profile. + const ordered = [ + ...pages.filter((t: any) => t.url?.includes("superhuman.com")), + ...pages.filter((t: any) => !t.url?.includes("superhuman.com")), + ]; + + for (const target of ordered) { + let client: CDP.Client | null = null; + try { + client = await CDP({ target: target.id, host, port }); + await withTimeout(client.Network.enable(), "Network.enable"); + const { cookies } = await withTimeout( + client.Network.getCookies({ urls: COOKIE_URLS }), + "Network.getCookies" + ); + if (cookies?.length) return cookies; + } catch { + // This target wedged or errored — try the next one. + } finally { + if (client) { + try { + await client.close(); + } catch { + // ignore + } + } + } + } + return null; +} + +/** + * Read Superhuman's session cookies out of the live browser via CDP and + * format them as a `Cookie` request header. + * + * Prefers the BROWSER-level target (`Storage.getCookies`), falling back to page + * targets (`Network.getCookies`). + * + * Why not page targets first: the extension's own targets refuse CDP attachment + * (see the module header), which is why this path reads cookies at all — but + * page targets are not dependable either. A CDP command routed through a page + * goes through its renderer, and a busy renderer simply never answers. Measured + * against a live browser with six page targets, `Network.getCookies` hung + * indefinitely on one while the other five answered in milliseconds, and *which* + * one hangs drifts (a tab that hung on one probe answered three hours later, + * while a different tab had started hanging). It tracks renderer busyness, not + * the site, so no page target can be assumed responsive. The browser endpoint + * has no renderer and cannot wedge. + * + * Why keep page targets at all: the browser-level read is verified against + * Chromium, but the desktop deployment is Electron (port 9252) and has not been + * verified to serve `Storage` on its browser endpoint. Falling back means the + * worst case there is the previous behaviour, not a regression. + * + * Both routes are bounded, and the fallback advances past a target that does + * not answer instead of betting on one tab — so a wedged renderer costs a + * timeout, not the refresh. + * + * Returns null when no browser is reachable or no Superhuman cookies exist. + */ +export async function readSessionCookieHeader( + port = getCDPPort() +): Promise { + const host = getCDPHost(); + + const fromBrowser = await readCookiesFromBrowserTarget(host, port); + const header = toCookieHeader(fromBrowser ?? undefined); + if (header) return header; + + return toCookieHeader((await readCookiesFromPageTargets(host, port)) ?? undefined); +} + /** Reject rather than wait forever — an unbounded CDP wait is the bug above. */ function withTimeout(p: Promise, what: string, ms = CDP_TIMEOUT_MS): Promise { return new Promise((resolve, reject) => { From efdc5eadb00e40b8577bb2b5afd54b29705c8a8d Mon Sep 17 00:00:00 2001 From: edwinhu Date: Thu, 16 Jul 2026 16:21:07 -0400 Subject: [PATCH 3/5] fix: harden the cookie read (adversarial review round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent adversarial reviews (a subagent and codex) converged on the same findings. Addressed here, plus one thing both missed that fell out of testing their claims. Fixed: - Bound the CDP attach. withTimeout only wrapped commands; `await CDP(...)` sat outside it, so a target advertising a websocket endpoint that never completes its handshake would hang before any bounded call ran — defeating the claim that both routes are bounded. (codex) - One global deadline for the whole read, replacing per-call timeouts. The page sweep was pages x 2 x timeout with no ceiling: Network.enable and getCookies were each timed separately, so six tabs could cost 120s, and a browser with dozens of tabs could stall the CLI for minutes. Measured: one wedged tab already burns the full 10s. (both) - Honour cookie PATH. Storage.getCookies takes no `urls` filter and returns the whole store, so the domain-only filter admitted cookies the old Network.getCookies({urls}) excluded. `csrf=current; Path=/` next to `csrf=old; Path=/legacy` would have sent both and let the backend choose. Now implements RFC 6265 §5.1.4 path-match against the real backend path. (both) - Validate CDP_TIMEOUT_MS. parseInt("garbage") is NaN and setTimeout(fn, NaN) fires immediately, so a config typo made every call "time out" and refresh return null — a silent stale token. Non-finite/non-positive now fall back to the default. (both) - Real unit tests for the pure logic, which had none: domain-matching (subdomains, host-only, label-vs-substring, case), path-matching, and toCookieHeader (media.* exclusion, path-scoped duplicates). 13 new tests. - Drop the Storage guard. chrome-remote-interface builds command stubs from /json/protocol fetched from the browser, not per-target, so `client.Storage?. getCookies` is always a function and the guard could never detect a browser that does not serve Storage. Such a browser rejects at call time and hits the catch. The comment claimed otherwise. (subagent) Known limitation, documented rather than half-fixed: Both reviews flagged that the fallback triggers on EMPTY, not on WRONG — a populated-but-stale default-context jar is returned and the page path never runs. Real: Storage.getCookies reads the default browser context, and a page can live in another (incognito, or an Electron partition). Gating on real authentication was implemented and then reverted, for two reasons. First, sessions.getCsrfToken is not an auth check: measured against the live backend it returns 200 and a csrfToken with bogus cookies AND with no Cookie header at all — so the pre-existing isSessionRefreshHealthy docstring ("a real proof the session is live") is wrong, and its claim is corrected here. Second, the only real proof is sessions.getTokens, and looping that over every candidate turns a fast "no session" failure into a full page sweep — which broke three read-hang regression tests (5s timeouts), the suite guarding exactly this class of bug. Trading a real regression for a theoretical fix is not worth it. The right fix is to enumerate browserContextId from CDP.List and read each context via Storage.getCookies({ browserContextId }) — no page attach, no sweep, no cost when only the default context exists. Left as a follow-up. Verified live: browser-level 5 cookies in 29ms; forced page-fallback 5 cookies in 11ms; both routes agree; dead port returns null; CDP_TIMEOUT_MS=garbage still works instead of silently disabling refresh. Tests 445 pass / 6 fail (the 6 are pre-existing live E2E --attach; +13 over main is the new tests). Hang regressions green. tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01UZvrft29zMGgks5abHVtVK --- src/__tests__/session-refresh.test.ts | 85 +++++++++- src/session-refresh.ts | 226 +++++++++++++++++++++----- 2 files changed, 270 insertions(+), 41 deletions(-) diff --git a/src/__tests__/session-refresh.test.ts b/src/__tests__/session-refresh.test.ts index a83b6d3..ca3d90a 100644 --- a/src/__tests__/session-refresh.test.ts +++ b/src/__tests__/session-refresh.test.ts @@ -1,9 +1,92 @@ -import { test, expect } from "bun:test"; +import { describe, test, expect } from "bun:test"; import { + cookieDomainMatches, + cookiePathMatches, readSessionCookieHeader, refreshViaSessionCookies, + toCookieHeader, } from "../session-refresh"; +describe("cookieDomainMatches", () => { + test("dot-prefixed cookies cover the domain and subdomains", () => { + expect(cookieDomainMatches(".superhuman.com", "accounts.superhuman.com")).toBe(true); + expect(cookieDomainMatches(".superhuman.com", "superhuman.com")).toBe(true); + }); + + test("host-only cookies must match exactly", () => { + expect(cookieDomainMatches("accounts.superhuman.com", "accounts.superhuman.com")).toBe(true); + // media.* carries its own device-id; it must not reach accounts/mail. + expect(cookieDomainMatches("media.superhuman.com", "accounts.superhuman.com")).toBe(false); + }); + + test("matches labels, not substrings", () => { + expect(cookieDomainMatches(".notsuperhuman.com", "accounts.superhuman.com")).toBe(false); + expect(cookieDomainMatches("superhuman.com.evil.test", "superhuman.com")).toBe(false); + }); + + test("is case-insensitive", () => { + expect(cookieDomainMatches(".SuperHuman.COM", "Accounts.Superhuman.com")).toBe(true); + }); +}); + +describe("cookiePathMatches", () => { + test('cookie-path "/" matches any request path', () => { + expect(cookiePathMatches("/", "/~backend/v3/")).toBe(true); + expect(cookiePathMatches("/", "/")).toBe(true); + }); + + test("a deeper cookie-path does not match a shallower request", () => { + // The bug this guards: Storage.getCookies has no `urls` filter, so a + // /legacy-scoped duplicate would otherwise ride along. + expect(cookiePathMatches("/legacy", "/~backend/v3/")).toBe(false); + }); + + test("prefix matches only at a boundary", () => { + expect(cookiePathMatches("/~backend", "/~backend/v3/")).toBe(true); + expect(cookiePathMatches("/~back", "/~backend/v3/")).toBe(false); + }); + + test("empty path is treated as /", () => { + expect(cookiePathMatches("", "/~backend/v3/")).toBe(true); + }); +}); + +describe("toCookieHeader", () => { + const c = (name: string, domain: string, path = "/") => ({ + name, + value: `v_${name}`, + domain, + path, + }); + + test("keeps cookies in scope for the backend path", () => { + expect(toCookieHeader([c("csrf", ".superhuman.com")])).toBe("csrf=v_csrf"); + }); + + test("drops other subdomains' same-named cookies", () => { + const header = toCookieHeader([ + c("device-id", "accounts.superhuman.com"), + c("device-id", "media.superhuman.com"), + ]); + // Exactly one device-id — not a duplicate with media's value. + expect(header).toBe("device-id=v_device-id"); + }); + + test("drops path-scoped duplicates the browser would not send", () => { + const header = toCookieHeader([ + c("csrf", ".superhuman.com", "/"), + { name: "csrf", value: "stale", domain: ".superhuman.com", path: "/legacy" }, + ]); + expect(header).toBe("csrf=v_csrf"); + }); + + test("null when nothing applies or the list is empty", () => { + expect(toCookieHeader([c("x", "example.com")])).toBeNull(); + expect(toCookieHeader([])).toBeNull(); + expect(toCookieHeader(undefined)).toBeNull(); + }); +}); + // An unused port: nothing is listening, so CDP discovery must fail cleanly. const DEAD_PORT = 9; diff --git a/src/session-refresh.ts b/src/session-refresh.ts index 5dbaa41..cef9b50 100644 --- a/src/session-refresh.ts +++ b/src/session-refresh.ts @@ -63,23 +63,35 @@ interface GetTokensResponse { }; } -/** Upper bound on the cookie read. Override with CDP_TIMEOUT_MS. */ -const CDP_TIMEOUT_MS = parseInt(process.env.CDP_TIMEOUT_MS || "10000", 10); +const DEFAULT_CDP_TIMEOUT_MS = 10_000; + +/** + * Upper bound on a single CDP round-trip, and on the whole cookie read. + * + * Validated rather than trusted: an unparseable or non-positive value would + * make setTimeout fire immediately (setTimeout(fn, NaN) is setTimeout(fn, 0)), + * so every call would "time out", the read would return null, and the caller + * would silently keep a stale token — a config typo turning into an invisible + * auth failure. + */ +function cdpTimeoutMs(): number { + const raw = process.env.CDP_TIMEOUT_MS; + if (!raw) return DEFAULT_CDP_TIMEOUT_MS; + const n = Number(raw); + if (!Number.isFinite(n) || n <= 0) return DEFAULT_CDP_TIMEOUT_MS; + return n; +} /** Hosts whose cookies authenticate the backend calls below. */ const COOKIE_HOSTS = ["accounts.superhuman.com", "mail.superhuman.com"]; /** - * Does a cookie apply to `host`, per RFC 6265 domain-matching? + * Does a cookie apply to `host`, per RFC 6265 §5.1.3 domain-matching? * * A leading dot means the cookie covers the domain and its subdomains; - * otherwise it is host-only and must match exactly. Storage.getCookies returns - * the entire store with no `urls` filter, so this reproduces the scoping that - * Network.getCookies({ urls }) used to do for us — without it we would also - * pick up same-named cookies from other superhuman.com subdomains (media.* - * carries its own device-id) and send duplicates with the wrong values. + * otherwise it is host-only and must match exactly. */ -function cookieAppliesToHost(domain: string, host: string): boolean { +export function cookieDomainMatches(domain: string, host: string): boolean { const d = domain.toLowerCase(); const h = host.toLowerCase(); if (d.startsWith(".")) { @@ -89,23 +101,70 @@ function cookieAppliesToHost(domain: string, host: string): boolean { return d === h; } -/** Shape both cookie reads return; only name/value/domain are used. */ +/** + * Does a cookie's path apply to `requestPath`, per RFC 6265 §5.1.4? + * + * Cookie-path "/" matches everything; "/foo" matches "/foo" and "/foo/bar" but + * NOT "/". Storage.getCookies takes no `urls` filter and returns the entire + * store, so without this we would admit cookies the browser would never send — + * Network.getCookies({ urls }) applied path scoping for us. A store holding + * `csrf=current; Path=/` alongside `csrf=old; Path=/legacy` would otherwise + * yield `csrf=current; csrf=old` and let the backend pick. + */ +export function cookiePathMatches(cookiePath: string, requestPath: string): boolean { + const p = cookiePath || "/"; + if (p === requestPath) return true; + if (!requestPath.startsWith(p)) return false; + return p.endsWith("/") || requestPath[p.length] === "/"; +} + +/** + * Does a cookie apply to a request for `host` at `path` over HTTPS? + * + * Reproduces the scoping Network.getCookies({ urls: COOKIE_URLS }) did for us. + * Without it we would pick up same-named cookies from other superhuman.com + * subdomains (media.* carries its own device-id) and path-scoped duplicates. + */ +function cookieApplies(c: RawCookie, host: string, path: string): boolean { + if (!cookieDomainMatches(c.domain, host)) return false; + return cookiePathMatches(c.path ?? "/", path); +} + +/** Shape both cookie reads return. */ interface RawCookie { name: string; value: string; domain: string; + path?: string; } +/** Path the backend calls actually hit — cookies must be in scope for it. */ +const BACKEND_PATH = "/~backend/v3/"; + /** Build the Cookie header from a raw cookie list, or null if none apply. */ -function toCookieHeader(cookies: RawCookie[] | undefined): string | null { +export function toCookieHeader(cookies: RawCookie[] | undefined): string | null { if (!cookies?.length) return null; const relevant = cookies.filter((c) => - COOKIE_HOSTS.some((h) => cookieAppliesToHost(c.domain, h)) + COOKIE_HOSTS.some((h) => cookieApplies(c, h, BACKEND_PATH)) ); if (!relevant.length) return null; return relevant.map((c) => `${c.name}=${c.value}`).join("; "); } +/** A deadline shared across every step of one cookie read. */ +interface Deadline { + remaining(): number; + expired(): boolean; +} + +function deadlineIn(ms: number): Deadline { + const end = Date.now() + ms; + return { + remaining: () => Math.max(0, end - Date.now()), + expired: () => Date.now() >= end, + }; +} + /** * Read cookies from the BROWSER-level target via Storage.getCookies. * @@ -117,7 +176,8 @@ function toCookieHeader(cookies: RawCookie[] | undefined): string | null { */ async function readCookiesFromBrowserTarget( host: string, - port: number + port: number, + deadline: Deadline ): Promise { // Attach by websocket URL. target:"browser" does NOT work — chrome-remote- // interface treats a target string as a target id and looks it up in @@ -126,7 +186,11 @@ async function readCookiesFromBrowserTarget( // "no network call without a browser" still hold. let browserWsUrl: string; try { - const ver = (await CDP.Version({ host, port })) as { webSocketDebuggerUrl?: string }; + const ver = (await withTimeout( + CDP.Version({ host, port }), + "CDP.Version", + deadline.remaining() + )) as { webSocketDebuggerUrl?: string }; if (!ver.webSocketDebuggerUrl) return null; browserWsUrl = ver.webSocketDebuggerUrl; } catch { @@ -135,14 +199,23 @@ async function readCookiesFromBrowserTarget( let client: CDP.Client | null = null; try { - client = await CDP({ target: browserWsUrl, host, port }); - if (!client.Storage?.getCookies) return null; + // The attach itself must be bounded: a target can advertise a websocket + // endpoint and then never complete the handshake, which would hang here + // before any bounded command ran. + client = await withTimeout( + CDP({ target: browserWsUrl, host, port }), + "CDP attach (browser)", + deadline.remaining() + ); const { cookies } = await withTimeout( client.Storage.getCookies({}), - "Storage.getCookies" + "Storage.getCookies", + deadline.remaining() ); return cookies ?? null; } catch { + // Includes an Electron/browser that does not serve Storage on its browser + // endpoint — the command rejects and we fall back to page targets. return null; } finally { if (client) { @@ -163,34 +236,44 @@ async function readCookiesFromBrowserTarget( * when one does not answer within the timeout, rather than betting everything * on a single tab. */ -async function readCookiesFromPageTargets( +async function* readCookiesFromPageTargets( host: string, - port: number -): Promise { + port: number, + deadline: Deadline +): AsyncGenerator { let targets: any[]; try { - targets = await CDP.List({ host, port }); + targets = await withTimeout(CDP.List({ host, port }), "CDP.List", deadline.remaining()); } catch { - return null; + return; } const pages = targets.filter((t: any) => t.type === "page"); - // Superhuman tabs first: most likely to be responsive and on the right profile. + // Superhuman tabs first: most likely responsive and on the right profile. const ordered = [ ...pages.filter((t: any) => t.url?.includes("superhuman.com")), ...pages.filter((t: any) => !t.url?.includes("superhuman.com")), ]; for (const target of ordered) { + // One shared deadline across the whole sweep — otherwise N wedged tabs cost + // N x timeout (measured: a single wedged tab burns the full 10s), and a + // browser with dozens of tabs could stall the CLI for minutes. + if (deadline.expired()) return; let client: CDP.Client | null = null; try { - client = await CDP({ target: target.id, host, port }); - await withTimeout(client.Network.enable(), "Network.enable"); + client = await withTimeout( + CDP({ target: target.id, host, port }), + "CDP attach (page)", + deadline.remaining() + ); + await withTimeout(client.Network.enable(), "Network.enable", deadline.remaining()); const { cookies } = await withTimeout( client.Network.getCookies({ urls: COOKIE_URLS }), - "Network.getCookies" + "Network.getCookies", + deadline.remaining() ); - if (cookies?.length) return cookies; + if (cookies?.length) yield cookies; } catch { // This target wedged or errored — try the next one. } finally { @@ -203,7 +286,6 @@ async function readCookiesFromPageTargets( } } } - return null; } /** @@ -238,17 +320,77 @@ async function readCookiesFromPageTargets( export async function readSessionCookieHeader( port = getCDPPort() ): Promise { + for await (const header of cookieHeaderCandidates(port)) return header; + return null; +} + +/** + * Yield candidate Cookie headers, best first, lazily. + * + * Browser-level first, then each page target. Lazy on purpose: when the first + * candidate authenticates, the page sweep never runs. + * + * Why more than one candidate — and why the caller must gate on AUTHENTICATION + * rather than on cookies merely existing: + * + * `Storage.getCookies` reads the DEFAULT browser context. A page can live in a + * different context (incognito, Target.createBrowserContext) or, on Electron, a + * `webPreferences.partition: "persist:..."` session — each with its own cookie + * jar. So the browser-level read can return cookies that are present but belong + * to the wrong jar: a stale, signed-out login in the default context while the + * live Superhuman session sits in a partition. Presence proves nothing — + * cookies survive sign-out (see isSessionRefreshHealthy). Returning the first + * non-empty header would hand back stale cookies and never fall back, which is + * exactly the silent stale-token failure this module exists to prevent. + */ +async function* cookieHeaderCandidates( + port: number, + deadline: Deadline = deadlineIn(cdpTimeoutMs()) +): AsyncGenerator { const host = getCDPHost(); + const seen = new Set(); - const fromBrowser = await readCookiesFromBrowserTarget(host, port); - const header = toCookieHeader(fromBrowser ?? undefined); - if (header) return header; + const fromBrowser = toCookieHeader( + (await readCookiesFromBrowserTarget(host, port, deadline)) ?? undefined + ); + if (fromBrowser) { + seen.add(fromBrowser); + yield fromBrowser; + } - return toCookieHeader((await readCookiesFromPageTargets(host, port)) ?? undefined); + for await (const cookies of readCookiesFromPageTargets(host, port, deadline)) { + const header = toCookieHeader(cookies); + // Skip a page whose jar is the same as one already tried (the common case + // on Chromium, where every page shares the default context). + if (header && !seen.has(header)) { + seen.add(header); + yield header; + } + } } +/** + * KNOWN LIMITATION — the first usable cookie jar wins. + * + * Storage.getCookies reads the DEFAULT browser context, and a page can live in + * another (incognito, or an Electron `webPreferences.partition`). So a populated + * but stale default-context jar is returned without trying the page fallback. + * + * Gating on real authentication instead was tried and reverted: sessions. + * getCsrfToken is NOT an auth check (measured: it returns 200 and a csrfToken + * with bogus cookies, and with no Cookie header at all), so the only real proof + * is sessions.getTokens — and looping that over every candidate turns a fast + * "no session" failure into a full page sweep per account, which the read-hang + * regression tests reject outright. + * + * Doing this properly means enumerating browserContextId from CDP.List and + * reading each context via Storage.getCookies({ browserContextId }) — no page + * attach, no sweep, no cost when there is only the default context. Left for a + * follow-up rather than shipped half-done. + */ + /** Reject rather than wait forever — an unbounded CDP wait is the bug above. */ -function withTimeout(p: Promise, what: string, ms = CDP_TIMEOUT_MS): Promise { +function withTimeout(p: Promise, what: string, ms = cdpTimeoutMs()): Promise { return new Promise((resolve, reject) => { const timer = setTimeout(() => reject(new Error(`${what} timed out after ${ms}ms`)), ms); p.then( @@ -259,18 +401,22 @@ function withTimeout(p: Promise, what: string, ms = CDP_TIMEOUT_MS): Promi } /** - * Health probe: is the browser's Superhuman session usable for refresh? + * Health probe: are Superhuman session cookies reachable in the browser? * - * Reads the cookies and exchanges them for a CSRF token — cheap, read-only, - * and a real proof the session is live (cookie presence alone isn't: they - * survive sign-out). Used by `superhuman doctor`. + * NOT a proof that the session is live. sessions.getCsrfToken returns 200 and a + * token even with bogus cookies, or none at all (measured against the live + * backend), so it cannot distinguish a signed-in session from a signed-out one + * — and cookies survive sign-out. Proving liveness needs sessions.getTokens, + * which requires an account and mints real credentials; too heavy for a probe. + * + * So this answers only "is there a browser with Superhuman cookies we could + * try", which is what `superhuman doctor` needs to decide whether to tell the + * user to relaunch an app. False positives are possible when signed out. */ export async function isSessionRefreshHealthy( port = getCDPPort() ): Promise { - const cookieHeader = await readSessionCookieHeader(port); - if (!cookieHeader) return false; - return (await getCsrfToken(cookieHeader)) !== null; + return (await readSessionCookieHeader(port)) !== null; } /** Fetch a CSRF token for the session (required by sessions.getTokens). */ From e2a51b0ad95d2463fc42f4353c72a180466e16fd Mon Sep 17 00:00:00 2001 From: edwinhu Date: Thu, 16 Jul 2026 16:31:03 -0400 Subject: [PATCH 4/5] fix: cap per-target time; drop unreachable candidate machinery (round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 adversarial review found that round 1 traded one bug for another, and that a chunk of what it added is dead. 1. The page sweep could not advance past a HUNG target (HIGH). Each per-target call was handed deadline.remaining() — the whole budget. So the first tab that hangs (rather than errors) consumed all 10s and the deadline then skipped every remaining candidate. Round 1 fixed N x timeout by replacing it with one-target-eats-everything: the sweep could only advance on a fast failure, never on a hang, which is the single failure mode it exists to survive. Reviewer measured, live: a wedged tab burned 10002ms and the sweep yielded nothing, while three responsive tabs sat further down the list answering in ~20ms. That is a silent stale token — the exact failure this module prevents. Cap each target at min(remaining, PER_TARGET_CAP_MS=2s). Responsive targets answer in ~20ms, so 2s is generous. Verified by reproducing the reviewer's scenario: force the fallback, put all 4 wedged targets FIRST, and disguise the superhuman tab's URL so preference ordering cannot rescue it. Before: yielded 0 in 10002ms. Now: advances past all 4 and returns 5 cookies in 8073ms. (4 of 8 live page targets are currently wedged — this is common, not exotic.) 2. The multi-candidate generator was unreachable (MEDIUM). readSessionCookieHeader returned the FIRST yielded header, so `seen` could never dedupe and no page candidate after the first ever ran. Worse, its docstring claimed the caller "gates on AUTHENTICATION", while the KNOWN LIMITATION block directly below admitted it gates on non-emptiness — two contradictory explanations in one file. Replaced with the plain sequential read it actually was, keeping one honest limitation note. Also fixed: isSessionRefreshHealthy's docstring claimed exchanging cookies for a CSRF token was "a real proof the session is live". It is not — measured, that endpoint returns 200 and a csrfToken with bogus cookies and with no Cookie header at all. It now says what it actually checks (cookies are reachable) and that false positives are possible when signed out. Confirmed correct by the reviewer and left alone: deadline arithmetic at the 0-boundary, cdpTimeoutMs validation across every dangerous input, generator cleanup on early return (no websocket leak), and Storage-guard removal. Accepted, not fixed: COOKIE_HOSTS admits mail.* cookies though requests only go to accounts.* — over-broad, but faithfully reproduces the old Network.getCookies({urls}) scoping, and narrowing it risks dropping cookies the backend needs. Live store has no host-only mail.* cookies. BACKEND_PATH is a prefix so a cookie pathed to the exact endpoint would be under-admitted; all 8 live cookies are Path=/. Tests 445 pass / 6 fail (6 pre-existing live E2E --attach). Hang regressions green. tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01UZvrft29zMGgks5abHVtVK --- src/session-refresh.ts | 90 ++++++++++++++++++------------------------ 1 file changed, 38 insertions(+), 52 deletions(-) diff --git a/src/session-refresh.ts b/src/session-refresh.ts index cef9b50..178e085 100644 --- a/src/session-refresh.ts +++ b/src/session-refresh.ts @@ -65,6 +65,18 @@ interface GetTokensResponse { const DEFAULT_CDP_TIMEOUT_MS = 10_000; +/** + * Per-target ceiling inside the page sweep. + * + * Without it, handing each call the whole remaining budget means the first tab + * that HANGS (rather than errors) consumes everything and the deadline then + * skips every later candidate — measured: a wedged tab burned 10002ms and the + * sweep yielded nothing while three responsive tabs sat further down the list. + * The sweep could only advance on a fast failure, never on a hang, which is the + * one failure mode it exists to survive. Responsive targets answer in ~20ms. + */ +const PER_TARGET_CAP_MS = 2_000; + /** * Upper bound on a single CDP round-trip, and on the whole cookie read. * @@ -262,16 +274,17 @@ async function* readCookiesFromPageTargets( if (deadline.expired()) return; let client: CDP.Client | null = null; try { + const budget = () => Math.min(deadline.remaining(), PER_TARGET_CAP_MS); client = await withTimeout( CDP({ target: target.id, host, port }), "CDP attach (page)", - deadline.remaining() + budget() ); - await withTimeout(client.Network.enable(), "Network.enable", deadline.remaining()); + await withTimeout(client.Network.enable(), "Network.enable", budget()); const { cookies } = await withTimeout( client.Network.getCookies({ urls: COOKIE_URLS }), "Network.getCookies", - deadline.remaining() + budget() ); if (cookies?.length) yield cookies; } catch { @@ -320,53 +333,19 @@ async function* readCookiesFromPageTargets( export async function readSessionCookieHeader( port = getCDPPort() ): Promise { - for await (const header of cookieHeaderCandidates(port)) return header; - return null; -} - -/** - * Yield candidate Cookie headers, best first, lazily. - * - * Browser-level first, then each page target. Lazy on purpose: when the first - * candidate authenticates, the page sweep never runs. - * - * Why more than one candidate — and why the caller must gate on AUTHENTICATION - * rather than on cookies merely existing: - * - * `Storage.getCookies` reads the DEFAULT browser context. A page can live in a - * different context (incognito, Target.createBrowserContext) or, on Electron, a - * `webPreferences.partition: "persist:..."` session — each with its own cookie - * jar. So the browser-level read can return cookies that are present but belong - * to the wrong jar: a stale, signed-out login in the default context while the - * live Superhuman session sits in a partition. Presence proves nothing — - * cookies survive sign-out (see isSessionRefreshHealthy). Returning the first - * non-empty header would hand back stale cookies and never fall back, which is - * exactly the silent stale-token failure this module exists to prevent. - */ -async function* cookieHeaderCandidates( - port: number, - deadline: Deadline = deadlineIn(cdpTimeoutMs()) -): AsyncGenerator { const host = getCDPHost(); - const seen = new Set(); + const deadline = deadlineIn(cdpTimeoutMs()); const fromBrowser = toCookieHeader( (await readCookiesFromBrowserTarget(host, port, deadline)) ?? undefined ); - if (fromBrowser) { - seen.add(fromBrowser); - yield fromBrowser; - } + if (fromBrowser) return fromBrowser; for await (const cookies of readCookiesFromPageTargets(host, port, deadline)) { const header = toCookieHeader(cookies); - // Skip a page whose jar is the same as one already tried (the common case - // on Chromium, where every page shares the default context). - if (header && !seen.has(header)) { - seen.add(header); - yield header; - } + if (header) return header; } + return null; } /** @@ -376,17 +355,18 @@ async function* cookieHeaderCandidates( * another (incognito, or an Electron `webPreferences.partition`). So a populated * but stale default-context jar is returned without trying the page fallback. * - * Gating on real authentication instead was tried and reverted: sessions. - * getCsrfToken is NOT an auth check (measured: it returns 200 and a csrfToken - * with bogus cookies, and with no Cookie header at all), so the only real proof - * is sessions.getTokens — and looping that over every candidate turns a fast - * "no session" failure into a full page sweep per account, which the read-hang - * regression tests reject outright. + * Gating on real authentication instead was implemented and reverted, for two + * reasons. sessions.getCsrfToken is NOT an auth check — measured against the + * live backend it returns 200 and a csrfToken with bogus cookies, and with no + * Cookie header at all — so the only real proof is sessions.getTokens, and + * looping that over every candidate turns a fast "no session" failure into a + * full page sweep per account. That broke three read-hang regression tests, the + * suite guarding exactly this class of bug. A real regression is not worth a + * theoretical fix. * - * Doing this properly means enumerating browserContextId from CDP.List and + * Doing it properly means enumerating browserContextId from CDP.List and * reading each context via Storage.getCookies({ browserContextId }) — no page - * attach, no sweep, no cost when there is only the default context. Left for a - * follow-up rather than shipped half-done. + * attach, no sweep, and no cost when only the default context exists. Follow-up. */ /** Reject rather than wait forever — an unbounded CDP wait is the bug above. */ @@ -394,8 +374,14 @@ function withTimeout(p: Promise, what: string, ms = cdpTimeoutMs()): Promi return new Promise((resolve, reject) => { const timer = setTimeout(() => reject(new Error(`${what} timed out after ${ms}ms`)), ms); p.then( - (v) => { clearTimeout(timer); resolve(v); }, - (e) => { clearTimeout(timer); reject(e); } + (v) => { + clearTimeout(timer); + resolve(v); + }, + (e) => { + clearTimeout(timer); + reject(e); + } ); }); } From bcefceb4cee65fc0a014074ce41165cb70d8513a Mon Sep 17 00:00:00 2001 From: edwinhu Date: Thu, 16 Jul 2026 16:33:50 -0400 Subject: [PATCH 5/5] fix: RFC 6265 cookie ordering; close late-landing attaches (round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's final adversarial pass found two the subagent missed. 1. Cookie ordering (medium). RFC 6265 §5.4 orders matching cookies by descending path length; Storage.getCookies returns the store in unspecified order and we preserved it. With `session` at both `/` and `/~backend` we emitted `session=root; session=backend`, where a browser sends the more specific first. A backend taking the first occurrence would authenticate with the wrong value — the same "let the server pick" failure the path filter was added to prevent, one level down. Now sorted longest-path-first. Mutation-verified: removing the sort fails the new test. 2. Late-landing attach leaked a websocket (low). withTimeout only stops waiting — it cannot abort a connect. A slow handshake completing after we gave up left a live websocket with no reference, one per attempt. attachWithTimeout now closes the client if it arrives after the timeout. Codex confirmed correct and left alone: the per-target cap's zero-budget behaviour, domain matching, path boundary matching, generator finalization (client closed on early return), no unhandled rejections, and browser/page route equivalence beyond the documented wrong-jar limitation. Test coverage remains thinner than ideal — the CDP paths themselves (attach, fallback-after-empty, ordering preference, the 2s/10s bounds) are exercised live rather than in unit tests, since faking them well enough to be meaningful means faking chrome-remote-interface wholesale. Noted rather than hidden. Tests 446 pass / 6 fail (6 pre-existing live E2E --attach). Hang regressions green. tsc --noEmit clean. Live: 5 cookies in 28ms, real CSRF exchange against accounts.superhuman.com returns true. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01UZvrft29zMGgks5abHVtVK --- src/__tests__/session-refresh.test.ts | 11 ++++++ src/session-refresh.ts | 49 ++++++++++++++++++++++++--- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/__tests__/session-refresh.test.ts b/src/__tests__/session-refresh.test.ts index ca3d90a..dbd4244 100644 --- a/src/__tests__/session-refresh.test.ts +++ b/src/__tests__/session-refresh.test.ts @@ -80,6 +80,17 @@ describe("toCookieHeader", () => { expect(header).toBe("csrf=v_csrf"); }); + test("orders longer paths first, per RFC 6265 §5.4", () => { + // Storage.getCookies' array order is unspecified. A backend taking the + // FIRST occurrence of a duplicated name must see the more specific cookie, + // exactly as a browser would send it. + const header = toCookieHeader([ + { name: "session", value: "root", domain: "accounts.superhuman.com", path: "/" }, + { name: "session", value: "backend", domain: "accounts.superhuman.com", path: "/~backend" }, + ]); + expect(header).toBe("session=backend; session=root"); + }); + test("null when nothing applies or the list is empty", () => { expect(toCookieHeader([c("x", "example.com")])).toBeNull(); expect(toCookieHeader([])).toBeNull(); diff --git a/src/session-refresh.ts b/src/session-refresh.ts index 178e085..6b5742b 100644 --- a/src/session-refresh.ts +++ b/src/session-refresh.ts @@ -153,14 +153,24 @@ interface RawCookie { /** Path the backend calls actually hit — cookies must be in scope for it. */ const BACKEND_PATH = "/~backend/v3/"; -/** Build the Cookie header from a raw cookie list, or null if none apply. */ +/** + * Build the Cookie header from a raw cookie list, or null if none apply. + * + * Ordered per RFC 6265 §5.4: longer paths first. Storage.getCookies returns the + * store in unspecified order, and a server that takes the FIRST occurrence of a + * duplicated name would otherwise see the wrong value — e.g. `session` at both + * `/` and `/~backend` must send the `/~backend` one first, as a browser would. + */ export function toCookieHeader(cookies: RawCookie[] | undefined): string | null { if (!cookies?.length) return null; const relevant = cookies.filter((c) => COOKIE_HOSTS.some((h) => cookieApplies(c, h, BACKEND_PATH)) ); if (!relevant.length) return null; - return relevant.map((c) => `${c.name}=${c.value}`).join("; "); + const ordered = [...relevant].sort( + (a, b) => (b.path ?? "/").length - (a.path ?? "/").length + ); + return ordered.map((c) => `${c.name}=${c.value}`).join("; "); } /** A deadline shared across every step of one cookie read. */ @@ -214,7 +224,7 @@ async function readCookiesFromBrowserTarget( // The attach itself must be bounded: a target can advertise a websocket // endpoint and then never complete the handshake, which would hang here // before any bounded command ran. - client = await withTimeout( + client = await attachWithTimeout( CDP({ target: browserWsUrl, host, port }), "CDP attach (browser)", deadline.remaining() @@ -275,7 +285,7 @@ async function* readCookiesFromPageTargets( let client: CDP.Client | null = null; try { const budget = () => Math.min(deadline.remaining(), PER_TARGET_CAP_MS); - client = await withTimeout( + client = await attachWithTimeout( CDP({ target: target.id, host, port }), "CDP attach (page)", budget() @@ -369,6 +379,37 @@ export async function readSessionCookieHeader( * attach, no sweep, and no cost when only the default context exists. Follow-up. */ +/** + * Bound an attach whose result must be closed if it lands after the timeout. + * + * withTimeout only stops waiting — it cannot abort the underlying connect. A + * slow handshake that completes after we gave up would otherwise leave a live + * websocket with no reference, leaking one per attempt. + */ +function attachWithTimeout( + p: Promise, + what: string, + ms: number +): Promise { + const bounded = withTimeout(p, what, ms); + bounded.catch(() => { + // We are no longer waiting; close it if it ever arrives. + p.then( + (client) => { + try { + void client.close(); + } catch { + // ignore + } + }, + () => { + // already rejected; nothing to close + } + ); + }); + return bounded; +} + /** Reject rather than wait forever — an unbounded CDP wait is the bug above. */ function withTimeout(p: Promise, what: string, ms = cdpTimeoutMs()): Promise { return new Promise((resolve, reject) => {