refactor: split handleCommand into handleCommandInternal + HTTP wrapper

Chain subcommands now route through handleCommandInternal for full security enforcement (scope, domain, tab ownership, rate limiting, content wrapping). Adds recursion guard for nested chains, rate-limit exemption for chain subcommands, and activity event suppression (1 event per chain, not per sub). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-05-02 03:35:09 +02:00 · 2026-04-05 11:05:11 -07:00
parent 49eac4c299
commit 905f1ddd38
4 changed files with 229 additions and 161 deletions
@@ -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> | void,
-  tokenInfo?: TokenInfo | null
+  tokenInfo?: TokenInfo | null,
+  opts?: MetaCommandOpts,
 ): Promise<string> {
  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}`);
        }
      }

@@ -811,58 +811,81 @@ function wrapError(err: any): string {
  return msg;
 }

-async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise<Response> {
+/** Internal command result — used by handleCommand and chain subcommand routing */
+interface CommandResult {
+  status: number;
+  result: string;
+  headers?: Record<string, string>;
+  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<CommandResult> {
  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<R
  if (tokenInfo && tokenInfo.clientId !== 'root' && WRITE_COMMANDS.has(command)) {
    const targetTab = tabId ?? browserManager.getActiveTabId();
    if (!browserManager.checkTabAccess(targetTab, tokenInfo.clientId, true)) {
-      return new Response(JSON.stringify({
-        error: 'Tab not owned by your agent. Use newtab to create your own tab.',
-        hint: `Tab ${targetTab} is owned by ${browserManager.getTabOwner(targetTab) || 'root'}. Your agent: ${tokenInfo.clientId}.`,
-      }), {
-        status: 403,
-        headers: { 'Content-Type': 'application/json' },
-      });
+      return {
+        status: 403, json: true,
+        result: JSON.stringify({
+          error: 'Tab not owned by your agent. Use newtab to create your own tab.',
+          hint: `Tab ${targetTab} is owned by ${browserManager.getTabOwner(targetTab) || 'root'}. Your agent: ${tokenInfo.clientId}.`,
+        }),
+      };
    }
  }

  // ─── newtab with ownership for scoped tokens ──────────────
  if (command === 'newtab' && tokenInfo && tokenInfo.clientId !== 'root') {
-    // Domain check for newtab URL (same as goto)
-    if (args[0] && !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' },
-      });
-    }
    const newId = await browserManager.newTab(args[0] || undefined, tokenInfo.clientId);
-    return new Response(JSON.stringify({
-      tabId: newId,
-      owner: tokenInfo.clientId,
-      hint: 'Include "tabId": ' + newId + ' in subsequent commands to target this tab.',
-    }), {
-      status: 200,
-      headers: { 'Content-Type': 'application/json' },
-    });
+    return {
+      status: 200, json: true,
+      result: JSON.stringify({
+        tabId: newId,
+        owner: tokenInfo.clientId,
+        hint: 'Include "tabId": ' + newId + ' in subsequent commands to target this tab.',
+      }),
+    };
  }

  // Block mutation commands while watching (read-only observation mode)
  if (browserManager.isWatching() && WRITE_COMMANDS.has(command)) {
-    return new Response(JSON.stringify({
-      error: 'Cannot run mutation commands while watching. Run `$B watch stop` first.',
-    }), {
-      status: 400,
-      headers: { 'Content-Type': 'application/json' },
-    });
+    return {
+      status: 400, json: true,
+      result: JSON.stringify({ error: 'Cannot run mutation commands while watching. Run `$B watch stop` first.' }),
+    };
  }

-  // Activity: emit command_start
+  // Activity: emit command_start (skipped for chain subcommands)
  const startTime = Date.now();
-  emitActivity({
-    type: 'command_start',
-    command,
-    args,
-    url: browserManager.getCurrentUrl(),
-    tabs: browserManager.getTabCount(),
-    mode: browserManager.getConnectionMode(),
-    clientId: tokenInfo?.clientId,
-  });
+  if (!opts?.skipActivity) {
+    emitActivity({
+      type: 'command_start',
+      command,
+      args,
+      url: browserManager.getCurrentUrl(),
+      tabs: browserManager.getTabCount(),
+      mode: browserManager.getConnectionMode(),
+      clientId: tokenInfo?.clientId,
+    });
+  }

  try {
    let result: string;

    if (READ_COMMANDS.has(command)) {
      result = await handleReadCommand(command, args, browserManager);
+      // Content wrapping for page-content commands (scoped vs root handled here)
+      // Chain subcommands: each gets wrapped individually here. Chain result is NOT re-wrapped.
      if (PAGE_CONTENT_COMMANDS.has(command)) {
        result = wrapUntrustedContent(result, browserManager.getCurrentUrl());
      }
    } else if (WRITE_COMMANDS.has(command)) {
      result = await handleWriteCommand(command, args, browserManager);
    } else if (META_COMMANDS.has(command)) {
-      result = await handleMetaCommand(command, args, browserManager, shutdown, tokenInfo);
+      // Pass chain depth + executeCommand callback so chain routes subcommands
+      // through the full security pipeline (scope, domain, tab, wrapping).
+      const chainDepth = (opts?.chainDepth ?? 0);
+      result = await handleMetaCommand(command, args, browserManager, shutdown, tokenInfo, {
+        chainDepth,
+        executeCommand: (body, ti) => 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<R
      }
    } else if (command === 'help') {
      const helpText = generateHelpText();
-      return new Response(helpText, {
-        status: 200,
-        headers: { 'Content-Type': 'text/plain' },
-      });
+      return { status: 200, result: helpText };
    } else {
-      return new Response(JSON.stringify({
-        error: `Unknown command: ${command}`,
-        hint: `Available commands: ${[...READ_COMMANDS, ...WRITE_COMMANDS, ...META_COMMANDS].sort().join(', ')}`,
-      }), {
-        status: 400,
-        headers: { 'Content-Type': 'application/json' },
-      });
+      return {
+        status: 400, json: true,
+        result: JSON.stringify({
+          error: `Unknown command: ${command}`,
+          hint: `Available commands: ${[...READ_COMMANDS, ...WRITE_COMMANDS, ...META_COMMANDS].sort().join(', ')}`,
+        }),
+      };
    }

-    // Activity: emit command_end (success)
-    emitActivity({
-      type: 'command_end',
-      command,
-      args,
-      url: browserManager.getCurrentUrl(),
-      duration: Date.now() - startTime,
-      status: 'ok',
-      result: result,
-      tabs: browserManager.getTabCount(),
-      mode: browserManager.getConnectionMode(),
-      clientId: tokenInfo?.clientId,
-    });
+    // Activity: emit command_end (skipped for chain subcommands)
+    if (!opts?.skipActivity) {
+      emitActivity({
+        type: 'command_end',
+        command,
+        args,
+        url: browserManager.getCurrentUrl(),
+        duration: Date.now() - startTime,
+        status: 'ok',
+        result: result,
+        tabs: browserManager.getTabCount(),
+        mode: browserManager.getConnectionMode(),
+        clientId: tokenInfo?.clientId,
+      });
+    }

    browserManager.resetFailures();
    // Restore original active tab if we pinned to a specific one
@@ -1001,10 +1025,7 @@ async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise<R
        console.warn('[browse] Failed to restore tab after command:', restoreErr.message);
      }
    }
-    return new Response(result, {
-      status: 200,
-      headers: { 'Content-Type': 'text/plain' },
-    });
+    return { status: 200, result };
  } catch (err: any) {
    // Restore original active tab even on error
    if (savedTabId !== null) {
@@ -1013,31 +1034,40 @@ async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise<R
      }
    }

-    // Activity: emit command_end (error)
-    emitActivity({
-      type: 'command_end',
-      command,
-      args,
-      url: browserManager.getCurrentUrl(),
-      duration: Date.now() - startTime,
-      status: 'error',
-      error: err.message,
-      tabs: browserManager.getTabCount(),
-      mode: browserManager.getConnectionMode(),
-      clientId: tokenInfo?.clientId,
-    });
+    // Activity: emit command_end (error) — skipped for chain subcommands
+    if (!opts?.skipActivity) {
+      emitActivity({
+        type: 'command_end',
+        command,
+        args,
+        url: browserManager.getCurrentUrl(),
+        duration: Date.now() - startTime,
+        status: 'error',
+        error: err.message,
+        tabs: browserManager.getTabCount(),
+        mode: browserManager.getConnectionMode(),
+        clientId: tokenInfo?.clientId,
+      });
+    }

    browserManager.incrementFailures();
    let errorMsg = wrapError(err);
    const hint = browserManager.getFailureHint();
    if (hint) errorMsg += '\n' + hint;
-    return new Response(JSON.stringify({ error: errorMsg }), {
-      status: 500,
-      headers: { 'Content-Type': 'application/json' },
-    });
+    return { status: 500, result: JSON.stringify({ error: errorMsg }), json: true };
  }
 }

+/** HTTP wrapper — converts CommandResult to Response */
+async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise<Response> {
+  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;
@@ -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
@@ -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');