-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix(execution): run pptx/docx/pdf generation inside isolated-vm sandbox #4217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
da14027
fix(execution): run pptx/docx/pdf generation inside isolated-vm sandbox
icecrasher321 928f584
improvement(sandbox): delegate timers to Node, add phase timings + sa…
icecrasher321 03a6dc6
fix(sandbox): thread AbortSignal into runSandboxTask at every call site
icecrasher321 6323be3
chore(build): raise node heap to 8GB for next build type-check
icecrasher321 198ae7e
fix lint
icecrasher321 a1c099c
Merge branch 'staging' into fix/urgent-billing-isolated
icecrasher321 e47b27f
refactor(copilot): dedup getDocumentFormatInfo across copilot file tools
icecrasher321 2645afe
fix(sandbox): propagate empty-message broker/fetch errors
icecrasher321 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| --- | ||
| description: Isolated-vm sandbox worker security policy. Hard rules for anything that lives in the worker child process that runs user code. | ||
| 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/**"] | ||
| --- | ||
|
|
||
| # Sim Sandbox — Worker Security Policy | ||
|
|
||
| The isolated-vm worker child process at | ||
| `apps/sim/lib/execution/isolated-vm-worker.cjs` runs untrusted user code inside | ||
| V8 isolates. The process itself is a trust boundary. Everything in this rule is | ||
| about what must **never** live in that process. | ||
|
|
||
| ## Hard rules | ||
|
|
||
| 1. **No app credentials in the worker process**. The worker must not hold, load, | ||
| or receive via IPC: database URLs, Redis URLs, AWS keys, Stripe keys, | ||
| session-signing keys, encryption keys, OAuth client secrets, internal API | ||
| secrets, or any LLM / email / search provider API keys. If you catch yourself | ||
| `require`'ing `@/lib/auth`, `@sim/db`, `@/lib/uploads/core/storage-service`, | ||
| or anything that imports `env` directly inside the worker, stop and use a | ||
| host-side broker instead. | ||
|
|
||
| 2. **Host-side brokers own all credentialed work**. The worker can only access | ||
| resources through `ivm.Reference` / `ivm.Callback` bridges back to the host | ||
| process. Today the only broker is `workspaceFileBroker` | ||
| (`apps/sim/lib/execution/sandbox/brokers/workspace-file.ts`); adding a new | ||
| one requires co-reviewing this file. | ||
|
|
||
| 3. **Host-side brokers must scope every resource access to a single tenant**. | ||
| The `SandboxBrokerContext` always carries `workspaceId`. Any new broker that | ||
| accesses storage, DB, or an external API must use `ctx.workspaceId` to scope | ||
| the lookup — never accept a raw path, key, or URL from isolate code without | ||
| validation. | ||
|
|
||
| 4. **Nothing that runs in the isolate is trusted, even if we wrote it**. The | ||
| task `bootstrap` and `finalize` strings in `apps/sim/sandbox-tasks/` execute | ||
| inside the isolate. They must treat `globalThis` as adversarial — no pulling | ||
| values from it that might have been mutated by user code. The hardening | ||
| script in `executeTask` undefines dangerous globals before user code runs. | ||
|
|
||
| ## Why | ||
|
|
||
| A V8 JIT bug (Chrome ships these roughly monthly) gives an attacker a native | ||
| code primitive inside the process that owns whatever that process can reach. | ||
| If the worker only holds `isolated-vm` + a single narrow workspace-file broker, | ||
| a V8 escape leaks one tenant's files. If the worker holds a Stripe key or a DB | ||
| connection, a V8 escape leaks the service. | ||
|
|
||
| The original `doc-worker.cjs` vulnerability (CVE-class, 225 production secrets | ||
| leaked via `/proc/1/environ`) was the forcing function for this architecture. | ||
| Keep the blast radius small. | ||
|
|
||
| ## Checklist for changes to `isolated-vm-worker.cjs` | ||
|
|
||
| Before landing any change that adds a new `require(...)` or `process.send(...)` | ||
| payload or `ivm.Reference` wrapper in the worker: | ||
|
|
||
| - [ ] Does it load a credential, key, connection string, or secret? If yes, | ||
| move it host-side and expose as a broker. | ||
| - [ ] Does it import from `@/lib/auth`, `@sim/db`, `@/lib/uploads/core/*`, | ||
| `@/lib/core/config/env`, or any module that reads `process.env` of the | ||
| main app? If yes, same — move host-side. | ||
| - [ ] Does it expose a resource that's workspace-scoped without taking a | ||
| `workspaceId`? If yes, re-scope. | ||
| - [ ] Did you update the broker limits (`IVM_MAX_BROKER_ARGS_JSON_CHARS`, | ||
| `IVM_MAX_BROKER_RESULT_JSON_CHARS`, `IVM_MAX_BROKERS_PER_EXECUTION`) if | ||
| the new broker can emit large payloads or fire frequently? | ||
|
|
||
| ## What the worker *may* hold | ||
|
|
||
| - `isolated-vm` module | ||
| - Node built-ins: `node:fs` (only for reading the checked-in bundle `.cjs` | ||
| files) and `node:path` | ||
| - The three prebuilt library bundles under | ||
| `apps/sim/lib/execution/sandbox/bundles/*.cjs` | ||
| - IPC message handlers for `execute`, `cancel`, `fetchResponse`, | ||
| `brokerResponse` | ||
|
|
||
| The worker deliberately has **no host-side logger**. All errors and | ||
| diagnostics flow through IPC back to the host, which has `@sim/logger`. Do | ||
| not add `createLogger` or console-based logging to the worker — it would | ||
| require pulling the main app's config / env, which is exactly what this | ||
| rule is preventing. | ||
|
|
||
| Anything else is suspect. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.