Skip to content

Commit e443a97

Browse files
improvement(security): isolated-vm env construction (v8s escape case) (#6116)
* improvement(security): isolated-vm env construction (v8s escape case) * upgrade main node to v24
1 parent 632041c commit e443a97

7 files changed

Lines changed: 116 additions & 55 deletions

File tree

.claude/rules/sim-sandbox.md

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@ about what must **never** live in that process.
2222
secrets, or any LLM / email / search provider API keys. If you catch yourself
2323
`require`'ing `@/lib/auth`, `@sim/db`, `@/lib/uploads/core/storage-service`,
2424
or anything that imports `env` directly inside the worker, stop and use a
25-
host-side broker instead.
25+
host-side broker instead. This includes the OS environment: the worker is
26+
spawned with the explicit allowlisted env from `buildWorkerEnv()` in
27+
`isolated-vm.ts` — never spawn it without an `env` option (Node would copy
28+
the app's full `process.env`, secrets included, into the worker).
2629

2730
2. **Host-side brokers own all credentialed work**. The worker can only access
2831
resources through `ivm.Reference` / `ivm.Callback` bridges back to the host
@@ -69,6 +72,10 @@ payload or `ivm.Reference` wrapper in the worker:
6972
- [ ] Did you update the broker limits (`IVM_MAX_BROKER_ARGS_JSON_CHARS`,
7073
`IVM_MAX_BROKER_RESULT_JSON_CHARS`, `IVM_MAX_BROKERS_PER_EXECUTION`) if
7174
the new broker can emit large payloads or fire frequently?
75+
- [ ] Does the worker read a new env var? Add it to the `buildWorkerEnv()`
76+
allowlist in `isolated-vm.ts` **and** to the allowlist regression test in
77+
`isolated-vm.test.ts` — the worker does not inherit the app environment,
78+
so an un-allowlisted var is simply absent in the child.
7279

7380
## What the worker *may* hold
7481

.github/workflows/test-build.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ jobs:
2525
- name: Setup Node
2626
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6
2727
with:
28-
node-version: 22
28+
node-version: 24
2929

3030
# Cache keys are scoped by event name, and fork PRs get their own
3131
# namespace on top: untrusted fork runs must never share a cache with
@@ -250,7 +250,7 @@ jobs:
250250
- name: Setup Node
251251
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6
252252
with:
253-
node-version: 22
253+
node-version: 24
254254

255255
- name: Mount Bun cache
256256
uses: ./.github/actions/cache-mount

apps/sim/lib/execution/isolated-vm.test.ts

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,68 @@ describe('isolated-vm scheduler', () => {
296296
expect(spawnMock).toHaveBeenCalledTimes(2)
297297
})
298298

299+
it('spawns workers with an allowlisted env, never the parent process.env', async () => {
300+
const ALLOWED_WORKER_ENV_KEYS = new Set([
301+
'PATH',
302+
'NODE_ENV',
303+
'IVM_MAX_STDOUT_CHARS',
304+
'IVM_MAX_FETCH_OPTIONS_JSON_CHARS',
305+
'TZ',
306+
'LANG',
307+
'LC_ALL',
308+
'SYSTEMROOT',
309+
'WINDIR',
310+
'COMSPEC',
311+
'PATHEXT',
312+
'TEMP',
313+
'TMP',
314+
])
315+
vi.stubEnv('SIM_SANDBOX_SECRET_CANARY', 'must-not-reach-worker')
316+
try {
317+
const { executeInIsolatedVM, spawnMock } = await loadExecutionModule({
318+
spawns: [() => createReadyProc('ok')],
319+
})
320+
321+
await executeInIsolatedVM({
322+
code: 'return "ok"',
323+
params: {},
324+
envVars: {},
325+
contextVariables: {},
326+
timeoutMs: 100,
327+
requestId: 'req-env',
328+
})
329+
330+
expect(spawnMock).toHaveBeenCalledTimes(1)
331+
const spawnOptions = spawnMock.mock.calls[0]?.[2] as
332+
| { env?: Record<string, string> }
333+
| undefined
334+
expect(spawnOptions?.env).toBeDefined()
335+
const workerEnv = spawnOptions?.env ?? {}
336+
337+
expect(workerEnv.SIM_SANDBOX_SECRET_CANARY).toBeUndefined()
338+
for (const secretKey of [
339+
'DATABASE_URL',
340+
'REDIS_URL',
341+
'ENCRYPTION_KEY',
342+
'BETTER_AUTH_SECRET',
343+
'STRIPE_SECRET_KEY',
344+
'OPENAI_API_KEY',
345+
'AWS_SECRET_ACCESS_KEY',
346+
]) {
347+
expect(workerEnv[secretKey]).toBeUndefined()
348+
}
349+
for (const key of Object.keys(workerEnv)) {
350+
expect(
351+
ALLOWED_WORKER_ENV_KEYS.has(key),
352+
`unexpected env var forwarded to sandbox worker: ${key}`
353+
).toBe(true)
354+
}
355+
expect(workerEnv.PATH).toBe(process.env.PATH)
356+
} finally {
357+
vi.unstubAllEnvs()
358+
}
359+
})
360+
299361
it('rejects new requests when the queue is full', async () => {
300362
const holder = createControllableReadyProc()
301363
const { executeInIsolatedVM } = await loadExecutionModule({

apps/sim/lib/execution/isolated-vm.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import path from 'node:path'
44
import { fileURLToPath } from 'node:url'
55
import { createLogger } from '@sim/logger'
66
import { getErrorMessage, toError } from '@sim/utils/errors'
7+
import { filterUndefined } from '@sim/utils/object'
78
import { randomFloat } from '@sim/utils/random'
89
import { env } from '@/lib/core/config/env'
910
import { getRedisClient } from '@/lib/core/config/redis'
@@ -890,6 +891,37 @@ function resetWorkerIdleTimeout(workerId: number) {
890891
}
891892
}
892893

894+
/**
895+
* Environment for the sandbox worker process. The worker runs untrusted user
896+
* code, so it must never inherit the app's `process.env` (DB URLs, encryption
897+
* keys, provider API keys — see `.claude/rules/sim-sandbox.md`): a V8 isolate
898+
* escape would read every inherited secret from the worker's environment.
899+
* Only an explicit allowlist is forwarded — `PATH` so `spawn('node', ...)` can
900+
* resolve the binary, `NODE_ENV`, the two `IVM_*` limits the worker reads,
901+
* timezone/locale vars so `Date`/`Intl` behavior inside isolates matches the
902+
* host, and the Windows system vars Node needs to boot (undefined elsewhere
903+
* and stripped).
904+
* Any new env var the worker reads must be added here and to the allowlist
905+
* regression test in `isolated-vm.test.ts`.
906+
*/
907+
function buildWorkerEnv(): NodeJS.ProcessEnv {
908+
const allowed: Record<string, string | undefined> = {
909+
PATH: process.env.PATH,
910+
IVM_MAX_STDOUT_CHARS: env.IVM_MAX_STDOUT_CHARS,
911+
IVM_MAX_FETCH_OPTIONS_JSON_CHARS: env.IVM_MAX_FETCH_OPTIONS_JSON_CHARS,
912+
TZ: process.env.TZ,
913+
LANG: process.env.LANG,
914+
LC_ALL: process.env.LC_ALL,
915+
SYSTEMROOT: process.env.SYSTEMROOT,
916+
WINDIR: process.env.WINDIR,
917+
COMSPEC: process.env.COMSPEC,
918+
PATHEXT: process.env.PATHEXT,
919+
TEMP: process.env.TEMP,
920+
TMP: process.env.TMP,
921+
}
922+
return { ...filterUndefined(allowed), NODE_ENV: process.env.NODE_ENV }
923+
}
924+
893925
function spawnWorker(): Promise<WorkerInfo> {
894926
const workerId = nextWorkerId++
895927
spawnInProgress++
@@ -955,6 +987,7 @@ function spawnWorker(): Promise<WorkerInfo> {
955987
const proc = spawn('node', ['--no-node-snapshot', workerPath], {
956988
stdio: ['ignore', 'pipe', 'pipe', 'ipc'],
957989
serialization: 'json',
990+
env: buildWorkerEnv(),
958991
})
959992
childProcess = proc
960993

apps/sim/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@
174174
"input-otp": "^1.4.2",
175175
"ioredis": "^5.6.0",
176176
"ipaddr.js": "2.3.0",
177-
"isolated-vm": "6.0.2",
177+
"isolated-vm": "6.1.2",
178178
"jose": "6.0.11",
179179
"js-tiktoken": "1.0.21",
180180
"js-yaml": "4.3.0",

0 commit comments

Comments
 (0)