Skip to content

Commit b003cf2

Browse files
claude[bot]claude
andauthored
Refuse an undeclared field a before-hook writes — the post-hook half of the declared-field door, one envelope on every driver (#13864)
* wip(objectql): post-hook declared-field door on insert and update (#13657) * test(objectql): pin the post-hook declared-field door across three driver flavours (#13657) * test(objectql): tighten three fixtures whose hooks wrote fields their objects never declared (#13657) * test(runtime): repoint the L2 body driver-split pin at the convergence #13657 creates * docs,lint: the L2 body undeclared-key answer is one envelope on every driver (#13657) * chore: changeset for the post-hook declared-field door (#13657) * lint: keep the tracker id out of the runtime message string (#13657) * test(objectql): drop an unused parameter from the driver double's refuse helper TEST_DEBT for @objectstack/objectql went 251 -> 252 in CI. The +1 is one TS6133 ('data' is declared but its value is never read) in the new engine-post-hook-undeclared-field.test.ts: the refuse helper took the payload it never reads, because the SQL arm quotes a fixed value in its statement string on purpose — that string reproduces the shape driver-sql used to leak, not the double's own payload. Fixed at the source rather than by raising the entry: the ledger is shrink-only and the gate names raising it maintainer-only. objectql's own typecheck excludes **/*.test.ts, which is why the package's local green said nothing about this file and the debt ledger is what caught it. * chore(gates): ratchet the query-options-erasure test surface DOWN, 240 -> 236 The ceiling is now above reality, and a ceiling left above reality silently licenses that many new erasures. Written by the gate's own prescribed `pnpm check:query-options-erasure --update`. The four sites left with the rewrite of undeclared-field-write-driver-split.integration.test.ts (its two divergence arms became one it.each over both families) and the three fixture repairs that went with it. The diff is one line — testSurface.sites 240 -> 236; the 67 non-test sites across 17 files are untouched, and nothing anywhere is raised. * docs(permissions): re-anchor the system-context census after this branch's engine.ts insertions The page anchors ~145 citations by file:line; the two pure insertions this branch makes in packages/objectql/src/engine.ts (+67 at old 9491, +28 at old 10469) shifted eight cited lines by +67/+95. Mechanical repair via `node scripts/check-system-context-census.mjs --fix` — line numbers only, same semantic sites (the engine.ts diff is insertion-only, so the old->new line map is exact). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L * docs(permissions): re-derive the system-context census anchors after the main merge The merge of origin/main hit `content/docs/permissions/system-context.mdx`, which `.gitattributes` routes `merge=os-regen`. That driver does not text-merge: it defers and leaves git's pre-filled OURS content standing, so the merge commit took the branch blob byte-identically and dropped all 39 anchor values main had re-pointed in the merge window. Re-derived with `node scripts/check-system-context-census.mjs --fix` against the merged tree, which is the only thing that can reach the correct values: they sit on NEITHER side. Every anchor now equals base + main's delta + this branch's delta (e.g. row 18: 10712 -> 10787 on main, -> 10807 on the branch, -> 10882 merged). Prose is untouched; a line-number-normalised compare of both sides is byte-identical, so nothing was chosen between. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L * docs(permissions): re-derive the census anchors after the second main merge main landed 18 further commits while this round ran, two of which moved the census page again. The os-regen driver deferred a second time and the merge took OURS byte-identically, dropping main's 5 re-pointed anchor lines; re-derived from the merged tree with `check-system-context-census.mjs --fix` (11 anchors). Prose is again byte-identical between the two sides once line numbers are normalised, so nothing was chosen between. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L * chore(changeset): regrade objectql to minor with the argued BREAKING banner Contract review on PR #13864 ruled the changeset grade dishonest: the file grades '@objectstack/objectql': patch for a change its own text calls a narrowing and a security fix. Regrade to minor, add the BREAKING accept-set banner and the argued bump level in the house form, and answer the ADR-0087 ledger question the banner triggers. '@objectstack/lint': patch is unchanged — that half of the diff is message and comment prose only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01489YWhZEoHT9oXshiyywQy * docs(permissions): re-derive the census anchors from the merged tree The os-regen merge driver deferred on content/docs/permissions/system-context.mdx and git kept OURS byte-identically, dropping main's re-point of row 21 (metadata-protocol/src/protocol.ts:1736 -> :1737). Re-derived with check-system-context-census.mjs --fix against the merged tree; no anchor hand-edited, no row added or removed (405 lines and 65 rows before and after). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01489YWhZEoHT9oXshiyywQy --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8ebda70 commit b003cf2

11 files changed

Lines changed: 762 additions & 106 deletions
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
---
2+
'@objectstack/objectql': minor
3+
'@objectstack/lint': patch
4+
---
5+
6+
Refuse an undeclared field a `before*` hook writes, identically on every driver
7+
8+
**BREAKING** accept-set narrowing at the post-hook write door, shipped as `minor`
9+
under the repo's launch-window convention for breaking changes.
10+
11+
**Bump level, argued**: `@objectstack/objectql` is `minor`, not `patch`. A
12+
`before*` hook or an L2 (`language:'js'`) body writing a key the object never
13+
declares **used to succeed** on the `memory` family — the value reached the
14+
store and persisted as a shadow column — and now **throws**, `INVALID_FIELD` /
15+
**400**, on every driver. That is a narrowing of the accept set on the record
16+
payload, a surface every hook body touches; it is not an instrument or a message
17+
fix, and a hook that relied on either driver-dependent outcome stops working at
18+
run time. The same-package sibling `.changeset/hook-input-symbol-key-refusal.md`
19+
argues exactly this shape — "used to succeed, and now throw. That is a narrowing
20+
of the accept set" — to `minor`, and the launch-window convention is what keeps
21+
it off `major` (pre-1.0 lockstep semantics: a breaking change does not burn a
22+
major version while the stack versions in lockstep — see
23+
`scripts/check-changeset-no-major.mjs`). `patch` would under-declare a change
24+
that turns a passing hook into a throwing one.
25+
26+
`'@objectstack/lint': patch` is deliberate and stays. That half of the diff is
27+
message and comment prose only: `validateHookBodyWrites` reports the same
28+
findings on the same bodies at the same severity, with wording that now names
29+
the runtime refusal instead of the driver split this change retires.
30+
31+
The declared-field door (#8682 on insert, #8738 on update) runs before the
32+
`before*` hooks — deliberately, so a payload about to be refused never consumes
33+
an autonumber (#8737). That left the payload the hooks themselves produce
34+
unjudged: a key a `beforeInsert` / `beforeUpdate` hook or an L2 (`language:'js'`)
35+
body wrote went straight to the driver, and the drivers disagreed. `memory`
36+
accepted it and stored a shadow column; `driver-sql` threw a raw `SQLITE_ERROR`
37+
with no `status` and the bound statement and its values quoted back in the
38+
message; `sqlite-wasm` threw a bare `Error` with neither. One app and one hook
39+
meant different things on two deployments, and nothing in the app could tell
40+
which one it was running on.
41+
42+
The same check now runs a second time over the post-hook payload, before any
43+
statement is built, so a hook-written undeclared key is refused with the caller
44+
path's envelope — `INVALID_FIELD` / **400**, `Unknown field 'x' on object 'y'`
45+
on every driver, because none of them is reached. The existing pre-hook door is
46+
unchanged and stays exactly where it is.
47+
48+
This is a security fix as well as a consistency one: `fieldPermissions` is keyed
49+
by declared field name and reports only fields explicitly marked non-editable, so
50+
a key the object never declares can carry no entry and could never be gated by
51+
field-level security. On `memory`-family stores such a value was persisted where
52+
no view, formula, index or permission could name it.
53+
54+
The platform's own stamps are unaffected. `created_at` / `updated_at` — the two
55+
the built-in audit hook writes unconditionally, because SQL drivers create them
56+
as built-in columns on every table — are already tolerated by this check
57+
alongside `id`; every other stamp (`created_by`, `updated_by`, `tenant_id`) is
58+
guarded by an explicit declaration test in the hook that writes it.
59+
60+
<!-- adr-0087: not-required (no-migration-prescription) No metadata key, spec symbol, Zod schema, object definition or stored representation is added, removed or renamed. This narrows which run-time record payload the engine accepts after the `before*` hooks have run; an undeclared key was never a declarable metadata surface, so `objectstack migrate meta` has nothing in a stored source to rewrite. The remedy for an affected hook body is to declare the field on the object or stop writing it, which is authoring guidance, not a mechanical rewrite of stored metadata. -->
61+

content/docs/automation/hook-bodies.mdx

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -204,19 +204,22 @@ A structured `writes` declaration was considered and dropped ([#3700](https://gi
204204

205205
#### What still happens at runtime
206206

207-
An unknown field is **not** caught at runtime, and it does not fail quietly either. The write-path validator walks the object's *declared* fields, so an undeclared key is neither rejected nor stripped, and the sandbox's mutations are copied back onto the payload verbatim. What happens next is the driver's call:
207+
An unknown field **is** caught at runtime, and the answer is the same on every driver. The sandbox's mutations are copied back onto the payload verbatim (`applyMutationsToInput` is a plain `Object.assign`) and the write-path validator still walks only the object's *declared* fields — but since [#13657](https://github.com/objectstack-ai/objectstack/issues/13657) the declared-field door runs a **second** time, over the payload the `before*` hooks produced, before any statement is built:
208208

209-
- **SQL drivers** put the stray column into the statement, so the **whole write fails** with a driver-level error (`table deal has no column named stagee`) — nothing is stored, and the error surfaces far from the authoring mistake.
210-
- **Schemaless drivers** (memory, MongoDB) silently persist the stray key alongside the real ones.
209+
```
210+
INVALID_FIELD / 400 / Unknown field 'stagee' on object 'deal'
211+
```
212+
213+
Identical on `memory`, `driver-sql` and `sqlite-wasm`, because none of them is reached. Before #13657 the driver decided instead, and the two families disagreed — SQL failed the whole write with an untyped `SQLITE_ERROR`, while schemaless drivers silently persisted the stray key as a column nothing downstream reads (and which field-level security, keyed by *declared* field name, could never gate). One app, one body, two meanings decided by which driver a deployment happened to run.
211214

212-
Neither outcome is the one you wanted, and the advisory warning is the earliest signal you get.
215+
The runtime refusal is now the backstop; the advisory warning is still the earliest signal you get, and the one that names the mistake where it was made.
213216

214217
Because the existence check is advisory, and every write-side check here is literal-only:
215218

216219
- **Treat `hook-body-write-unknown-field` as a build failure by convention.** It does not gate, but the rule is tuned for near-zero false positives — in practice a warning is a real typo.
217220
- **Check by hand what the parser cannot see.** Computed keys, spreads, aliased input and dynamic object names are invisible to the rule; for an array or `"*"` hook, every field must exist on every target.
218221
- **Prefer a flow `update_record` node when the write set is fixed — and for *this* check most of all.** A flow node's writes are structured config: they diff field-by-field, render in the Console designer, and since [#4271](https://github.com/objectstack-ai/objectstack/issues/4271) the field-existence check gates there too — `flow-node-write-unknown-field` is an **error**, not the advisory warning a body gets, because a node's `fields` is a literal map next to a literal `objectName`: there is no parser in between that could have mis-extracted it, so a finding is a certainty rather than a best effort. (The *writability* check now has a hook-side counterpart — see [Writing a `readonly` field](#writing-a-readonly-field) below — but it covers only the `ctx.api` channel.)
219-
- **Exercise the hook against a real object before shipping**on SQL drivers the mistake surfaces on the first write; schemaless drivers won't tell you.
222+
- **Exercise the hook against a real object before shipping** — the mistake surfaces on the first write, identically on every driver.
220223

221224
### Signature conventions
222225

content/docs/permissions/system-context.mdx

Lines changed: 8 additions & 8 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:10787` |
113-
| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:10949` |
114-
| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9680` |
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:10882` |
113+
| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11044` |
114+
| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9747` |
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:1737` |
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:9717`, `readonly-strict-errors.ts:66` |
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:9784`, `readonly-strict-errors.ts:66` |
117117
| 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:5705` |
118118
| 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:3574`, `:3584`, `:3611` |
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` |
121121
| 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:6403` |
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:11535` |
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:11464` |
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:11630` |
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:11559` |
124124

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

@@ -180,7 +180,7 @@ a reader tracing where elevation travels needs them.
180180
| # | Site | Package | What it does |
181181
|:--|:---|:---|:---|
182182
| 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:13876` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag |
183+
| 63 | `objectql/src/engine.ts:13971` | 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:9663``9680` |
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:9730``9747` |
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` |

packages/lint/src/validate-hook-body-writes.ts

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,24 +4,30 @@
44
//
55
// An L2 body that writes a field the target object never declares —
66
// `ctx.input.amout = 0`, `ctx.api.object('deal').update({ stag: 'won' })` —
7-
// runs clean in the QuickJS sandbox and reaches the driver UNFILTERED:
7+
// runs clean in the QuickJS sandbox and reaches the write path UNFILTERED:
88
// `applyMutationsToInput` (runtime/src/sandbox/body-runner.ts) is a plain
99
// `Object.assign`, and `validateRecord` walks declared fields on insert and
10-
// `continue`s past a key with no field def on update. What happens after that
11-
// is DRIVER-DEPENDENT, and neither half is acceptable:
10+
// `continue`s past a key with no field def on update.
1211
//
13-
// • SQL — the stray column enters the knex statement and the WHOLE write
14-
// fails with a driver-level error (`table deal has no column named
15-
// stagee`). The write is lost, and the error surfaces far from the
16-
// authoring mistake that caused it.
17-
// • Schemaless (memory, MongoDB) — the driver spreads the payload, so the
18-
// stray key IS persisted: an undeclared column nothing downstream reads.
12+
// [#13657] What happens after that used to be DRIVER-DEPENDENT, and neither
13+
// half was acceptable — SQL failed the whole write with an untyped
14+
// `SQLITE_ERROR` far from the authoring mistake, while schemaless drivers
15+
// (memory, MongoDB) spread the payload and PERSISTED the stray key as a column
16+
// nothing downstream reads. #13657 closed that: the declared-field door now
17+
// runs a second time over the payload the `before*` hooks produced, so the key
18+
// is refused `INVALID_FIELD` / 400 identically on every driver, before any
19+
// statement is built.
1920
//
20-
// Either way the mistake is invisible where it is MADE — the #4001 family, if
21-
// not literally its silent-no-op shape. Both runtime outcomes are pinned by
21+
// ⚠️ That does NOT retire this rule — it changes what it is worth. The runtime
22+
// refusal arrives at WRITE time, on whichever record first exercises the
23+
// branch; this rule arrives at AUTHOR time and names the field, the object and
24+
// the body. The mistake is still invisible where it is MADE, which is the
25+
// #4001 family and the whole reason for a build-time check.
26+
//
27+
// The runtime answer is pinned by
2228
// `runtime/src/sandbox/undeclared-field-write-driver-split.integration.test.ts`
23-
// so this rule's wording cannot drift from what the runtime does; the same
24-
// split is documented in `content/docs/automation/hook-bodies.mdx`.
29+
// so this rule's wording cannot drift from what the runtime does; the same is
30+
// documented in `content/docs/automation/hook-bodies.mdx`.
2531
//
2632
// The read side (`hook.condition`, ADR-0032) and the capability surface are
2733
// statically checked; until this rule, the write side was the one blind face
@@ -817,9 +823,12 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] {
817823
path,
818824
message:
819825
`body writes '${w.field}' to its input, but ${objDesc} ${declares}. The sandboxed script runs ` +
820-
`clean and the value is copied back onto the record payload unfiltered — on a SQL driver the ` +
821-
`stray column then fails the WHOLE write with a driver-level error far from here; on a ` +
822-
`schemaless driver (memory, MongoDB) it is persisted as an undeclared key (#4271).`,
826+
// The post-hook declared-field door (#13657) is what refuses it; the
827+
// id stays in this comment rather than in the string, which reaches
828+
// authors and operators who cannot resolve a tracker number.
829+
`clean and the value is copied back onto the record payload unfiltered, so the write is then ` +
830+
`REFUSED at run time — INVALID_FIELD / 400, identically on every driver (#4271). The ` +
831+
`record is never written, and the refusal names the field far from the body that wrote it.`,
823832
hint: fixHint(w.field, unionCandidates(targetSets)),
824833
});
825834
} else {

0 commit comments

Comments
 (0)