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",