feat(core): deterministic sandbox hardening - #3045
Conversation
🦋 Changeset detectedLatest commit: f809de2 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✅ All tests passed E2E Test SummarySummary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
✅ vercel-multi-region
|
📊 Workflow Benchmarkscommit Backend:
📜 Previous results (6)db1665fFri, 24 Jul 2026 23:23:29 GMT · run logs
9abf488Fri, 24 Jul 2026 21:50:34 GMT · run logs
8e23cfaThu, 23 Jul 2026 23:42:57 GMT · run logs
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
ℹ️ Metric definitions & methodologyBest/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) · SO: stream overhead (end-to-end write+consume time beyond the modelled generation window) 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) · stream overhead (text): writer streams 300 variable-length text token deltas paced at 100/s for 3s (a haiku-size LLM's token throughput) while a parallel reader drains the whole stream; SO is the end-to-end write+consume time beyond the 3s generation window (overhead/backpressure) · stream overhead (structured): same workload as stream overhead (text), but each delta is an AI-SDK-style structured object ({ type: 'text-delta', id, text }) instead of a raw string, so the SO gap vs the text scenario is the added serialization cost 🔴 marks a percentile over its target (within target is left unmarked). Targets (p75/p90/p99, ms) — TTFS 200/300/600 · SL 50/60/125 · SO 250/500/1000 · 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.
…t, explicit subtle throwers
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Re-reviewed after the update. My blocking item is fixed, both nits are addressed, and the sandbox surface got materially stronger than what I reviewed last time. Approving.
Blocking item resolved: the changeset is now major on @workflow/core + workflow, and the description accurately enumerates the removals (Atomics.waitAsync, async WebAssembly, WeakRef, FinalizationRegistry) alongside the digest change — that's the signal a consumer needs to know a global disappeared.
Both nits addressed, and one beyond what I asked:
- The V8-wording assertion is gone; the new test asserts
/not available inside a workflow function/against the SDK's own error, so a Node bump can't break it. - The subtle proxy now explicitly blocks every non-digest method with a clear
WorkflowRuntimeErrornaming the method and pointing at step functions. Previously those methods were passed through and merely happened to reject on a V8 brand check through the proxy receiver — same practical outcome, but the failure was incidental and the message inscrutable. This makes the invariant intentional and self-documenting, and the new loop test locks inimportKey/encrypt/sign/deriveBits. - Bonus fidelity: an unrecognized algorithm name now rejects with
DOMException(..., 'NotSupportedError'), matching WebCrypto instead of a generic error.
I re-verified the causation question the stricter proxy raises: packages/core/src/encryption.ts does call crypto.subtle.importKey/encrypt/decrypt, so I checked whether any of it runs VM-side. It doesn't — workflow.ts imports it import type only (CryptoKey), the encrypt/decrypt call sites live in the host-side serialization paths, and pre-PR those methods already failed inside the sandbox anyway (the proxy returned them unbound). So encrypted workflows are unaffected; this is strictly a better error for code that could never have worked. Also re-confirmed no VM-side runtime or workbench workflow uses any of the four removed globals.
Verified locally: 34/34 vm tests, full core suite 1526 (+3 pre-existing expected-fails). The new "does not expose any API that settles on host timing" test is a good invariant guard, and keeping synchronous new WebAssembly.Module()/Instance() available while dropping the async entry points is the right line.
CI: all three failures are non-causal, and two share one remedy — merge main:
Unit Tests (windows-latest):world-testing'sinline-batches-debugtimeout — the shared-data-dir flake fixed by #3055, which merged after this branch's base and isn't present here.Benchmark (vercel, nextjs-turbopack):404 Benchmark workflow "benchSoWorkflow" not found. Neat artifact of PR staleness — Actions tests the PR merge commit (which carries main's newer bench test referencingbenchSoWorkflow) while Vercel deploys the branch head (whose workbench predates that workflow). The branch itself is self-consistent (neither its test nor its workbench mentions it), so it's not a bad merge.E2E Local Prod Tests (nextjs-turbopack - stable):webhookWorkflowtimeout, no plausible path from this diff (webhooks touch none of the removed globals; hook-token generation goes through the already-deterministiccrypto.randomUUIDoverride).
A main merge should clear the first two and re-roll the third.
|
No backport to This is intentional breaking behavior change for the next major (the changeset marks To override, re-run the Backport to stable workflow manually via |
3 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.