refactor(core): simplify hardened serialization and close the bound-getter reporting hole - #3288
Conversation
…onal-capture states, close the bound-getter reporting hole - Every captured intrinsic exists on all supported engines (Node 18+), so the optional-capture layer (intrinsicGetter-returns-undefined, canReadUrl/ canReadUrlSearchParams/canReadHeaders, per-use fallbacks) is replaced by captures that throw at import if absent. - URLSearchParams.prototype.size (the one genuinely missing member on Node 18) is not needed: emptiness falls out of the captured toString() result, which the reducer already computes. Node 18 now serializes URLSearchParams natively instead of falling back to devalue's default handling. - The call/get tables and their re-export aliases flatten into direct typed exports; readProxyAware and the viewInfo getter-indirection unroll into two-branch functions. - isEngineAccessor: drop the WeakMap memo and try/catch (descriptor getters are always callable); exclude bound functions, which stringify as native code but run their target — previously workflow code could launder a side-effectful getter past the report with fn.bind() (test added). - 763 -> 625 lines, byte output unchanged (parity checked for DataView and typed-array subviews on top of the existing test suite).
🦋 Changeset detectedLatest commit: d710362 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 |
🧪 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:
📈 STSO distribution vs main (inline / queue-hop histograms)1020 steps (inline) Cumulative STSO time: main 437759ms → this run 386891ms (Δ -50868ms, -12%) 1020 steps (queue-hop) Cumulative STSO time: main 8262ms → this run 8658ms (Δ +396ms, +5%) ℹ️ Metric definitions & methodologyThe collapsed STSO distribution section above buckets every step gap of the sequential-steps run (not a sampled window), split by whether the step ending the gap ran inline — in the same warm process as the step before it, so the gap is pure framework overhead — or after a queue-hop — the first step of a fresh process, which pays queue dispatch, client reinit and event-log replay. Bars overlay the two runs: 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) · 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 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 |
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Reviewed the capture-table change, the bound-getter fix, and the wire-parity claims — each against the failure modes a hardening layer can introduce. Approving.
Import-throw is safe in every context this module loads. The concern with assert-at-import is a consumer that used to limp along now crashing at boot — so I checked the import graph: hardened.ts is reached only from core's serialization modules (host/Node contexts where it already ran), and the browser surface (web-shared hydration) imports only the serialization-format revivers subpath, never this module. The node:util dependency predates this PR. And the claim that every remaining capture exists on Node 18+ holds: ES intrinsics plus Headers/URL/URLSearchParams (global since 18.0) — the one genuine gap in the old table, URLSearchParams.prototype.size (18.16+), is eliminated by design rather than papered over, which also upgrades Node 18.0–18.15 from degraded output to native URLSearchParams serialization. Root engines still includes ^18.0.0, so that's a real fix, not dead reasoning.
The emptiness-probe replacement is exactly wire-equivalent. size === 0 ⟺ toString() === '' holds with no edge cases — any entry renders at least =, zero entries always render '' — so the . sentinel contract is byte-identical on engines that took the old path, and the change is observable only where the old path didn't exist (Node <18.16, in the correctness direction, with a tag every reader already understands).
The bound-getter fix is right, and honestly scoped. Screen ordering is correct (proxy → bound -name → native-toString / host-proto), the false-positive direction is the safe one (a re-bound genuine native getter gets reported, never silently trusted), and the evasion I went looking for — name is configurable on bound functions, so guest code can rename to re-launder — is explicitly documented as costing "a missing report entry, like the other impersonation caveats," which is consistent with the module's stated non-adversarial threat model from the sandbox-hardening work. The rewritten isEngineAccessor doc now coherently enumerates all three impersonation paths in one place. The new test is the full contract in one assertion set: the getter ran (once), the value serialized, and the report captured it — closing the reporting hole without changing behavior, which is precisely the PR's claim.
The deletions I checked rather than assumed:
- Dropping the defensive try/catch is sound: descriptor
.getis callable-or-undefined by bothdefinePropertyvalidation and proxy invariants, proxies are screened beforefunctionToString, andtoStringon a callable non-proxy never throws. - The cross-realm
getPrototypeOf(getter) === Function.prototypebranch that survives the flatten looked alarming out of context (any ordinary function matches!) — but it's host-realm provenance: VM-guest functions carry the sandbox'sFunction.prototype, so only host-realm accessors match. Pre-existing, correct, and worth having re-verified since the refactor moved it. - The WeakMap memo removal trades a cache for a cheap
toStringper accessor read — noise, and it removes a cache whose key was guest-reachable.
Verified locally: serialization suite 548/548, full core 1787 (+3 pre-existing expected-fails). CI is 102/102 with zero failures. Changeset patch is right — no API surface changes, and the Node 18 delta is bug-fix-shaped.
Nice extraction, too — landing this independently of the retained-VM stack keeps #3046's review surface honest.
|
No backport to This is primarily a refactor/simplification of the hardened serialization layer, and that layer does not exist on To override, re-run the Backport to stable workflow manually via |
2 participants
Summary
Simplification + one correctness fix for the hardened serialization layer from #3257, extracted from the retained-VM stack (#3046) so it can be reviewed and land independently — it has no dependency on retention.
intrinsicGetter-returns-undefinedlayer,canReadUrl/canReadUrlSearchParams/canReadHeaders, and the per-use?./asfallbacks are replaced by captures that throw at import if absent. A missing intrinsic on a supported engine is a bug in the capture table, and failing at boot beats serializing through a live (patchable) lookup later.URLSearchParams.prototype.sizeis not needed — it was the one genuinely missing member on Node 18, and the emptiness probe it powered falls out of the capturedtoString()result the reducer already computes. Node 18 now serializesURLSearchParamsnatively instead of degrading to devalue's default handling.call/gettables and their re-export aliases flatten into direct typed exports;readProxyAwareand theviewInfogetter-indirection unroll into plain two-branch functions.fn.bind()stringifies as[native code], so workflow code could run a side-effectful getter with an empty guest-code report. The toString text is identical to real native getters; the one passive distinguisher V8 exposes is thebound-prefixednameown property, which is now checked (test added). Also drops theWeakMapmemo and the defensive try/catch (descriptor getters are always callable).hardened.ts: 763 → 625 lines. Wire output unchanged — parity verified for DataView and typed-array subviews on top of the existing suite.Validation
Full
packages/coreserialization suite: 548 passed (including the 800-linehardened.test.tsunchanged except the new bound-getter test). Full unit suite green on the retained-VM branch this was developed on.Extracted from #3046 (originally commit
6084f8f0e); #3046 and #3047 no longer carry it.