From 50d07eb2347e7fcc209e65ced779b3593a60b36c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 7 May 2026 14:54:15 -0700 Subject: [PATCH] fix(browse): apply codex adversarial findings on the new lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex outside-voice review caught five real production-failure modes in the v1.28.0.0 proxy/headed lifecycle. Fixed: 1) `browse disconnect` skip-graceful for proxy-only daemons (browse/src/cli.ts). The graceful /command POST went out with stray `domains,` shorthand and (even fixed) the server's disconnect handler only tears down headed mode — proxy-only daemons returned 200 "Not in headed mode" while leaving the bridge running. Now disconnect short-circuits to force-cleanup for non-headed daemons, which kicks process.on('exit') in server.ts to close the bridge + Xvfb. 2) sendCommand crash retry preserves --proxy / --headed (browse/src/cli.ts). The ECONNRESET retry path called startServer() with no extraEnv, silently dropping the proxied flags. A daemon that died mid-command would silently restart in default direct/headless mode and bypass the SOCKS bridge. Now reapplies BROWSE_PROXY_URL, BROWSE_HEADED, and BROWSE_CONFIG_HASH from the resolved global flags. 3) `connect` honors --proxy (browse/src/cli.ts). The headed-mode `connect` command built its own serverEnv that didn't include BROWSE_PROXY_URL, so `browse --proxy connect` launched headed Chromium without the proxy. Now threads proxyUrl + configHash into the connect serverEnv. 4) SOCKS5 bridge handles fragmented TCP frames (browse/src/socks-bridge.ts). Previously used once('data') and parsed each chunk as a complete SOCKS5 frame — TCP doesn't preserve message boundaries and split greetings/CONNECT requests caused intermittent handshake failures. Replaced with a single state machine that buffers chunks and uses size predicates on the SOCKS5 header to know when a complete frame has arrived. Pauses the client socket during upstream connect and replays any remainder bytes into the upstream on success. 5) Xvfb cleanup-then-state-delete ordering (browse/src/server.ts). emergencyCleanup() previously deleted the state file BEFORE any Xvfb cleanup could read it, orphaning Xvfb on uncaughtException / unhandledRejection. Now reads the state file first, calls cleanupXvfb() (which validates cmdline + start-time before kill), then deletes the state file. Adds a regression test for #4: writes the SOCKS5 greeting + CONNECT one byte at a time with 5ms ticks, asserts a clean round trip after the fragmented handshake. Codex's sixth finding (bridge advertises NO_AUTH on 127.0.0.1, so any co-located process can use the authenticated upstream) is documented as a known limitation — gstack's threat model assumes single-user hosts. Adding bridge-side auth is a separate change. Co-Authored-By: Claude Opus 4.7 (1M context) --- browse/src/cli.ts | 63 +++++++++----- browse/src/server.ts | 25 ++++++ browse/src/socks-bridge.ts | 136 +++++++++++++++++++++---------- browse/test/socks-bridge.test.ts | 81 ++++++++++++++++++ 4 files changed, 244 insertions(+), 61 deletions(-) diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 099fa2dc..3ddbf2f3 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -497,13 +497,26 @@ async function sendCommand(state: ServerState, command: string, args: string[], if (oldState && oldState.pid) { await killServer(oldState.pid); } - const newState = await startServer(); + // Reapply --proxy / --headed flags from this invocation when restarting + // after a crash. Without this, a proxied daemon that dies mid-command + // would silently restart in default direct/headless mode and bypass + // the SOCKS bridge. + const restartEnv: Record = {}; + if (_globalFlags?.proxyUrl) restartEnv.BROWSE_PROXY_URL = _globalFlags.proxyUrl; + if (_globalFlags?.headed) restartEnv.BROWSE_HEADED = '1'; + if (_globalFlags?.configHash) restartEnv.BROWSE_CONFIG_HASH = _globalFlags.configHash; + const newState = await startServer(Object.keys(restartEnv).length ? restartEnv : undefined); return sendCommand(newState, command, args, retries + 1); } throw err; } } +// Module-level reference to the resolved global flags from main(). Used by +// sendCommand's crash-retry path so a daemon restart after ECONNRESET doesn't +// silently drop --proxy / --headed. +let _globalFlags: GlobalFlags | null = null; + // ─── Ngrok Detection ─────────────────────────────────────────── /** Check if ngrok is installed and authenticated (native config or gstack env). */ @@ -877,6 +890,7 @@ async function main() { } throw err; } + _globalFlags = globalFlags; const args = globalFlags.args; if (args.length === 0 || args[0] === '--help' || args[0] === '-h') { @@ -992,6 +1006,11 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: // it would kill the server ~15s later. Cleanup happens via browser // disconnect event or $B disconnect. BROWSE_PARENT_PID: '0', + // Apply --proxy from this invocation if present. Without this, + // `browse --proxy connect` would launch headed Chromium + // bypassing the SOCKS bridge entirely. + ...(globalFlags.proxyUrl ? { BROWSE_PROXY_URL: globalFlags.proxyUrl } : {}), + ...(globalFlags.configHash ? { BROWSE_CONFIG_HASH: globalFlags.configHash } : {}), }; const newState = await startServer(serverEnv); @@ -1064,25 +1083,31 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: console.log('Not in headed/custom-config mode — nothing to disconnect.'); process.exit(0); } - // Try graceful shutdown via server - try { - const resp = await fetch(`http://127.0.0.1:${existingState.port}/command`, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - 'Authorization': `Bearer ${existingState.token}`, - }, - body: JSON.stringify({ - domains, - command: 'disconnect', args: [] }), - signal: AbortSignal.timeout(3000), - }); - if (resp.ok) { - console.log('Disconnected from real browser.'); - process.exit(0); + // For headed-mode daemons: try graceful shutdown via the server's + // /command endpoint. For proxy-only / custom-config daemons (no headed + // mode), the server's `disconnect` handler currently only tears down + // headed state — it returns 200 "Not in headed mode" without cleaning + // up the bridge or Xvfb. So we skip the graceful path for those and + // jump straight to force-cleanup, which kills the daemon process and + // lets process.on('exit') in server.ts close the bridge + Xvfb. + if (existingState.mode === 'headed') { + try { + const resp = await fetch(`http://127.0.0.1:${existingState.port}/command`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Authorization': `Bearer ${existingState.token}`, + }, + body: JSON.stringify({ command: 'disconnect', args: [] }), + signal: AbortSignal.timeout(3000), + }); + if (resp.ok) { + console.log('Disconnected from real browser.'); + process.exit(0); + } + } catch { + // Server not responding — fall through to force cleanup } - } catch { - // Server not responding — force cleanup } // Force kill + cleanup if (isProcessAlive(existingState.pid)) { diff --git a/browse/src/server.ts b/browse/src/server.ts index af3259ae..4df55ad8 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -996,6 +996,31 @@ if (process.platform === 'win32') { function emergencyCleanup() { if (isShuttingDown) return; isShuttingDown = true; + // Xvfb cleanup MUST happen before state-file deletion. spawnXvfb detaches + // the child, so without this, an uncaught exception leaves the Xvfb + // running with no PID record — orphan accumulates and eventually + // exhausts the :99-:120 display range. Read the state file FIRST, + // call cleanupXvfb (validates cmdline + start-time before kill), THEN + // delete the state file. + try { + if (fs.existsSync(config.stateFile)) { + const raw = fs.readFileSync(config.stateFile, 'utf-8'); + const state = JSON.parse(raw); + if (state.xvfbPid && state.xvfbStartTime) { + // Lazy import — emergencyCleanup may run on platforms where + // ./xvfb's Linux-specific helpers fail to load. Best effort. + try { + const { cleanupXvfb } = require('./xvfb'); + cleanupXvfb({ + pid: state.xvfbPid, + startTime: state.xvfbStartTime, + display: state.xvfbDisplay || ':99', + }); + } catch { /* best effort */ } + } + } + } catch { /* state file unparseable — fall through to lock + state cleanup */ } + // Clean Chromium profile locks const profileDir = path.join(process.env.HOME || '/tmp', '.gstack', 'chromium-profile'); for (const lockFile of ['SingletonLock', 'SingletonSocket', 'SingletonCookie']) { diff --git a/browse/src/socks-bridge.ts b/browse/src/socks-bridge.ts index a91bd9e6..dc8b2e21 100644 --- a/browse/src/socks-bridge.ts +++ b/browse/src/socks-bridge.ts @@ -109,61 +109,113 @@ export async function startSocksBridge(opts: { const requestedPort = opts.port ?? 0; const inFlight = new Set(); + // Frame-size predicates for the two SOCKS5 messages we read from the + // client. Both return null when we don't yet have enough bytes to know + // the frame size, or a positive integer when we do. + function greetingSize(buf: Buffer): number | null { + if (buf.length < 2) return null; + return 2 + buf[1]; // VER NMETHODS + N method bytes + } + function connectSize(buf: Buffer): number | null { + if (buf.length < 5) return null; + const atyp = buf[3]; + if (atyp === ATYP_IPV4) return 10; // VER CMD RSV ATYP + 4 + 2 + if (atyp === ATYP_IPV6) return 22; // VER CMD RSV ATYP + 16 + 2 + if (atyp === ATYP_DOMAINNAME) return 7 + buf[4]; // VER CMD RSV ATYP LEN + N + 2 + return null; + } + + type State = 'greeting' | 'connect' | 'connecting' | 'piped' | 'closed'; + const server = net.createServer((clientSocket) => { inFlight.add(clientSocket); clientSocket.once('close', () => inFlight.delete(clientSocket)); - // Handshake step 1: client greeting → respond no-auth. - clientSocket.once('data', (greeting) => { - if (greeting[0] !== SOCKS5_VERSION) { - clientSocket.destroy(); - return; - } - try { clientSocket.write(Buffer.from([SOCKS5_VERSION, NO_AUTH_METHOD])); } - catch { clientSocket.destroy(); return; } + let state: State = 'greeting'; + let buf = Buffer.alloc(0); + let upstreamSocket: net.Socket | null = null; - // Handshake step 2: client CONNECT request. - clientSocket.once('data', async (reqData) => { + const killBoth = (reason?: string) => { + void reason; + state = 'closed'; + try { clientSocket.destroy(); } catch { /* already gone */ } + if (upstreamSocket) { + try { upstreamSocket.destroy(); } catch { /* already gone */ } + } + }; + + const handshakeTimeout = setTimeout(() => { + if (state === 'greeting' || state === 'connect' || state === 'connecting') { + killBoth('handshake timeout'); + } + }, 30000); + clientSocket.once('close', () => clearTimeout(handshakeTimeout)); + + const onData = (chunk: Buffer) => { + if (state === 'closed' || state === 'piped') return; + buf = buf.length === 0 ? chunk : Buffer.concat([buf, chunk]); + + if (state === 'greeting') { + const sz = greetingSize(buf); + if (sz == null || buf.length < sz) return; + const greeting = buf.subarray(0, sz); + buf = buf.subarray(sz); + if (greeting[0] !== SOCKS5_VERSION) { killBoth('bad version'); return; } + try { clientSocket.write(Buffer.from([SOCKS5_VERSION, NO_AUTH_METHOD])); } + catch { killBoth('write greeting reply failed'); return; } + state = 'connect'; + // Fall through — buf may already contain CONNECT bytes (coalesced). + } + + if (state === 'connect') { + const sz = connectSize(buf); + if (sz == null || buf.length < sz) return; + const reqData = buf.subarray(0, sz); + const remainder = buf.subarray(sz); const dest = parseConnectRequest(reqData); if (!dest) { writeReply(clientSocket, REPLY_GENERAL_FAILURE); - clientSocket.destroy(); + killBoth('bad connect request'); return; } - - let upstreamSocket: net.Socket; - try { - const result = await SocksClient.createConnection({ - proxy: upstreamProxy, - command: 'connect', - destination: { host: dest.host, port: dest.port }, - timeout: UPSTREAM_CONNECT_TIMEOUT_MS, - }); + state = 'connecting'; + // Pause client reads so any post-handshake bytes don't get dropped. + // We replay `remainder` after upstream is established. + clientSocket.pause(); + SocksClient.createConnection({ + proxy: upstreamProxy, + command: 'connect', + destination: { host: dest.host, port: dest.port }, + timeout: UPSTREAM_CONNECT_TIMEOUT_MS, + }).then((result) => { + if (state === 'closed') { + try { result.socket.destroy(); } catch { /* shutdown */ } + return; + } upstreamSocket = result.socket; - } catch { + writeReply(clientSocket, REPLY_SUCCESS); + // Replay any pre-buffered post-handshake bytes BEFORE we pipe. + if (remainder.length > 0) { + try { upstreamSocket.write(remainder); } catch { killBoth('replay write failed'); return; } + } + // Wire the rest of the connection through the pipe. + upstreamSocket.on('error', () => killBoth('upstream error')); + upstreamSocket.on('close', () => { try { clientSocket.destroy(); } catch { /* already gone */ } }); + clientSocket.removeListener('data', onData); + clientSocket.pipe(upstreamSocket); + upstreamSocket.pipe(clientSocket); + clientSocket.resume(); + state = 'piped'; + }).catch(() => { writeReply(clientSocket, REPLY_HOST_UNREACHABLE); - clientSocket.destroy(); - return; - } + killBoth('upstream connect failed'); + }); + return; + } + }; - writeReply(clientSocket, REPLY_SUCCESS); - - // Pipe bidirectionally. On any error, kill BOTH sockets (no retries). - const killBoth = () => { - try { clientSocket.destroy(); } catch { /* already gone */ } - try { upstreamSocket.destroy(); } catch { /* already gone */ } - }; - clientSocket.on('error', killBoth); - upstreamSocket.on('error', killBoth); - clientSocket.on('close', () => { try { upstreamSocket.destroy(); } catch { /* already gone */ } }); - upstreamSocket.on('close', () => { try { clientSocket.destroy(); } catch { /* already gone */ } }); - - clientSocket.pipe(upstreamSocket); - upstreamSocket.pipe(clientSocket); - }); - }); - - clientSocket.on('error', () => clientSocket.destroy()); + clientSocket.on('data', onData); + clientSocket.on('error', () => killBoth('client error')); }); await new Promise((resolve, reject) => { diff --git a/browse/test/socks-bridge.test.ts b/browse/test/socks-bridge.test.ts index 4a8bb630..dc6b859c 100644 --- a/browse/test/socks-bridge.test.ts +++ b/browse/test/socks-bridge.test.ts @@ -283,6 +283,87 @@ describe('startSocksBridge', () => { } }); + test('handles SOCKS5 handshake split across multiple TCP packets (codex finding)', async () => { + // TCP doesn't preserve message boundaries — production networks regularly + // fragment small writes. This test simulates that by writing the greeting + // and CONNECT request one byte at a time. If the bridge uses once('data') + // and assumes each event is a complete frame, this test fails because + // it parses the first byte as a frame. + const echo = await startEcho(); + const upstream = await startMockUpstream({ expectedUser: 'u', expectedPass: 'p' }); + const bridge = await startSocksBridge({ + upstream: { host: '127.0.0.1', port: upstream.port, userId: 'u', password: 'p' }, + }); + + try { + // Build the greeting + CONNECT request manually. + const greeting = Buffer.from([0x05, 0x01, 0x00]); + const hostBuf = Buffer.from(echo.host); + const connect = Buffer.alloc(7 + hostBuf.length); + connect[0] = 0x05; connect[1] = 0x01; connect[2] = 0x00; connect[3] = 0x03; + connect[4] = hostBuf.length; + hostBuf.copy(connect, 5); + connect.writeUInt16BE(echo.port, 5 + hostBuf.length); + + const sock = net.createConnection({ host: '127.0.0.1', port: bridge.port }); + await new Promise((r, rej) => { + sock.once('connect', () => r()); + sock.once('error', rej); + }); + + // Persistent buffered reader. Using a single long-lived 'data' + // listener avoids the bytes-dropped race that happens when you + // attach `sock.once('data')`, get one event, and re-attach later — + // any data arriving between those two attaches gets dropped because + // the socket is in flowing mode without a listener. + const inbox: Buffer[] = []; + sock.on('data', (chunk) => inbox.push(chunk)); + const readAtLeast = async (n: number, timeoutMs = 2000): Promise => { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + const total = inbox.reduce((s, b) => s + b.length, 0); + if (total >= n) { + const all = Buffer.concat(inbox); + inbox.length = 0; + if (all.length > n) inbox.push(all.subarray(n)); + return all.subarray(0, n); + } + await new Promise((r) => setTimeout(r, 10)); + } + throw new Error(`timeout waiting for ${n} bytes (have ${inbox.reduce((s, b) => s + b.length, 0)})`); + }; + + // Write greeting one byte at a time. + for (let i = 0; i < greeting.length; i++) { + sock.write(Buffer.from([greeting[i]])); + await new Promise((r) => setTimeout(r, 5)); + } + const greetingReply = await readAtLeast(2); + expect(greetingReply[0]).toBe(0x05); + expect(greetingReply[1]).toBe(0x00); + + // Write CONNECT one byte at a time. + for (let i = 0; i < connect.length; i++) { + sock.write(Buffer.from([connect[i]])); + await new Promise((r) => setTimeout(r, 5)); + } + const connectReply = await readAtLeast(10); + expect(connectReply[0]).toBe(0x05); + expect(connectReply[1]).toBe(0x00); + + // Round trip should still work after the fragmented handshake. + const payload = Buffer.from('payload-after-split-handshake'); + sock.write(payload); + const received = await readAtLeast(payload.length); + expect(received.toString()).toBe(payload.toString()); + sock.destroy(); + } finally { + await bridge.close(); + await upstream.close(); + await echo.close(); + } + }); + test('close() tears down listener and in-flight clients', async () => { const upstream = await startMockUpstream({ expectedUser: 'u', expectedPass: 'p' }); const bridge = await startSocksBridge({