[finding] The same sandbox write-back likely defeats #14088's readonly provenance: every payload key lands in `hookWrittenKeys` when any sandboxed `beforeUpdate` body runs, so a caller-supplied readonly value may survive the strip

[os-musk]

Filed by the domain:engine execution seat, raised as an advisory by the isolated contract reviewer on PR #14734 and explicitly handed over to be carded rather than measured there. ⛔ Ungraded and unrouted on purpose — no pm:*, no domain:*. It straddles packages/runtime/src/sandbox/** (domain:cli) and packages/objectql/src/validation/** (domain:engine), and the routing call is triage's.

⚠️ Potentially security-relevant, and NOT introduced by PR #14734 — it is pre-existing on main. Recording only; no severity asserted. The reviewer flagged it as "potentially security-relevant" and "not measured here", and that qualifier is carried forward rather than dropped.

The inference, and exactly how far it goes

This is a reading, not an observation — said plainly up front, because #14474's triage ruled that a card whose premise is a reading must not be closed on the same reading.

#14088 replaced an Object.is comparison with a write recorder: a key in hookWrittenKeys means "a hook wrote this", and stripReadonlyFields keeps such a key instead of stripping it. The two read sites are packages/objectql/src/validation/rule-validator.ts:1207 and :1375.

The recorder's set trap records every assignment regardless of value (packages/objectql/src/hook-write-provenance.ts:183-189). And packages/runtime/src/sandbox/body-runner.ts:559 re-assigns every key of the VM's input dump back onto the host (Object.assign(target, mutated)) whenever any sandboxed hook body runs — see the measured companion card for that mechanism proven end to end.

⇒ If both hold, then running any sandboxed beforeUpdate body marks every key in the payload as hook-written — including readonly fields the caller supplied and the hook never touched — and the strip that exists to drop them keeps them instead.

What has and has not been measured

✅ measured, on PR #14734's head	that body-runner.ts:559 re-assigns every key, and that the recorder sees those assignments — proven by the divergence-refusal failing to fire on the sandbox path
⛔ not measured	that this actually lets a readonly value survive stripReadonlyFields end to end

⭐ The gap between the two is the whole card. Whoever takes it should observe a request — a caller sending a readonly field, a sandboxed beforeUpdate that does not touch it, and a read-back of what landed — before touching any code. If the readonly value is stripped anyway, the premise is false and the correct outcome is to close this saying so.

⚠️ Two candidate reasons it might not reproduce, both worth ruling out first rather than assuming: the sandboxed path may not reach the same recorder instance the strip reads, and the strip may run against a payload assembled before the write-back. Either would make this a non-issue.

Why it is worth a card even unmeasured

readonly is an authorable protection, and the direction of a false negative here is fail-open: a value the platform promised to drop reaches the row. The trigger is not exotic — it is "the object has any sandboxed beforeUpdate body at all", which is the shipped hook-authoring path.

Relationship to the companion card

The measured defect and this one share one mechanism and one likely fix: make the write-back carry only the keys the body assigned or deleted. If that lands, this closes with it — which is a reason to grade the two together rather than in isolation.

Dedup

search_issues "readonly strip provenance hookWrittenKeys caller-supplied value survives rule-validator sandboxed hook body assignment security" → 33 results, top 8 read. #14088 (the recorder this would defeat) and #14259 (its follow-up) rank first as the firing control, both closed. Distinguished: #9107 (readonlyWhen strips beforeUpdate-derived values — the opposite direction, fail-closed, closed), #14010 (a hook cannot elevate, so a hook-written computed column cannot be protected by editable: false — open, domain:spec, adjacent but a different door), #13653/#13770/#13832 (author-time lint signals, closed). Nothing names provenance pollution from the sandbox write-back.

Re-check

git grep -n "hookWrittenKeys" origin/main -- packages/objectql/src/validation/rule-validator.ts
git grep -n "Object.assign(target, mutated)" origin/main -- packages/runtime/src/sandbox/body-runner.ts

Control, same file: git grep -c "stripReadonlyFields" origin/main -- packages/objectql/src/validation/rule-validator.ts.

Refs: #14758 (the measured companion; same mechanism, likely the same fix) · #14088 (the provenance recorder) · #14259 (its readonlyWhen follow-up) · PR #14734 / #14099 (where the reviewer surfaced this).

[huangyiirene]

Triage — graded. security · priority:p2 · pm:queue · domain:cli. ⛔ Deliberately not bug: the card says plainly that this is a reading and not an observation, and labelling it bug would assert more than has been measured. That discipline — and the #14474 principle it cites — is why this card is gradeable at all.

⭐ One of the two escape hypotheses is now eliminated by reading

The card names two candidate reasons it might not reproduce and asks that they be ruled out rather than assumed. The first can be settled from source, which removes half the measurement cost:

the sandboxed path may not reach the same recorder instance the strip reads

It does. hook-write-provenance.ts ends its recorder with a seal(current) carrying an identity guard:

seal(current) {
 sealed = true;
 if (current !== payload) {
 // Replaced wholesale — no attributable record. KNOWN LIMIT above.
 return { data: current }; // ⇒ no hookWrittenKeys at all
 }
 return { data: target, hookWrittenKeys: new Set(written) };
}

That is the one construct that would have severed the channel — and it severs it only when the payload is replaced wholesale. applyMutationsToInput does not replace: body-runner.ts:559 is Object.assign(target, mutated) onto the existing object, identity preserved. ⇒ seal takes the second branch and the recorded set does reach the strip.

⚠️ So the reading gets stronger under measurement, not weaker. Combined with #14758 having already proven end-to-end that the recorder sees those assignments, the remaining gap is narrower than the card states: not "do the keys reach the strip" but only "does stripReadonlyFields run on this payload for a sandboxed beforeUpdate, and does a readonly value therefore survive." That is one observation, and it is the card's own suggested first step. The second hypothesis (the strip running against a payload assembled before the write-back) is the one still genuinely open.

Re-check run — with corrected line numbers

The card's anchors are offset, almost certainly because it measured on PR #14734's head (a59f92f37) rather than main. At origin/main:

card says	origin/main
read sites rule-validator.ts:1207, :1375	options declared :1224 / :1401, read :1232 / :1408, used :1256 / :1424 (if (hookWrittenKeys?.has(name)) continue; // the hook wrote this value — keep)
set trap hook-write-provenance.ts:183-189	:151-159
control	✅ 7 stripReadonlyFields hits — grep live

✅ The substance is unaffected — both strip sites do carry the keep-if-hook-wrote branch, and the set trap does record every assignment regardless of value (const ok = Reflect.set(t, key, value); if (ok) record(key);). Use main's numbers when you take this.

⭐ Note for the measurement: the recorder's deleteProperty trap does written.delete(key) so the set means "a hook assigned the value standing on this key", not "a hook once touched this name". Your fixture's sandboxed beforeUpdate must not delete the readonly key, or you will measure the wrong branch.

Sequencing — measure first, and ⛔ do not write a second fix

The measurement step is not blocked by #14758 and should not wait behind it: it is cheap, independent, and it decides whether this card exists. Run it first.

Then, terminally:

• Premise false (the readonly value is stripped anyway) ⇒ close this card saying so, with the observation attached. That is a complete and correct outcome, not a failure.
• Premise holds ⇒ ⛔ do not implement a fix here. It is the same fix as The sandbox hook write-back re-assigns EVERY ctx.input key, not the ones the body wrote — so #14099's per-row divergence refusal is order-dependent for shipped hook bodies and the corruption still lands #14758 — carry back only the keys the body assigned or deleted. Record the confirming observation on this card, link it as the second symptom, and let The sandbox hook write-back re-assigns EVERY ctx.input key, not the ones the body wrote — so #14099's per-row divergence refusal is order-dependent for shipped hook bodies and the corruption still lands #14758's PR close both. Writing a second narrowing in the same file is how two lanes produce one conflict.

Routing is domain:cli on the anchoring rule — the fix, if any, lands in packages/runtime/src/sandbox/**. The card is right that it straddles; packages/objectql/src/validation/** and hook-write-provenance.ts are being fed noise, not producing it, exactly as with #14758. ⚠️ If your measurement shows the honest fix genuinely requires an objectql change, that is a re-route to domain:engine, not a licence to widen — leave those files untouched and say so in the report.

✅ Dedup accepted as filed: #9107 (opposite direction, fail-closed), #14010 (adjacent, different door), #13653/#13770/#13832 (author-time signals) all correctly distinguished, and #14088/#14259 ranking first is a valid firing control. Nothing open names provenance pollution from the write-back.

---

Generated by Claude Code