feat(domains): migrate execute_js and run_script with path allowlist + timeout + return validation

3 files changed, 203 insertions, 197 deletions

diff --git a/src/domains/js.ts b/src/domains/js.ts
new file mode 100644
index 0000000..bd4c3e1
--- /dev/null
+++ b/src/domains/js.ts
@@ -0,0 +1,197 @@
+import { readFile } from "node:fs/promises";
+import { resolve, isAbsolute } from "node:path";
+import { tmpdir } from "node:os";
+import { createRequire } from "node:module";
+import { Type } from "typebox";
+import { type Result, err, ok } from "../util/result";
+import { defineBrowserTool, type ToolErr, type ToolOk } from "../util/tool";
+import { applyTruncation } from "../util/truncate";
+
+// AsyncFunction constructor — the documented Node.js mechanism for compiling
+// arbitrary user-supplied source. Equivalent to `new Function` but produces an
+// async function we can `await`. The full RCE risk is contained at the source
+// boundary: see the path allowlist + timeout below. A plain `unknown` cast
+// would lose the constructor signature; we keep this single typed cast.
+const AsyncFunction = Object.getPrototypeOf(async () => {}).constructor as
+  new (...args: ReadonlyArray<string>) => (...args: ReadonlyArray<unknown>) => Promise<unknown>;
+
+// ESNext module — bare `require` is not in scope. createRequire provides the
+// CJS resolver anchored to this file's URL so scripts can require() builtins
+// and installed CJS packages.
+const requireFromHere = createRequire(import.meta.url);
+
+const ExecuteJsArgs = Type.Object({
+  expression: Type.String({ description: "JavaScript expression. `return X` is auto-wrapped in an IIFE." }),
+  targetId: Type.Optional(Type.String({ description: "Optional iframe targetId; default = current page." })),
+});
+
+export const executeJsTool = defineBrowserTool({
+  name: "browser_execute_js",
+  label: "Browser Execute JS",
+  description:
+    "Run JavaScript in the page (or a specific iframe target). `return X` gets auto-wrapped in an IIFE for convenience. Always use safeJs / JSON.stringify when interpolating untrusted strings into source.",
+  promptSnippet: "Execute JS in the page (or iframe)",
+  promptGuidelines: [
+    "`return foo` is auto-wrapped in an IIFE — both `foo` and `(() => foo)()` work.",
+    "For iframes, get a targetId via browser_execute_js with `Object.values(document.querySelectorAll('iframe'))` then call browser_execute_js with targetId.",
+    "Result must be JSON-serializable (Runtime.evaluate returnByValue=true).",
+  ],
+  parameters: ExecuteJsArgs,
+  async handler(args, { client }): Promise<Result<ToolOk, ToolErr>> {
+    const r = await client.evaluateJs(args.expression, args.targetId);
+    if (!r.success) return err({ kind: "cdp_error", message: r.error.message });
+    const valueStr = r.data === undefined ? "undefined" : JSON.stringify(r.data);
+    const truncated = await applyTruncation(valueStr, "js");
+    return ok({
+      text: truncated.text,
+      details: truncated.fullOutputPath !== undefined
+        ? { valueLength: valueStr.length, fullOutputPath: truncated.fullOutputPath }
+        : { valueLength: valueStr.length },
+    });
+  },
+});
+
+const RunScriptArgs = Type.Object({
+  path: Type.String({ description: "Absolute path to the script file (.js or .mjs)" }),
+  params: Type.Optional(
+    Type.Object({}, { additionalProperties: true, description: "Args passed to the script as `params`" }),
+  ),
+  timeoutMs: Type.Optional(
+    Type.Integer({
+      default: 60_000,
+      minimum: 100,
+      maximum: 600_000,
+      description: "Hard timeout in ms. Default 60s, max 600s.",
+    }),
+  ),
+});
+
+const allowedRoots = (): ReadonlyArray<string> => {
+  const env = process.env["BH_SCRIPT_DIR"];
+  return [tmpdir(), process.cwd(), ...(env !== undefined ? [env] : [])].map((d) => resolve(d));
+};
+
+const isPathAllowed = (p: string): boolean => {
+  const abs = resolve(p);
+  return allowedRoots().some((root) => abs === root || abs.startsWith(`${root}/`));
+};
+
+const MAX_SOURCE_BYTES = 1_000_000;
+
+type ContentItem = { readonly type: "text"; readonly text: string };
+
+const isContentItem = (v: unknown): v is ContentItem =>
+  typeof v === "object" && v !== null
+    && (v as Readonly<Record<string, unknown>>)["type"] === "text"
+    && typeof (v as Readonly<Record<string, unknown>>)["text"] === "string";
+
+export const runScriptTool = defineBrowserTool({
+  name: "browser_run_script",
+  label: "Browser Run Script",
+  description:
+    "Execute a temporary JavaScript script with full Node.js + browser-daemon access. Path must be inside tmpdir, cwd, or BH_SCRIPT_DIR. Mandatory timeout. The script is full RCE on the harness's process — only invoke scripts you wrote and reviewed.",
+  promptSnippet: "Run a temporary script with daemon + Node access (security-bounded)",
+  promptGuidelines: [
+    "Write the script with the write tool first; pass its absolute path.",
+    "Path must be inside tmpdir, cwd, or BH_SCRIPT_DIR — otherwise rejected.",
+    "Default timeout is 60s; pass timeoutMs to extend (max 600s).",
+    "Script bindings: params, daemon, require, signal, onUpdate, ctx, console, fetch, JSON, Buffer, setTimeout, clearTimeout.",
+    "Script MUST return { content: [{ type: 'text', text: '...' }], details?: {...} }. Throw on errors.",
+  ],
+  parameters: RunScriptArgs,
+  async handler(args, { client, signal, onUpdate, extensionCtx }): Promise<Result<ToolOk, ToolErr>> {
+    if (!isAbsolute(args.path)) {
+      return err({ kind: "invalid_state", message: "Script path must be absolute" });
+    }
+    if (!isPathAllowed(args.path)) {
+      return err({
+        kind: "invalid_state",
+        message: `Script path outside allowed directories (allowed: ${allowedRoots().join(", ")})`,
+      });
+    }
+    let source: string;
+    try {
+      source = await readFile(args.path, "utf8");
+    } catch (e) {
+      return err({
+        kind: "io_error",
+        message: `Failed to read script: ${e instanceof Error ? e.message : String(e)}`,
+      });
+    }
+    if (source.length === 0) return err({ kind: "invalid_state", message: "Script is empty" });
+    if (source.length > MAX_SOURCE_BYTES) {
+      return err({ kind: "invalid_state", message: `Script exceeds ${MAX_SOURCE_BYTES}B size cap` });
+    }
+
+    let executeFn: (...a: ReadonlyArray<unknown>) => Promise<unknown>;
+    try {
+      executeFn = new AsyncFunction(
+        "params", "daemon", "require", "signal", "onUpdate", "ctx",
+        "console", "fetch", "JSON", "Buffer", "setTimeout", "clearTimeout",
+        `"use strict";\n${source}`,
+      );
+    } catch (e) {
+      return err({
+        kind: "invalid_state",
+        message: `Syntax error: ${e instanceof Error ? e.message : String(e)}`,
+      });
+    }
+
+    const timeoutMs = args.timeoutMs ?? 60_000;
+    const ac = new AbortController();
+    const onAbort = (): void => ac.abort();
+    if (signal !== undefined) signal.addEventListener("abort", onAbort, { once: true });
+    const timeoutTimer = setTimeout(() => ac.abort(), timeoutMs);
+
+    try {
+      const scriptPromise = executeFn(
+        args.params ?? {},
+        client,                     // bound as `daemon` — name preserved for back-compat
+        requireFromHere,
+        ac.signal,
+        (u: unknown) => {
+          if (typeof u !== "object" || u === null) return;
+          const content = (u as Readonly<Record<string, unknown>>)["content"];
+          if (!Array.isArray(content) || content.length === 0) return;
+          const first = content[0];
+          if (typeof first !== "object" || first === null) return;
+          const txt = (first as Readonly<Record<string, unknown>>)["text"];
+          if (typeof txt !== "string") return;
+          try { onUpdate({ text: txt }); } catch { /* swallow */ }
+        },
+        extensionCtx ?? { cwd: process.cwd() },
+        console, fetch, JSON, Buffer, setTimeout, clearTimeout,
+      );
+      const abortPromise = new Promise<never>((_, reject) => {
+        ac.signal.addEventListener("abort", () => reject(new Error("script aborted (timeout or cancellation)")), { once: true });
+      });
+      const result = await Promise.race([scriptPromise, abortPromise]);
+      clearTimeout(timeoutTimer);
+      if (signal !== undefined) signal.removeEventListener("abort", onAbort);
+
+      if (typeof result !== "object" || result === null) {
+        return err({ kind: "invalid_state", message: `Script must return an object; got ${JSON.stringify(result)}` });
+      }
+      const content = (result as Readonly<Record<string, unknown>>)["content"];
+      if (!Array.isArray(content)) {
+        return err({ kind: "invalid_state", message: "Script return value must have a content array" });
+      }
+      if (!content.every(isContentItem)) {
+        return err({ kind: "invalid_state", message: "Script content array must contain { type: 'text', text: string } items" });
+      }
+      const textOut = content.map((c) => c.text).join("\n");
+      const details = (result as Readonly<Record<string, unknown>>)["details"];
+      const isPlainObject = typeof details === "object" && details !== null && !Array.isArray(details);
+      return isPlainObject
+        ? ok({ text: textOut, details: details as Readonly<Record<string, unknown>> })
+        : ok({ text: textOut });
+    } catch (e) {
+      clearTimeout(timeoutTimer);
+      if (signal !== undefined) signal.removeEventListener("abort", onAbort);
+      return err({
+        kind: "internal",
+        message: `Script execution failed: ${e instanceof Error ? e.message : String(e)}`,
+      });
+    }
+  },
+});
diff --git a/src/registry.ts b/src/registry.ts
index 892d656..5f9d7da 100644
--- a/src/registry.ts
+++ b/src/registry.ts
@@ -14,6 +14,7 @@ import { uploadFileTool, downloadTool, printToPdfTool } from "./domains/files";
 import { viewportResizeTool } from "./domains/viewport";
 import { dragAndDropTool } from "./domains/drag";
 import { httpGetTool, getNetworkLogTool } from "./domains/network";
+import { executeJsTool, runScriptTool } from "./domains/js";
 
 const TOOLS: ReadonlyArray<AnyBrowserToolDefinition> = [
   clickTool,
@@ -42,6 +43,8 @@ const TOOLS: ReadonlyArray<AnyBrowserToolDefinition> = [
   dragAndDropTool,
   httpGetTool,
   getNetworkLogTool,
+  executeJsTool,
+  runScriptTool,
 ];
 
 export const registerAllTools = (pi: ExtensionAPI, client: BrowserClient): void => {
diff --git a/src/tools.ts b/src/tools.ts
index b1d27a9..0c8a05b 100644
--- a/src/tools.ts
+++ b/src/tools.ts
@@ -95,201 +95,7 @@ async function applyTruncation(
 
 // ── Registration ─────────────────────────────────────────────────────────────
 
-export function registerTools(pi: ExtensionAPI, daemon: BrowserDaemon): void {
-// ═══════════════════════════════════════════════════════════════════════════
-  // browser_execute_js
-  // ═══════════════════════════════════════════════════════════════════════════
-  pi.registerTool({
-    name: "browser_execute_js",
-    label: "Browser Execute JS",
-    description:
-      'Execute JavaScript in the current page and return the result. Use for data extraction, DOM inspection, and page manipulation. Expressions with "return" are automatically wrapped in an IIFE.',
-    promptSnippet: "Execute JavaScript in the page and return the result",
-    promptGuidelines: [
-      "Use browser_execute_js for extracting structured data from the page (text, attributes, JSON from embedded scripts).",
-      "Use browser_execute_js for DOM inspection when coordinates from screenshots aren't sufficient.",
-      "For bulk data extraction, prefer browser_http_get for API calls — it's 10-50x faster than browser DOM scraping.",
-      "To execute JS inside an iframe, first find the iframe target via browser_list_tabs (includeChrome: true to see iframe targets), then pass targetId to browser_execute_js.",
-      "Compositor clicks (browser_click) already pass through iframes — only use iframe JS when you need to read/change iframe DOM.",
-    ],
-    parameters: Type.Object({
-      expression: Type.String({ description: "JavaScript expression to evaluate. 'return' statements are auto-wrapped in IIFE." }),
-      targetId: Type.Optional(Type.String({ description: "Target ID of an iframe to evaluate in (optional)" })),
-    }),
-    async execute(_id, params) {
-      try {
-        await daemon.ensureAlive();
-        const value = await daemon.evaluateJS(params.expression, params.targetId);
-        const raw = value === undefined ? "undefined" : JSON.stringify(value, null, 2);
-        const { text, fullOutputPath } = await applyTruncation(raw, "js");
-        return {
-          content: [{ type: "text" as const, text }],
-          details: { valueLength: raw.length, fullOutputPath },
-        };
-      } catch (err) {
-        return {
-          isError: true,
-          content: [{ type: "text" as const, text: `JS execution failed: ${err instanceof Error ? err.message : String(err)}` }],
-          details: undefined,
-        };
-      }
-    },
-    renderResult(result, { expanded }, theme) {
-      const details = result.details as {
-        valueLength?: number;
-        fullOutputPath?: string;
-      } | undefined;
-
-      const len = details?.valueLength ?? 0;
-
-      if (expanded) {
-        // Expanded (CTRL+O): show the full JS result
-        const content = result.content[0];
-        const raw = content?.type === "text" ? content.text : "";
-        return new Text(raw || "undefined", 0, 0);
-      }
-
-      // Collapsed (default): size + first-line preview
-      const content = result.content[0];
-      const raw = content?.type === "text" ? content.text : "";
-      const firstLine = raw.split("\n")[0] || "";
-      const preview =
-        firstLine.length > 120 ? firstLine.slice(0, 117) + "..." : firstLine;
-
-      const summary = `📜 JS result (${formatSize(len)})`;
-      return new Text(
-        theme
-          ? theme.fg("dim", summary) + "\n" + theme.fg("muted", preview)
-          : summary + "\n" + preview,
-        0,
-        0,
-      );
-    },
-  });
-
-  // ═══════════════════════════════════════════════════════════════════════════
-  // browser_run_script
-  // ═══════════════════════════════════════════════════════════════════════════
-  pi.registerTool({
-    name: "browser_run_script",
-    label: "Browser Run Script",
-    description:
-      "Execute a temporary JavaScript script file with access to the browser daemon " +
-      "and Node.js APIs. Use this when the built-in browser_* tools are insufficient " +
-      "for a multi-step workflow — write a script to disk, then run it with this tool. " +
-      "The script receives these bindings in scope:\n" +
-      "  params    — the arguments passed to this tool\n" +
-      "  daemon    — the browser daemon (daemon.cdp(), daemon.evaluateJS(), daemon.getPageInfo(), etc.)\n" +
-      "  require   — Node.js require() for builtins and installed packages\n" +
-      "  signal    — AbortSignal for cancellation\n" +
-      "  onUpdate  — progress callback: onUpdate({ content: [{ type: 'text', text: '...' }] })\n" +
-      "  ctx       — ExtensionContext with cwd, sessionManager, ui, signal, etc.\n" +
-      "  console, fetch, JSON, Buffer, setTimeout, clearTimeout\n\n" +
-      "The script MUST return { content: [{ type: 'text', text: '...' }], details?: {...} }.\n" +
-      "For errors, throw: throw new Error('something went wrong').",
-    promptSnippet: "Run a temporary script with browser daemon access (write script to disk first)",
-    promptGuidelines: [
-      "Use write to create a temporary script file, then browser_run_script to execute it — no dynamic tool registration needed.",
-      "Scripts are written to disk, making them auditable and re-runnable. The user can inspect them.",
-      "The script has access to the browser daemon via `daemon` — use daemon.cdp(), daemon.evaluateJS(), daemon.getPageInfo(), etc.",
-      "For pure data processing or file operations, use require() to access Node.js modules like fs, path, crypto.",
-      "If the script only needs built-in browser_* tools, break those into separate tool calls before or after — do not try to invoke them from within the script.",
-    ],
-    parameters: Type.Object({
-      path: Type.String({ description: "Path to the temporary script file (.js or .mjs) to execute" }),
-      params: Type.Optional(Type.Object({}, { additionalProperties: true, description: "Optional parameters to pass to the script as `params`" })),
-    }),
-    async execute(_id, p, signal, onUpdate, ctx) {
-      const { path: scriptPath, params: scriptParams } = p as {
-        path: string;
-        params?: Record<string, unknown>;
-      };
-
-      // ── Read the script from disk ────────────────────────────────────
-      let source: string;
-      try {
-        source = await readFile(scriptPath, "utf8");
-      } catch (err) {
-        const msg = err instanceof Error ? err.message : String(err);
-        return {
-          isError: true,
-          content: [{
-            type: "text" as const,
-            text: `Failed to read script: ${msg}\n\nPath: ${scriptPath}\nCheck that the file exists and is readable.`,
-          }],
-          details: { error: "read_failed", message: msg },
-        };
-      }
-
-      if (source.trim().length === 0) {
-        return {
-          isError: true,
-          content: [{ type: "text" as const, text: `Script is empty: ${scriptPath}` }],
-          details: { error: "empty_script" },
-        };
-      }
-
-      // ── Compile ─────────────────────────────────────────────────────
-      let executeFn: (...args: unknown[]) => Promise<unknown>;
-      try {
-        const wrapped = `"use strict";\n${source}`;
-        executeFn = new AsyncFunction(
-          "params", "daemon", "require", "signal", "onUpdate", "ctx",
-          "console", "fetch", "JSON", "Buffer", "setTimeout", "clearTimeout",
-          wrapped,
-        );
-      } catch (err) {
-        const msg = err instanceof Error ? err.message : String(err);
-        return {
-          isError: true,
-          content: [{
-            type: "text" as const,
-            text: `Syntax error in script: ${msg}\n\nFile: ${scriptPath}\nFix the JavaScript syntax and try again.`,
-          }],
-          details: { error: "syntax_error", message: msg },
-        };
-      }
-
-      // ── Execute ─────────────────────────────────────────────────────
-      try {
-        const result = await executeFn(
-          scriptParams ?? {},
-          daemon,
-          require,
-          signal,
-          onUpdate ?? ((_update: unknown) => {}),
-          ctx ?? { cwd: process.cwd() },
-          console,
-          fetch,
-          JSON,
-          Buffer,
-          setTimeout,
-          clearTimeout,
-        );
-
-        if (!result || typeof result !== "object" || !Array.isArray((result as any).content)) {
-          return {
-            isError: true,
-            content: [{
-              type: "text" as const,
-              text: `Script must return { content: [{ type: 'text', text: '...' }], details?: {...} }.\nGot: ${JSON.stringify(result)}\n\nFile: ${scriptPath}`,
-            }],
-            details: { error: "invalid_return" },
-          };
-        }
-
-        return result as { content: Array<{ type: "text"; text: string }>; details: Record<string, unknown> | undefined };
-      } catch (err) {
-        const msg = err instanceof Error ? err.message : String(err);
-        return {
-          isError: true,
-          content: [{
-            type: "text" as const,
-            text: `Script execution failed: ${msg}\n\nFile: ${scriptPath}\nCheck for runtime errors, undefined variables, or broken require() calls.`,
-          }],
-          details: { error: "execution_failed", message: msg },
-        };
-      }
-    },
-  });
+// All tools migrated to src/domains/* in v0.3 — see registerAllTools.
+export function registerTools(_pi: ExtensionAPI, _daemon: BrowserDaemon): void {
+  // intentionally empty
 }