Skip to content

Commit 356bd71

Browse files
claude[bot]claude
andauthored
fix(objectql): decide the read-only strip on hook-write provenance, so a hook can clear a field the caller also sent (#14258)
* fix(objectql): decide the read-only strip on hook-write provenance (#14088) `stripReadonlyFields` asked `Object.is(payload[k], supplied[k])` to answer "who wrote this key". Value equality cannot: when a hook writes the value the caller also sent, the comparison reads it as "the hook never touched it" and deletes the hook's write. Measured downstream on a `readonly` `completed_at` cleared by a reopen hook against a caller that round-tripped the record — the row committed `in_progress` carrying its old completion timestamp, silently. Record the keys the before-phase hook chain actually ASSIGNS (`recordHookPayloadWrites`), armed after the caller's entry snapshot and sealed at the post-hook confluence both update branches share, and let the strip keep a key a hook demonstrably wrote. Not a `null` sentinel: `0`, `''`, `false` and shared references collide identically. Not a relaxation: a caller-supplied read-only value no hook wrote is still stripped, and a caller cannot enter the record because echoing a value is not an assignment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5 * chore(docs): re-anchor the system-context census after the engine.ts insertion (#14088) Pure line rot: the #14088 import block shifts every subsequent engine.ts line by five, so all 15 anchors on content/docs/permissions/system-context.mdx moved. Repaired with the gate's own `--fix`; no prose changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5 * chore(docs): re-anchor the census page on the merged tree (#14088) The merge driver treats content/docs/permissions/system-context.mdx as generated and does not text-merge it, so the merge left this branch's pre-merge copy in place — which would have dropped the rows PR #14249 added on main. Took main's copy wholesale and re-derived the anchors from the MERGED tree with the repo's own tooling (pnpm gen:system-context-census). Result verified against main's copy: 12 rows differ and every difference is an engine.ts line number inside backticks; all prose is byte-identical, so #14249's rows survive intact. The 15 rewritten anchors are the same 15 this branch's engine.ts insertion shifts, and the census totals are unchanged (109 read sites / 145 anchors / 27 declared non-read), so this is line rot and not a population change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5WBDtaUnoz5XuJ6jk8pQ5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f645d6f commit 356bd71

7 files changed

Lines changed: 1025 additions & 16 deletions

File tree

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
---
2+
"@objectstack/objectql": patch
3+
---
4+
5+
fix(objectql): decide the read-only strip on hook-write PROVENANCE, so a hook can clear a field the caller also sent (#14088)
6+
7+
`stripReadonlyFields` decided whether a `readonly` field's value came from a
8+
before-phase hook or from the caller with `Object.is(payload[name],
9+
supplied[name])`. Value equality cannot carry that question, and this is it
10+
failing: when both sides hold the same value the comparison cannot separate
11+
*the hook deliberately wrote it* from *the hook never touched it*, and the two
12+
demand opposite verdicts. The hook's write was deleted along with the caller's.
13+
14+
**Measured downstream** (published 17.2.0, `duly_task`): a `readonly`
15+
`completed_at` stamped by a `beforeUpdate` hook on the transition into `done`
16+
and cleared on the transition out. Reopening a completed task works — unless
17+
the caller also sends `completed_at: null`, which is exactly what a form
18+
round-trip of the whole record does. `Object.is(null, null)` is `true`, the
19+
hook's clear is stripped, and the row commits `status = in_progress` still
20+
carrying its **old completion timestamp**, with no error. That row is the one
21+
a validation rule structurally cannot catch ("a completed task must carry a
22+
completion timestamp" has no purchase on its inverse), nothing downstream can
23+
tell it from a genuinely completed one, and every on-time metric reading
24+
`completed_at` counts it.
25+
26+
This is the second failure of one sentence, not a new defect. #5591 / #6339
27+
retired the key-SET judgement because it made the strip's own written contract
28+
— "hook-written keys are NOT caller-supplied" — true only *by accident*. Value
29+
equality is accidental in precisely the same way; `null == null` is just its
30+
most common collision.
31+
32+
**The repair is provenance, recorded rather than inferred.** A new
33+
`recordHookPayloadWrites` view is armed over the update payload after the
34+
caller's entry snapshot and sealed at the engine's post-hook confluence, where
35+
`hookContext.input.data` is final on **both** update branches. It records only
36+
the fact that an assignment executed — never the payload's contents — and
37+
`stripReadonlyFields` now keeps a key a hook demonstrably assigned. Both
38+
branches consume the one sealed record, so a bulk write and a by-id write can
39+
never reach different verdicts about who wrote a key.
40+
41+
**Not a `null` special case.** `0`, `''`, `false` and a shared object
42+
reference collide identically, and all of them are echoed back by the same
43+
whole-record write-back idiom. A sentinel fix would have left every one of them
44+
open.
45+
46+
**Not a relaxation of #2948 / #3003 / #5503.** A caller-supplied read-only
47+
value that no hook wrote is still dropped, still warns with the same text, and
48+
still reports through `onFieldsDropped` / `strictReadonlyWrites`. The
49+
discriminator is pinned in both directions: the same caller payload
50+
(`completed_at: null` over a stored timestamp) now **clears** when a hook wrote
51+
the null and is still **stripped** when no hook did. A caller cannot enter the
52+
record — echoing a key or a value is not an assignment — so no caller write can
53+
become hook-owned.
54+
55+
**Known limit, deliberately fail-safe and pinned as a test.** A hook that
56+
*replaces* the payload object (`ctx.input.data = { ...ctx.input.data, x: 1 }`)
57+
rather than mutating it leaves no attributable record, and that call falls back
58+
to the previous value comparison — i.e. it keeps the old over-strip. Reading a
59+
replacement's keys as hook-owned would launder a caller's forged `created_by`
60+
into a platform write, so the fallback direction is the only safe one. A hook
61+
that means to own a read-only column should ASSIGN to it. The pre-existing
62+
shallow-snapshot limit (a hook mutating a caller-supplied object *in place*) is
63+
unchanged for the same reason.

content/docs/permissions/system-context.mdx

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -109,18 +109,18 @@ that silently does not happen.
109109

110110
| # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor |
111111
|:--|:---|:---|:---|:---|
112-
| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:10914` |
113-
| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11076` |
114-
| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9772` |
112+
| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:10981` |
113+
| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11149` |
114+
| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9777` |
115115
| 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1746` |
116-
| 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:9809`, `readonly-strict-errors.ts:66` |
117-
| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5730` |
118-
| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3599`, `:3609`, `:3636` |
116+
| 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:9814`, `readonly-strict-errors.ts:66` |
117+
| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5735` |
118+
| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3604`, `:3614`, `:3641` |
119119
| 25 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `system-write-guard.ts:96`, `:120` |
120120
| 26 | Identity write guard bypassed (ADR-0092) | plugin-auth | Get: direct writes to identity tables through the generic data path | `identity-write-guard.ts:98` |
121-
| 27 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6428` |
122-
| 28 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:11662` |
123-
| 29 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:11591` |
121+
| 27 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6433` |
122+
| 28 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:11742` |
123+
| 29 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:11671` |
124124

125125
### 3. Sharing (`plugin-sharing`)
126126

@@ -179,8 +179,8 @@ a reader tracing where elevation travels needs them.
179179

180180
| # | Site | Package | What it does |
181181
|:--|:---|:---|:---|
182-
| 62 | `objectql/src/engine.ts:3406` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes |
183-
| 63 | `objectql/src/engine.ts:14011` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag |
182+
| 62 | `objectql/src/engine.ts:3411` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes |
183+
| 63 | `objectql/src/engine.ts:14091` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag |
184184
| 64 | `plugin-reports/src/report-service.ts:556` | plugin-reports | Threads the flag into the engine call that runs a report |
185185
| 65 | `body-runner.ts:279` | runtime | Rebuilds an `ExecutionContext` from a hook session, carrying the flag across |
186186

@@ -195,7 +195,7 @@ assuming `isSystem` covers it is a documented source of bugs.
195195
|:---|:---|:---|
196196
| "It suppresses triggers / record-change automation" | **No.** Only `skipTriggers` does. A bare `{ isSystem: true }` on a seed write re-fired automation on freshly seeded rows and wedged first boot | `metadata-protocol/src/seed-loader.ts:1909` (rationale at `:1819``1821`, #3760), `flow.zod.ts:685` |
197197
| "It skips the state machine" | **No.** That is `skipStateMachine`, carried by seed replay and by `treatAsHistorical` imports | `objectql/src/engine.ts` FSM gate; see [State Machine](/docs/protocol/objectql/state-machine) |
198-
| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:9755``9772` |
198+
| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:9760``9777` |
199199
| "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1516` (#3493 / #6640) |
200200
| "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280``281` |
201201
| "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:286` |

0 commit comments

Comments
 (0)