From 5df153d0b1f1d85287e80d17b878bfae5790bfaf Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 14 Aug 2026 19:29:48 -0700 Subject: [PATCH] fix(security): one session-cookie registry implementation, two instances MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- browse/src/pty-session-cookie.ts | 85 ++++---------------- browse/src/session-cookie-store.ts | 124 +++++++++++++++++++++++++++++ browse/src/sse-session-cookie.ts | 81 +++---------------- 3 files changed, 153 insertions(+), 137 deletions(-) create mode 100644 browse/src/session-cookie-store.ts diff --git a/browse/src/pty-session-cookie.ts b/browse/src/pty-session-cookie.ts index cfeb97104..ce6e6a0a9 100644 --- a/browse/src/pty-session-cookie.ts +++ b/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 - * scoped-token-must-not-be-valid-as-root invariant, same opportunistic - * pruning. Two registries instead of one because the cookie names are - * different (`gstack_sse` vs `gstack_pty`) and the token spaces must not - * overlap — an SSE-read cookie must never grant PTY access, and vice versa. + * Shares the registry implementation with sse-session-cookie.ts via + * createSessionCookieStore. Two INSTANCES instead of one because the cookie + * names are different (`gstack_sse` vs `gstack_pty`) and the token spaces + * must not overlap — an SSE-read cookie must never grant PTY access, and + * vice versa. */ -import * as crypto from 'crypto'; - -interface Session { - createdAt: number; - expiresAt: number; -} +import { createSessionCookieStore } from './session-cookie-store'; const TTL_MS = 30 * 60 * 1000; // 30 minutes — matches SSE cookie -const MAX_SESSIONS = 10_000; -const sessions = new Map(); 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'); - const now = Date.now(); - const expiresAt = now + TTL_MS; - sessions.set(token, { createdAt: now, expiresAt }); - pruneExpired(now); - return { token, expiresAt }; + return store.mint(); } /** @@ -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; - 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; + return store.validate(token); } /** @@ -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; - sessions.delete(token); + store.revoke(token); } /** Parse the PTY session token from a Cookie header. */ export function extractPtyCookie(req: Request): string | null { - const cookieHeader = req.headers.get('cookie'); - 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; + return store.extract(req); } -/** - * 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. - */ +/** Build the Set-Cookie header value for the PTY session cookie. */ export function buildPtySetCookie(token: string): string { - const maxAge = Math.floor(TTL_MS / 1000); - 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); - } + return store.buildSetCookie(token); } // Test-only reset. export function __resetPtySessions(): void { - sessions.clear(); + store.__reset(); } diff --git a/browse/src/session-cookie-store.ts b/browse/src/session-cookie-store.ts new file mode 100644 index 000000000..7e0f77c68 --- /dev/null +++ b/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(); + + 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(); + }, + }; +} diff --git a/browse/src/sse-session-cookie.ts b/browse/src/sse-session-cookie.ts index feb53ccbb..baf17d564 100644 --- a/browse/src/sse-session-cookie.ts +++ b/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'; - -interface Session { - createdAt: number; - expiresAt: number; -} +import { createSessionCookieStore } from './session-cookie-store'; const TTL_MS = 30 * 60 * 1000; // 30 minutes -const MAX_SESSIONS = 10_000; // Upper bound on registry size -const sessions = new Map(); 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) - 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 }; + return store.mint(); } /** @@ -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; - 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; + return store.validate(token); } /** Parse the SSE session token from a Cookie header. */ export function extractSseCookie(req: Request): string | null { - const cookieHeader = req.headers.get('cookie'); - 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; + return store.extract(req); } -/** - * 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. - */ +/** Build the Set-Cookie header value for the SSE session cookie. */ export function buildSseSetCookie(token: string): string { - const maxAge = Math.floor(TTL_MS / 1000); - 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); - } + return store.buildSetCookie(token); } // Test-only reset. export function __resetSseSessions(): void { - sessions.clear(); + store.__reset(); }