Skip to content

Commit 928f584

Browse files
committed
improvement(sandbox): delegate timers to Node, add phase timings + saturation logs
Follow-ups on top of the isolated-vm migration (da14027): Timer delegation (laverdet/isolated-vm#136 recommended pattern): - setTimeout / setInterval / clearTimeout / clearImmediate delegate to Node's real timer heap via ivm.Reference. Real delays are honored; clearTimeout actually cancels; ms is clamped to the script timeout so callbacks can't fire after the isolate is disposed. - Per-execution timer tracking + dispose-sweep in finally. Zero stale callbacks post-dispose. - unwrapPrimitive helper normalizes ivm.Reference-wrapped primitives (arguments: { reference: true } applies uniformly to all args). - _polyfills.ts shrinks from ~130 lines to the global->globalThis alias. Timers / TextEncoder / TextDecoder / console all install per-execution from the worker via ivm bridges. AbortSignal race fix (pre-existing bug surfaced by the timer smoke): - Listener is registered after await tryAcquireDistributedLease. If the signal aborted during that ~200ms window (Redis down), AbortSignal doesn't fire listeners registered after the fact — the abort was silently missed. Now re-checks signal.aborted synchronously after addEventListener. Observability: - executeTask returns IsolatedVMTaskTimings (setup, runtimeBootstrap, bundles, brokerInstall, taskBootstrap, harden, userCode, finalize, total) in every success + error path. run-task.ts logs these with workspaceId + queueMs so 'which tenant is slow' is queryable. - Pool saturation events now emit structured logger.warn with reason codes: queue_full_global, queue_full_owner, queue_wait_timeout, distributed_lease_limit. Matches the existing broker reject pattern. Security policy: - New .cursor/rules/sim-sandbox.mdc codifies the hard rules for the worker process: no app credentials, all credentialed work goes through host-side brokers, every broker scopes by workspaceId. Pre-merge checklist for future changes to isolated-vm-worker.cjs. Measured phase breakdown (local smoke, Redis down): pptx wall=~310ms with bundles=~16ms, finalize=~83ms; docx ~290ms / 17ms / 70ms; pdf ~235ms / 17ms / 5ms. Bundle compilation is not the bottleneck — library finalize is. Made-with: Cursor
1 parent da14027 commit 928f584

File tree

8 files changed

+467
-187
lines changed

8 files changed

+467
-187
lines changed

.cursor/rules/sim-sandbox.mdc

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
---
2+
description: Isolated-vm sandbox worker security policy. Hard rules for anything that lives in the worker child process that runs user code.
3+
globs: ["apps/sim/lib/execution/isolated-vm-worker.cjs", "apps/sim/lib/execution/isolated-vm.ts", "apps/sim/lib/execution/sandbox/**", "apps/sim/sandbox-tasks/**"]
4+
---
5+
6+
# Sim Sandbox — Worker Security Policy
7+
8+
The isolated-vm worker child process at
9+
`apps/sim/lib/execution/isolated-vm-worker.cjs` runs untrusted user code inside
10+
V8 isolates. The process itself is a trust boundary. Everything in this rule is
11+
about what must **never** live in that process.
12+
13+
## Hard rules
14+
15+
1. **No app credentials in the worker process**. The worker must not hold, load,
16+
or receive via IPC: database URLs, Redis URLs, AWS keys, Stripe keys,
17+
session-signing keys, encryption keys, OAuth client secrets, internal API
18+
secrets, or any LLM / email / search provider API keys. If you catch yourself
19+
`require`'ing `@/lib/auth`, `@sim/db`, `@/lib/uploads/core/storage-service`,
20+
or anything that imports `env` directly inside the worker, stop and use a
21+
host-side broker instead.
22+
23+
2. **Host-side brokers own all credentialed work**. The worker can only access
24+
resources through `ivm.Reference` / `ivm.Callback` bridges back to the host
25+
process. Today the only broker is `workspaceFileBroker`
26+
(`apps/sim/lib/execution/sandbox/brokers/workspace-file.ts`); adding a new
27+
one requires co-reviewing this file.
28+
29+
3. **Host-side brokers must scope every resource access to a single tenant**.
30+
The `SandboxBrokerContext` always carries `workspaceId`. Any new broker that
31+
accesses storage, DB, or an external API must use `ctx.workspaceId` to scope
32+
the lookup — never accept a raw path, key, or URL from isolate code without
33+
validation.
34+
35+
4. **Nothing that runs in the isolate is trusted, even if we wrote it**. The
36+
task `bootstrap` and `finalize` strings in `apps/sim/sandbox-tasks/` execute
37+
inside the isolate. They must treat `globalThis` as adversarial — no pulling
38+
values from it that might have been mutated by user code. The hardening
39+
script in `executeTask` undefines dangerous globals before user code runs.
40+
41+
## Why
42+
43+
A V8 JIT bug (Chrome ships these roughly monthly) gives an attacker a native
44+
code primitive inside the process that owns whatever that process can reach.
45+
If the worker only holds `isolated-vm` + a single narrow workspace-file broker,
46+
a V8 escape leaks one tenant's files. If the worker holds a Stripe key or a DB
47+
connection, a V8 escape leaks the service.
48+
49+
The original `doc-worker.cjs` vulnerability (CVE-class, 225 production secrets
50+
leaked via `/proc/1/environ`) was the forcing function for this architecture.
51+
Keep the blast radius small.
52+
53+
## Checklist for changes to `isolated-vm-worker.cjs`
54+
55+
Before landing any change that adds a new `require(...)` or `process.send(...)`
56+
payload or `ivm.Reference` wrapper in the worker:
57+
58+
- [ ] Does it load a credential, key, connection string, or secret? If yes,
59+
move it host-side and expose as a broker.
60+
- [ ] Does it import from `@/lib/auth`, `@sim/db`, `@/lib/uploads/core/*`,
61+
`@/lib/core/config/env`, or any module that reads `process.env` of the
62+
main app? If yes, same — move host-side.
63+
- [ ] Does it expose a resource that's workspace-scoped without taking a
64+
`workspaceId`? If yes, re-scope.
65+
- [ ] Did you update the broker limits (`IVM_MAX_BROKER_ARGS_JSON_CHARS`,
66+
`IVM_MAX_BROKER_RESULT_JSON_CHARS`, `IVM_MAX_BROKERS_PER_EXECUTION`) if
67+
the new broker can emit large payloads or fire frequently?
68+
69+
## What the worker *may* hold
70+
71+
- `isolated-vm` module
72+
- Node built-ins: `node:fs` (only for reading the checked-in bundle `.cjs`
73+
files) and `node:path`
74+
- The three prebuilt library bundles under
75+
`apps/sim/lib/execution/sandbox/bundles/*.cjs`
76+
- IPC message handlers for `execute`, `cancel`, `fetchResponse`,
77+
`brokerResponse`
78+
79+
The worker deliberately has **no host-side logger**. All errors and
80+
diagnostics flow through IPC back to the host, which has `@sim/logger`. Do
81+
not add `createLogger` or console-based logging to the worker — it would
82+
require pulling the main app's config / env, which is exactly what this
83+
rule is preventing.
84+
85+
Anything else is suspect.

0 commit comments

Comments
 (0)