fix(middleware): run operator hook code in hardened vm sandbox instead of new Function (#5872) (#5885)

CHANGELOG.md

@@ -46,6 +46,8 @@
 
 ### 🔧 Bug Fixes
 
+- **middleware (hook sandbox):** operator-authored pre-request hook code now runs inside a hardened Node `vm` sandbox (minimal context, no ambient globals/`process.env`, execution timeout, no `require`) instead of `new Function()` in the main process — closing the Hard Rule #3 / SonarCloud S1523 exposure. Regression guard: `tests/unit/middleware-hook-sandbox-5872.test.ts`. ([#5872](https://github.com/diegosouzapw/OmniRoute/issues/5872))
+
 - **mcp-server (auth forwarding):** the per-caller MCP identity forwarded via `withMcpHttpAuthContext` now wins over the static `OMNIROUTE_API_KEY` env fallback in the internal-fetch helpers (`apiFetch`, `omniRouteFetch`) — previously the env key was spread after the forwarded headers and clobbered the caller's `Authorization`. Regression guard: `open-sse/mcp-server/__tests__/httpAuthContext.test.ts`. ([#5819](https://github.com/diegosouzapw/OmniRoute/issues/5819))
 
 - **dashboard (Modal provider — two-field auth):** the Modal provider connection form now exposes **two fields — Token ID + Token Secret —** instead of a single API-key input, since Modal authenticates with `Authorization: Bearer <token-id>:<token-secret>`. The dashboard combines the two fields into the `id:secret` credential before saving (`combineModalCredential`, trims both parts), while a value pasted in the legacy single-field format keeps working verbatim (empty secret → passthrough), so existing saved connections need no migration; the key-help link points at Modal's token settings. Regression guard: `tests/unit/modal-credential-combine.test.ts` (5). ([#5881](https://github.com/diegosouzapw/OmniRoute/pull/5881), closes [#5446](https://github.com/diegosouzapw/OmniRoute/issues/5446))

src/lib/middleware/registry.ts

@@ -12,6 +12,8 @@
  *   - Skip remaining hooks
  */
 
+import * as vm from "vm";
+
 import {
   type HookMiddleware,
   type HookConfig,
@@ -51,18 +53,116 @@ function getRegistryState() {
 
 // ── Compile hook code into middleware function ────────────────────────────
 
+/**
+ * Max wall-clock time a single operator-authored hook may run.
+ * Synchronous runaway loops are cut off by the `vm` timeout; async work that
+ * never settles is cut off by the Promise.race guard below.
+ */
+const HOOK_EXECUTION_TIMEOUT_MS = 5000;
+
+/**
+ * Build the minimal, capability-free context object exposed to hook code.
+ *
+ * TRUST MODEL: Node's `vm` is NOT a hard security boundary (it shares the host
+ * V8 heap and prototype-chain escapes exist). Its purpose here is to remove
+ * *ambient* authority — hook code compiled from `HookConfig.code` must not see
+ * `process`, `require`, `global`/`globalThis`, `fetch`, `Buffer`, timers, or
+ * the module scope. Only the request `context` and pure/deterministic globals
+ * are reachable, so a hook cannot read `process.env`, spawn processes, open
+ * sockets, or `require()` arbitrary modules. Combined with the operator-only
+ * write path (hooks are authored locally), this closes the `new Function()`
+ * ambient-authority exposure (Hard Rule #3 / SonarCloud S1523).
+ */
+function createHookSandbox(context: PreRequestHookContext): Record<string, unknown> {
+  return {
+    context,
+    // Pure / deterministic globals only — no I/O, no ambient authority.
+    JSON,
+    Math,
+    Date,
+    Array,
+    Object,
+    String,
+    Number,
+    Boolean,
+    RegExp,
+    Error,
+    TypeError,
+    RangeError,
+    SyntaxError,
+    URIError,
+    Map,
+    Set,
+    WeakMap,
+    WeakSet,
+    Symbol,
+    Promise,
+    parseInt,
+    parseFloat,
+    isNaN,
+    isFinite,
+    URL,
+    URLSearchParams,
+    // Deliberately absent: process, require, module, exports, global,
+    // globalThis, fetch, Buffer, setTimeout/setInterval, __dirname, __filename.
+  };
+}
+
 function compileHookCode(code: string, hookName: string): HookMiddleware {
+  // Compile-once: parse the source into a reusable vm.Script. This throws on
+  // syntax errors at registration time (preserving the original behavior) and
+  // is cached in the returned closure so each execution only pays for a fresh
+  // minimal context, not re-parsing.
+  let script: vm.Script;
   try {
-    // Wrap in async function that returns HookResult
-    // eslint-disable-next-line no-new-func
-    const fn = new Function("context", `return (async () => { ${code} })();`) as (
-      context: PreRequestHookContext
-    ) => Promise<HookResult>;
-    return fn;
+    script = new vm.Script(`(async () => { ${code} })();`, {
+      filename: `omniroute-hook:${hookName}`,
+    });
   } catch (err: unknown) {
     const message = err instanceof Error ? err.message : "Compilation error";
     throw new Error(`Failed to compile hook "${hookName}": ${message}`);
   }
+
+  return async (context: PreRequestHookContext): Promise<HookResult> => {
+    const sandbox = createHookSandbox(context);
+    const vmContext = vm.createContext(sandbox, {
+      codeGeneration: { strings: false, wasm: false },
+    });
+
+    let timer: ReturnType<typeof setTimeout> | undefined;
+    try {
+      // The `vm` timeout only interrupts *synchronous* runaway code; the
+      // Promise.race below bounds async work that never settles.
+      const execution: unknown = script.runInContext(vmContext, {
+        timeout: HOOK_EXECUTION_TIMEOUT_MS,
+      });
+
+      const timeoutGuard = new Promise<never>((_resolve, reject) => {
+        timer = setTimeout(() => {
+          reject(
+            new Error(`Hook "${hookName}" timed out after ${HOOK_EXECUTION_TIMEOUT_MS}ms`)
+          );
+        }, HOOK_EXECUTION_TIMEOUT_MS);
+      });
+
+      const result = await Promise.race([Promise.resolve(execution), timeoutGuard]);
+      return (result ?? {}) as HookResult;
+    } catch (err: unknown) {
+      // Errors thrown from inside the vm context use the context's own
+      // constructors, so they are not `instanceof` the host Error. Normalize
+      // to a host Error carrying a readable message so callers/observability
+      // classify it correctly.
+      const message =
+        err instanceof Error
+          ? err.message
+          : typeof err === "object" && err !== null && "message" in err
+            ? String((err as { message: unknown }).message)
+            : String(err);
+      throw new Error(message);
+    } finally {
+      if (timer) clearTimeout(timer);
+    }
+  };
 }
 
 // ── Default context factory ──────────────────────────────────────────────

tests/unit/middleware-hook-sandbox-5872.test.ts

@@ -0,0 +1,164 @@
+/**
+ * Regression guard for issue #5872 — operator-authored pre-request hook code
+ * must execute inside a hardened Node `vm` sandbox (minimal context, no ambient
+ * globals / process.env, execution timeout, no require) instead of
+ * `new Function()` running with full main-process authority.
+ *
+ * Closes the Hard Rule #3 / SonarCloud S1523 exposure.
+ */
+
+import { test, beforeEach, after } from "node:test";
+import assert from "node:assert/strict";
+
+import {
+  registerHook,
+  updateHook,
+  runHooks,
+  createHookContext,
+  getHook,
+  clearAllHooks,
+} from "../../src/lib/middleware/registry.ts";
+import { HookPriority, type HookConfig } from "../../src/lib/middleware/types.ts";
+
+function baseConfig(overrides: Partial<HookConfig> & Pick<HookConfig, "name" | "code">): HookConfig {
+  return {
+    description: "test hook",
+    priority: HookPriority.NORMAL,
+    scope: { type: "global" },
+    enabled: true,
+    createdAt: new Date().toISOString(),
+    updatedAt: new Date().toISOString(),
+    runCount: 0,
+    ...overrides,
+  };
+}
+
+function ctx() {
+  return createHookContext({
+    body: { messages: [] },
+    headers: {},
+    model: "gpt-4o",
+  });
+}
+
+beforeEach(() => {
+  clearAllHooks();
+});
+
+after(() => {
+  clearAllHooks();
+});
+
+test("(a) valid hook compiles, runs in sandbox, and returns a HookResult applied to context", async () => {
+  registerHook(
+    baseConfig({
+      name: "valid-hook",
+      code: `
+        context.body.injected = "yes";
+        return { body: { added: true }, model: "gpt-4o-mini" };
+      `,
+    })
+  );
+
+  const { context } = await runHooks(ctx());
+
+  // Result body/model merged by runHooks.
+  assert.equal(context.body.added, true);
+  assert.equal(context.model, "gpt-4o-mini");
+  // Direct context mutation also visible.
+  assert.equal(context.body.injected, "yes");
+  assert.equal(getHook("valid-hook")?.lastError, undefined);
+});
+
+test("(b) hook cannot reach ambient authority: process / require / globalThis are undefined", async () => {
+  registerHook(
+    baseConfig({
+      name: "no-ambient-hook",
+      code: `
+        return {
+          body: {
+            typeofProcess: typeof process,
+            typeofRequire: typeof require,
+            typeofFetch: typeof fetch,
+            // globalThis inside a vm context is the sandbox itself — it must
+            // not expose host ambient authority.
+            globalHasProcess: typeof globalThis.process,
+            globalHasRequire: typeof globalThis.require,
+          },
+        };
+      `,
+    })
+  );
+
+  const { context } = await runHooks(ctx());
+
+  assert.equal(context.body.typeofProcess, "undefined");
+  assert.equal(context.body.typeofRequire, "undefined");
+  assert.equal(context.body.typeofFetch, "undefined");
+  assert.equal(context.body.globalHasProcess, "undefined");
+  assert.equal(context.body.globalHasRequire, "undefined");
+  assert.equal(getHook("no-ambient-hook")?.lastError, undefined);
+});
+
+test("(b') hook attempting to read process.env throws inside the sandbox (no leak)", async () => {
+  process.env.__SECRET_5872 = "top-secret";
+  try {
+    registerHook(
+      baseConfig({
+        name: "env-read-hook",
+        code: `return { body: { stolen: process.env.__SECRET_5872 } };`,
+      })
+    );
+
+    const { context } = await runHooks(ctx());
+
+    // The hook throws (process is undefined) → runHooks records the error and
+    // never applies a body, so the secret cannot leak into the request.
+    assert.equal(context.body.stolen, undefined);
+    const err = getHook("env-read-hook")?.lastError ?? "";
+    assert.match(err, /process is not defined/);
+  } finally {
+    delete process.env.__SECRET_5872;
+  }
+});
+
+test("(c) runaway synchronous hook is aborted by the execution timeout", async () => {
+  registerHook(
+    baseConfig({
+      name: "runaway-hook",
+      code: `while (true) {}`,
+    })
+  );
+
+  const { context } = await runHooks(ctx());
+
+  // runHooks catches the timeout error, records it, and leaves the context
+  // untouched instead of hanging the process.
+  const err = getHook("runaway-hook")?.lastError ?? "";
+  assert.match(err, /timed out|Script execution timed out/);
+  assert.equal(context.model, "gpt-4o");
+});
+
+test("(d) recompilation on update reuses the cached middleware and runs the new code", async () => {
+  registerHook(
+    baseConfig({
+      name: "recompile-hook",
+      code: `return { body: { version: 1 } };`,
+    })
+  );
+
+  let out = await runHooks(ctx());
+  assert.equal(out.context.body.version, 1);
+
+  // Update code → recompile. The Map entry is replaced once, and subsequent
+  // runs reuse the same compiled closure (no per-request recompilation).
+  const updated = updateHook("recompile-hook", { code: `return { body: { version: 2 } };` });
+  assert.equal(updated, true);
+
+  out = await runHooks(ctx());
+  assert.equal(out.context.body.version, 2);
+
+  // Cached closure is stable across repeated invocations.
+  out = await runHooks(ctx());
+  assert.equal(out.context.body.version, 2);
+});