From c4e2233832911a33e87109506d17cd8aa4f89834 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 16 Aug 2026 09:32:16 -0700 Subject: [PATCH] fix(browse): capture daemon stdout/stderr to browse-daemon.log + Windows polyfill spawn fixes (re-derived from #2461) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The detached daemon's stdout/stderr were wired to 'ignore' on every platform, so every console.error('[browse] FATAL: ...') from a Chromium crash, uncaughtException, or unhandledRejection was discarded at the OS level — a crash-and-respawn looked identical to every other dropped session, with nothing on disk recording why. Both spawn paths now redirect to /browse-daemon.log (append mode, accumulates across respawns): the Unix path via an fd from openDaemonLogSink(), the Windows path by opening the fd INSIDE the node -e launcher string (an fd opened in cli.ts would not cross the spawn boundary). Unwritable state dir falls back to 'ignore' rather than failing the launch. Capturing daemon output is what surfaced the PR's second fix, still valid on current main: bun-polyfill.cjs's Bun.spawn/spawnSync called Node's child_process with a bare command name, which Windows can't resolve without PATHEXT lookup ("spawn bun ENOENT" from the terminal-agent respawn path). Routed through cross-spawn on win32 (now a direct dependency; already in the tree transitively via @modelcontextprotocol/sdk) — the PR verified empirically that shell:true does NOT neutralize cmd.exe metacharacters reachable via `$B skill run` arg passthrough, and that Node refuses .cmd spawns without a shell (CVE-2024-27980), so cross-spawn's combined PATHEXT resolution + argument escaping is the only correct shape. The PR's third fix (resolveDisconnectCause throwing "browser?.process is not a function") already landed on main via the #2085 typeof guard — not re-applied. F6 log hygiene (daemon-log-hygiene.test.ts): needle tests pin the log wiring on both spawn paths (and that stdio 'ignore','ignore','ignore' never returns), that bun-polyfill stays on cross-spawn with no shell:true, that NO console.* call in src/ passes a token value (interpolated or bare arg), and that the page-content carrier modules (tab-session, buffers, content-security, activity) stay console-free — so neither AUTH_TOKEN nor unsanitized page-derived strings can reach browse-daemon.log. Tests: daemon-log-hygiene + bun-polyfill + windows-spawn-hide + cli-setsid-daemonize 21 pass; stop-dead-daemon + busy-daemon-iron-rule (exercises a REAL daemon boot through the new log-fd wiring) 10 pass. Re-derived from PR #2461 by @phuttimatebenchanakatkul. Co-authored-by: phuttimatebenchanakatkul Co-Authored-By: Claude Fable 5 --- browse/src/bun-polyfill.cjs | 44 +++++++++++- browse/src/cli.ts | 36 +++++++++- browse/test/daemon-log-hygiene.test.ts | 95 ++++++++++++++++++++++++++ bun.lock | 1 + package.json | 1 + 5 files changed, 172 insertions(+), 5 deletions(-) create mode 100644 browse/test/daemon-log-hygiene.test.ts diff --git a/browse/src/bun-polyfill.cjs b/browse/src/bun-polyfill.cjs index 89e16c4fd..e383042c7 100644 --- a/browse/src/bun-polyfill.cjs +++ b/browse/src/bun-polyfill.cjs @@ -11,7 +11,43 @@ 'use strict'; const http = require('http'); -const { spawnSync, spawn } = require('child_process'); +const { spawnSync: nodeSpawnSync, spawn: nodeSpawn } = require('child_process'); +// Node's spawn on Windows without shell:true only matches an EXACT +// executable name — no PATHEXT resolution the way a real shell (or +// Bun.spawn, which this file exists to polyfill) does. A bare command +// name like 'bun' (no .exe/.cmd) then fails ENOENT even though `bun` +// works fine typed at a prompt (confirmed live in #2461: this is what +// produced "[browse] FATAL uncaught exception: spawn bun ENOENT" from +// terminal-agent-control.ts's respawn path, once daemon output was +// actually being captured to a file instead of silently discarded). +// +// Two things this is NOT fixed with, both tried and rejected in #2461: +// +// 1. shell:true + array args. This file is also reached (via server.ts → +// write-commands.ts/meta-commands.ts → cookie-import-browser.ts/ +// browser-skill-commands.ts) by calls that pass genuinely variable +// content — browser-skill-commands.ts spreads `...opts.skillArgs`, +// sourced from `$B skill run --arg k=v`'s passthrough CLI args, +// into the spawned argv. shell:true on Windows routes through cmd.exe, +// and Node's own array-arg handling for that combination does NOT +// neutralize cmd.exe metacharacters (& | ^ % < >) — verified in #2461 by +// directly spawning a resolved .cmd path with an arg containing +// `& echo INJECTED > proof.txt`: the file was created. Hand-rolled +// double-quote-only escaping doesn't close that either. +// +// 2. Resolve the .exe/.cmd path ourselves and spawn it with NO shell. +// Works for .exe targets, but Node refuses (EINVAL) to spawn a +// .cmd/.bat file without shell:true — deliberately, as part of Node's +// CVE-2024-27980 fix for implicit unsafe .cmd execution. bun's own +// Windows install (npm global) is exactly a .cmd shim, so this path is +// not optional to support. +// +// cross-spawn (previously a transitive dep, now direct) is the established +// library for precisely this problem: PATHEXT resolution AND correct +// Windows/cmd.exe argument escaping together. #2461 verified the injection +// payload above reaches the child as a single literal argument while +// normal resolution (`bun --version`) still works. +const crossSpawn = require('cross-spawn'); globalThis.Bun = { serve(options) { @@ -66,7 +102,8 @@ globalThis.Bun = { spawnSync(cmd, options = {}) { const [command, ...args] = cmd; - const result = spawnSync(command, args, { + const spawnSyncFn = process.platform === 'win32' ? crossSpawn.sync : nodeSpawnSync; + const result = spawnSyncFn(command, args, { stdio: [ options.stdin || 'pipe', options.stdout === 'pipe' ? 'pipe' : 'ignore', @@ -92,7 +129,8 @@ globalThis.Bun = { spawn(cmd, options = {}) { const [command, ...args] = cmd; const stdio = options.stdio || ['pipe', 'pipe', 'pipe']; - const proc = spawn(command, args, { + const spawnFn = process.platform === 'win32' ? crossSpawn : nodeSpawn; + const proc = spawnFn(command, args, { stdio, env: options.env, cwd: options.cwd, diff --git a/browse/src/cli.ts b/browse/src/cli.ts index b4b1faea2..1651fed4b 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -365,6 +365,29 @@ function raiseHeadedWindowMacOS(): void { } // ─── Server Lifecycle ────────────────────────────────────────── +// The detached daemon's stdout/stderr used to be wired to 'ignore' on every +// platform, so console.error('[browse] FATAL: ...') from a Chromium crash, +// an uncaughtException, or an unhandledRejection (see server.ts's handlers +// and browser-manager.ts's handleChromiumDisconnect) went nowhere — not to +// a file, not to the terminal, discarded at the OS level (#2461). That made +// a crash-and-respawn indistinguishable from any other cause of a dropped +// session: nothing on disk ever recorded WHY. Redirect both streams to +// /browse-daemon.log — append mode, so it accumulates across the +// daemon's full lifetime and every respawn stays visible in one place. +// +// F6 log hygiene: nothing that reaches the daemon's stdout/stderr may carry +// an auth token or unsanitized page-derived strings — +// browse/test/daemon-log-hygiene.test.ts pins this with needle tests. +function openDaemonLogSink(): number | 'ignore' { + try { + return fs.openSync(path.join(config.stateDir, 'browse-daemon.log'), 'a'); + } catch { + // stateDir not writable (permissions, disk full) — fall back to the + // previous behavior rather than fail the whole launch over logging. + return 'ignore'; + } +} + async function startServer(extraEnv?: Record): Promise { ensureStateDir(config); @@ -393,10 +416,18 @@ async function startServer(extraEnv?: Record): Promise): Promise/browse-daemon.log + * (both spawn paths) instead of 'ignore'. That makes crashes diagnosable — + * and makes it load-bearing that NOTHING secret or page-derived reaches the + * daemon's console streams: + * + * - No console.* call anywhere in src/ may pass a token VALUE (AUTH_TOKEN, + * state.token, attachToken, INTERNAL_TOKEN, setup keys). Names like + * tokenInfo.clientId are fine — the needle targets expressions whose + * value IS a token. + * - The page-content carrier modules (tab-session, buffers, + * content-security, activity) stay console-free, so raw page-derived + * strings can't be echoed into the log unsanitized. + * + * Source-level, same style as windows-spawn-hide.test.ts. + */ + +import { describe, expect, test } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; + +const SRC_DIR = path.join(import.meta.dir, '../src'); +const SRC = (f: string) => fs.readFileSync(path.join(SRC_DIR, f), 'utf-8'); + +describe('#2461 daemon log wiring', () => { + test('both daemon spawn paths capture stdout/stderr to browse-daemon.log', () => { + const cli = SRC('cli.ts'); + // Unix path: fd from openDaemonLogSink wired into stdio. + expect(cli).toContain("stdio: ['ignore', daemonLogFd, daemonLogFd]"); + expect(cli).toMatch(/openDaemonLogSink/); + // Windows path: the fd must be opened INSIDE the node -e launcher (an fd + // opened in cli.ts wouldn't cross the spawn boundary). + expect(cli).toContain("stdio:['ignore',logFd,logFd]"); + expect(cli).toContain('browse-daemon.log'); + // The old fully-discarded wiring must not come back on either daemon path. + expect(cli).not.toContain("stdio:['ignore','ignore','ignore']"); + }); + + test('log sink is append-mode (accumulates across respawns)', () => { + const cli = SRC('cli.ts'); + expect(cli).toMatch(/openSync\(path\.join\(config\.stateDir, 'browse-daemon\.log'\), 'a'\)/); + expect(cli).toMatch(/openSync\(\$\{daemonLogPathStr\},'a'\)/); + }); + + test('bun-polyfill routes Windows spawns through cross-spawn (ENOENT + cmd.exe injection fix)', () => { + const polyfill = SRC('bun-polyfill.cjs'); + expect(polyfill).toContain("require('cross-spawn')"); + expect(polyfill).toMatch(/process\.platform === 'win32' \? crossSpawn\.sync : nodeSpawnSync/); + expect(polyfill).toMatch(/process\.platform === 'win32' \? crossSpawn : nodeSpawn/); + // The rejected-for-cause alternative must not creep back in: shell:true + // on Windows routes through cmd.exe and does NOT neutralize & | ^ % < >. + // (Strip comments — the header documents WHY shell:true was rejected.) + const code = polyfill.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, ''); + expect(code).not.toMatch(/shell:\s*true/); + }); +}); + +describe('F6 log hygiene: nothing secret or page-derived reaches daemon console', () => { + const files = fs.readdirSync(SRC_DIR).filter((f) => f.endsWith('.ts') || f.endsWith('.cjs')); + + test('no console.* call passes a token value', () => { + const offenders: string[] = []; + for (const file of files) { + const content = SRC(file); + for (const [idx, line] of content.split('\n').entries()) { + if (!/console\.(log|error|warn|info)\(/.test(line)) continue; + // Interpolated token values: ${...token} / ${...Token} — the + // expression ENDS in token, i.e. the value IS the token. Names like + // ${tokenInfo.clientId} don't match. + if (/\$\{[^}]*[tT]oken\s*\}/.test(line)) { + offenders.push(`${file}:${idx + 1}: ${line.trim().slice(0, 120)}`); + continue; + } + // Bare token args: console.log('x', token) / (..., authToken) + if (/console\.(log|error|warn|info)\([^)]*[^a-zA-Z_.][tT]oken\s*[,)]/.test(line)) { + offenders.push(`${file}:${idx + 1}: ${line.trim().slice(0, 120)}`); + } + } + } + expect(offenders).toEqual([]); + }); + + test('page-content carrier modules are console-free', () => { + // Page-derived strings flow through these modules. Keeping them + // console-free guarantees raw page content can't be echoed into + // browse-daemon.log without passing an egress sanitizer first. + for (const file of ['tab-session.ts', 'buffers.ts', 'content-security.ts', 'activity.ts']) { + const content = SRC(file); + const calls = content.match(/console\.(log|error|warn|info)\(/g) || []; + expect({ file, count: calls.length }).toEqual({ file, count: 0 }); + } + }); +}); diff --git a/bun.lock b/bun.lock index 16406f0a7..724d0cd42 100644 --- a/bun.lock +++ b/bun.lock @@ -7,6 +7,7 @@ "dependencies": { "@huggingface/transformers": "^4.1.0", "@ngrok/ngrok": "^1.7.0", + "cross-spawn": "^7.0.6", "diff": "^9.0.0", "html-to-docx": "1.8.0", "marked": "^18.0.2", diff --git a/package.json b/package.json index aa5a26353..e6667c14c 100644 --- a/package.json +++ b/package.json @@ -53,6 +53,7 @@ "dependencies": { "@huggingface/transformers": "^4.1.0", "@ngrok/ngrok": "^1.7.0", + "cross-spawn": "^7.0.6", "diff": "^9.0.0", "html-to-docx": "1.8.0", "marked": "^18.0.2",