feat(core): deterministic sandbox hardening#3045
Conversation
🦋 Changeset detectedLatest commit: 8e23cfa The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
02636d5 to
cb05683
Compare
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests📋 Other (1 failed)e2e-vercel-prod-nest (1 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 📋 Other
✅ vercel-multi-region
❌ Some E2E test jobs failed:
Check the workflow run for details. |
📊 Workflow Benchmarkscommit Backend:
📜 Previous results (3)9395b2dThu, 23 Jul 2026 23:12:23 GMT · run logs
b908241Wed, 22 Jul 2026 17:59:42 GMT · run logs
cb05683Wed, 22 Jul 2026 02:47:52 GMT · run logs
Best/P75/P90/P99 deltas compare against the most recent benchmark run on Metrics — TTFS: time to first step body (in-deployment start() → first step body, deployment clocks) · STSO: step-to-step overhead (gap between consecutive step bodies) · WO: workflow overhead (whole-run time outside step bodies, in-deployment anchored) · SL: stream latency (in-deployment write → read propagation, readAt - writtenAt) Scenarios — step: one trivial no-op step, no stream; no hooks, so the run stays in turbo mode (in-process fast path) · stream: one streaming step; no hooks, so the run stays in turbo mode (in-process fast path) · hook + stream: registers a hook before one step, which exits turbo mode (dispatch path) · 1020 steps: 1020 trivial sequential steps; STSO is measured between consecutive steps in the given step ranges, and WO is the whole-run overhead outside step bodies · stream latency: parallel reader/writer steps on a dedicated stream; SL is the in-deployment write->read propagation (readAt - writtenAt) 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · STSO (1-20) 20/30/60 · STSO (101-120) 30/45/90 · STSO (1001-1020) 40/60/120 All metrics are measured from deployment-side timestamps only. Runs are triggered by an in-deployment route that stamps the anchor ( Cold starts are kept in the numbers on purpose — they are part of real bursty-workload latency. The workbench deployment cold-starts the |
- crypto.subtle.digest computes synchronously via node:crypto: byte-identical values, promise settles on a deterministic microtask, full BufferSource validation (internal-slot view reads, SAB rejection) - Atomics.waitAsync (a wall-clock timer), async WebAssembly compilation, WeakRef, and FinalizationRegistry are removed from the sandbox — wall clock and GC observation are unreplayable; sync WebAssembly constructors remain - freezeSerializationIntrinsics pins the universal dispatch surfaces: Object.prototype/Array.prototype/Function.prototype are frozen (every missed property read and hasInstance lookup terminates there) and serialization-referenced global bindings are non-writable. Value-type prototypes and constructor statics stay patchable so polyfills (Temporal's Date.prototype.toTemporalInstant, core-js Set.prototype .union / Object.groupBy) keep working — the retained-input gate verifies the members serialization executes per boundary instead. Groundwork for retained-VM replay (#2990).
cb05683 to
b908241
Compare
…escence The remaining async subtle methods reject immediately through the crypto proxy (brand check — the receiver is not a real SubtleCrypto), so they can never mint a host-timing promise. Narrow the quiescence comment to what the code actually enforces and add a test so the unreachability is not silently "fixed" later.
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Reviewed with the Slack thread context in mind (including the rescope that dropped prototype freezing / global pinning in favor of part-3 hardened serialization — which I think was the right call, and matches the compatibility concerns raised in the thread).
What I verified
- Sync digest correctness: the captured-intrinsic view-metadata reads (
buffer/byteOffset/byteLengthvia prototype getters +util.typesbrand checks) are exactly the right pattern — cross-realm safe, immune to shadowed own properties, and consistent with the hardened-serialization direction for part 3. Copying out of the pooledBufferallocation is a nice catch. Value parity + BufferSource contract (TypeError on non-buffers, SAB rejection, subviews) are all test-covered. - The quiescence argument: I walked the sandbox surface looking for other host-timed promise sources — remaining
crypto.subtlemethods (brand-check rejection through the proxy receiver is deterministic and test-locked), dynamicimport()(microtask rejection), injected web APIs (all sync), syncAtomics.wait(return value is deterministic in a single-agent realm — correctly left in). I found no gaps. - No internal usage: nothing in
packages/core(VM-side runtime) or the workbench fixtures usesWeakRef/FinalizationRegistry/Atomics.waitAsync/async WASM.
Blocking
One item, inline: the changeset bump type should be major, not patch (details on the comment). Trivial fix.
Non-blocking notes
- The E2E failure (
writableForwardedFromStepWorkflowonnest) is an unrelated streams test — needs a rerun before merge but isn't caused by this diff. digest({ name: 123 })/digest({})rejects withTypeError(fromname.toUpperCase()), where WebCrypto's algorithm normalization does ToString first and rejects withNotSupportedError. Trivial divergence; fine to leave.- The
/must be of type SubtleCrypto/assertion matches V8's error wording, which isn't guaranteed stable across Node majors. Consider also accepting a looser match (e.g. asserting rejection withTypeError) so a Node bump doesn't break the suite — the important invariant is "rejects deterministically before scheduling crypto work", not the message.
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| '@workflow/core': patch | |||
| 'workflow': patch | |||
There was a problem hiding this comment.
Blocking: this should be major, not patch.
Removing WeakRef, FinalizationRegistry, Atomics.waitAsync, and async WebAssembly compilation from the sandbox is a breaking change to the public workflow-globals surface (the thread itself frames it as one). On main the bump type doesn't affect beta numbering — but per the repo's versioning policy it does carry through to stable backports: as written, a backport would remove APIs from v4 in a patch release, which violates semver for anyone pinned to ~4.x.
Marking it major both signals the break correctly and forces the backport decision to be deliberate rather than auto-mergeable. (If the intent is that this ships v5-only, major also matches that: the human reviewing the auto-backport PR gets an obvious signal to close it.)
Suggested:
---
'@workflow/core': major
'workflow': major
---2 participants
Summary
First of a 3-PR stack extracted from #2990 (retained-VM replay) for easier review. This PR contains only the sandbox determinism changes — every piece is a standalone win, independent of retention:
crypto.subtle.digestcomputes synchronously vianode:crypto— byte-identical to WebCrypto (verified for SHA-1/256/384/512;createHashis stable and undeprecated through Node 26), so the promise settles at a deterministic point during replay instead of host threadpool timing. Full BufferSource contract: TypeError on non-buffer input, internal-slot view metadata reads, SharedArrayBuffer-backed views rejected.Atomics.waitAsync(a wall-clock timer via SharedArrayBuffer), asyncWebAssemblycompilation (compile/instantiate/…Streaming; the syncnew WebAssembly.Module()/Instance()constructors remain),WeakRef, andFinalizationRegistry. Wall-clock timing and GC observation are unreplayable with or without retention.No prototypes are frozen and no global bindings are pinned — workflow code can patch built-ins freely (Temporal / core-js polyfills keep working). Serialization safety is instead handled in part 3 of the stack, where serialization reads values through captured host intrinsics and reports when it had to execute workflow code.
Review focus
The user-facing semantic changes of the whole stack are isolated here: the compat question is "does deleting X break any real workflow bundle?"
Validation
pnpm --filter @workflow/coretypecheck + buildStack: this PR → #3046 (feature, primitives-gated) → #3047 (full argument-type support via taint-reporting hardened serialization). #2990 stays open as the reference implementation.