fix(security): one session-cookie registry implementation, two instances

pty-session-cookie.ts and sse-session-cookie.ts were byte-identical modulo
the cookie name — mint/validate/parse/prune/TTL, the exact code a security
fix would have to land in twice (and a third hand-rolled cookie parse in
terminal-agent.ts had already diverged; unified next commit).
createSessionCookieStore() owns the implementation; both modules become thin
instantiations keeping every exported name, their distinct threat-model
docstrings, and separate token spaces (an SSE-read cookie must never grant
PTY access). pty-session-lease.ts deliberately stays out — different contract
(sessionId/secret split, refresh, env TTL).

The factory imports nothing from token-registry (cookie-picker-auth-isolation
invariant, still pinned by sse-session-cookie.test.ts).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

browse/src/pty-session-cookie.ts

@@ -4,7 +4,7 @@
  * Why this exists: WebSocket clients in browsers cannot send Authorization
  * headers on the upgrade request. The terminal-agent's /ws upgrade therefore
  * authenticates via cookie. We never put the PTY token in /health (codex
- * outside-voice finding #2: /health already leaks AUTH_TOKEN to any
+ * outside-voice finding #2: /health already leaked AUTH_TOKEN to any
  * localhost caller in headed mode; reusing that path for shell access would
  * widen an existing bug). Instead, the extension does an authenticated
  * POST /pty-session with the bootstrap AUTH_TOKEN; the server mints a
@@ -12,33 +12,23 @@
  * agent via loopback. The browser then carries the cookie automatically on
  * the WS upgrade.
  *
- * Design mirrors `sse-session-cookie.ts` deliberately. Same TTL, same
+ * Shares the registry implementation with sse-session-cookie.ts via
- * scoped-token-must-not-be-valid-as-root invariant, same opportunistic
+ * createSessionCookieStore. Two INSTANCES instead of one because the cookie
- * pruning. Two registries instead of one because the cookie names are
+ * names are different (`gstack_sse` vs `gstack_pty`) and the token spaces
- * different (`gstack_sse` vs `gstack_pty`) and the token spaces must not
+ * must not overlap — an SSE-read cookie must never grant PTY access, and
- * overlap — an SSE-read cookie must never grant PTY access, and vice versa.
+ * vice versa.
  */
-import * as crypto from 'crypto';
+import { createSessionCookieStore } from './session-cookie-store';
-
-interface Session {
-  createdAt: number;
-  expiresAt: number;
-}
 
 const TTL_MS = 30 * 60 * 1000; // 30 minutes — matches SSE cookie
-const MAX_SESSIONS = 10_000;
-const sessions = new Map<string, Session>();
 
 export const PTY_COOKIE_NAME = 'gstack_pty';
 
+const store = createSessionCookieStore({ cookieName: PTY_COOKIE_NAME, ttlMs: TTL_MS });
+
 /** Mint a fresh PTY session token. */
 export function mintPtySessionToken(): { token: string; expiresAt: number } {
-  const token = crypto.randomBytes(32).toString('base64url');
+  return store.mint();
-  const now = Date.now();
-  const expiresAt = now + TTL_MS;
-  sessions.set(token, { createdAt: now, expiresAt });
-  pruneExpired(now);
-  return { token, expiresAt };
 }
 
 /**
@@ -47,18 +37,7 @@ export function mintPtySessionToken(): { token: string; expiresAt: number } {
  * every call so the registry stays bounded under reconnect pressure.
  */
 export function validatePtySessionToken(token: string | null | undefined): boolean {
-  if (!token) return false;
+  return store.validate(token);
-  const s = sessions.get(token);
-  if (!s) {
-    pruneExpired(Date.now());
-    return false;
-  }
-  if (Date.now() > s.expiresAt) {
-    sessions.delete(token);
-    pruneExpired(Date.now());
-    return false;
-  }
-  return true;
 }
 
 /**
@@ -66,52 +45,20 @@ export function validatePtySessionToken(token: string | null | undefined): boole
  * replayed against a new PTY).
  */
 export function revokePtySessionToken(token: string | null | undefined): void {
-  if (!token) return;
+  store.revoke(token);
-  sessions.delete(token);
 }
 
 /** Parse the PTY session token from a Cookie header. */
 export function extractPtyCookie(req: Request): string | null {
-  const cookieHeader = req.headers.get('cookie');
+  return store.extract(req);
-  if (!cookieHeader) return null;
-  for (const part of cookieHeader.split(';')) {
-    const [name, ...valueParts] = part.trim().split('=');
-    if (name === PTY_COOKIE_NAME) {
-      return valueParts.join('=') || null;
-    }
-  }
-  return null;
 }
 
-/**
+/** Build the Set-Cookie header value for the PTY session cookie. */
- * Build the Set-Cookie header value for the PTY session cookie.
- * - HttpOnly: not readable from JS (mitigates XSS exfiltration).
- * - SameSite=Strict: not sent on cross-site requests (mitigates CSWSH).
- * - Path=/: scope to whole origin so /ws and /pty-session both see it.
- * - Max-Age matches the TTL.
- *
- * Secure is intentionally omitted: the daemon binds to 127.0.0.1 over plain
- * HTTP; setting Secure would prevent the browser from ever sending it back.
- */
 export function buildPtySetCookie(token: string): string {
-  const maxAge = Math.floor(TTL_MS / 1000);
+  return store.buildSetCookie(token);
-  return `${PTY_COOKIE_NAME}=${token}; HttpOnly; SameSite=Strict; Path=/; Max-Age=${maxAge}`;
-}
-
-function pruneExpired(now: number): void {
-  let checked = 0;
-  for (const [token, session] of sessions) {
-    if (checked++ >= 20) break;
-    if (session.expiresAt <= now) sessions.delete(token);
-  }
-  while (sessions.size > MAX_SESSIONS) {
-    const first = sessions.keys().next().value;
-    if (!first) break;
-    sessions.delete(first);
-  }
 }
 
 // Test-only reset.
 export function __resetPtySessions(): void {
-  sessions.clear();
+  store.__reset();
 }

browse/src/session-cookie-store.ts

@@ -0,0 +1,124 @@
+/**
+ * Factory for expiring session-cookie registries.
+ *
+ * pty-session-cookie.ts and sse-session-cookie.ts were byte-identical modulo
+ * the cookie name — a security-critical parser/TTL/prune implementation that
+ * had to be fixed in two places (and a third hand-rolled copy of the cookie
+ * parse had already diverged in terminal-agent.ts). One implementation now;
+ * the two modules are thin instantiations that keep their names and their
+ * distinct threat-model docstrings.
+ *
+ * Deliberately NOT unified here: pty-session-lease.ts — that's a different
+ * contract (sessionId/secret separation, refresh, env-overridable TTL).
+ *
+ * SECURITY INVARIANT: this module must never import token-registry — cookie
+ * session tokens must not be valid as scoped tokens (the
+ * cookie-picker-auth-isolation pattern). Pinned by sse-session-cookie.test.ts.
+ */
+import * as crypto from 'crypto';
+
+interface Session {
+  createdAt: number;
+  expiresAt: number;
+}
+
+export interface SessionCookieStore {
+  mint(): { token: string; expiresAt: number };
+  validate(token: string | null | undefined): boolean;
+  revoke(token: string | null | undefined): void;
+  extract(req: Request): string | null;
+  buildSetCookie(token: string): string;
+  /** Test-only reset. */
+  __reset(): void;
+}
+
+export function createSessionCookieStore(opts: {
+  cookieName: string;
+  ttlMs: number;
+  maxSessions?: number;
+}): SessionCookieStore {
+  const { cookieName, ttlMs } = opts;
+  const maxSessions = opts.maxSessions ?? 10_000;
+  const sessions = new Map<string, Session>();
+
+  function pruneExpired(now: number): void {
+    // Opportunistic cleanup: check up to 20 entries per call so we don't
+    // stall on a massive registry. O(1) amortized. Runs on every mint AND
+    // on every validate so a steady reconnect flow can't outpace it.
+    let checked = 0;
+    for (const [token, session] of sessions) {
+      if (checked++ >= 20) break;
+      if (session.expiresAt <= now) sessions.delete(token);
+    }
+    // Hard cap as a backstop — if something still gets past opportunistic
+    // cleanup (e.g., all unexpired but registry enormous), drop the oldest.
+    while (sessions.size > maxSessions) {
+      const first = sessions.keys().next().value;
+      if (!first) break;
+      sessions.delete(first);
+    }
+  }
+
+  return {
+    mint() {
+      // 32 random bytes → 43-char URL-safe base64 (no padding). 256 bits.
+      const token = crypto.randomBytes(32).toString('base64url');
+      const now = Date.now();
+      const expiresAt = now + ttlMs;
+      sessions.set(token, { createdAt: now, expiresAt });
+      pruneExpired(now);
+      return { token, expiresAt };
+    },
+
+    validate(token) {
+      if (!token) return false;
+      const s = sessions.get(token);
+      if (!s) {
+        pruneExpired(Date.now());
+        return false;
+      }
+      if (Date.now() > s.expiresAt) {
+        sessions.delete(token);
+        pruneExpired(Date.now());
+        return false;
+      }
+      return true;
+    },
+
+    revoke(token) {
+      if (!token) return;
+      sessions.delete(token);
+    },
+
+    extract(req) {
+      const cookieHeader = req.headers.get('cookie');
+      if (!cookieHeader) return null;
+      for (const part of cookieHeader.split(';')) {
+        const [name, ...valueParts] = part.trim().split('=');
+        if (name === cookieName) {
+          return valueParts.join('=') || null;
+        }
+      }
+      return null;
+    },
+
+    /**
+     * Set-Cookie value:
+     * - HttpOnly: not readable from JS (mitigates XSS exfiltration).
+     * - SameSite=Strict: not sent on cross-site requests (mitigates
+     *   CSRF/CSWSH).
+     * - Path=/: scope to the whole origin.
+     * - Max-Age matches the TTL.
+     * Secure is intentionally omitted: the daemon binds 127.0.0.1 over plain
+     * HTTP; Secure would prevent the browser from ever sending it back.
+     */
+    buildSetCookie(token) {
+      const maxAge = Math.floor(ttlMs / 1000);
+      return `${cookieName}=${token}; HttpOnly; SameSite=Strict; Path=/; Max-Age=${maxAge}`;
+    },
+
+    __reset() {
+      sessions.clear();
+    },
+  };
+}

browse/src/sse-session-cookie.ts

@@ -21,29 +21,22 @@
  * - In-memory only. No persistence across daemon restarts — extension
  *   re-mints on reconnect.
  * - Tokens are 32 random bytes (URL-safe base64). 256 bits, unbruteforceable.
+ *
+ * Shares the registry implementation with pty-session-cookie.ts via
+ * createSessionCookieStore; separate INSTANCE so the token spaces never
+ * overlap.
  */
-import * as crypto from 'crypto';
+import { createSessionCookieStore } from './session-cookie-store';
-
-interface Session {
-  createdAt: number;
-  expiresAt: number;
-}
 
 const TTL_MS = 30 * 60 * 1000; // 30 minutes
-const MAX_SESSIONS = 10_000; // Upper bound on registry size
-const sessions = new Map<string, Session>();
 
 export const SSE_COOKIE_NAME = 'gstack_sse';
 
+const store = createSessionCookieStore({ cookieName: SSE_COOKIE_NAME, ttlMs: TTL_MS });
+
 /** Mint a fresh view-only SSE session token. */
 export function mintSseSessionToken(): { token: string; expiresAt: number } {
-  // 32 random bytes → 43-char URL-safe base64 (no padding)
+  return store.mint();
-  const token = crypto.randomBytes(32).toString('base64url');
-  const now = Date.now();
-  const expiresAt = now + TTL_MS;
-  sessions.set(token, { createdAt: now, expiresAt });
-  pruneExpired(now);
-  return { token, expiresAt };
 }
 
 /**
@@ -53,68 +46,20 @@ export function mintSseSessionToken(): { token: string; expiresAt: number } {
  * unboundedly under sustained mint + reconnect pressure.
  */
 export function validateSseSessionToken(token: string | null | undefined): boolean {
-  if (!token) return false;
+  return store.validate(token);
-  const s = sessions.get(token);
-  if (!s) {
-    pruneExpired(Date.now());
-    return false;
-  }
-  if (Date.now() > s.expiresAt) {
-    sessions.delete(token);
-    pruneExpired(Date.now());
-    return false;
-  }
-  return true;
 }
 
 /** Parse the SSE session token from a Cookie header. */
 export function extractSseCookie(req: Request): string | null {
-  const cookieHeader = req.headers.get('cookie');
+  return store.extract(req);
-  if (!cookieHeader) return null;
-  for (const part of cookieHeader.split(';')) {
-    const [name, ...valueParts] = part.trim().split('=');
-    if (name === SSE_COOKIE_NAME) {
-      return valueParts.join('=') || null;
-    }
-  }
-  return null;
 }
 
-/**
+/** Build the Set-Cookie header value for the SSE session cookie. */
- * Build the Set-Cookie header value for the SSE session cookie.
- * - HttpOnly: not readable from JS (mitigates XSS token exfiltration)
- * - SameSite=Strict: not sent on cross-site requests (mitigates CSRF)
- * - Path=/: scope to the whole origin so SSE endpoints can read it
- * - Max-Age matches the TTL
- *
- * Secure is intentionally omitted: the daemon binds to 127.0.0.1 over
- * plain HTTP, and setting Secure would prevent the browser from ever
- * sending the cookie back. If gstack ever ships over HTTPS, add Secure.
- */
 export function buildSseSetCookie(token: string): string {
-  const maxAge = Math.floor(TTL_MS / 1000);
+  return store.buildSetCookie(token);
-  return `${SSE_COOKIE_NAME}=${token}; HttpOnly; SameSite=Strict; Path=/; Max-Age=${maxAge}`;
-}
-
-function pruneExpired(now: number): void {
-  // Opportunistic cleanup: check up to 20 entries per call so we don't
-  // stall on a massive registry. O(1) amortized.  Runs on every mint
-  // AND on every validate so a steady reconnect flow can't outpace it.
-  let checked = 0;
-  for (const [token, session] of sessions) {
-    if (checked++ >= 20) break;
-    if (session.expiresAt <= now) sessions.delete(token);
-  }
-  // Hard cap as a backstop — if something still gets past opportunistic
-  // cleanup (e.g., all unexpired but registry enormous), drop the oldest.
-  while (sessions.size > MAX_SESSIONS) {
-    const first = sessions.keys().next().value;
-    if (!first) break;
-    sessions.delete(first);
-  }
 }
 
 // Test-only reset.
 export function __resetSseSessions(): void {
-  sessions.clear();
+  store.__reset();
 }