diff --git a/src/__tests__/session-refresh.test.ts b/src/__tests__/session-refresh.test.ts index 99c88e3..dbd4244 100644 --- a/src/__tests__/session-refresh.test.ts +++ b/src/__tests__/session-refresh.test.ts @@ -1,9 +1,103 @@ -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("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(); + expect(toCookieHeader(undefined)).toBeNull(); + }); +}); + // An unused port: nothing is listening, so CDP discovery must fail cleanly. const DEAD_PORT = 9; @@ -18,6 +112,34 @@ test("refreshViaSessionCookies returns null (never throws) with no browser", asy .toBeNull(); }); +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; + const order: string[] = []; + CDP.Version = async (...args: unknown[]) => { + order.push("browser"); + return realVersion(...args); + }; + CDP.List = async (...args: unknown[]) => { + order.push("pages"); + return realList(...args); + }; + try { + await readSessionCookieHeader(DEAD_PORT); + // 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; + } +}); + 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..6b5742b 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,6 +49,7 @@ 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). */ @@ -58,41 +63,181 @@ interface GetTokensResponse { }; } +const DEFAULT_CDP_TIMEOUT_MS = 10_000; + /** - * Read Superhuman's session cookies out of the live browser via CDP and - * format them as a `Cookie` request header. + * Per-target ceiling inside the page sweep. * - * 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. + * 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. * - * Returns null when no browser is reachable or no Superhuman cookies exist. + * 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. */ -export async function readSessionCookieHeader( - port = getCDPPort() -): Promise { - const host = getCDPHost(); - let targets: any[]; +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 §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. + */ +export function cookieDomainMatches(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; +} + +/** + * 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. + * + * 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; + 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. */ +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. + * + * 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. + * + * Returns null (never throws) if the endpoint is unreachable or the browser + * does not serve Storage there, so the caller can fall back. + */ +async function readCookiesFromBrowserTarget( + host: string, + 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 + // 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 { - targets = await CDP.List({ host, port }); + const ver = (await withTimeout( + CDP.Version({ host, port }), + "CDP.Version", + deadline.remaining() + )) 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 }); - if (!cookies?.length) return null; - return cookies.map((c) => `${c.name}=${c.value}`).join("; "); + // 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 attachWithTimeout( + CDP({ target: browserWsUrl, host, port }), + "CDP attach (browser)", + deadline.remaining() + ); + const { cookies } = await withTimeout( + client.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) { @@ -106,18 +251,199 @@ export async function readSessionCookieHeader( } /** - * Health probe: is the browser's Superhuman session usable for refresh? + * Fallback: read cookies via a page target's Network.getCookies. * - * 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`. + * 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, + deadline: Deadline +): AsyncGenerator { + let targets: any[]; + try { + targets = await withTimeout(CDP.List({ host, port }), "CDP.List", deadline.remaining()); + } catch { + return; + } + + const pages = targets.filter((t: any) => t.type === "page"); + // 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 { + const budget = () => Math.min(deadline.remaining(), PER_TARGET_CAP_MS); + client = await attachWithTimeout( + CDP({ target: target.id, host, port }), + "CDP attach (page)", + budget() + ); + await withTimeout(client.Network.enable(), "Network.enable", budget()); + const { cookies } = await withTimeout( + client.Network.getCookies({ urls: COOKIE_URLS }), + "Network.getCookies", + budget() + ); + if (cookies?.length) yield cookies; + } catch { + // This target wedged or errored — try the next one. + } finally { + if (client) { + try { + await client.close(); + } catch { + // ignore + } + } + } + } +} + +/** + * 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 deadline = deadlineIn(cdpTimeoutMs()); + + const fromBrowser = toCookieHeader( + (await readCookiesFromBrowserTarget(host, port, deadline)) ?? undefined + ); + if (fromBrowser) return fromBrowser; + + for await (const cookies of readCookiesFromPageTargets(host, port, deadline)) { + const header = toCookieHeader(cookies); + if (header) return header; + } + return null; +} + +/** + * 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 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 it properly means enumerating browserContextId from CDP.List and + * reading each context via Storage.getCookies({ browserContextId }) — no page + * 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) => { + 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: are Superhuman session cookies reachable in the browser? + * + * 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). */