diff --git a/browse/src/read-commands.ts b/browse/src/read-commands.ts index 486eac18a..4e1371a17 100644 --- a/browse/src/read-commands.ts +++ b/browse/src/read-commands.ts @@ -13,7 +13,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { TEMP_DIR } from './platform'; import { inspectElement, formatInspectorResult, getModificationHistory } from './cdp-inspector'; -import { validateReadPath } from './path-security'; +import { validateReadPath, validateOutputPath } from './path-security'; import { stripLoneSurrogates } from './sanitize'; // Re-export for backward compatibility (tests import from read-commands) export { validateReadPath } from './path-security'; @@ -46,6 +46,117 @@ function wrapForEvaluate(code: string): string { : `(async()=>(${trimmed}))()`; } +/** Flags split out of `js`/`eval` args by parseOutArgs. */ +export interface OutArgs { + outPath?: string; + raw: boolean; + rest: string[]; +} + +/** + * Parse `--out ` / `--out=` and `--raw` / `--raw=true|false` out of an + * arg list, returning the flags plus the remaining positional args (`rest`). + * + * Single source of truth shared by the js/eval handlers and the write-capability + * gate in server.ts, so the two never disagree on what counts as an `--out` + * invocation. Throws on malformed usage (repeated `--out`, missing value, bad + * `--raw` value) so the user gets a clear error instead of a silent misparse. + */ +export function parseOutArgs(args: string[]): OutArgs { + let outPath: string | undefined; + let raw = false; + const rest: string[] = []; + for (let i = 0; i < args.length; i++) { + const a = args[i]; + if (a === '--out') { + if (outPath !== undefined) throw new Error('--out specified more than once'); + const val = args[i + 1]; + if (val === undefined || val.startsWith('--')) throw new Error('--out requires a file path'); + outPath = val; + i++; + } else if (a.startsWith('--out=')) { + if (outPath !== undefined) throw new Error('--out specified more than once'); + const val = a.slice('--out='.length); + if (val === '') throw new Error('--out requires a file path'); + outPath = val; + } else if (a === '--raw') { + raw = true; + } else if (a.startsWith('--raw=')) { + const v = a.slice('--raw='.length).toLowerCase(); + if (v !== 'true' && v !== 'false') throw new Error('--raw must be true or false'); + raw = v === 'true'; + } else { + rest.push(a); + } + } + return { outPath, raw, rest }; +} + +/** + * True iff an arg list contains an `--out` flag in any accepted form + * (`--out ` or `--out=`). Used by the write-capability gate to + * decide whether an otherwise-read command (`js`/`eval`) is actually a write + * invocation. Mirrors parseOutArgs's `--out` recognition exactly. Never throws — + * a malformed `--out=` still counts as an out attempt (fail safe: gate it). + */ +export function hasOutArg(args: string[]): boolean { + return args.some(a => a === '--out' || a.startsWith('--out=')); +} + +/** + * Convert an evaluate() result to its string form — the exact conversion `js`/`eval` + * used inline before `--out` existed. Kept byte-for-byte: `typeof === 'object'` + * (which includes `null`) goes through JSON.stringify (so `null` → `"null"`); + * everything else via `String(result ?? '')` (so `undefined` → `''`). JSON.stringify + * still throws on circular / BigInt-bearing results, same as before. + */ +export function resultToString(result: unknown): string { + return typeof result === 'object' + ? JSON.stringify(result, null, 2) + : String(result ?? ''); +} + +/** + * Write an evaluate result string to disk for `--out`, returning bytes written. + * + * When the result is a base64 data URL (`data:;...;base64,`) and + * `raw` is false, decode the payload to raw bytes — this is the Excalidraw / og-image + * path where a render function returns a PNG data URL. The header is parsed + * case-insensitively and split on the FIRST comma (data URLs can contain commas in + * the payload). The payload is validated against the base64 charset before decoding, + * because `Buffer.from(_, 'base64')` silently drops invalid characters and would + * otherwise write corrupted bytes. `--raw` forces a literal write even for data URLs. + * + * Non-base64 strings are surrogate-sanitized (matching what the stdout egress path + * did before) and written as UTF-8. Parent directories are created — validateOutputPath + * gates the location but does not mkdir. + */ +export function writeEvalResult(outPath: string, str: string, opts: { raw: boolean }): number { + validateOutputPath(outPath); + fs.mkdirSync(path.dirname(path.resolve(outPath)), { recursive: true }); + + if (!opts.raw && str.startsWith('data:')) { + const comma = str.indexOf(','); + if (comma !== -1) { + const header = str.slice('data:'.length, comma); + const tokens = header.split(';').map(t => t.trim().toLowerCase()); + if (tokens.includes('base64')) { + const payload = str.slice(comma + 1).replace(/\s+/g, ''); + if (!/^[A-Za-z0-9+/]*={0,2}$/.test(payload)) { + throw new Error('--out: malformed base64 in data URL (decode would corrupt output)'); + } + const buf = Buffer.from(payload, 'base64'); + fs.writeFileSync(outPath, buf); + return buf.length; + } + } + } + + const buf = Buffer.from(stripLoneSurrogates(str), 'utf-8'); + fs.writeFileSync(outPath, buf); + return buf.length; +} + /** * Extract clean text from a page (strips script/style/noscript/svg). * Exported for DRY reuse in meta-commands (diff). @@ -179,24 +290,36 @@ export async function handleReadCommand( } case 'js': { - const expr = args[0]; - if (!expr) throw new Error('Usage: browse js '); + const { outPath, raw, rest } = parseOutArgs(args); + const expr = rest[0]; + if (!expr) throw new Error('Usage: browse js [--out ] [--raw]'); if (bm) assertJsOriginAllowed(bm, page.url()); const wrapped = wrapForEvaluate(expr); const result = await target.evaluate(wrapped); - return typeof result === 'object' ? JSON.stringify(result, null, 2) : String(result ?? ''); + const str = resultToString(result); + if (outPath) { + const n = writeEvalResult(outPath, str, { raw }); + return `JS result written: ${outPath} (${n} bytes)`; + } + return str; } case 'eval': { - const filePath = args[0]; - if (!filePath) throw new Error('Usage: browse eval '); + const { outPath, raw, rest } = parseOutArgs(args); + const filePath = rest[0]; + if (!filePath) throw new Error('Usage: browse eval [--out ] [--raw]'); if (bm) assertJsOriginAllowed(bm, page.url()); validateReadPath(filePath); if (!fs.existsSync(filePath)) throw new Error(`File not found: ${filePath}`); const code = fs.readFileSync(filePath, 'utf-8'); const wrapped = wrapForEvaluate(code); const result = await target.evaluate(wrapped); - return typeof result === 'object' ? JSON.stringify(result, null, 2) : String(result ?? ''); + const str = resultToString(result); + if (outPath) { + const n = writeEvalResult(outPath, str, { raw }); + return `Eval result written: ${outPath} (${n} bytes)`; + } + return str; } case 'css': { diff --git a/browse/src/server.ts b/browse/src/server.ts index 6f75551ff..301781acc 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -14,7 +14,7 @@ */ import { BrowserManager } from './browser-manager'; -import { handleReadCommand } from './read-commands'; +import { handleReadCommand, hasOutArg } from './read-commands'; import { handleWriteCommand } from './write-commands'; import { handleMetaCommand } from './meta-commands'; import { handleCookiePickerRoute, hasActivePicker } from './cookie-picker-routes'; @@ -330,9 +330,15 @@ export const TUNNEL_COMMANDS = new Set([ * without standing up an HTTP listener. Behavior is identical to the inline * check; the function canonicalizes the command (so aliases hit the same set) * and returns false for null/undefined input. + * + * `args` is consulted so an `--out` invocation (e.g. `eval --out `) is + * NEVER tunnel-dispatchable: `--out` turns an otherwise-readable command into a + * local-disk WRITE, and the tunnel surface never grants disk-write capability to + * remote paired agents. Omitting `args` preserves the old command-only behavior. */ -export function canDispatchOverTunnel(command: string | undefined | null): boolean { +export function canDispatchOverTunnel(command: string | undefined | null, args?: string[]): boolean { if (typeof command !== 'string' || command.length === 0) return false; + if (Array.isArray(args) && hasOutArg(args)) return false; const cmd = canonicalizeCommand(command); return TUNNEL_COMMANDS.has(cmd); } @@ -716,6 +722,19 @@ if (BROWSE_PARENT_PID > 0 && !IS_HEADED_WATCHDOG) { import { READ_COMMANDS, WRITE_COMMANDS, META_COMMANDS } from './commands'; export { READ_COMMANDS, WRITE_COMMANDS, META_COMMANDS }; +/** + * Whether an invocation should be treated as a WRITE for capability gating + * (scope, watch-mode block, tab ownership, tunnel). A command is a write if it + * mutates state (`WRITE_COMMANDS`) OR it carries an `--out` flag — `js`/`eval + * --out` writes the evaluate result to local disk, so the capability is + * per-invocation, not per-command-name. This deliberately does NOT change + * dispatch routing: `js`/`eval` still route to `handleReadCommand`; only the + * security gates consult this. + */ +function isWriteInvocation(command: string, args: string[]): boolean { + return WRITE_COMMANDS.has(command) || hasOutArg(args); +} + // ─── Inspector State (in-memory) ────────────────────────────── let inspectorData: InspectorResult | null = null; let inspectorTimestamp: number = 0; @@ -957,6 +976,19 @@ async function handleCommandInternalImpl( }; } + // `--out` writes the evaluate result to local disk, which is a WRITE + // capability distinct from the JS-exec (admin) capability js/eval need. + // Require write scope so an admin-but-not-write token can't write files. + if (hasOutArg(args) && !tokenInfo.scopes.includes('write')) { + return { + status: 403, json: true, + result: JSON.stringify({ + error: `"--out" writes to disk and requires the "write" scope`, + hint: `Your scopes: ${tokenInfo.scopes.join(', ')}. Re-pair with write access to use --out.`, + }), + }; + } + // Domain check for navigation commands if ((command === 'goto' || command === 'newtab') && args[0]) { if (!checkDomain(tokenInfo, args[0])) { @@ -1011,7 +1043,7 @@ async function handleCommandInternalImpl( // Skip for `newtab` — it creates a tab rather than accessing one. if (command !== 'newtab' && tokenInfo && tokenInfo.clientId !== 'root' && tokenInfo.tabPolicy === 'own-only') { const targetTab = tabId ?? browserManager.getActiveTabId(); - if (!browserManager.checkTabAccess(targetTab, tokenInfo.clientId, { isWrite: WRITE_COMMANDS.has(command), ownOnly: true })) { + if (!browserManager.checkTabAccess(targetTab, tokenInfo.clientId, { isWrite: isWriteInvocation(command, args), ownOnly: true })) { return { status: 403, json: true, result: JSON.stringify({ @@ -1035,8 +1067,9 @@ async function handleCommandInternalImpl( }; } - // Block mutation commands while watching (read-only observation mode) - if (browserManager.isWatching() && WRITE_COMMANDS.has(command)) { + // Block mutation commands while watching (read-only observation mode). + // `--out` invocations count as mutations (they write the result to disk). + if (browserManager.isWatching() && isWriteInvocation(command, args)) { return { status: 400, json: true, result: JSON.stringify({ error: 'Cannot run mutation commands while watching. Run `$B watch stop` first.' }), @@ -2650,11 +2683,11 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { // Paired remote agents drive the browser but cannot configure the // daemon, launch new browsers, import cookies, or rotate tokens. if (surface === 'tunnel') { - if (!canDispatchOverTunnel(body?.command)) { + if (!canDispatchOverTunnel(body?.command, body?.args)) { logTunnelDenial(req, url, `disallowed_command:${body?.command}`); return new Response(JSON.stringify({ error: `Command '${body?.command}' is not allowed over the tunnel surface`, - hint: `Tunnel commands: ${[...TUNNEL_COMMANDS].sort().join(', ')}`, + hint: `Tunnel commands: ${[...TUNNEL_COMMANDS].sort().join(', ')}. Note: --out (disk write) is never allowed over the tunnel.`, }), { status: 403, headers: { 'Content-Type': 'application/json' } }); } } diff --git a/browse/test/commands.test.ts b/browse/test/commands.test.ts index b3870c0cc..9382cb27e 100644 --- a/browse/test/commands.test.ts +++ b/browse/test/commands.test.ts @@ -9,7 +9,7 @@ import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; import { startTestServer } from './test-server'; import { BrowserManager } from '../src/browser-manager'; import { resolveServerScript } from '../src/cli'; -import { handleReadCommand as _handleReadCommand } from '../src/read-commands'; +import { handleReadCommand as _handleReadCommand, parseOutArgs, hasOutArg, resultToString } from '../src/read-commands'; import { handleWriteCommand as _handleWriteCommand } from '../src/write-commands'; import { handleMetaCommand } from '../src/meta-commands'; import { consoleBuffer, networkBuffer, dialogBuffer, addConsoleEntry, addNetworkEntry, addDialogEntry, CircularBuffer } from '../src/buffers'; @@ -23,6 +23,65 @@ const handleReadCommand = (cmd: string, args: string[], b: BrowserManager) => const handleWriteCommand = (cmd: string, args: string[], b: BrowserManager) => _handleWriteCommand(cmd, args, b.getActiveSession(), b); +// ─── Pure arg-parser + result-conversion unit tests (no browser) ─── +describe('parseOutArgs / hasOutArg', () => { + test('--out splits the flag from the positional', () => { + expect(parseOutArgs(['expr', '--out', '/tmp/x'])).toEqual({ outPath: '/tmp/x', raw: false, rest: ['expr'] }); + }); + + test('--out= form is equivalent', () => { + expect(parseOutArgs(['expr', '--out=/tmp/x'])).toEqual({ outPath: '/tmp/x', raw: false, rest: ['expr'] }); + }); + + test('flag ordering does not matter', () => { + expect(parseOutArgs(['--out', '/tmp/x', 'expr'])).toEqual({ outPath: '/tmp/x', raw: false, rest: ['expr'] }); + }); + + test('--raw and --raw=true|false', () => { + expect(parseOutArgs(['e', '--out', '/tmp/x', '--raw']).raw).toBe(true); + expect(parseOutArgs(['e', '--out', '/tmp/x', '--raw=true']).raw).toBe(true); + expect(parseOutArgs(['e', '--out', '/tmp/x', '--raw=false']).raw).toBe(false); + }); + + test('repeated --out throws', () => { + expect(() => parseOutArgs(['e', '--out', '/a', '--out', '/b'])).toThrow(/more than once/); + }); + + test('--out with a missing value throws', () => { + expect(() => parseOutArgs(['e', '--out'])).toThrow(/requires a file path/); + expect(() => parseOutArgs(['e', '--out', '--raw'])).toThrow(/requires a file path/); + expect(() => parseOutArgs(['e', '--out='])).toThrow(/requires a file path/); + }); + + test('bad --raw value throws', () => { + expect(() => parseOutArgs(['e', '--out', '/a', '--raw=maybe'])).toThrow(/--raw must be true or false/); + }); + + test('hasOutArg matches --out and --out= exactly, not lookalikes', () => { + expect(hasOutArg(['a', '--out', 'b'])).toBe(true); + expect(hasOutArg(['a', '--out=b'])).toBe(true); + expect(hasOutArg(['a'])).toBe(false); + expect(hasOutArg(['a', '--output', 'b'])).toBe(false); + expect(hasOutArg(['a', '--outx'])).toBe(false); + }); +}); + +describe('resultToString — byte-for-byte with pre-refactor behavior', () => { + test('null becomes "null" (typeof null === object → JSON.stringify)', () => { + expect(resultToString(null)).toBe('null'); + }); + test('undefined becomes empty string', () => { + expect(resultToString(undefined)).toBe(''); + }); + test('objects are pretty-printed JSON', () => { + expect(resultToString({ a: 1 })).toBe(JSON.stringify({ a: 1 }, null, 2)); + }); + test('primitives use String()', () => { + expect(resultToString(42)).toBe('42'); + expect(resultToString(true)).toBe('true'); + }); +}); + let testServer: ReturnType; let bm: BrowserManager; let baseUrl: string; @@ -225,6 +284,102 @@ describe('Inspection', () => { expect(result).toBe('3'); }); + // ─── js/eval --out (render-to-file) ─────────────────────────── + + test('js (no --out) returns a multi-MB string without truncation', async () => { + // Handler-level guarantee: the result is not sliced/capped before return. + // (Full HTTP egress path is exercised elsewhere; this pins the handler.) + const result = await handleReadCommand('js', ["'x'.repeat(3 * 1024 * 1024)"], bm); + expect(result.length).toBe(3 * 1024 * 1024); + }); + + test('js --out writes the result to disk and returns a short status, not the payload', async () => { + const out = `/tmp/browse-out-large-${Date.now()}.txt`; + try { + const result = await handleReadCommand('js', ["'y'.repeat(2 * 1024 * 1024)", '--out', out], bm); + expect(result).toContain('JS result written:'); + expect(result).toContain(out); + expect(result).toContain(`(${2 * 1024 * 1024} bytes)`); + expect(result.length).toBeLessThan(200); // status, not the 2MB payload + expect(fs.statSync(out).size).toBe(2 * 1024 * 1024); + } finally { + fs.rmSync(out, { force: true }); + } + }); + + test('js --out decodes a base64 PNG data URL to real bytes', async () => { + // 1x1 transparent PNG. + const b64 = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+M9QDwADhgGAWjR9awAAAABJRU5ErkJggg=='; + const out = `/tmp/browse-out-png-${Date.now()}.png`; + try { + const result = await handleReadCommand('js', [`'data:image/png;base64,' + '${b64}'`, '--out', out], bm); + const buf = fs.readFileSync(out); + // PNG magic bytes: 89 50 4E 47 + expect([buf[0], buf[1], buf[2], buf[3]]).toEqual([0x89, 0x50, 0x4e, 0x47]); + const expectedLen = Buffer.from(b64, 'base64').length; + expect(buf.length).toBe(expectedLen); + expect(result).toContain(`(${expectedLen} bytes)`); + } finally { + fs.rmSync(out, { force: true }); + } + }); + + test('js --out --raw writes the literal data-URL string (no decode)', async () => { + const dataUrl = 'data:text/plain;base64,aGVsbG8='; + const out = `/tmp/browse-out-raw-${Date.now()}.txt`; + try { + await handleReadCommand('js', [`'${dataUrl}'`, '--out', out, '--raw'], bm); + expect(fs.readFileSync(out, 'utf-8')).toBe(dataUrl); + } finally { + fs.rmSync(out, { force: true }); + } + }); + + test('js --out throws on a malformed base64 data URL instead of writing corrupt bytes', async () => { + const out = `/tmp/browse-out-bad-${Date.now()}.png`; + try { + await expect( + handleReadCommand('js', ["'data:image/png;base64,!!!not-base64!!!'", '--out', out], bm) + ).rejects.toThrow(/malformed base64/); + expect(fs.existsSync(out)).toBe(false); + } finally { + fs.rmSync(out, { force: true }); + } + }); + + test('js --out rejects a path outside the safe directories', async () => { + await expect( + handleReadCommand('js', ['1 + 1', '--out', '/etc/browse-should-not-write.txt'], bm) + ).rejects.toThrow(); + }); + + test('js --out creates a missing parent directory', async () => { + // validateOutputPath resolves the parent's realpath, so it permits one level + // of missing dir under a safe root (/tmp). mkdir then materializes it. + const root = `/tmp/browse-out-nested-${Date.now()}`; + const out = `${root}/result.txt`; + try { + await handleReadCommand('js', ["'nested'", '--out', out], bm); + expect(fs.readFileSync(out, 'utf-8')).toBe('nested'); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); + + test('eval --out writes the file result to disk (parity with js)', async () => { + const script = `/tmp/browse-eval-out-src-${Date.now()}.js`; + const out = `/tmp/browse-eval-out-${Date.now()}.txt`; + fs.writeFileSync(script, "'from eval'"); + try { + const result = await handleReadCommand('eval', [script, '--out', out], bm); + expect(result).toContain('Eval result written:'); + expect(fs.readFileSync(out, 'utf-8')).toBe('from eval'); + } finally { + fs.rmSync(script, { force: true }); + fs.rmSync(out, { force: true }); + } + }); + test('css returns computed property', async () => { const result = await handleReadCommand('css', ['h1', 'color'], bm); // Navy color diff --git a/browse/test/tunnel-gate-unit.test.ts b/browse/test/tunnel-gate-unit.test.ts index f6d61c13a..6fcdd9e51 100644 --- a/browse/test/tunnel-gate-unit.test.ts +++ b/browse/test/tunnel-gate-unit.test.ts @@ -95,3 +95,35 @@ describe('canDispatchOverTunnel — alias canonicalization', () => { expect(canDispatchOverTunnel('closetab')).toBe(true); }); }); + +describe('canDispatchOverTunnel — --out writes are never tunnel-dispatchable', () => { + // `--out` turns an otherwise-readable command into a local-disk WRITE. The + // tunnel surface never grants disk-write to remote paired agents, so any + // --out invocation must be 403'd even when the bare command is allowlisted. + test('bare eval dispatches, but eval --out does not', () => { + expect(canDispatchOverTunnel('eval', ['/tmp/x.js'])).toBe(true); + expect(canDispatchOverTunnel('eval', ['/tmp/x.js', '--out', '/tmp/o.png'])).toBe(false); + }); + + test('--out= form is rejected too (no parser-shape bypass)', () => { + expect(canDispatchOverTunnel('eval', ['/tmp/x.js', '--out=/tmp/o.png'])).toBe(false); + }); + + test('--out anywhere in args is caught regardless of ordering', () => { + expect(canDispatchOverTunnel('eval', ['--out', '/tmp/o.png', '/tmp/x.js'])).toBe(false); + }); + + test('args without --out still dispatch', () => { + expect(canDispatchOverTunnel('goto', ['https://example.com'])).toBe(true); + expect(canDispatchOverTunnel('eval', ['/tmp/x.js'])).toBe(true); + }); + + test('omitting args preserves the old command-only behavior', () => { + expect(canDispatchOverTunnel('eval')).toBe(true); + }); + + test('a lookalike flag (--output) is NOT treated as --out', () => { + // hasOutArg matches '--out' exactly or '--out='; '--output' must not trip it. + expect(canDispatchOverTunnel('eval', ['/tmp/x.js', '--output', '/tmp/o'])).toBe(true); + }); +});