Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions .changeset/post-hook-undeclared-field-door.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
---
'@objectstack/objectql': patch
'@objectstack/lint': patch
---

Refuse an undeclared field a `before*` hook writes, identically on every driver

The declared-field door (#8682 on insert, #8738 on update) runs before the
`before*` hooks — deliberately, so a payload about to be refused never consumes
an autonumber (#8737). That left the payload the hooks themselves produce
unjudged: a key a `beforeInsert` / `beforeUpdate` hook or an L2 (`language:'js'`)
body wrote went straight to the driver, and the drivers disagreed. `memory`
accepted it and stored a shadow column; `driver-sql` threw a raw `SQLITE_ERROR`
with no `status` and the bound statement and its values quoted back in the
message; `sqlite-wasm` threw a bare `Error` with neither. One app and one hook
meant different things on two deployments, and nothing in the app could tell
which one it was running on.

The same check now runs a second time over the post-hook payload, before any
statement is built, so a hook-written undeclared key is refused with the caller
path's envelope — `INVALID_FIELD` / **400**, `Unknown field 'x' on object 'y'` —
on every driver, because none of them is reached. The existing pre-hook door is
unchanged and stays exactly where it is.

This is a security fix as well as a consistency one: `fieldPermissions` is keyed
by declared field name and reports only fields explicitly marked non-editable, so
a key the object never declares can carry no entry and could never be gated by
field-level security. On `memory`-family stores such a value was persisted where
no view, formula, index or permission could name it.

The platform's own stamps are unaffected. `created_at` / `updated_at` — the two
the built-in audit hook writes unconditionally, because SQL drivers create them
as built-in columns on every table — are already tolerated by this check
alongside `id`; every other stamp (`created_by`, `updated_by`, `tenant_id`) is
guarded by an explicit declaration test in the hook that writes it.
13 changes: 8 additions & 5 deletions content/docs/automation/hook-bodies.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -204,19 +204,22 @@ A structured `writes` declaration was considered and dropped ([#3700](https://gi

#### What still happens at runtime

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:
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:

- **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.
- **Schemaless drivers** (memory, MongoDB) silently persist the stray key alongside the real ones.
```
INVALID_FIELD / 400 / Unknown field 'stagee' on object 'deal'
```

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.

Neither outcome is the one you wanted, and the advisory warning is the earliest signal you get.
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.

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

- **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.
- **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.
- **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.)
- **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.
- **Exercise the hook against a real object before shipping** — the mistake surfaces on the first write, identically on every driver.

### Signature conventions

Expand Down
16 changes: 8 additions & 8 deletions content/docs/permissions/system-context.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -109,18 +109,18 @@ that silently does not happen.

| # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor |
|:--|:---|:---|:---|:---|
| 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:10712` |
| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:10874` |
| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9605` |
| 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:10807` |
| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:10969` |
| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9672` |
| 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:1576` |
| 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:9642`, `readonly-strict-errors.ts:66` |
| 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:9709`, `readonly-strict-errors.ts:66` |
| 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:5639` |
| 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` |
| 25 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `system-write-guard.ts:96`, `:120` |
| 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` |
| 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:6337` |
| 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:11460` |
| 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:11389` |
| 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:11555` |
| 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:11484` |

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

Expand Down Expand Up @@ -180,7 +180,7 @@ a reader tracing where elevation travels needs them.
| # | Site | Package | What it does |
|:--|:---|:---|:---|
| 62 | `objectql/src/engine.ts:3406` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes |
| 63 | `objectql/src/engine.ts:13801` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag |
| 63 | `objectql/src/engine.ts:13896` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag |
| 64 | `plugin-reports/src/report-service.ts:556` | plugin-reports | Threads the flag into the engine call that runs a report |
| 65 | `body-runner.ts:279` | runtime | Rebuilds an `ExecutionContext` from a hook session, carrying the flag across |

Expand All @@ -195,7 +195,7 @@ assuming `isSystem` covers it is a documented source of bugs.
|:---|:---|:---|
| "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` |
| "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) |
| "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:9588`–`9605` |
| "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:9655`–`9672` |
| "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:1514` (#3493 / #6640) |
| "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` |
| "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` |
Expand Down
41 changes: 25 additions & 16 deletions packages/lint/src/validate-hook-body-writes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,30 @@
//
// An L2 body that writes a field the target object never declares —
// `ctx.input.amout = 0`, `ctx.api.object('deal').update({ stag: 'won' })` —
// runs clean in the QuickJS sandbox and reaches the driver UNFILTERED:
// runs clean in the QuickJS sandbox and reaches the write path UNFILTERED:
// `applyMutationsToInput` (runtime/src/sandbox/body-runner.ts) is a plain
// `Object.assign`, and `validateRecord` walks declared fields on insert and
// `continue`s past a key with no field def on update. What happens after that
// is DRIVER-DEPENDENT, and neither half is acceptable:
// `continue`s past a key with no field def on update.
//
// • SQL — the stray column enters the knex statement and the WHOLE write
// fails with a driver-level error (`table deal has no column named
// stagee`). The write is lost, and the error surfaces far from the
// authoring mistake that caused it.
// • Schemaless (memory, MongoDB) — the driver spreads the payload, so the
// stray key IS persisted: an undeclared column nothing downstream reads.
// [#13657] What happens after that used to be DRIVER-DEPENDENT, and neither
// half was acceptable — SQL failed the whole write with an untyped
// `SQLITE_ERROR` far from the authoring mistake, while schemaless drivers
// (memory, MongoDB) spread the payload and PERSISTED the stray key as a column
// nothing downstream reads. #13657 closed that: the declared-field door now
// runs a second time over the payload the `before*` hooks produced, so the key
// is refused `INVALID_FIELD` / 400 identically on every driver, before any
// statement is built.
//
// Either way the mistake is invisible where it is MADE — the #4001 family, if
// not literally its silent-no-op shape. Both runtime outcomes are pinned by
// ⚠️ That does NOT retire this rule — it changes what it is worth. The runtime
// refusal arrives at WRITE time, on whichever record first exercises the
// branch; this rule arrives at AUTHOR time and names the field, the object and
// the body. The mistake is still invisible where it is MADE, which is the
// #4001 family and the whole reason for a build-time check.
//
// The runtime answer is pinned by
// `runtime/src/sandbox/undeclared-field-write-driver-split.integration.test.ts`
// so this rule's wording cannot drift from what the runtime does; the same
// split is documented in `content/docs/automation/hook-bodies.mdx`.
// so this rule's wording cannot drift from what the runtime does; the same is
// documented in `content/docs/automation/hook-bodies.mdx`.
//
// The read side (`hook.condition`, ADR-0032) and the capability surface are
// statically checked; until this rule, the write side was the one blind face
Expand Down Expand Up @@ -817,9 +823,12 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] {
path,
message:
`body writes '${w.field}' to its input, but ${objDesc} ${declares}. The sandboxed script runs ` +
`clean and the value is copied back onto the record payload unfiltered — on a SQL driver the ` +
`stray column then fails the WHOLE write with a driver-level error far from here; on a ` +
`schemaless driver (memory, MongoDB) it is persisted as an undeclared key (#4271).`,
// The post-hook declared-field door (#13657) is what refuses it; the
// id stays in this comment rather than in the string, which reaches
// authors and operators who cannot resolve a tracker number.
`clean and the value is copied back onto the record payload unfiltered, so the write is then ` +
`REFUSED at run time — INVALID_FIELD / 400, identically on every driver (#4271). The ` +
`record is never written, and the refusal names the field far from the body that wrote it.`,
hint: fixHint(w.field, unionCandidates(targetSets)),
});
} else {
Expand Down
Loading
Loading