diff --git a/browse/src/meta-commands.ts b/browse/src/meta-commands.ts index 970ec7cd..a93c8894 100644 --- a/browse/src/meta-commands.ts +++ b/browse/src/meta-commands.ts @@ -45,12 +45,20 @@ function tokenizePipeSegment(segment: string): string[] { return tokens; } +/** Options passed from handleCommandInternal for chain routing */ +export interface MetaCommandOpts { + chainDepth?: number; + /** Callback to route subcommands through the full security pipeline (handleCommandInternal) */ + executeCommand?: (body: { command: string; args?: string[]; tabId?: number }, tokenInfo?: TokenInfo | null) => Promise<{ status: number; result: string; json?: boolean }>; +} + export async function handleMetaCommand( command: string, args: string[], bm: BrowserManager, shutdown: () => Promise | void, - tokenInfo?: TokenInfo | null + tokenInfo?: TokenInfo | null, + opts?: MetaCommandOpts, ): Promise { switch (command) { // ─── Tabs ────────────────────────────────────────── @@ -230,10 +238,6 @@ export async function handleMetaCommand( .map(seg => tokenizePipeSegment(seg.trim())); } - const results: string[] = []; - const { handleReadCommand } = await import('./read-commands'); - const { handleWriteCommand } = await import('./write-commands'); - // Pre-validate ALL subcommands against the token's scope before executing any. // This prevents partial execution where some subcommands succeed before a // scope violation is hit, leaving the browser in an inconsistent state. @@ -249,29 +253,60 @@ export async function handleMetaCommand( } } + // Route each subcommand through handleCommandInternal for full security: + // scope, domain, tab ownership, content wrapping — all enforced per subcommand. + // Chain-specific options: skip rate check (chain = 1 request), skip activity + // events (chain emits 1 event), increment chain depth (recursion guard). + const executeCmd = opts?.executeCommand; + const results: string[] = []; let lastWasWrite = false; - for (const cmd of commands) { - const [name, ...cmdArgs] = cmd; - try { - let result: string; - if (WRITE_COMMANDS.has(name)) { - result = await handleWriteCommand(name, cmdArgs, bm); - lastWasWrite = true; - } else if (READ_COMMANDS.has(name)) { - result = await handleReadCommand(name, cmdArgs, bm); - if (PAGE_CONTENT_COMMANDS.has(name)) { - result = wrapUntrustedContent(result, bm.getCurrentUrl()); - } - lastWasWrite = false; - } else if (META_COMMANDS.has(name)) { - result = await handleMetaCommand(name, cmdArgs, bm, shutdown, tokenInfo); - lastWasWrite = false; + + if (executeCmd) { + // Full security pipeline via handleCommandInternal + for (const cmd of commands) { + const [name, ...cmdArgs] = cmd; + const cr = await executeCmd( + { command: name, args: cmdArgs }, + tokenInfo, + ); + if (cr.status === 200) { + results.push(`[${name}] ${cr.result}`); } else { - throw new Error(`Unknown command: ${name}`); + // Parse error from JSON result + let errMsg = cr.result; + try { errMsg = JSON.parse(cr.result).error || cr.result; } catch {} + results.push(`[${name}] ERROR: ${errMsg}`); + } + lastWasWrite = WRITE_COMMANDS.has(name); + } + } else { + // Fallback: direct dispatch (CLI mode, no server context) + const { handleReadCommand } = await import('./read-commands'); + const { handleWriteCommand } = await import('./write-commands'); + + for (const cmd of commands) { + const [name, ...cmdArgs] = cmd; + try { + let result: string; + if (WRITE_COMMANDS.has(name)) { + result = await handleWriteCommand(name, cmdArgs, bm); + lastWasWrite = true; + } else if (READ_COMMANDS.has(name)) { + result = await handleReadCommand(name, cmdArgs, bm); + if (PAGE_CONTENT_COMMANDS.has(name)) { + result = wrapUntrustedContent(result, bm.getCurrentUrl()); + } + lastWasWrite = false; + } else if (META_COMMANDS.has(name)) { + result = await handleMetaCommand(name, cmdArgs, bm, shutdown, tokenInfo, opts); + lastWasWrite = false; + } else { + throw new Error(`Unknown command: ${name}`); + } + results.push(`[${name}] ${result}`); + } catch (err: any) { + results.push(`[${name}] ERROR: ${err.message}`); } - results.push(`[${name}] ${result}`); - } catch (err: any) { - results.push(`[${name}] ERROR: ${err.message}`); } } diff --git a/browse/src/server.ts b/browse/src/server.ts index 04e061ca..aad15e51 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -811,58 +811,81 @@ function wrapError(err: any): string { return msg; } -async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise { +/** Internal command result — used by handleCommand and chain subcommand routing */ +interface CommandResult { + status: number; + result: string; + headers?: Record; + json?: boolean; // true if result is JSON (errors), false for text/plain +} + +/** + * Core command execution logic. Returns a structured result instead of HTTP Response. + * Used by both the HTTP handler (handleCommand) and chain subcommand routing. + * + * Options: + * skipRateCheck: true when called from chain (chain counts as 1 request) + * skipActivity: true when called from chain (chain emits 1 event for all subcommands) + * chainDepth: recursion guard — reject nested chains (depth > 0 means inside a chain) + */ +async function handleCommandInternal( + body: { command: string; args?: string[]; tabId?: number }, + tokenInfo?: TokenInfo | null, + opts?: { skipRateCheck?: boolean; skipActivity?: boolean; chainDepth?: number }, +): Promise { const { command, args = [], tabId } = body; if (!command) { - return new Response(JSON.stringify({ error: 'Missing "command" field' }), { - status: 400, - headers: { 'Content-Type': 'application/json' }, - }); + return { status: 400, result: JSON.stringify({ error: 'Missing "command" field' }), json: true }; + } + + // ─── Recursion guard: reject nested chains ────────────────── + if (command === 'chain' && (opts?.chainDepth ?? 0) > 0) { + return { status: 400, result: JSON.stringify({ error: 'Nested chain commands are not allowed' }), json: true }; } // ─── Scope check (for scoped tokens) ────────────────────────── if (tokenInfo && tokenInfo.clientId !== 'root') { if (!checkScope(tokenInfo, command)) { - return new Response(JSON.stringify({ - error: `Command "${command}" not allowed by your token scope`, - hint: `Your scopes: ${tokenInfo.scopes.join(', ')}. Ask the user to re-pair with --admin for eval/cookies/storage access.`, - }), { - status: 403, - headers: { 'Content-Type': 'application/json' }, - }); + return { + status: 403, json: true, + result: JSON.stringify({ + error: `Command "${command}" not allowed by your token scope`, + hint: `Your scopes: ${tokenInfo.scopes.join(', ')}. Ask the user to re-pair with --admin for eval/cookies/storage access.`, + }), + }; } // Domain check for navigation commands - if (command === 'goto' && args[0]) { + if ((command === 'goto' || command === 'newtab') && args[0]) { if (!checkDomain(tokenInfo, args[0])) { - return new Response(JSON.stringify({ - error: `Domain not allowed by your token scope`, - hint: `Allowed domains: ${tokenInfo.domains?.join(', ') || 'none configured'}`, - }), { - status: 403, - headers: { 'Content-Type': 'application/json' }, - }); + return { + status: 403, json: true, + result: JSON.stringify({ + error: `Domain not allowed by your token scope`, + hint: `Allowed domains: ${tokenInfo.domains?.join(', ') || 'none configured'}`, + }), + }; } } - // Rate check - const rateResult = checkRate(tokenInfo); - if (!rateResult.allowed) { - return new Response(JSON.stringify({ - error: 'Rate limit exceeded', - hint: `Max ${tokenInfo.rateLimit} requests/second. Retry after ${rateResult.retryAfterMs}ms.`, - }), { - status: 429, - headers: { - 'Content-Type': 'application/json', - 'Retry-After': String(Math.ceil((rateResult.retryAfterMs || 1000) / 1000)), - }, - }); + // Rate check (skipped for chain subcommands — chain counts as 1 request) + if (!opts?.skipRateCheck) { + const rateResult = checkRate(tokenInfo); + if (!rateResult.allowed) { + return { + status: 429, json: true, + result: JSON.stringify({ + error: 'Rate limit exceeded', + hint: `Max ${tokenInfo.rateLimit} requests/second. Retry after ${rateResult.retryAfterMs}ms.`, + }), + headers: { 'Retry-After': String(Math.ceil((rateResult.retryAfterMs || 1000) / 1000)) }, + }; + } } // Record command execution for idempotent key exchange tracking - if (tokenInfo.token) recordCommand(tokenInfo.token); + if (!opts?.skipRateCheck && tokenInfo.token) recordCommand(tokenInfo.token); } // Pin to a specific tab if requested (set by BROWSE_TAB env var in sidebar agents). @@ -881,73 +904,75 @@ async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise handleCommandInternal(body, ti, { + skipRateCheck: true, // chain counts as 1 request + skipActivity: true, // chain emits 1 event for all subcommands + chainDepth: chainDepth + 1, // recursion guard + }), + }); // Start periodic snapshot interval when watch mode begins if (command === 'watch' && args[0] !== 'stop' && browserManager.isWatching()) { const watchInterval = setInterval(async () => { @@ -966,33 +991,32 @@ async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise { + const cr = await handleCommandInternal(body, tokenInfo); + const contentType = cr.json ? 'application/json' : 'text/plain'; + return new Response(cr.result, { + status: cr.status, + headers: { 'Content-Type': contentType, ...cr.headers }, + }); +} + async function shutdown() { if (isShuttingDown) return; isShuttingDown = true; diff --git a/browse/test/server-auth.test.ts b/browse/test/server-auth.test.ts index 0f509fdd..2a676213 100644 --- a/browse/test/server-auth.test.ts +++ b/browse/test/server-auth.test.ts @@ -44,10 +44,13 @@ describe('Server auth security', () => { }); // Test 1c: newtab must check domain restrictions (CSO finding #5) + // Domain check for newtab is now unified with goto in the scope check section: + // (command === 'goto' || command === 'newtab') && args[0] → checkDomain test('newtab enforces domain restrictions', () => { - const newtabBlock = sliceBetween(SERVER_SRC, "newtab with ownership for scoped tokens", "Block mutation commands while watching"); - expect(newtabBlock).toContain('checkDomain'); - expect(newtabBlock).toContain('Domain not allowed'); + const scopeBlock = sliceBetween(SERVER_SRC, "Scope check (for scoped tokens)", "Pin to a specific tab"); + expect(scopeBlock).toContain("command === 'newtab'"); + expect(scopeBlock).toContain('checkDomain'); + expect(scopeBlock).toContain('Domain not allowed'); }); // Test 2: /refs endpoint requires auth via validateAuth @@ -168,7 +171,7 @@ describe('Server auth security', () => { // Test 10d: server passes tokenInfo to handleMetaCommand test('server passes tokenInfo to handleMetaCommand', () => { - expect(SERVER_SRC).toContain('handleMetaCommand(command, args, browserManager, shutdown, tokenInfo)'); + expect(SERVER_SRC).toContain('handleMetaCommand(command, args, browserManager, shutdown, tokenInfo,'); }); // Test 10e: activity attribution includes clientId diff --git a/browse/test/sidebar-agent.test.ts b/browse/test/sidebar-agent.test.ts index 872bbd34..e28a9c00 100644 --- a/browse/test/sidebar-agent.test.ts +++ b/browse/test/sidebar-agent.test.ts @@ -502,12 +502,12 @@ describe('BROWSE_TAB tab pinning (cross-tab isolation)', () => { expect(cliSrc).toContain('tabId: parseInt(browseTab'); }); - test('handleCommand accepts tabId from request body', () => { + test('handleCommandInternal accepts tabId from request body', () => { const handleFn = serverSrc.slice( - serverSrc.indexOf('async function handleCommand('), - serverSrc.indexOf('\nasync function ', serverSrc.indexOf('async function handleCommand(') + 1) > 0 - ? serverSrc.indexOf('\nasync function ', serverSrc.indexOf('async function handleCommand(') + 1) - : serverSrc.indexOf('\n// ', serverSrc.indexOf('async function handleCommand(') + 200), + serverSrc.indexOf('async function handleCommandInternal('), + serverSrc.indexOf('\n/** HTTP wrapper', serverSrc.indexOf('async function handleCommandInternal(') + 1) > 0 + ? serverSrc.indexOf('\n/** HTTP wrapper', serverSrc.indexOf('async function handleCommandInternal(') + 1) + : serverSrc.indexOf('\nasync function ', serverSrc.indexOf('async function handleCommandInternal(') + 200), ); // Should destructure tabId from body expect(handleFn).toContain('tabId'); @@ -516,10 +516,10 @@ describe('BROWSE_TAB tab pinning (cross-tab isolation)', () => { expect(handleFn).toContain('switchTab(tabId'); }); - test('handleCommand restores active tab after command (success path)', () => { + test('handleCommandInternal restores active tab after command (success path)', () => { // On success, should restore savedTabId without stealing focus const handleFn = serverSrc.slice( - serverSrc.indexOf('async function handleCommand('), + serverSrc.indexOf('async function handleCommandInternal('), serverSrc.length, ); // Count restore calls — should appear in both success and error paths @@ -527,18 +527,18 @@ describe('BROWSE_TAB tab pinning (cross-tab isolation)', () => { expect(restoreCount).toBeGreaterThanOrEqual(2); // success + error paths }); - test('handleCommand restores active tab on error path', () => { + test('handleCommandInternal restores active tab on error path', () => { // The catch block should also restore const catchBlock = serverSrc.slice( - serverSrc.indexOf('} catch (err: any) {', serverSrc.indexOf('async function handleCommand(')), + serverSrc.indexOf('} catch (err: any) {', serverSrc.indexOf('async function handleCommandInternal(')), ); expect(catchBlock).toContain('switchTab(savedTabId'); }); test('tab pinning only activates when tabId is provided', () => { const handleFn = serverSrc.slice( - serverSrc.indexOf('async function handleCommand('), - serverSrc.indexOf('try {', serverSrc.indexOf('async function handleCommand(') + 1), + serverSrc.indexOf('async function handleCommandInternal('), + serverSrc.indexOf('try {', serverSrc.indexOf('async function handleCommandInternal(') + 1), ); // Should check tabId is not undefined/null before switching expect(handleFn).toContain('tabId !== undefined');