fix(security): run pre-request hooks in an isolated realm; only JSON crosses the boundary (GHSA-9p9m-h9rj-rhhg) (#14916)

Pre-request hooks ran in a vm context built from host objects, so hook code could reach the server realm. Hooks now run in a realm created from a null-prototype object; the request context goes in and the result, mutated context and log lines come back only as JSON strings; the host never awaits or calls anything from the realm. Hook contract unchanged except URL/URLSearchParams are no longer provided. Reported by @alienkeric. Remaining CI reds are the release-tip base-reds tracked in #14866.

changelog.d/fixes/hook-sandbox-isolated-realm.md

@@ -0,0 +1 @@
+- **Security (GHSA-9p9m-h9rj-rhhg, critical):** pre-request middleware hooks ran in a `vm` context built from host objects, so hook code could reach the server realm — directly (`this.constructor.constructor("return process")()` returned the host `process`) and by polluting the server's `Object.prototype`, which a later `worker_threads` Worker inherits as `env` (`NODE_OPTIONS=--require …`). Hooks now run in an isolated realm created from a null-prototype object: the request context goes in and the result, mutated context and log lines come back only as JSON strings, and the host never awaits or calls anything from the realm. The hook contract is unchanged except that the non-JS globals `URL` / `URLSearchParams` are no longer provided inside hooks. Reported by @alienkeric.

src/lib/middleware/registry.ts

@@ -54,68 +54,164 @@ 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.
+ * Max wall-clock time a single operator-authored hook may run. With
+ * `microtaskMode: "afterEvaluate"` the vm timeout covers the hook's async
+ * continuations too: the sandbox has no timers or I/O, so every `await` inside it
+ * resolves as a microtask that runs before `runInContext()` returns.
  */
 const HOOK_EXECUTION_TIMEOUT_MS = 5000;
 
+const HOOK_INPUT_GLOBAL = "__omnirouteHookInput";
+const HOOK_OUTPUT_GLOBAL = "__omnirouteHookOutput";
+
 /**
- * Build the minimal, capability-free context object exposed to hook code.
+ * GHSA-9p9m-h9rj-rhhg — the hook runs in its OWN realm and only JSON crosses the
+ * boundary.
  *
- * 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).
+ * The previous sandbox handed the hook the host's `Object`, `Array`, `Promise`, … and
+ * the live `context` object, so hook code could write the SERVER's `Object.prototype`
+ * (e.g. `Object.prototype.env = { NODE_OPTIONS: "--require …" }`, which a later
+ * `worker_threads` Worker inherits → code execution). Now:
+ *
+ * - the vm context is created from a null-prototype object, so the hook sees the fresh
+ *   realm's own intrinsics — polluting its `Object.prototype` never reaches the host,
+ *   and `this.constructor.constructor` resolves to that realm's `Function`, which
+ *   `codeGeneration.strings: false` blocks;
+ * - no host object or function is exposed: the request context goes in as a JSON
+ *   string and is parsed inside the realm; `context.log.*` buffers into an array;
+ * - the hook's result, its mutated context and the buffered log lines come back as ONE
+ *   JSON string written to a sandbox global. The host never awaits or calls anything
+ *   from the realm (a hook could replace `Promise.prototype.then`, and a host `await`
+ *   would hand its own resolve functions — and with them the host `Function` — to it).
+ *
+ * Node's `vm` is still not a hard security boundary; the write path stays loopback/LAN
+ * only (`/api/middleware/` in LOCAL_ONLY_API_PREFIXES). This removes the shared-realm
+ * escape, not the need to trust hook authors.
  */
-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 buildHookSource(code: string): string {
+  return `(async () => {
+  const __omnirouteLogs = [];
+  let __omniroutePayload;
+  try {
+    const context = JSON.parse(${HOOK_INPUT_GLOBAL});
+    context.log = {
+      info: (tag, msg) => { __omnirouteLogs.push(["info", String(tag), String(msg)]); },
+      warn: (tag, msg) => { __omnirouteLogs.push(["warn", String(tag), String(msg)]); },
+      error: (tag, msg) => { __omnirouteLogs.push(["error", String(tag), String(msg)]); },
+    };
+    const __omnirouteResult = await (async () => { ${code}
+    })();
+    delete context.log;
+    __omniroutePayload = {
+      ok: true,
+      result: __omnirouteResult === undefined || __omnirouteResult === null ? {} : __omnirouteResult,
+      context,
+      logs: __omnirouteLogs,
+    };
+  } catch (__omnirouteError) {
+    let message = "Hook threw";
+    try {
+      message = String(
+        __omnirouteError && __omnirouteError.message !== undefined
+          ? __omnirouteError.message
+          : __omnirouteError
+      );
+    } catch {}
+    __omniroutePayload = { ok: false, error: message, logs: __omnirouteLogs };
+  }
+  globalThis.${HOOK_OUTPUT_GLOBAL} = JSON.stringify(__omniroutePayload);
+})();`;
+}
+
+type HookPayload = {
+  ok?: unknown;
+  error?: unknown;
+  result?: unknown;
+  context?: unknown;
+  logs?: unknown;
+};
+
+function isPlainRecord(value: unknown): value is Record<string, unknown> {
+  return typeof value === "object" && value !== null && !Array.isArray(value);
+}
+
+/** Parse realm output on the host; `__proto__` keys are dropped, never assigned. */
+function parseHookPayload(raw: string, hookName: string): HookPayload {
+  try {
+    const parsed: unknown = JSON.parse(raw, (key, value) =>
+      key === "__proto__" ? undefined : value
+    );
+    if (isPlainRecord(parsed)) return parsed as HookPayload;
+  } catch {
+    // fall through
+  }
+  throw new Error(`Hook "${hookName}" returned an unreadable result`);
+}
+
+/** Only an own data property holding a string is accepted — never a getter. */
+function readHookOutput(sandbox: object): string | null {
+  const descriptor = Object.getOwnPropertyDescriptor(sandbox, HOOK_OUTPUT_GLOBAL);
+  return descriptor && "value" in descriptor && typeof descriptor.value === "string"
+    ? descriptor.value
+    : null;
+}
+
+/**
+ * Message of an error that escaped `runInContext()` (the vm timeout is raised from the
+ * realm, so it is not `instanceof Error` here). Read as an own data property only: a
+ * getter would run realm code outside the timeout.
+ */
+function ownStringMessage(err: unknown): string | null {
+  if (err instanceof Error) return err.message;
+  if (typeof err !== "object" || err === null) return null;
+  const descriptor = Object.getOwnPropertyDescriptor(err, "message");
+  return descriptor && "value" in descriptor && typeof descriptor.value === "string"
+    ? descriptor.value
+    : null;
+}
+
+function toHookInput(context: PreRequestHookContext): string {
+  return JSON.stringify({
+    body: context.body,
+    headers: context.headers,
+    model: context.model,
+    combo: context.combo,
+    apiKeyInfo: context.apiKeyInfo,
+    metadata: context.metadata,
+  });
+}
+
+/** Copy the hook's in-place mutations back onto the host context (data fields only). */
+function applyContextMutations(context: PreRequestHookContext, mutated: unknown): void {
+  if (!isPlainRecord(mutated)) return;
+  if (isPlainRecord(mutated.body)) context.body = mutated.body;
+  if (isPlainRecord(mutated.headers)) {
+    context.headers = mutated.headers as PreRequestHookContext["headers"];
+  }
+  if (typeof mutated.model === "string") context.model = mutated.model;
+  if (typeof mutated.combo === "string") context.combo = mutated.combo;
+  else if (mutated.combo === undefined || mutated.combo === null) context.combo = undefined;
+  if (isPlainRecord(mutated.metadata)) context.metadata = mutated.metadata;
+}
+
+function replayHookLogs(context: PreRequestHookContext, logs: unknown): void {
+  if (!Array.isArray(logs)) return;
+  for (const entry of logs) {
+    if (!Array.isArray(entry) || entry.length !== 3) continue;
+    const [level, tag, msg] = entry;
+    if (level !== "info" && level !== "warn" && level !== "error") continue;
+    context.log?.[level]?.(String(tag), String(msg));
+  }
 }
 
 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.
+  // isolated context, not re-parsing.
   let script: vm.Script;
   try {
-    script = new vm.Script(`(async () => { ${code} })();`, {
+    script = new vm.Script(buildHookSource(code), {
       filename: `omniroute-hook:${hookName}`,
     });
   } catch (err: unknown) {
@@ -124,44 +220,32 @@ function compileHookCode(code: string, hookName: string): HookMiddleware {
   }
 
   return async (context: PreRequestHookContext): Promise<HookResult> => {
-    const sandbox = createHookSandbox(context);
+    const sandbox: Record<string, unknown> = Object.create(null);
+    sandbox[HOOK_INPUT_GLOBAL] = toHookInput(context);
     const vmContext = vm.createContext(sandbox, {
       codeGeneration: { strings: false, wasm: false },
+      microtaskMode: "afterEvaluate",
     });
 
-    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;
+      script.runInContext(vmContext, { timeout: HOOK_EXECUTION_TIMEOUT_MS });
     } 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);
+      // A vm timeout / realm error: its constructor is not the host Error. Normalize
+      // to a readable host Error without calling into the realm.
+      throw new Error(ownStringMessage(err) ?? `Hook "${hookName}" failed`);
     }
+
+    const raw = readHookOutput(sandbox);
+    if (raw === null) {
+      throw new Error(`Hook "${hookName}" did not finish (awaited something that never settles?)`);
+    }
+    const payload = parseHookPayload(raw, hookName);
+    replayHookLogs(context, payload.logs);
+    if (payload.ok !== true) {
+      throw new Error(typeof payload.error === "string" ? payload.error : "Hook threw");
+    }
+    applyContextMutations(context, payload.context);
+    return (isPlainRecord(payload.result) ? payload.result : {}) as HookResult;
   };
 }
 

stryker.conf.json

@@ -522,7 +522,8 @@
       "tests/unit/client-abort-propagates-upstream.test.ts",
       "tests/unit/data-dir-private-perms.test.ts",
       "tests/unit/translation-failure-skips-account-cooldown-14815.test.ts",
-      "tests/unit/sudo-password-never-reaches-command-stdin.test.ts"
+      "tests/unit/sudo-password-never-reaches-command-stdin.test.ts",
+      "tests/unit/middleware-hook-realm-isolation.test.ts"
     ],
     "nodeArgs": [
       "--import",

tests/unit/middleware-hook-realm-isolation.test.ts

@@ -0,0 +1,139 @@
+/**
+ * GHSA-9p9m-h9rj-rhhg — pre-request hook code must not reach the host realm.
+ *
+ * The old sandbox handed hook code the host `Object`, `Promise`, … and the live
+ * `context`, so a hook could write the SERVER's `Object.prototype`
+ * (`Object.prototype.env = { NODE_OPTIONS: "--require …" }`), which a later
+ * `worker_threads` Worker inherited → code execution. Each case below is a way to reach
+ * the host realm from hook code; none of them may leave a trace in the host.
+ */
+import { test, beforeEach, after } from "node:test";
+import assert from "node:assert/strict";
+
+import {
+  registerHook,
+  runHooks,
+  createHookContext,
+  getHook,
+  clearAllHooks,
+} from "../../src/lib/middleware/registry.ts";
+import { HookPriority, type HookConfig } from "../../src/lib/middleware/types.ts";
+
+function hook(name: string, code: string): HookConfig {
+  return {
+    name,
+    code,
+    description: "realm isolation",
+    priority: HookPriority.NORMAL,
+    scope: { type: "global" },
+    enabled: true,
+    createdAt: new Date().toISOString(),
+    updatedAt: new Date().toISOString(),
+    runCount: 0,
+  };
+}
+
+function ctx(log?: { info: (t: string, m: string) => void }) {
+  return createHookContext({
+    body: { messages: [] },
+    headers: { "x-test": "1" },
+    model: "gpt-4o",
+    log: log ? { info: log.info, warn: () => {}, error: () => {} } : undefined,
+  });
+}
+
+const PROBES = ["__ghsa9p9mEnv", "__ghsa9p9mProto", "__ghsa9p9mCtor", "__ghsa9p9mArr"];
+
+function hostPolluted(): string[] {
+  const probe: Record<string, unknown> = {};
+  return PROBES.filter((key) => key in probe || key in []);
+}
+
+beforeEach(() => clearAllHooks());
+after(() => {
+  clearAllHooks();
+  for (const key of PROBES) {
+    delete (Object.prototype as Record<string, unknown>)[key];
+    delete (Array.prototype as unknown as Record<string, unknown>)[key];
+  }
+  delete (globalThis as Record<string, unknown>).__ghsa9p9mEscaped;
+});
+
+test("writing Object.prototype inside a hook does not pollute the server", async () => {
+  registerHook(
+    hook(
+      "pollute-object",
+      `Object.prototype.__ghsa9p9mEnv = { NODE_OPTIONS: "--require /tmp/x.js" };
+       Array.prototype.__ghsa9p9mArr = 1;`
+    )
+  );
+  await runHooks(ctx());
+  assert.deepEqual(hostPolluted(), []);
+  assert.equal(getHook("pollute-object")?.lastError, undefined, "the hook itself still runs");
+});
+
+test("reaching Object.prototype through the context object does not pollute the server", async () => {
+  registerHook(
+    hook(
+      "pollute-via-context",
+      `context.__proto__.__ghsa9p9mProto = 1;
+       context.constructor.prototype.__ghsa9p9mCtor = 1;
+       context.body.messages.constructor.prototype.__ghsa9p9mArr = 1;`
+    )
+  );
+  await runHooks(ctx());
+  assert.deepEqual(hostPolluted(), []);
+});
+
+test("the constructor-chain escape cannot compile code", async () => {
+  registerHook(
+    hook(
+      "ctor-escape",
+      `const F = this.constructor.constructor;
+       return { body: { got: typeof F("return process")() } };`
+    )
+  );
+  const { context } = await runHooks(ctx());
+  assert.equal(context.body.got, undefined);
+  assert.match(getHook("ctor-escape")?.lastError ?? "", /[Cc]ode generation from strings/);
+});
+
+test("a replaced Promise.prototype.then never receives a host function", async () => {
+  registerHook(
+    hook(
+      "then-hijack",
+      `Promise.prototype.then = function (resolve) {
+         try { resolve.constructor("globalThis.__ghsa9p9mEscaped = true")(); } catch {}
+       };
+       return { model: "still-here" };`
+    )
+  );
+  await runHooks(ctx());
+  assert.equal((globalThis as Record<string, unknown>).__ghsa9p9mEscaped, undefined);
+});
+
+test("contract kept: in-place mutations, result merge and log lines reach the host", async () => {
+  const lines: string[] = [];
+  registerHook(
+    hook(
+      "contract",
+      `context.body.injected = "yes";
+       context.metadata.seen = true;
+       context.log.info("HOOK", "ran for " + context.model);
+       return { body: { added: true }, model: "gpt-4o-mini" };`
+    )
+  );
+  const { context } = await runHooks(ctx({ info: (t, m) => lines.push(`${t}:${m}`) }));
+  assert.equal(context.body.injected, "yes");
+  assert.equal(context.body.added, true);
+  assert.equal(context.model, "gpt-4o-mini");
+  assert.equal(context.metadata.seen, true);
+  assert.deepEqual(lines, ["HOOK:ran for gpt-4o"]);
+});
+
+test("a hook awaiting something that never settles fails instead of hanging", async () => {
+  registerHook(hook("never-settles", `await new Promise(() => {}); return { model: "x" };`));
+  const { context } = await runHooks(ctx());
+  assert.equal(context.model, "gpt-4o");
+  assert.match(getHook("never-settles")?.lastError ?? "", /did not finish/);
+});