From 2b9abebef84bd91bebf2940ea1b17f226c1499e0 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Thu, 2 Jul 2026 02:45:32 -0300 Subject: [PATCH] fix(middleware): run operator hook code in hardened vm sandbox instead of new Function (#5872) (#5885) --- CHANGELOG.md | 2 + src/lib/middleware/registry.ts | 112 +++++++++++- .../unit/middleware-hook-sandbox-5872.test.ts | 164 ++++++++++++++++++ 3 files changed, 272 insertions(+), 6 deletions(-) create mode 100644 tests/unit/middleware-hook-sandbox-5872.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index bc76f76c6b..74ea7f633b 100644 --- a/CHANGELOG.md +++ b/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 :`. 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)) diff --git a/src/lib/middleware/registry.ts b/src/lib/middleware/registry.ts index 6a46d4491f..4132f7a680 100644 --- a/src/lib/middleware/registry.ts +++ b/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 { + 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; - 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 => { + const sandbox = createHookSandbox(context); + const vmContext = vm.createContext(sandbox, { + codeGeneration: { strings: false, wasm: false }, + }); + + let timer: ReturnType | 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((_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 ────────────────────────────────────────────── diff --git a/tests/unit/middleware-hook-sandbox-5872.test.ts b/tests/unit/middleware-hook-sandbox-5872.test.ts new file mode 100644 index 0000000000..3b6c803381 --- /dev/null +++ b/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 & Pick): 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); +});