From b0fa0f2e36567b210431042d7e1781ae78badf90 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Tue, 9 Jun 2026 19:13:52 -0700 Subject: [PATCH] fix(brainstorm-server): fix auth-integration bugs from full-branch review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second adversarial review of the merged branch found that combining the session-key auth with the feature work created real bugs the (vacuous) tests missed: - [Critical] GET /files/ (empty name) resolved to CONTENT_DIR and crashed the process with uncaught EISDIR — newly reachable because the query-stripping refactor turns /files/?key=... into /files/. Reject non-regular-file names. - [High] --open opened a KEYLESS url, which the auth gate 403s — the headline feature landed on the error page. Open the keyed url. - [High] Same-port restart regenerated the token (port persisted, token not), so the open tab's old cookie 403'd and never reconnected — contradicting the documented promise. Persist the token (BRAINSTORM_TOKEN_FILE / .last-token) alongside the port. - [Medium] Token sat in world-readable server-info/server.log (0644 in /tmp). umask 077 in start-server.sh + mode 0600 on server-info/.last-token. - [Medium] touchActivity() ran before the auth check, so unauthenticated requests defeated the idle timeout. Count activity only after authorization. - [Low] COOKIE_NAME embedded the pre-fallback port; derive it from the actual bound port (also prevents a cross-server cookie-jar collision on fallback). Tests added/strengthened (previously passed vacuously): /files/ no-crash; the auto-open url carries the key and is reachable (200); restart reuses the same key not just the port; unauthenticated requests don't reset the idle clock. Full suite green (ws-protocol 32, helper 12, auth 13, server 29, lifecycle 8, stop-server 4); restart smoke confirms same port+key and old URL -> 200. --- skills/brainstorming/scripts/server.cjs | 43 ++++++++++++++----- skills/brainstorming/scripts/start-server.sh | 9 +++- tests/brainstorm-server/lifecycle.test.js | 45 ++++++++++++++++---- tests/brainstorm-server/server.test.js | 8 ++++ 4 files changed, 84 insertions(+), 21 deletions(-) diff --git a/skills/brainstorming/scripts/server.cjs b/skills/brainstorming/scripts/server.cjs index 22af144d..86024f05 100644 --- a/skills/brainstorming/scripts/server.cjs +++ b/skills/brainstorming/scripts/server.cjs @@ -110,8 +110,20 @@ let ownerPid = process.env.BRAINSTORM_OWNER_PID ? Number(process.env.BRAINSTORM_ // remote binds — and defeats DNS rebinding — where a Host/Origin allowlist // cannot. It rides the served URL as ?key= and is mirrored into a cookie on // first load so same-origin subresources and the WebSocket carry it for free. -const TOKEN = process.env.BRAINSTORM_TOKEN || crypto.randomBytes(32).toString('hex'); -const COOKIE_NAME = 'brainstorm-key-' + PORT; +// Persisted alongside the port (BRAINSTORM_TOKEN_FILE) so a restart keeps the +// same key and an already-open tab's cookie still validates. +const TOKEN_FILE = process.env.BRAINSTORM_TOKEN_FILE || null; +const TOKEN = (() => { + if (process.env.BRAINSTORM_TOKEN) return process.env.BRAINSTORM_TOKEN; + if (TOKEN_FILE) { + try { + const t = fs.readFileSync(TOKEN_FILE, 'utf-8').trim(); + if (/^[0-9a-f]{32,}$/i.test(t)) return t; + } catch (e) { /* no prior token recorded */ } + } + return crypto.randomBytes(32).toString('hex'); +})(); +let COOKIE_NAME = 'brainstorm-key-' + PORT; // refined to the actual bound port in onListen const MIME_TYPES = { '.html': 'text/html', '.css': 'text/css', '.js': 'application/javascript', @@ -207,13 +219,12 @@ function pathnameOf(url) { // ========== HTTP Request Handler ========== function handleRequest(req, res) { - touchActivity(); - if (!isAuthorized(req)) { res.writeHead(403, { 'Content-Type': 'text/html; charset=utf-8' }); res.end(FORBIDDEN_PAGE); return; } + touchActivity(); // only authorized requests count as activity // Mirror the key into a cookie so same-origin subresources (/files/*) and the // WebSocket handshake carry it automatically, whatever URL style the agent @@ -240,7 +251,9 @@ function handleRequest(req, res) { } else if (req.method === 'GET' && pathname.startsWith('/files/')) { const fileName = path.basename(pathname.slice(7)); const filePath = path.join(CONTENT_DIR, fileName); - if (fileName.startsWith('.') || !fs.existsSync(filePath)) { + // Reject empty/dotfile names and anything that isn't a regular file — + // `/files/` would otherwise resolve to CONTENT_DIR and crash readFileSync (EISDIR). + if (!fileName || fileName.startsWith('.') || !fs.existsSync(filePath) || !fs.statSync(filePath).isFile()) { res.writeHead(404); res.end('Not found'); return; @@ -351,7 +364,7 @@ function maybeOpenBrowser() { if (!process.env.BRAINSTORM_OPEN) return; // opt-in: only after the user approves the companion if (HOST !== '127.0.0.1' && HOST !== 'localhost') return; if (clients.size > 0) return; // the user already opened it - const url = 'http://' + URL_HOST + ':' + PORT; + const url = 'http://' + URL_HOST + ':' + PORT + '/?key=' + TOKEN; // must carry the key or the gate 403s it const cp = require('child_process'); // Operator-provided launcher: run as given (this env var is trusted operator input). if (process.env.BRAINSTORM_OPEN_CMD) { @@ -483,12 +496,19 @@ function startServer() { let triedFallback = false; function onListen() { - // Record the bound port so the next restart of this session reuses it — but - // ONLY when we got our preferred port. On a fallback we bound a *different* - // port because someone else holds the preferred one; persisting it would - // overwrite the shared .last-port and strand that other session's open tab. + // Cookie name keys on the ACTUAL bound port (may differ from the preferred + // one after an EADDRINUSE fallback) so it can't collide with another server's + // cookie in the shared localhost jar. + COOKIE_NAME = 'brainstorm-key-' + PORT; + // Record the bound port AND token so the next restart of this session reuses + // them — but ONLY when we got our preferred port. On a fallback we bound a + // *different* port because someone else holds the preferred one; persisting + // would overwrite the shared files and strand that other session's open tab. if (PORT_FILE && !triedFallback) { try { fs.writeFileSync(PORT_FILE, String(PORT)); } catch (e) { /* best effort */ } + if (TOKEN_FILE) { + try { fs.writeFileSync(TOKEN_FILE, TOKEN, { mode: 0o600 }); } catch (e) { /* best effort */ } + } } const info = JSON.stringify({ type: 'server-started', port: Number(PORT), host: HOST, @@ -496,7 +516,8 @@ function startServer() { screen_dir: CONTENT_DIR, state_dir: STATE_DIR, idle_timeout_ms: IDLE_TIMEOUT_MS }); console.log(info); - fs.writeFileSync(path.join(STATE_DIR, 'server-info'), info + '\n'); + // server-info embeds the key — keep it owner-only. + fs.writeFileSync(path.join(STATE_DIR, 'server-info'), info + '\n', { mode: 0o600 }); } server.on('error', (err) => { diff --git a/skills/brainstorming/scripts/start-server.sh b/skills/brainstorming/scripts/start-server.sh index d4e5724a..5e9a00cb 100755 --- a/skills/brainstorming/scripts/start-server.sh +++ b/skills/brainstorming/scripts/start-server.sh @@ -94,14 +94,19 @@ if [[ "$FOREGROUND" != "true" && "$FORCE_BACKGROUND" != "true" ]]; then fi fi +# Session files (server.log, server-info, .last-token) embed the session key — +# keep everything this script and the server create owner-only. +umask 077 + # Generate unique session directory SESSION_ID="$$-$(date +%s)" if [[ -n "$PROJECT_DIR" ]]; then SESSION_DIR="${PROJECT_DIR}/.superpowers/brainstorm/${SESSION_ID}" - # Persist the bound port per project so a restart reuses it and an already-open - # browser tab reconnects to the same URL. + # Persist the bound port and key per project so a restart reuses them and an + # already-open browser tab reconnects to the same URL with a valid cookie. export BRAINSTORM_PORT_FILE="${PROJECT_DIR}/.superpowers/brainstorm/.last-port" + export BRAINSTORM_TOKEN_FILE="${PROJECT_DIR}/.superpowers/brainstorm/.last-token" else SESSION_DIR="/tmp/brainstorm-${SESSION_ID}" fi diff --git a/tests/brainstorm-server/lifecycle.test.js b/tests/brainstorm-server/lifecycle.test.js index 9203322f..cd186a7d 100644 --- a/tests/brainstorm-server/lifecycle.test.js +++ b/tests/brainstorm-server/lifecycle.test.js @@ -82,26 +82,30 @@ async function runTests() { } }); - await test('persists the bound port and restores it on restart', async () => { + await test('persists the bound port AND key, and restores both on restart', async () => { const dir = fs.mkdtempSync('/tmp/bs-port-'); const portFile = path.join(dir, '.last-port'); - const env = { ...process.env, BRAINSTORM_PORT_FILE: portFile, BRAINSTORM_LIFECYCLE_CHECK_MS: 100000 }; + const tokenFile = path.join(dir, '.last-token'); + const env = { ...process.env, BRAINSTORM_PORT_FILE: portFile, BRAINSTORM_TOKEN_FILE: tokenFile, BRAINSTORM_LIFECYCLE_CHECK_MS: 100000 }; const a = spawn('node', [SERVER], { env: { ...env, BRAINSTORM_DIR: path.join(dir, 's1') } }); let outA = ''; a.stdout.on('data', d => outA += d.toString()); for (let i = 0; i < 60 && !outA.includes('server-started'); i++) await sleep(50); - const portA = firstServerStarted(outA).port; - assert(fs.existsSync(portFile), 'should write the port file'); - assert.strictEqual(Number(fs.readFileSync(portFile, 'utf8').trim()), portA, 'port file holds the bound port'); + const infoA = firstServerStarted(outA); + const keyA = new URL(infoA.url).searchParams.get('key'); + assert(fs.existsSync(portFile) && fs.existsSync(tokenFile), 'should write the port and token files'); a.kill(); await sleep(400); // free the port const b = spawn('node', [SERVER], { env: { ...env, BRAINSTORM_DIR: path.join(dir, 's2') } }); let outB = ''; b.stdout.on('data', d => outB += d.toString()); for (let i = 0; i < 60 && !outB.includes('server-started'); i++) await sleep(50); - const portB = firstServerStarted(outB).port; + const infoB = firstServerStarted(outB); + const keyB = new URL(infoB.url).searchParams.get('key'); b.kill(); await sleep(100); fs.rmSync(dir, { recursive: true, force: true }); - assert.strictEqual(portB, portA, 'restart should reuse the same port'); + assert.strictEqual(infoB.port, infoA.port, 'restart should reuse the same port'); + // Same key too — otherwise the open tab's cookie would 403 against the restart. + assert.strictEqual(keyB, keyA, 'restart should reuse the same session key'); }); await test('falls back to a random port when the preferred port is taken', async () => { @@ -143,12 +147,19 @@ async function runTests() { fs.writeFileSync(path.join(dir, 'content', 'second.html'), '

Second

'); await sleep(700); - srv.kill(); await sleep(100); const lines = fs.existsSync(marker) ? fs.readFileSync(marker, 'utf8').trim().split('\n').filter(Boolean) : []; + // The opened URL must carry the key AND be reachable — a keyless URL hits 403. + let status = 0; + if (lines[0]) { + status = await new Promise(r => require('http').get(lines[0], res => { res.resume(); r(res.statusCode); }).on('error', () => r(0))); + } + srv.kill(); await sleep(100); fs.rmSync(dir, { recursive: true, force: true }); assert.strictEqual(lines.length, 1, 'should open exactly once'); assert(lines[0].includes('3417'), `should open the server URL, got: ${lines[0]}`); + assert(/[?&]key=/.test(lines[0]), `opened URL must carry the session key, got: ${lines[0]}`); + assert.strictEqual(status, 200, 'the opened URL must be reachable (valid key), not the 403 page'); }); await test('does NOT auto-open unless approved (BRAINSTORM_OPEN unset)', async () => { @@ -167,6 +178,24 @@ async function runTests() { assert(!opened, 'must not open the browser without explicit approval'); }); + await test('unauthenticated requests do not defeat the idle timeout', async () => { + const dir = fs.mkdtempSync('/tmp/bs-life-'); + const srv = spawn('node', [SERVER], { env: { ...process.env, BRAINSTORM_PORT: 3419, BRAINSTORM_DIR: dir, BRAINSTORM_TOKEN: 'authtok', BRAINSTORM_IDLE_TIMEOUT_MS: 400, BRAINSTORM_LIFECYCLE_CHECK_MS: 100 } }); + let out = ''; srv.stdout.on('data', d => out += d.toString()); + let exited = false; srv.on('exit', () => { exited = true; }); + for (let i = 0; i < 60 && !out.includes('server-started'); i++) await sleep(50); + + // Flood with UNAUTHENTICATED (keyless → 403) requests. These must NOT count + // as activity, so the idle timeout still fires and the process exits. + const hammer = setInterval(() => { require('http').get('http://localhost:3419/', r => r.resume()).on('error', () => {}); }, 60); + for (let i = 0; i < 40 && !exited; i++) await sleep(100); + clearInterval(hammer); + if (!exited) srv.kill(); + fs.rmSync(dir, { recursive: true, force: true }); + + assert(exited, 'idle shutdown must still fire despite a flood of unauthenticated requests'); + }); + console.log(`\n--- Results: ${passed} passed, ${failed} failed ---`); if (failed > 0) process.exit(1); } diff --git a/tests/brainstorm-server/server.test.js b/tests/brainstorm-server/server.test.js index 9455ba16..25ea7204 100644 --- a/tests/brainstorm-server/server.test.js +++ b/tests/brainstorm-server/server.test.js @@ -202,6 +202,14 @@ async function runTests() { assert.strictEqual(res.status, 404, '/files/ must 404 on dotfiles'); }); + await test('GET /files/ (empty name) returns 404 and does not crash the server', async () => { + const res = await fetch(`http://localhost:${TEST_PORT}/files/`); + assert.strictEqual(res.status, 404, '/files/ (the content dir) must 404, not EISDIR-crash'); + // The server must still be alive afterward. + const alive = await fetch(`http://localhost:${TEST_PORT}/`); + assert.strictEqual(alive.status, 200, 'server must survive a /files/ request'); + }); + await test('returns 404 for non-root paths', async () => { const res = await fetch(`http://localhost:${TEST_PORT}/other`); assert.strictEqual(res.status, 404);