feat(extensions): unify electron + web on a Worker-based sandbox (drop isolated-vm)#676
Open
0xdeafcafe wants to merge 8 commits into
Open
feat(extensions): unify electron + web on a Worker-based sandbox (drop isolated-vm)#6760xdeafcafe wants to merge 8 commits into
0xdeafcafe wants to merge 8 commits into
Conversation
Captures the rationale for dropping `isolated-vm` and converging the
two extension runtimes (Electron's isolated-vm, Web's Web Worker) on a
single Worker-based model. The Web side already proved Workers
sufficient; this ADR turns that into the shared substrate so both
hosts can use it.
Motivation:
- macOS notarization rejects the unsigned `.node` prebuilds + the
stray .tgz tarball that ship inside `isolated-vm` inside the
app.asar.unpacked.
- electron-rebuild postinstall + native ABI fragility are real
operational cost for an Electron sandbox we no longer benefit from.
- One worker source + one host-agnostic manager beats two parallel
implementations of the same lifecycle.
The ADR documents the explicit trust model ("user-installed extensions,
sandbox prevents accidents not adversaries") so a future tightening
(e.g. `vm.runInContext`) is a deliberate decision, not a regression.
…kers
Implements ADR-0003. The Electron host now runs each extension in a
`node:worker_threads` Worker; the Web host keeps its Web Worker; both
share a single `WORKER_SOURCE` + `WorkerExtensionManager` in
`@beak/runtime-shared/extensions`. `isolated-vm` is gone.
Changes:
- `packages/runtime-shared/src/extensions/worker-source.ts` — new.
Holds the shared worker source (the Web-Worker-style script that
runs in both runtimes) plus a tiny Node shim that aliases
`self`/`postMessage`/`addEventListener` onto `parentPort` so the
same script works under `worker_threads`.
- `packages/runtime-shared/src/extensions/worker-manager.ts` — new.
The host-agnostic lifecycle (`load` / `unload` / `resetProject` /
variable invocation) parameterised on a small `WorkerProvider` and
two callbacks (`parseValueSections`, `log`). Both hosts now
sub-class it.
- `apps-host/web/src/lib/extension/manager.ts` — shrinks to a Web
Worker adapter + the webIpcMain `parseValueSections` bridge.
`apps-host/web/src/lib/extension/worker-source.ts` deleted.
- `apps-host/electron/src/lib/extension/index.ts` — replaces the
467-line isolated-vm implementation with a ~120-line Worker
adapter that wraps `node:worker_threads`, routes the recursive
`parse-value-sections` callback through `ipcMain`, and logs via
`tslog`. The bootstrap shim, `ivm.Reference.apply` ceremony,
`ExternalCopy` plumbing — all gone.
- `apps-host/electron/package.json` — drop `isolated-vm` dep + the
`electron-rebuild -f -w isolated-vm` postinstall hook.
- `apps-host/electron/esbuild.main.config.ts` — drop the
`isolated-vm` entry from `external` (was needed to keep the
.node binding resolving from node_modules at runtime).
- The two `extensions-service.ts` files learn the new manager
signature: `callerCtx` (the renderer WebContents on Electron;
`null` on the web) flows through every variable invocation so
the manager can route the recursive parse-value-sections roundtrip
back to the right host context. `load()` now takes
`{ validateScriptPath }` instead of an event reference.
Trust model is unchanged and documented in ADR-0003: Worker isolation
covers accidental crashes + cross-extension memory; not adversarial
code. If we tighten later, the entry point is the `evaluateUserSource`
helper inside `WORKER_SOURCE` — wrap it in `vm.runInContext` with a
stripped global, no SDK impact.
Verified locally: `pnpm lint`, `pnpm typecheck:apps-host`, `pnpm build`
all exit 0 across every workspace.
There was a problem hiding this comment.
Pull request overview
This PR implements ADR-0003 by unifying the Electron and web extension sandboxes around a shared Worker-based runtime, removing isolated-vm (and its native rebuild/signing baggage) from the Electron host while keeping the extension SDK contract unchanged.
Changes:
- Introduces a shared
WORKER_SOURCE+WORKER_RUNTIME_NODE_SHIMand a new host-agnosticWorkerExtensionManagerin@beak/runtime-shared/extensions. - Refactors both the web host and Electron host extension managers into thin adapters that provide a
WorkerProviderand IPC-basedparseValueSectionsbridging. - Removes
isolated-vmfrom Electron build/dependencies (package.json, lockfile, esbuild externals, postinstall rebuild).
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Removes isolated-vm (and node-gyp-build) from the lockfile graph. |
| packages/runtime-shared/src/extensions/worker-source.ts | Centralizes the worker script and adds a Node runtime shim for worker_threads. |
| packages/runtime-shared/src/extensions/worker-manager.ts | Adds the new shared WorkerExtensionManager (lifecycle + call routing + PVS bridge + logging). |
| packages/runtime-shared/src/extensions/registry-base.ts | Updates registry docs to reflect the unified Worker-based record shape. |
| packages/runtime-shared/src/extensions/index.ts | Exports the new worker source/shim and manager types/classes. |
| docs/adr/README.md | Adds ADR-0003 to the ADR index. |
| docs/adr/0003-unified-worker-extensions.md | New ADR documenting the unified worker-based extension sandbox and trust model. |
| apps-host/web/src/lib/extension/manager.ts | Replaces bespoke web manager with an adapter over WorkerExtensionManager + Web Worker provider. |
| apps-host/web/src/ipc/extensions-service.ts | Updates IPC handlers to pass callerCtx (null on web) into the shared manager API. |
| apps-host/electron/src/lib/extension/index.ts | Replaces isolated-vm implementation with a worker_threads adapter over WorkerExtensionManager. |
| apps-host/electron/src/ipc-layer/extensions-service.ts | Updates install/list/update and variable IPC routing for new load(...) signature + callerCtx (WebContents). |
| apps-host/electron/package.json | Drops isolated-vm dependency and electron-rebuild postinstall hook. |
| apps-host/electron/esbuild.main.config.ts | Removes isolated-vm from the esbuild external list. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+271
to
+276
| try { | ||
| const callerCtx = this.callerCtxByWorker.get(record.worker) as TCallerCtx; | ||
| const parsed = await withTimeout( | ||
| this.callbacks.parseValueSections(callerCtx, ctx, parts, 0), | ||
| PARSE_VALUE_SECTIONS_TIMEOUT_MS, | ||
| 'parse_value_sections_timeout', |
Comment on lines
+175
to
+177
| const { record, variableId } = this.resolve(projectId, type); | ||
| this.callerCtxByWorker.set(record.worker, callerCtx); | ||
| return (await this.call(record, [variableId, 'getValue'], [varCtx, payload, recursiveDepth])) as string; |
Comment on lines
+318
to
+333
| return await new Promise((resolve, reject) => { | ||
| const timer = setTimeout(() => { | ||
| record.pendingCalls.delete(callId); | ||
| reject(new Squawk('extension_call_timeout', { path, timeoutMs: CALL_TIMEOUT_MS })); | ||
| }, CALL_TIMEOUT_MS); | ||
|
|
||
| record.pendingCalls.set(callId, { | ||
| resolve: value => { | ||
| clearTimeout(timer); | ||
| resolve(value); | ||
| }, | ||
| reject: error => { | ||
| clearTimeout(timer); | ||
| reject(error); | ||
| }, | ||
| }); |
Comment on lines
+76
to
+83
| export class WorkerExtensionManager<TCallerCtx = unknown> { | ||
| private readonly registry = new ProjectExtensionRegistry<WorkerRecord>({ | ||
| dispose: terminate, | ||
| }); | ||
| private readonly providers: Providers; | ||
| private readonly workerProvider: WorkerProvider; | ||
| private readonly callbacks: WorkerManagerCallbacks<TCallerCtx>; | ||
| /** |
…dbox
The first cut of WorkerExtensionManager left Node `worker_threads`
running user code with the worker's full globalThis in scope — so
`globalThis.process`, dynamic `import('node:fs')`, and friends were
all reachable from extension code. The previous `isolated-vm` setup
gave us ECMAScript-only; this restores that posture.
The Electron runtime shim now:
- builds a `vm.createContext` with a hand-curated ECMAScript-only
global (Object/Array/Promise/Math/Date/JSON/RegExp, the typed-array
family, Reflect/Proxy/Intl, Error + subclasses, parse/encode/decode
helpers, plus `Function` — sandboxed by the vm context);
- sets `codeGeneration: { strings: false, wasm: false }` to block
`eval`, the Function-from-string constructor, and dynamic WASM;
- exposes the evaluator as `__beak_vm_evaluate`; `evaluateUserSource`
in `WORKER_SOURCE` picks that up automatically when present and
falls back to `new Function` on Web Workers (where we don't have a
vm-style API).
`process`, `Buffer`, `require`, dynamic `import()`, `fetch`,
`console`, `setTimeout`, `URL`, `SharedArrayBuffer`, `Atomics`,
`WebAssembly`, `eval` — all deliberately omitted. Extensions get the
language plus `extCtx` (the host bridge passed in as an argument).
ADR-0003 §3 (Trust model) updated to reflect both sides:
- Electron: tight by construction, parity with the old isolated-vm
posture.
- Web: best-effort; tightening properly needs Realms / iframe
sandboxes (out of scope, tracked as follow-up).
Memory isolation in both runtimes is identical to isolated-vm — each
worker is its own V8 isolate with its own heap.
drive-security smoke test on PR #676 demonstrated a P0 escape in the previous shim: `module.constructor.constructor('return process')()` returned the real Node `process` object. Root cause is that the shim assigned outer-realm primordials (Object, Function, Array, …) into `sandboxGlobals` and passed outer-realm `module`/`exports` objects in. `codeGeneration: { strings: false }` only applies within the sandbox context — once user code holds a reference to an outer-realm constructor, calling it goes back through the outer realm where codeGeneration is unrestricted. Switch to `vm.createContext({})` so the sandbox gets its own realm primordials (its own Object, Function, Array, …). User code can still walk `anyValue.constructor.constructor`, but the chain stays inside the sandbox where codeGeneration: strings:false makes the Function constructor refuse string arguments. Create `module`/`exports` INSIDE the sandbox via `vm.runInContext` so they're also sandbox-realm objects. Strip the default globals we don't want (`console`, `Atomics`, `SharedArrayBuffer`, `WebAssembly`, `escape`, `unescape`, `eval`) — they're present on `vm.createContext`'s default realm but extensions have no use for them. `wasm: false` already blocks `WebAssembly.compile` even if the binding lingered, but removing the object is cleaner. Verified with `node --eval` running the actual TS template-literal contents (resolved through `eval('`' + raw + '`')` so the runtime matches what the TS compiler produces). All probes return undefined or throw `Code generation from strings disallowed for this context`: process, require, Buffer, console, Atomics, SharedArrayBuffer, WebAssembly, setTimeout, fetch → undefined module.constructor.constructor('return process')() → throws exports.constructor.constructor('return process')() → throws Function('return process')() → throws ({}).constructor.constructor('return process')() → throws Promise.resolve(0).constructor.constructor('return process')() → throws new WebAssembly.Module(...) → throws (object deleted) eval → undefined Posture now matches isolated-vm: extensions get ECMAScript language plus `extCtx`. Full stop.
Mechanical fixes from `biome check --write` on the PR's touched files. - organizeImports across the four files where the existing order had drifted after the last set of edits. - Two ternary expressions in WorkerExtensionManager.attachListener picked up explicit parentheses around the right-hand `??` chain to silence the noStrayDanglingTernary precedence ambiguity. No behaviour change.
Two contained refactors from drive-code:
- Extract `serialiseErrorForWorker(error, defaultCode)` mirroring the
existing `deserialiseWorkerError`. The parse-value-sections error
path in `attachListener` had two nearly-identical inline conditional
chains (Squawk → .code, Error → .message, etc.) — now one call.
- Rename module-level `terminate(record)` to `disposeRecord(record)`.
The function calls `record.worker.terminate()` internally, so the
same name at both levels read like a recursion when scanning.
- Normalise `UnifiedWorker.terminate()` return type to `Promise<unknown>`
(was `void | Promise<unknown>`, which biome's noConfusingVoidType
flagged). Web adapter wraps its sync `worker.terminate()` in an
`async` method; Node adapter already returns the Promise from
`worker_threads`. The manager only `void`-discards the return, so
no caller cares.
Lint, typecheck, build still green.
The previous error handler only logged the worker's uncaught exception and left in-flight calls hanging on their 30-second timeout. Node `worker_threads` actually exits on an uncaught exception, so those calls were never going to resolve; Web Workers stay alive in an undefined state, which is no better. Fail fast with a Squawk so the caller can act on it now. Recorded as a P1 by /drive-feature on PR #676. Same minimal fix would have applied to the previous `isolated-vm` setup — not a regression this PR is introducing, just one we can clean up while we're here.
Two new test files, 21 tests:
worker-source.test.ts (11 tests)
Boots a real `node:worker_threads` worker against the shared
`WORKER_SOURCE + WORKER_RUNTIME_NODE_SHIM` and drives the wire
protocol end-to-end. Acts as the regression net for ADR-0003 §3:
- Strips `process`, `require`, `Buffer`, `console`, `Atomics`,
`SharedArrayBuffer`, `WebAssembly`, `eval`, `setTimeout`, `fetch`,
`globalThis.process`.
- Blocks every Function-constructor escape vector found during the
drive-security audit (`.constructor.constructor('return process')`
from `module`, `exports`, `Function`, `{}`, `Promise.resolve(0)`).
- Blocks `WebAssembly.Module` construction.
- Init-error codes match what the host expects
(extension_invalid_export, extension_unsupported_api_version,
extension_no_variables, extension_variable_missing_id,
extension_editor_incomplete).
- Metadata extraction returns the expected shape.
- Happy path: getValue routes a call + returns the value; thrown
errors propagate as `error` messages.
worker-manager.test.ts (10 tests)
Stubs `WorkerProvider` / `UnifiedWorker` to drive the manager's wire
handling without spawning real workers. Covers:
- load → init → variable surface populated.
- load init-error rejects with the worker's code.
- variableGetValue routes the call message + resolves with the
worker's result.
- Worker error messages reject with a Squawk shape.
- Unknown type throws `unknown_registered_extension`.
- `getAssetRef` on non-binary variables short-circuits to null
without posting to the worker.
- `unload` terminates + rejects in-flight calls with
`extension_terminated`.
- Worker uncaught error rejects pendingCalls fast with
`extension_worker_error` (was the fix in 5a45f6e).
- `log` messages route through the host's log callback.
- `parse-value-sections` round-trips: worker → host callback →
`parse-value-sections-result` back to the worker.
Replaced `.at(-1)` with `[arr.length - 1]` to avoid the ES lib
mismatch in the tsgo typecheck.
Suite goes from 99/99 → 120/120 in @beak/runtime-shared.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Implements ADR-0003. Drops
isolated-vmentirely; both Electron and Web hosts now run extensionsin a Worker (Node
worker_threadsand Web Worker, respectively),driven by one shared
WORKER_SOURCE+WorkerExtensionManagerin@beak/runtime-shared/extensions.Why now: PR #672 surfaced macOS notarization rejecting the unsigned
.nodeprebuilds + the strayisolated-vm-X.Y.Z.tgzshipping insideapp.asar.unpacked. The tactical fix was anafterPackprune; thisPR is the strategic fix — stop shipping native code for the extension
sandbox in the first place.
Highlights
packages/runtime-shared/src/extensions/worker-source.ts. Web style(
self.addEventListener('message', …), top-levelpostMessage).A small shim aliases those onto
parentPortforworker_threads.WorkerExtensionManager(new) handles every lifecycle bit thatused to live twice: load, unload, resetProject, the call-router
with timeouts, the recursive
parse-value-sectionsbridge, errorserialisation, log forwarding. Parameterised on a tiny
WorkerProviderand two callbacks; both hosts subclass it.isolated-vmglue to ~120 lines of Worker adapter + IPC routing.
isolated-vmremoved fromapps-host/electron/package.json,along with its
electron-rebuild -f -w isolated-vmpostinstall andthe
external: ['…', 'isolated-vm']esbuild entry. No more nativerebuild on cold install, no more unsigned
.nodefiles in the macOSbundle.
Sandbox tightness — matches
isolated-vmon ElectronThe first cut of this PR left Node
worker_threadsrunning extensioncode with the worker's full
globalThisin scope. That regressed theisolation guarantee
isolated-vmhad given us. Fixed in3119c01b:The Electron runtime shim now builds a
vm.createContextwith ahand-curated ECMAScript-only global (Object/Array/Promise/Math/Date/
JSON/RegExp, the typed-array family, Reflect/Proxy/Intl, Error and
subclasses, parse/encode/decode helpers, plus
Function— sandboxedby the vm context).
codeGeneration: { strings: false, wasm: false }blocks
eval, the Function-from-string constructor, and dynamic WASM.Deliberately omitted (security):
process,Buffer,require,dynamic
import(),fetch,console,setTimeout/setInterval,URL,SharedArrayBuffer,Atomics,WebAssembly. Extensions getthe language plus
extCtx— full stop. Identical posture to the oldisolated-vmsetup.Web Workers don't have a
vm-style context API, so the web sideremains at the looser baseline (
new Functioninside the worker'sglobalThis). Tightening it properly needs Realms (Stage 3, not widely
shipped) or iframe sandboxes — out of scope for this PR; tracked as
follow-up in ADR-0003 §3.
Memory isolation in both runtimes is identical to
isolated-vm—each worker is its own V8 isolate with its own heap.
SDK contract
@getbeak/extension-sdkis unchanged. Extensions written againstdefineExtension({ apiVersion: 1, variables: [...] })keep working;the validation shape inside the worker is identical.
Test plan
pnpm lintexit 0pnpm typecheck:apps-hostexit 0 across every workspacepnpm buildexit 0 across every workspacegetValue+ recursiveparseValueSections— needs hands-on (no automated extension testfixture in the repo yet)
process/require/
import('node:fs')/fetchthrows ReferenceError or failscleanly (the
vmcontext should block all of these)isolated-vmartefacts (CI will tell us)