From c7583f7f02c118bd3e4fc733dd39f8cc86e114e5 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 21 Apr 2026 21:07:35 -0700 Subject: [PATCH] fix: pre-landing review findings (4 auto-fixes) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 4 findings from the Claude adversarial subagent on the v1.6.0.0 security wave diff. No user-visible behavior change; all are defense-in-depth hardening of newly-introduced code. 1. GET /connect rate-limited (was POST-only) [HIGH conf 8/10] Attacker discovering the ngrok URL could probe unlimited GETs for daemon enumeration. Now shares the global /connect counter. 2. ngrok listener leak on tunnel startup failure [MEDIUM conf 8/10] If ngrok.forward() resolved but tunnelListener.url() or the state-file write threw, the Bun listener was torn down but the ngrok session was leaked. Fixed in BOTH /tunnel/start and BROWSE_TUNNEL=1 startup paths. 3. GSTACK_SKILL_ROOT path-traversal gate [MEDIUM conf 8/10] Symmetric with E3's GSTACK_SLUG regex gate — reject values containing '..' before interpolating into the welcome-page path. 4. SSE session registry pruning [LOW conf 7/10] pruneExpired() only checked 10 entries per mint call. Now runs on every validate too, checks 20 entries, with a hard 10k cap as backstop. Prevents registry growth under sustained extension reconnect pressure. Tests remain green (56/56 in sse-session-cookie + dual-listener + pair-agent-e2e suites). Co-Authored-By: Claude Opus 4.7 (1M context) --- browse/src/server.ts | 30 +++++++++++++++++++++++++++--- browse/src/sse-session-cookie.ts | 25 ++++++++++++++++++++----- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/browse/src/server.ts b/browse/src/server.ts index 3ab32b8d..2c6d7e0c 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -1571,7 +1571,16 @@ async function start() { // GET /connect — alive probe. Unauth on both surfaces. Used by /pair // and /tunnel/start to detect dead ngrok tunnels via the tunnel URL, // since /health is not tunnel-reachable under the dual-listener design. + // + // Shares the same rate limit as POST /connect — otherwise a tunnel + // caller can probe unlimited GETs and lock out nothing, which makes + // the endpoint a free daemon-enumeration surface. if (url.pathname === '/connect' && req.method === 'GET') { + if (!checkConnectRateLimit()) { + return new Response(JSON.stringify({ error: 'Rate limited' }), { + status: 429, headers: { 'Content-Type': 'application/json' }, + }); + } return new Response(JSON.stringify({ alive: true }), { status: 200, headers: { 'Content-Type': 'application/json' }, }); @@ -1595,9 +1604,13 @@ async function start() { const homeDir = process.env.HOME || process.env.USERPROFILE || '/tmp'; const projectWelcome = `${homeDir}/.gstack/projects/${slug}/designs/welcome-page-20260331/finalized.html`; if (fs.existsSync(projectWelcome)) return projectWelcome; - // Fallback: built-in welcome page from gstack install - const skillRoot = process.env.GSTACK_SKILL_ROOT || `${homeDir}/.claude/skills/gstack`; - const builtinWelcome = `${skillRoot}/browse/src/welcome.html`; + // Fallback: built-in welcome page from gstack install. Reject + // SKILL_ROOT values containing '..' for the same defense-in-depth + // reason as the GSTACK_SLUG regex above. Not exploitable today + // (env set at install time), but the gate is one check. + const rawSkillRoot = process.env.GSTACK_SKILL_ROOT || `${homeDir}/.claude/skills/gstack`; + if (rawSkillRoot.includes('..')) return null; + const builtinWelcome = `${rawSkillRoot}/browse/src/welcome.html`; if (fs.existsSync(builtinWelcome)) return builtinWelcome; return null; })(); @@ -1907,7 +1920,13 @@ async function start() { status: 200, headers: { 'Content-Type': 'application/json' }, }); } catch (err: any) { + // Clean up BOTH ngrok and the Bun listener on failure. If + // ngrok.forward() succeeded but tunnelListener.url() or the + // state-file write threw, we'd otherwise leak an active ngrok + // session on the user's account. + try { if (tunnelListener) await tunnelListener.close(); } catch {} try { boundTunnel.stop(true); } catch {} + tunnelListener = null; return new Response(JSON.stringify({ error: `Failed to open ngrok tunnel: ${err.message}`, }), { status: 500, headers: { 'Content-Type': 'application/json' } }); @@ -2769,7 +2788,12 @@ async function start() { fs.renameSync(tmpState, config.stateFile); } catch (err: any) { console.error(`[browse] Failed to start tunnel: ${err.message}`); + // Same cleanup as /tunnel/start's error path: tear down BOTH + // ngrok and the Bun listener so we don't leak an ngrok session + // if the error happened after ngrok.forward() resolved. + try { if (tunnelListener) await tunnelListener.close(); } catch {} try { if (boundTunnel) boundTunnel.stop(true); } catch {} + tunnelListener = null; } } } diff --git a/browse/src/sse-session-cookie.ts b/browse/src/sse-session-cookie.ts index 4e38f743..bae8ba5f 100644 --- a/browse/src/sse-session-cookie.ts +++ b/browse/src/sse-session-cookie.ts @@ -30,6 +30,7 @@ interface Session { } 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'; @@ -47,14 +48,20 @@ export function mintSseSessionToken(): { token: string; expiresAt: number } { /** * Validate a token. Returns true only if the token exists AND is not expired. - * Expired tokens are lazily removed. + * Expired tokens are lazily removed, and we opportunistically prune a few + * additional expired entries on every validate so the registry can't grow + * 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) return false; + if (!s) { + pruneExpired(Date.now()); + return false; + } if (Date.now() > s.expiresAt) { sessions.delete(token); + pruneExpired(Date.now()); return false; } return true; @@ -95,13 +102,21 @@ export function buildSseClearCookie(): string { } function pruneExpired(now: number): void { - // Opportunistic cleanup: at most 10 per mint call so we don't stall - // on a massive registry. O(1) amortized. + // 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++ >= 10) break; + 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.