fix: read session cookies from the browser target, not an arbitrary page

[edwinhu]

Follow-up to 635b763. That commit is right about why it reads cookies — the extension's own targets never answer CDP — but the fallback it uses to read them rests on a premise my measurements contradict.

The premise

Uses a page target: Network.getCookies reads the shared browser cookie store [...] and unlike the extension's own targets, page targets respond to CDP reliably.

const target = pages.find(t => t.url.includes("superhuman.com")) ?? pages[0];

The measurement

Probing all six page targets in a live browser, Network.getCookies hung indefinitely on one while the other five answered in milliseconds:

target	result
www.amazon.com/s?k=...	answers
wrds-www.wharton.upenn.edu/pages/get-d…	hangs
music.youtube.com/playlist	answers
read.readwise.io/later	answers
web.morgen.so/	answers
mail.superhuman.com/…	answers

And which target hangs drifts. Three hours earlier music.youtube.com hung reliably (twice, with and without Network.enable) while wrds-www was fine — the exact reverse. It tracks a busy renderer, not a site. So no page target can be assumed responsive, and pages[0] is simply an arbitrary tab.

Why it matters

With no Superhuman tab open, ?? pages[0] attaches to whatever is first. If that renderer is wedged the read never returns — and a hang is not a rejection, so the surrounding 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 — the attachment-download 401 it was written to prevent.

Latent today only because a Superhuman tab happens to be open, so the preferred branch wins. It is not fixed, just unrolled.

The fix

Read from the browser-level target via Storage.getCookies: no renderer to wedge, and no Superhuman tab — or any tab — need be open. The read is bounded (CDP_TIMEOUT_MS, default 10s) so a future stall degrades to null instead of hanging.

Two details worth a reviewer's eye:

• 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 (it TypeErrors). Using CDP.Version() rather than fetch() also keeps discovery on the library's own transport, so the existing "no network call without a browser" test still holds. My first draft used fetch() and broke that test — correctly.
• Storage.getCookies takes no urls filter and returns the whole store, so I scope it 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 cookie names with different values (I hit this — it still worked, which is exactly why it would have been a nasty latent bug).

Verification

Live, against the real browser and the real backend:

• 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
• isSessionRefreshHealthy() → true — a real CSRF exchange against accounts.superhuman.com, i.e. the backend accepts the header this builds
• dead port → null, unchanged

432 pass / 6 fail — exactly matching main (the 6 are pre-existing live E2E --attach failures, unrelated). tsc --noEmit clean — this repo does gate on tsc.

The new "never attaches to a page target" test is mutation-verified: reverting discovery to CDP.List page enumeration fails it.

Note

Based on main (635b763). I left fix/account-sync-chrome-extension (#27) untouched.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UZvrft29zMGgks5abHVtVK

[edwinhu]

Adversarial review: 3 rounds (subagent + codex each), converged

Both reviewers, run independently, found real bugs in my code every round. Summary of what they caught and what changed.

Round 1 — both converged on the same critical flaw

The fallback was gated on empty, not on wrong. Storage.getCookies reads the default browser context; a page can live in another (incognito, or an Electron webPreferences.partition). A populated-but-stale default jar was returned and the page path never ran — so my "worst case on Electron is the previous behaviour" claim was false.

Codex additionally caught that await CDP(...) sat outside withTimeout — so "both routes are bounded" was also false.

Fixed: bounded the attach; one global deadline; RFC 6265 path scoping (the domain-only filter admitted cookies the old Network.getCookies({urls}) excluded); CDP_TIMEOUT_MS validation (parseInt("garbage") → NaN → setTimeout(fn, NaN) fires immediately → every call "times out" → silent stale token); 13 real unit tests for logic that had none.

Reverted: gating on authentication. Two reasons, both measured — sessions.getCsrfToken is not an auth check (returns 200 + a csrfToken with bogus cookies and with no Cookie header at all), so the only real proof is sessions.getTokens; and looping that over candidates broke three read-hang regression tests — the suite guarding this exact bug class. A real regression for a theoretical fix is a bad trade. Documented as a known limitation instead.

That also means the pre-existing isSessionRefreshHealthy docstring was wrong ("a real proof the session is live"). Corrected — doctor can false-positive when signed out.

Round 2 — my round-1 fix had traded one bug for another

Each per-target call got deadline.remaining(), i.e. the whole budget. So the first tab that hangs ate all 10s and the deadline skipped every later candidate. The sweep could only advance on a fast failure — never on a hang, the one mode it exists to survive. Reviewer measured: yielded=0 in 10002ms while three responsive tabs sat further down.

Fixed with min(remaining, PER_TARGET_CAP_MS=2s). Reproduced their scenario to prove it: forced the fallback, put all 4 wedged targets first, disguised the superhuman tab so ordering could not rescue it → advances past all 4, returns 5 cookies in 8073ms. (4 of 8 live page targets are wedged right now — this is common, not exotic.)

Also: the multi-candidate generator was unreachable dead code (nothing consumed past the first yield, so seen never deduped), and its docstring contradicted the limitation note below it. Replaced with the plain sequential read it actually was.

Round 3 — codex found two the subagent missed

• Cookie ordering: RFC 6265 §5.4 wants descending path length; I preserved Storage.getCookies' unspecified order. session at / and /~backend emitted the wrong one first. Mutation-verified fix.
• Late-landing attach leaked a websocket: withTimeout stops waiting but cannot abort a connect. Now closed if it arrives late.

Confirmed correct and left alone: deadline arithmetic at the 0-boundary, cdpTimeoutMs across every dangerous input, generator finalization, Storage-guard removal, no unhandled rejections.

Accepted, not fixed (documented in-code)

• Wrong-jar: the right fix is enumerating browserContextId from CDP.List and reading each via Storage.getCookies({ browserContextId }) — no page attach, no sweep, no cost when only the default context exists. Follow-up.
• COOKIE_HOSTS admits mail.* though requests go only to accounts.* — over-broad, but faithfully reproduces prior scoping; narrowing risks dropping needed cookies. No host-only mail.* cookies live.
• BACKEND_PATH is a prefix, so a cookie pathed to the exact endpoint under-admits. All 8 live cookies are Path=/.

Final state

446 pass / 6 fail (the 6 are pre-existing live E2E --attach, identical on main). Hang regressions green. tsc --noEmit clean. Live: 5 cookies in 28ms; isSessionRefreshHealthy — a real CSRF exchange against accounts.superhuman.com — returns true; forced page-fallback agrees with the browser route.

Still unverified: Electron (9252). No Superhuman.app on this Linux box. The fallback exists precisely so that if Electron does not serve Storage on its browser endpoint, the worst case is the old behaviour rather than a break — but that path has not been executed. Worth one run on the Mac before merge.