Skip to content

Commit ee32e1c

Browse files
os-litantclaude
andauthored
fix(runtime): a sandboxed hook body no longer launders an untouched readonly field onto the row (#15411)
* fix(runtime): compare the entry snapshot as the VM saw it in the write-back key set Leg 2 of `carriedInputKeys` decides "did the body write THROUGH this object-valued key?" by comparing the host entry snapshot against the VM's exit dump. The dump is JSON; the snapshot was not. A host `Date` therefore compared unequal to its own ISO projection, was carried back onto the engine's flat-input Proxy, recorded by the #14088 `set` trap as hook-written, and kept by `stripReadonlyFields` — a caller-supplied readonly field no hook ever touched landing on the row. Normalise the entry value through the same round-trip the VM saw before comparing. The #14758 fail-open is preserved per key: a value the round-trip cannot evaluate (cycle, bigint) is still reported as changed and carried. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N * test(runtime): pin that an untouched readonly key is not laundered by the sandbox write-back Eight cases over the same line. Five drive real ObjectQL + real SqlDriver + real QuickJSScriptRunner: the readonly `Date` on both strip sites, the wider `json`-with-an-undefined-member case, and two controls that stop a green from being vacuous — a body that DOES assign the readonly key still lands its write, and a body that mutates a readonly object in place is still carried by leg 2. Three read the same line at the write-back boundary, where the driver cannot normalise the evidence away: an untouched host `Date` and an untouched object keep their identity, and a cyclic entry value is still carried, which is #14758's fail-open unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 6ed4b81 commit ee32e1c

3 files changed

Lines changed: 488 additions & 19 deletions

File tree

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
---
2+
'@objectstack/runtime': patch
3+
---
4+
5+
fix(runtime): a sandboxed hook body no longer launders an untouched `readonly` field onto the row
6+
7+
A `beforeUpdate`/`beforeInsert` body running in the sandbox made the engine believe it had
8+
written payload keys it never named, and a `readonly` field the caller supplied then survived
9+
the readonly strip and landed. Measured end to end: with `locked_at` declared
10+
`{ type: 'datetime', readonly: true }` and seeded to `2020-01-01`, a caller sending
11+
`locked_at: new Date('2099-12-31…')` alongside a body whose whole source is
12+
`ctx.input.touched_by = 'hook'` stored the caller's 2099 value — while the same object's
13+
readonly `text` field was correctly stripped in the same request.
14+
15+
The cause was a comparison of unlike things. The write-back decides whether a body wrote
16+
*through* an object-valued key by comparing the host payload value against the VM's exit dump,
17+
and the dump has been through `JSON.stringify`/`JSON.parse` while the host value has not. A
18+
`Date` therefore never compared equal to its own ISO projection, took the documented
19+
"cannot prove equal ⇒ carry it back" path, and was re-asserted onto the proxy that records
20+
which keys a hook wrote. The class was every object-valued value a JSON round-trip cannot
21+
prove equal — an object carrying an `undefined` member included, a `Date` being only its most
22+
reachable member.
23+
24+
The entry value is now normalised through the same round-trip the VM saw before it is
25+
compared. The same change ends a fidelity loss on non-readonly fields: an untouched key is no
26+
longer carried at all, so a host `Date` is no longer replaced by an ISO string on its way to
27+
the driver.
28+
29+
Fail-open behaviour is unchanged for values the round-trip genuinely cannot evaluate: a cyclic
30+
or bigint-bearing payload value is still reported as changed and still carried, per key.

packages/runtime/src/sandbox/body-runner.ts

Lines changed: 68 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,45 @@ function vmVisibleEntryKeys(entryInput: unknown): string[] {
504504
return out;
505505
}
506506

507+
/**
508+
* [#14760] The entry value as the VM could actually have seen it, or `ok:
509+
* false` for a host value the round-trip cannot evaluate at all.
510+
*
511+
* Leg 2 of {@link carriedInputKeys} compares an entry snapshot against the VM's
512+
* exit dump. The dump has been through `JSON.stringify` on the way in and
513+
* `JSON.parse` on the way out; the snapshot had not. Comparing them raw asks
514+
* whether a HOST value equals its own JSON projection, which for a `Date` is
515+
* always false — so an untouched caller-supplied `Date` was reported as written
516+
* THROUGH, carried back onto the engine's flat-input Proxy, recorded by the
517+
* #14088 `set` trap as hook-written, and therefore KEPT by
518+
* `stripReadonlyFields`. Measured end to end before this fix: a readonly
519+
* `datetime` field no body ever names lands the CALLER's value on the row,
520+
* while the same run's readonly `text` field is correctly stripped. The class
521+
* is wider than `Date` — it is every object-valued entry value a round-trip
522+
* cannot prove equal, an object carrying an `undefined` member included.
523+
*
524+
* The write-back's second harm has the same one cause: a key carried by this
525+
* leg is re-asserted FROM THE DUMP, so the host `Date` reached the driver as an
526+
* ISO string and the object lost its `undefined` member even where nothing was
527+
* readonly. Normalising the comparison closes both, because an untouched key is
528+
* no longer carried at all and the host simply keeps its own value.
529+
*
530+
* ⛔ The fail-open is NOT reversed. #14758 chose "anything we cannot prove
531+
* equal is reported as changed and therefore CARRIED" deliberately, and a value
532+
* that throws here — a cycle, a bigint, a `toJSON` returning `undefined` — still
533+
* takes exactly that path. What changes is that the fail-open stops firing on
534+
* values the round-trip CAN evaluate, which is where it was never needed. The
535+
* verdict is per KEY: one unrepresentable entry value must not decide the set.
536+
*/
537+
function jsonSeenByVm(value: unknown): { ok: true; value: unknown } | { ok: false } {
538+
try {
539+
return { ok: true, value: JSON.parse(JSON.stringify(value)) as unknown };
540+
} catch {
541+
/* unrepresentable (cycle, bigint) — #14758's fail-open, for this key only */
542+
return { ok: false };
543+
}
544+
}
545+
507546
/**
508547
* [#14758] Which keys of the exit dump the write-back should re-assert, or
509548
* `undefined` to assert all of them (the pre-#14758 behaviour).
@@ -519,13 +558,17 @@ function vmVisibleEntryKeys(entryInput: unknown): string[] {
519558
* assigned and then deleted, or assigned `undefined`, is absent from the
520559
* dump and belongs to the deletion leg, not to this merge.
521560
* 2. Object-valued entry keys whose dumped value no longer matches the entry
522-
* snapshot. A body that writes THROUGH a value it read (`ctx.input.meta.x =
523-
* 1`) never trips a trap on `ctx.input`, so (1) cannot list it and dropping
524-
* it would be exactly the silent loss this card exists to end. The
525-
* comparison is confined to keys whose ENTRY value is an object because a
526-
* primitive cannot be mutated in place — every change to one is an
527-
* assignment (1) already saw — and confining it there is what keeps this
528-
* leg from re-widening into the value diff #14099's ruling refused.
561+
* snapshot **as the VM saw it** ({@link jsonSeenByVm}). A body that writes
562+
* THROUGH a value it read (`ctx.input.meta.x = 1`) never trips a trap on
563+
* `ctx.input`, so (1) cannot list it and dropping it would be exactly the
564+
* silent loss this card exists to end. The comparison is confined to keys
565+
* whose ENTRY value is an object because a primitive cannot be mutated in
566+
* place — every change to one is an assignment (1) already saw — and
567+
* confining it there is what keeps this leg from re-widening into the value
568+
* diff #14099's ruling refused. [#14760] Normalising the entry side is what
569+
* makes the comparison answer "did the body write through this?" instead of
570+
* "is this host value already JSON?"; without it every `Date`-valued key
571+
* answered the second question, in the wrong direction, forever.
529572
*
530573
* `undefined` (no narrowing) whenever the evidence is not there: no recorder,
531574
* or no usable entry snapshot to read leg 2 from.
@@ -546,7 +589,8 @@ function carriedInputKeys(
546589
if (carried.has(key)) continue;
547590
if (!before || typeof before !== 'object') continue;
548591
if (!Object.prototype.hasOwnProperty.call(mutated, key)) continue;
549-
if (!sameJsonValue(before, mutated[key])) carried.add(key);
592+
const seen = jsonSeenByVm(before);
593+
if (!seen.ok || !sameJsonValue(seen.value, mutated[key])) carried.add(key);
550594
}
551595
return [...carried];
552596
}
@@ -557,14 +601,18 @@ function carriedInputKeys(
557601
*
558602
* Key ORDER is deliberately not significant: a JSON round-trip through the VM
559603
* preserves insertion order for string keys but reorders integer-like ones, and
560-
* a reorder is not a write. Every failure direction is the safe one — anything
561-
* this cannot prove equal is reported as changed and therefore CARRIED, which
562-
* is the pre-#14758 behaviour for that key. That covers the host values JSON
563-
* cannot represent (a `Date` arrives back as a string and compares unequal, so
564-
* it is carried exactly as it was before this card).
565-
*
566-
* Terminates on a cyclic `a`: `b` is always JSON-parsed and therefore finite,
567-
* so the walk is bounded by `b`'s depth.
604+
* a reorder is not a write.
605+
*
606+
* [#14760] BOTH sides are JSON values by the time they reach here: `b` is the
607+
* VM's exit dump, and `a` is the entry snapshot already put through
608+
* {@link jsonSeenByVm}. So this compares like for like, and it no longer stands
609+
* in for the round-trip itself. It used to: an unequal verdict meant either
610+
* "the body wrote this" or "JSON cannot represent this host value", and the
611+
* caller could not tell the two apart — which is how an untouched `Date` was
612+
* read as a write. Distinguishing them is now {@link jsonSeenByVm}'s job, and
613+
* the fail-open direction lives there with it, unchanged.
614+
*
615+
* Terminates: both sides are JSON-parsed and therefore finite.
568616
*/
569617
function sameJsonValue(a: unknown, b: unknown): boolean {
570618
if (a === b) return true;
@@ -661,9 +709,10 @@ function sameJsonValue(a: unknown, b: unknown): boolean {
661709
* itself ever fires, so the recorder cannot list `meta`. The dump is the only
662710
* witness for those, and {@link carriedInputKeys} reads it the narrowest way
663711
* available: an OBJECT-valued entry key whose dumped value no longer matches
664-
* the entry snapshot was written through, and is carried. Primitives need no
665-
* such leg — a primitive cannot be mutated in place, so every change to one
666-
* is an assignment the recorder saw.
712+
* the entry snapshot — [#14760] as {@link jsonSeenByVm} shows it to the VM —
713+
* was written through, and is carried. Primitives need no such leg: a
714+
* primitive cannot be mutated in place, so every change to one is an
715+
* assignment the recorder saw.
667716
*/
668717
function applyMutationsToInput(
669718
engineCtx: any,

0 commit comments

Comments
 (0)