Skip to content

fix(runtime): a sandboxed hook body no longer launders an untouched readonly field onto the row - #15411

Merged
os-litant merged 3 commits into
mainfrom
claude/issue-14760-readonly-provenance-pollution
Sep 4, 2026
Merged

fix(runtime): a sandboxed hook body no longer launders an untouched readonly field onto the row#15411
os-litant merged 3 commits into
mainfrom
claude/issue-14760-readonly-provenance-pollution

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Fixes #14760

A sandboxed hook body made the engine believe it had written payload keys it never named, and a caller-supplied readonly field then survived the readonly strip and landed on the row.

The defect, measured

carriedInputKeys in packages/runtime/src/sandbox/body-runner.ts decides which keys of the VM's exit dump the write-back should re-assert. Leg 2 answers "did the body write THROUGH this object-valued key?" by comparing the host entry snapshot against the dump — but the dump has been through JSON.stringify on the way in and JSON.parse on the way out, and the snapshot had not. A host Date therefore never compared equal to its own ISO projection, took sameJsonValue's documented "cannot prove equal ⇒ carried" path, and was assigned onto engineCtx.input, which is the flat-input Proxy the #14088 provenance recorder watches. The key entered hookWrittenKeys, and stripReadonlyFields keeps what a hook wrote.

Measured end to end on main before this change, by-id path:

seeded   locked_at = 2020-01-01T00:00:00.000Z   locked_note = 'SEEDED'
caller   locked_at = new Date('2099-12-31T23:59:59.000Z')   locked_note = 'CALLER'
body     ctx.input.touched_by = 'hook'          (names no readonly key)
row      locked_at   = 2099-12-31T23:59:59.000Z  <- the CALLER's value
         locked_note = 'SEEDED'                  <- correctly stripped

The readonly TEXT field stripped in the same run is the control that makes it conclusive: the strip is present and working, and the survival is specific to that one comparison. Both strip sites are affected (by-id and predicate/multi), and an in-process hook that writes the payload does not carry it, which isolates the cause to the sandbox write-back rather than "any hook at all".

The class is wider than Date: it is every object-valued entry value a JSON round-trip cannot prove equal. A readonly json value carrying an undefined member survived too; a plainly round-trippable object of the same shape was stripped.

The change

One line in leg 2: normalise the entry value through the same round-trip the VM saw before comparing it, via a new local jsonSeenByVm helper. The comparison now asks "did the body write through this?" instead of "is this host value already JSON?".

The same change ends a second harm on the same line. A carried key is re-asserted from the dump, so an untouched host Date used to reach the driver as an ISO string and an untouched object used to lose its undefined member even where nothing was readonly. An untouched key is no longer carried at all, so the host simply keeps its own value.

The fail-open is preserved, per key. #14758 chose "anything we cannot prove equal is reported as changed and therefore carried" deliberately, and this does not reverse that choice — it stops applying it to values the round-trip can evaluate. safeJsonStringify lets a cyclic or bigint-bearing value cross into the VM in degraded form, so such a key is in the exit dump and does reach the comparison, where a plain round-trip of the host value throws. It is still carried, exactly as before, and one unrepresentable value cannot decide the whole set.

Three docblocks that this change makes false are corrected in the same diff — sameJsonValue's declared fail-safe named the Date behaviour verbatim, and carriedInputKeys' leg-2 paragraph and applyMutationsToInput's third bullet both described the un-normalised comparison.

Nothing in packages/objectql changes: hook-write-provenance.ts and rule-validator.ts are fed this noise, not producing it.

The pin

hook-input-writeback-readonly-provenance.integration.test.ts, 8 cases. Five drive real ObjectQL + real SqlDriver (better-sqlite3) + real QuickJSScriptRunner behind hookBodyRunnerFactory; seeding goes through driver.create so the insert-side strip cannot empty the measured columns and make every case pass vacuously.

Three of them are controls, because "the readonly value was stripped" also passes a write-back that has stopped working entirely:

  • the firing control — a body that does assign the readonly key still lands its write, so provenance can still say KEEP;
  • the write-THROUGH control — a body that mutates a readonly json value in place trips no trap on ctx.input, so only leg 2 can carry it; it must still be carried and still land;
  • in every case, the body's own write and the caller's writable key both land.

Three more read the same line at the write-back boundary, where a driver column would normalise the evidence away: an untouched host Date and an untouched object keep their identity, and a cyclic entry value is still carried.

Verification

All exit codes captured before any pipe; gate results quoted from each gate's own verdict line. Union run at 5f4a3abab1, the final commit.

  • pnpm --filter @objectstack/runtime test225 files / 3216 tests passed.
  • pnpm --filter @objectstack/runtime typecheck — exit 0. check:test-typecheck: OK, ledger unchanged at 27 files / 191 errors / 69 pinned signatures. The package's tsconfig.json excludes **/*.test.ts, so that leg is what actually reads the new pin — confirmed with --listFiles: the new file and body-runner.ts are both in the program.
  • pnpm lint (eslint . --no-inline-config, whole repo) — exit 0. No narrowing claimed.
  • Gate union derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack from the merge base, on a clean tree at final HEAD: 42 commands, 40 exit 0. Two are NOT MEASURED rather than green — check:dual-build-cjs-loads and check:type-check-debt both exit 3 PREREQUISITE NOT MET (they read built output for every package; CI builds the workspace). The five roster gates whose baseline sits under a directory these paths touch were run rather than read as silent: check-changeset-fixed, check:authz-resolver, check:error-code-casing, check:filter-alias-parity, check:swallow-census-controls — all exit 0.

Ablation — both legs, mutation and restore proved on disk

The subject is imported relatively (./body-runner.js), so vitest reads the source and no dist rebuild is in the loop; the red below is itself the proof the run read the mutated bytes. Each leg proved its mutation landed by grepping the injected marker and the removed text and by comparing git hash-object against the HEAD blob, restored via git checkout HEAD -- ABSOLUTE_PATH from a trap, and proved the restore by blob equality plus an empty git diff HEAD.

Leg A — drop the normalisation, compare the raw host value again. Predicted before running: the three "does not land" cases plus the two fidelity cases go red; the firing control, the write-through control, the fail-open case and the whole #14758 sibling stay green. Observed exactly that — 5 failed / 7 passed, with the by-id case reporting expected '2099-12-31T23:59:59.000Z' to be '2020-01-01T00:00:00.000Z', which is the originally measured defect reproduced.

Leg B — drop the try/catch in jsonSeenByVm. Predicted: only the fail-open case goes red. Observed exactly that — 1 failed / 11 passed, TypeError: Converting circular structure to JSON through jsonSeenByVm to carriedInputKeys to applyMutationsToInput to boundBodyHandler. The guard is load-bearing.

An earlier attempt at leg A is recorded as void rather than quietly re-run: it executed with the wrong working directory and never reached vitest (Command "vitest" not found, exit 254). That is NOT MEASURED, not a result.

The whole phase-1 table, re-measured

Every case from the measurement that scoped this card was re-run against the built dist after the change. A, E and G — the three that landed the caller's value — no longer do. B, C, F and H are unchanged. D (a body that assigns the readonly key) still lands the hook's value, and I and J (#14088's idempotent-write case, driven through QuickJS) still land theirs, so nothing legitimately accepted became refused.


🤖 Generated with Claude Code

https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N


Generated by Claude Code

…e-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
… 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
@github-actions github-actions Bot added the size/l label Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 24 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json a12b15e3947bae48165580d89a74464f17cf4702packageMentionDocs.

Which tree this was computed on

This run read content/docs from 011faa45e04f4cc52bb60f823044a79eb20e8d77 — the merge of head 5f4a3abab1a7a4c47347718608c2dbd4f1dd59c0 into base a12b15e3947bae48165580d89a74464f17cf4702, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 011faa45e04f4cc52bb60f823044a79eb20e8d77 && git checkout 011faa45e04f4cc52bb60f823044a79eb20e8d77
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin a12b15e3947bae48165580d89a74464f17cf4702 5f4a3abab1a7a4c47347718608c2dbd4f1dd59c0 && git checkout -B drift-repro a12b15e3947bae48165580d89a74464f17cf4702 && git merge --no-ff 5f4a3abab1a7a4c47347718608c2dbd4f1dd59c0

node scripts/docs-audit/affected-docs.mjs --json a12b15e3947bae48165580d89a74464f17cf4702

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants