diff --git a/.changeset/hook-input-is-the-persist-image.md b/.changeset/hook-input-is-the-persist-image.md new file mode 100644 index 0000000000..52bcdb6727 --- /dev/null +++ b/.changeset/hook-input-is-the-persist-image.md @@ -0,0 +1,55 @@ +--- +"@objectstack/spec": minor +"@objectstack/objectql": minor +"@objectstack/plugin-auth": patch +--- + +fix(objectql)!: `beforeUpdate` receives the record the engine intends to persist, and the caller's submission travels on `ctx.submitted` (#16344) + + + +**BREAKING** — what a `beforeUpdate` handler reads on `ctx.input.data` changes. A `readonly` field the caller supplied a value for is no longer there. The hidden set is the update strip's own subject set: author-declared `readonly: true` **and** the types whose value the runtime owns end to end (`autonumber`, implicitly read-only since #5503). `readonlyWhen` locks are deliberately not hidden. + +## The defect + +On update, a value sent for a field declared `readonly: true` was correctly **not persisted** — and was still handed to the object's `beforeUpdate` hook. A hook deriving columns from the incoming record therefore derived them from a value the row would never contain, and **those derived writes persisted**, because they are the hook's own. + +Measured on a real app (17.2.0, sqlite, dev runtime) and reproduced in `packages/objectql/src/engine-readonly-hook-input.test.ts`. One `PATCH { actual_value: 380, target_value: 1, weight: 1 }` against a `readonly` `target_value`: + +``` +read back: target_value 400 weight 10 ← the strip worked + score 1.2 calc_trace "实际 380 / 目标 1 … 权重 1%" +``` + +The row's own audit trail cites values the row does not hold. No error, no warning, 200, and `droppedFields` correctly reporting the strip the whole time — every channel said the write was fine, because by every channel's own lights it was. The only way for an application to be safe was for every hook to re-read its read-only columns and ignore the incoming record, which defeats declaring them read-only at all. + +## What changed + +**`ctx.input.data` on `beforeUpdate` is now the record the engine intends to persist.** Caller-supplied values for `readonly` fields are taken out of the hooks' view before the before phase is dispatched, and handed back at the engine's post-hook confluence — so the payload every engine-owned consumer below reads is byte-for-byte what it read before. `onFieldsDropped` reports the same fields with the same `readonly` reason, the read-only WARN says the same sentence, and `strictReadonlyWrites` refuses exactly the same writes. + +**The caller's submission travels on a new `HookContext` member, `ctx.submitted`** (`@objectstack/spec`, `HookContextSchema`) — the payload as sent, snapshotted at engine entry before any middleware or hook stamp, frozen, and documented as *diagnostics only, never the persist image*. It is bound on the update verb, both phases, and every per-row dispatch of one caller write. + +Two things deliberately did **not** move: + +- **The enforcement pass is still after the hooks.** It is the only point that can tell a hook's stamp from a caller's forgery (`hookWrittenKeys`), so a `beforeUpdate` that stamps a read-only column still lands — including when the caller echoed the same key back, which is the whole subject of #5591 / #14088. +- **`beforeInsert` is untouched.** The create side's strip position is settled post-hook by ruling C (#14147, "one semantics, one enforcement point"), and `readonlyWhen`-locked fields stay hook-writable per #9107. + +`@objectstack/plugin-auth`'s ADR-0092 identity write guard is migrated onto the new member in the same change, which is why nothing degrades: its 403 and its security warn still name the non-whitelisted field the caller sent. Without that migration the identical request answers `None of the submitted fields (—) are editable` — as strong a refusal, saying nothing about what was refused. Both readings are pinned side by side in `identity-write-guard.test.ts`. + +Ruled 2026-09-08 (maintainer, verbatim 「批 #87 同意」, director seat, decision batch #87). The refused primary was the same strip move **without** the new member: the ADR-0092 diagnostic degrades and every third-party `beforeUpdate` guard reading `ctx.input.data` degrades with it, silently. The refused alternative on the other side was documenting that hooks must read read-only columns from `ctx.previous` — which outsources the invariant to every application, the exact shape triage had already rejected. + +## Who is affected + +A `beforeUpdate` handler that **reads a `readonly` field (declared, or runtime-owned) out of `ctx.input.data`**, on a non-`isSystem` write. Three shapes, and the fix is one line each: + +- **deriving a value from it** — this is the defect; the handler now derives from `ctx.previous`, or from `ctx.input.data` with the payload's absence meaning "unchanged", which is what it always meant for a field the caller never sent. +- **reporting on what the caller sent** (a guard naming the offending key) — read `ctx.submitted`. +- **a self-assignment** (`data.x = data.x`) on such a field — this used to promote the caller's forged value to hook-owned and commit it. It is now a **no-op**: the key the hook reads is gone, so the line re-creates it holding `undefined`, and the engine treats set-to-undefined of a hidden read-only key as the no-op it is — deleting the key, dropping it from the hook-write record, and letting the ordinary hand-back put the caller's value back for the strip to judge. **The stored value stands**, and the write reports exactly as it would with no hook at all (stripped, `onFieldsDropped`, the WARN, `strictReadonlyWrites` refusing). Persisting the `undefined` instead would erase the stored value on the memory driver and hand knex an undefined binding on a SQL one — neither is the record the engine intends to persist. That laundering route closing is intended, and it is re-pinned in both directions rather than removed. + +⚠️ **The sharpest edge is a sandboxed `body` hook, and it is a refusal rather than a quiet change.** A body that reaches *through* such a key — `ctx.input.locked_meta.who = 'hook'` — now dereferences `undefined` and throws, and a `body`'s default `onError` is `abort`, so the caller's **whole write is rejected** where it used to succeed. What that body used to do was persist a value derived from the caller's forgery, so refusing is the correct direction; but the message the author sees is a raw `TypeError` from their own dereference and names nothing actionable. Measured end to end through a real QuickJS sandbox and pinned in `packages/runtime/src/sandbox/hook-input-writeback-readonly-provenance.integration.test.ts`. + +A body hook cannot read `ctx.submitted`: it is deliberately not marshalled onto the sandbox face, for the reason `dispatch.scope` is not — that face is assembled key by key, and a key added there is a second published contract with its own compatibility story. A body deriving a column from a read-only field reads **`ctx.previous`**, the stored row, which is the correct source either way. + +⚠️ **One ADR-0092 boundary changes a status code, and no in-repo object hits it today.** On an object whose UPDATE whitelist admits a field that is ALSO declared `readonly`, a whitelist-only payload now answers **403** where it used to answer **200 having written nothing**. The identity write guard composes its refused list from what the engine left it, and a whitelisted key is excluded from that list by design, so the refusal reads `None of the submitted fields (—) are editable` — naming nothing. The write was already being dropped by the read-only strip before this change; what moves is that the caller is now told, and told imprecisely. `sys_user`'s three writable fields are not read-only, so nothing in this repository is on that boundary; an application that puts a `readonly` field in an UPDATE whitelist should take it out, which is what the whitelist meant either way. + +An `isSystem` caller sees no change at all: the strip has never applied to one, and neither does the hide. diff --git a/content/docs/automation/hooks.mdx b/content/docs/automation/hooks.mdx index 7cc7f7ef99..3b7ba885b4 100644 --- a/content/docs/automation/hooks.mdx +++ b/content/docs/automation/hooks.mdx @@ -181,6 +181,12 @@ record's fields **directly on `ctx.input`** (a flat view over the internal `{ data, options }` wrapper — reads and writes of record fields route through `ctx.input.data`): +On **update**, "the incoming record" means *the record the engine intends to +persist* — a caller-supplied value for a `readonly` field is not on it, and the +caller's submission is on `ctx.submitted` instead (diagnostics only). See +[Static `readonly` fields on the write path](/docs/protocol/objectql/security#static-readonly-fields-on-the-write-path) +for the five rules and the migration. + These four names always resolve to the envelope, never to a record field — even if your object declares a field with one of those names. A field named diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 978b9dfdb2..76d26caf59 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -9,7 +9,7 @@ the seed loader replaying package fixtures, a plugin's boot reconciler, a service self-write, a migration. This page is **the authority** for what that flag actually does. It exists -because the flag is not one concept: it is a single boolean read at **108 +because the flag is not one concept: it is a single boolean read at **109 distinct sites across 20 packages**, and knowing three of those behaviours gives no hint that the other hundred-and-four exist. Every documented app-side bug traced to `isSystem` had the same shape — the metadata was complete and correct, @@ -132,7 +132,7 @@ that silently does not happen. ### 3. Sharing (`plugin-sharing`) -The largest single consumer — **17 of the 108 sites**. +The largest single consumer — **17 of the 109 sites**. | # | Behaviour when `isSystem` | What you get / what you lose | Anchor | |:--|:---|:---|:---| @@ -278,7 +278,7 @@ Ownership injection, `readonly` bypass and sharing materialisation are independent decisions, and a seed loader plausibly wants the first two but not the third. The concept is nevertheless **staying as one boolean**: -- **Shipped semantics.** `isSystem` is a published contract with 108 read sites +- **Shipped semantics.** `isSystem` is a published contract with 109 read sites in 20 packages. Splitting it is a breaking contract change across all of them. (The ruling was taken when the census read 80 sites in 18 packages; the count has grown, which strengthens rather than weakens the argument.) @@ -352,12 +352,12 @@ still holds equal to the census on every pull request: | Appearances of the bare identifier `isSystem` in non-test sources | 813 | — | | — parsed as a declaration | 23 | ✅ | | — parsed as an object-literal / type key (producers and option objects) | 310 | — | -| — parsed as a property **read** | 114 | ✅ | +| — parsed as a property **read** | 115 | ✅ | | — parsed in some other syntactic position (a local, a cast, a conditional) | 9 | ✅ | | — the remainder: text inside comments and string literals | 358 | — | | Of those reads: reads of one of the unrelated metadata fields | 6 | ✅ | -| Of those reads: reads of `ExecutionContext.isSystem` | **108** | ✅ | -| — behaviour-bearing (rows 1–63 above) | 104 | ✅ | +| Of those reads: reads of `ExecutionContext.isSystem` | **109** | ✅ | +| — behaviour-bearing (rows 1–63 above) | 105 | ✅ | | — carry the flag onward only (rows 64–67 above) | 4 | ✅ | | Packages containing at least one elevation read | **20** | ✅ | | Files containing at least one elevation read | 45 | ✅ | diff --git a/content/docs/protocol/objectql/security.mdx b/content/docs/protocol/objectql/security.mdx index 004f8ef47b..c323f276a8 100644 --- a/content/docs/protocol/objectql/security.mdx +++ b/content/docs/protocol/objectql/security.mdx @@ -259,7 +259,7 @@ rejecting — the offending key is removed from the payload and the rest of the committed. The write therefore **succeeds** (REST answers `200`), and the read-only column simply keeps its stored value. -Four rules decide whether a given value survives: +Five rules decide whether a given value survives: | # | Rule | Effect | |:--|:---|:---| @@ -267,10 +267,37 @@ Four rules decide whether a given value survives: | 2 | **Only *caller-supplied* keys are candidates** | The engine snapshots the payload's keys at entry (`suppliedKeys`), *before* middleware and `beforeUpdate` hooks run. Only keys in that snapshot can be stripped. | | 3 | **Hook / middleware backfill survives** | A key a `beforeUpdate` hook *adds* to `data` is absent from the entry snapshot, so it is not a candidate — this is why the built-in `updated_by` / `updated_at` stamps land even though those columns are `readonly`. | | 4 | **`context.preserveAudit` admits a whitelist — on UPDATE only** | An opt-in historical import reinstates the audit/timestamp family and author-declared business `readonly` fields; platform-managed `system` columns (tenancy, generated) stay stripped. This exemption exists on the **UPDATE** path and nowhere else — see below. | - -Rule 2 is scoped to keys, not values: a key the caller sent stays a strip candidate even if -a hook later overwrites its value. So a `beforeUpdate` hook can *backfill* a read-only -field, but cannot *rescue* one the caller supplied. +| 5 | **Hooks are shown the persist image, not the submission** (#16344) | On UPDATE, a caller-supplied value for a `readonly` field is hidden from `ctx.input.data` *before* `beforeUpdate` is dispatched, so a hook cannot derive a persisted column from a value the row will never hold. What the caller actually sent is on **`ctx.submitted`** — diagnostics only, never the persist image. | + +Rule 2 selects the *candidates*; rules 3 and 5 decide what a hook can do about one. A key +the caller never sent is not a candidate at all, which is why the built-in `updated_by` / +`updated_at` stamps land. A key the caller **did** send stays a candidate — but a hook that +**assigns** it owns the value standing on it and the strip keeps that write (#5591 / +#14088: authorship is *recorded* while the hook writes happen, not inferred from value +equality afterwards). So a `beforeUpdate` hook can both *backfill* a read-only field and +*overwrite* one the caller supplied; what it can no longer do is *rescue the caller's own +value*, because since #16344 that value is not on `ctx.input.data` for it to echo back. + + +This is a **breaking** change to what a hook reads, not to what is stored: the accept / +refuse set is unchanged, `onFieldsDropped` reports the same fields under the same +`readonly` reason, the WARN says the same sentence, and `strictReadonlyWrites` refuses the +same writes. What moved is the hook's view. + +- A handler **deriving** a column from a read-only field reads the stored row on + `ctx.previous`, or treats the key's absence as "unchanged" — which is what absence + always meant for a field the caller never sent. +- A handler **reporting on what the caller sent** (a guard naming an offending key) reads + `ctx.submitted`. +- A self-assignment (`data.x = data.x`) on such a field is now a **no-op** — the stored + value stands, and the write is stripped and reported exactly as an un-hooked one is. +- `ctx.submitted` is **not** marshalled onto the sandboxed `body` face. A `body` deriving + from a read-only column reads `ctx.previous`, the stored row, which is the correct + source either way. + +`beforeInsert` is untouched (#14147), and `readonlyWhen` locks are deliberately still +hook-writable (#9107). + **`preserveAudit` is an UPDATE-path exemption. It does not apply on INSERT (#6640).** diff --git a/content/docs/references/data/hook.mdx b/content/docs/references/data/hook.mdx index c91f44bf03..fd5a929e8e 100644 --- a/content/docs/references/data/hook.mdx +++ b/content/docs/references/data/hook.mdx @@ -93,6 +93,7 @@ L2 sandboxed JS body — runs inside an isolated VM with declared capabilities | **input** | `Record` | ✅ | Mutable input parameters | | **result** | `any` | optional | Operation result (After hooks only) | | **previous** | `Record` | optional | Record state before operation | +| **submitted** | `Record` | optional | What the caller submitted, as sent (update only) — diagnostics only, never the persist image | | **dispatch** | `{ mode: Enum<'record' \| 'per-row'>; index: integer; scope: Record }` | optional | How this hook call relates to the caller's write (engine-produced) | | **session** | `{ userId?: string; actor?: string; organizationId?: string; accessToken?: string; … }` | optional | Current session context | | **provenance** | `{ flowRunId?: string; attributedUserId?: string }` | optional | Server-stamped write provenance (never client-supplied, never an authorization input) | diff --git a/packages/objectql/src/engine-readonly-hook-input.test.ts b/packages/objectql/src/engine-readonly-hook-input.test.ts new file mode 100644 index 0000000000..85b7e9d940 --- /dev/null +++ b/packages/objectql/src/engine-readonly-hook-input.test.ts @@ -0,0 +1,409 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #16344 — UPDATE side: a caller-supplied value for a statically `readonly` +// field must not be visible to `beforeUpdate`. +// +// The strip itself was never the defect. `stripReadonlyFields` does keep the +// caller's value out of the SET clause, and the read-back proves it. What +// leaked was the HOOK INPUT: the strip ran AFTER `triggerHooks('beforeUpdate')`, +// so a hook computing a derived column off the incoming record computed it from +// a number the row would never contain — and THAT write, being the hook's own, +// persisted. The committed row then cited values it does not hold: +// +// PATCH { actual_value: 380, target_value: 1, weight: 1 } -> 200 +// read back: target_value 400 weight 10 (the strip worked) +// score 1.2 calc_trace "实际 380 / 目标 1 … 权重 1%" +// +// The invariant this suite pins is the triage ruling's, verbatim: +// 「交给生命周期钩子的记录,就是它打算持久化的那条记录。」 +// +// The ruling that decided the SHAPE is the maintainer's, decision batch #87 +// (2026-09-08), and it has two halves — both pinned here: +// +// 1. `ctx.input.data` on `beforeUpdate` becomes the record the engine intends +// to persist: caller-forged static `readonly` values are hidden before the +// before phase is dispatched. +// 2. The caller's submission AS SENT travels on `ctx.submitted`, a declared +// `HookContext` member — "diagnostics only, never the persist image" — so +// a guard that reports on what the caller sent keeps naming it. Without +// half 2, half 1 silently degrades plugin-auth's ADR-0092 identity write +// guard from `403 … (role) …` to `403 … (—) …`; that degradation was +// measured, and it is why the two halves ship together. +// +// ⚠️ Read the ORDERING probe below as the card's whole claim. It is measured, +// not inferred from where the two call sites sit in the file: the probe records +// what the hook actually observed, and a same-write CONTROL on a writable field +// proves the probe can see payload values at all — so an empty reading on the +// read-only key is a reading, not a broken probe. +// +// Pre-fix reading on this branch's base (`fd5cff209`, 3 failed / 4 passed of +// the 7 cases that existed then): ORDERING failed at `readonlyKeyPresent` +// ("expected true to be false") with its control leg PASSING, THE REPORT +// committed `score` 380 against a baseline of 9.5, and the PREDICATE branch +// failed identically. +// +// What this suite is NOT: a relaxation of #5591 / #14088. A hook writing a +// read-only column is still the hook's write and still lands; those controls are +// re-pinned here so the two verdicts are read together, and so a future repair +// of one cannot silently reintroduce the other. The ENFORCEMENT pass did not +// move — only what the hooks are SHOWN did. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; + +function makeDriver() { + const stores = new Map>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]: [string, any]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + return row?.[k] === v; + }); + }; + let n = 0; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(object: string, ast: any) { + const rows = Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + // The caller's bound, applied AFTER the filter and by PRESENCE + // (`check:objectql-double-limit`): a double that silently ignores + // `limit` answers with rows the engine asked it not to return, which is + // the one way a fake driver can make a paging bug pass. + return typeof ast?.limit === 'number' ? rows.slice(0, ast.limit) : rows; + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + n += 1; + const id = (data.id as string) ?? `r_${n}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const row = { ...s.get(id), ...data, id }; + s.set(id, row); + return row; + }, + async updateMany(object: string, ast: any, data: Record) { + const s = storeFor(object); + let count = 0; + for (const row of [...s.values()]) { + if (!matches(row, ast?.where)) continue; + s.set(row.id, { ...row, ...data, id: row.id }); + count += 1; + } + return count; + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r, undefined))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, storeFor }; +} + +/** What the `beforeUpdate` hook observed on `ctx.input.data`, per call. */ +type HookSighting = { + readonlyKeyPresent: boolean; + readonlyValue: unknown; + writableKeyPresent: boolean; + writableValue: unknown; + /** The declared submission channel, read in the SAME dispatch. */ + submittedKeys: string[] | undefined; + submittedReadonlyValue: unknown; + submittedIsFrozen: boolean | undefined; +}; + +describe('#16344 — caller-forged readonly values are hidden from beforeUpdate', () => { + let engine: ObjectQL; + let storeFor: ReturnType['storeFor']; + let sightings: HookSighting[]; + let warns: string[]; + + beforeEach(async () => { + warns = []; + sightings = []; + const logger: any = { + warn: (m: string) => warns.push(String(m)), + debug() {}, info() {}, error() {}, trace() {}, fatal() {}, + child() { return logger; }, + }; + engine = new ObjectQL({ logger }); + const d = makeDriver(); + storeFor = d.storeFor; + engine.registerDriver(d.driver, true); + await engine.init(); + + // The reported object, trimmed to the fields the repro turns on. + engine.registry.registerObject({ + name: 'kpi_entry_line', + fields: { + target_value: { type: 'number', readonly: true, scale: 4 }, + weight: { type: 'number', readonly: true, scale: 2 }, + actual_value: { type: 'number', scale: 4 }, + score: { type: 'number', scale: 4 }, + calc_trace: { type: 'text' }, + reviewed_at: { type: 'datetime', readonly: true }, + }, + } as any); + + const seed = () => ({ + id: 'kpi_1', target_value: 400, weight: 10, actual_value: 100, + score: 2.5, calc_trace: 'seed', reviewed_at: null, + }); + storeFor('kpi_entry_line').set('kpi_1', seed()); + storeFor('kpi_entry_line').set('kpi_2', { ...seed(), id: 'kpi_2' }); + + // ── The ORDERING probe, and its control in the same hook ──────────────── + // + // `target_value` is read-only, `actual_value` is not, and the repro's PATCH + // carries BOTH. One hook invocation therefore yields both legs of the + // measurement: if the writable key is visible and the read-only key is not, + // the strip provably ran first. If NEITHER is visible the probe is broken + // and the reading is void — which is the whole reason the control is here. + engine.registerHook('beforeUpdate', async (ctx: any) => { + const data = (ctx.input?.data ?? {}) as Record; + const submitted = ctx.submitted as Record | undefined; + sightings.push({ + readonlyKeyPresent: Object.prototype.hasOwnProperty.call(data, 'target_value'), + readonlyValue: data.target_value, + writableKeyPresent: Object.prototype.hasOwnProperty.call(data, 'actual_value'), + writableValue: data.actual_value, + submittedKeys: submitted ? Object.keys(submitted) : undefined, + submittedReadonlyValue: submitted?.target_value, + submittedIsFrozen: submitted ? Object.isFrozen(submitted) : undefined, + }); + }, { object: 'kpi_entry_line', priority: 10 }); + + // The reported app hook: recompute the derived columns from the incoming + // record, falling back to the stored row for anything the payload omits. + // That fallback is the ordinary shape — and it is exactly what the leak + // defeated, because the payload DID carry a `target_value`, just not one + // that would ever be stored. + engine.registerHook('beforeUpdate', async (ctx: any) => { + const data = ctx.input.data as Record; + const prev = (ctx.previous ?? {}) as Record; + const target = data.target_value ?? prev.target_value; + const weight = data.weight ?? prev.weight; + const actual = data.actual_value ?? prev.actual_value; + const rate = (actual / target) * 100; + data.score = Number(((weight / 100) * rate).toFixed(4)); + data.calc_trace = `实际 ${actual} / 目标 ${target} → 完成率 ${rate}%;权重 ${weight}%`; + }, { object: 'kpi_entry_line', priority: 50 }); + + // An unrelated read-only column the hook STAMPS. It is the #5591/#14088 + // control: a hook's own write to a read-only field must survive, and must + // keep surviving after this card moves the strip. + engine.registerHook('beforeUpdate', async (ctx: any) => { + ctx.input.data.reviewed_at = '2026-09-08T00:00:00.000Z'; + }, { object: 'kpi_entry_line', priority: 60 }); + }); + + const row = (id = 'kpi_1') => storeFor('kpi_entry_line').get(id); + + it('ORDERING (the card): the hook does not see a caller-supplied readonly value, and DOES see the writable one', async () => { + await engine.update('kpi_entry_line', { + id: 'kpi_1', actual_value: 380, target_value: 1, weight: 1, + }); + + expect(sightings).toHaveLength(1); + const seen = sightings[0]!; + // CONTROL leg — the probe can observe the payload at all. + expect(seen.writableKeyPresent).toBe(true); + expect(seen.writableValue).toBe(380); + // MEASUREMENT leg — the strip ran first, so the key is simply gone. + expect(seen.readonlyKeyPresent).toBe(false); + expect(seen.readonlyValue).toBeUndefined(); + }); + + it('THE REPORT: the persisted derived columns cite values the row actually holds', async () => { + // Baseline — write only the writable field. + await engine.update('kpi_entry_line', { id: 'kpi_1', actual_value: 380 }); + const baseline = { score: row().score, calc_trace: row().calc_trace }; + expect(baseline.score).toBe(9.5); + + // The same request plus values for the two read-only fields. The reported + // symptom is that this diverges from the baseline; it must not. + await engine.update('kpi_entry_line', { + id: 'kpi_2', actual_value: 380, target_value: 1, weight: 1, + }); + + expect(row('kpi_2').target_value).toBe(400); + expect(row('kpi_2').weight).toBe(10); + expect(row('kpi_2').score).toBe(baseline.score); + expect(row('kpi_2').calc_trace).toBe(baseline.calc_trace); + // Stated as the value it must NOT be. 380 is what THIS fixture read on + // `origin/main` before the fix, not the report's 1.2 — the reported app + // caps its completion rate and this one does not, so the number differs + // while the defect is the same one: a score derived from `target_value: 1` + // on a row that holds 400. + expect(row('kpi_2').score).not.toBe(380); + expect(row('kpi_2').calc_trace).not.toContain('目标 1'); + }); + + it('the strip still reports and still warns — the caller is not told the write was whole', async () => { + const dropped: any[] = []; + await engine.update( + 'kpi_entry_line', + { id: 'kpi_1', actual_value: 380, target_value: 1, weight: 1 }, + { onFieldsDropped: (e: any) => dropped.push(e) } as any, + ); + const fields = dropped.flatMap((e) => e.fields).sort(); + expect(fields).toEqual(['target_value', 'weight']); + expect(dropped.every((e) => e.reason === 'readonly')).toBe(true); + expect(warns.some((w) => w.includes("Field 'target_value'"))).toBe(true); + }); + + it('#5591/#14088 CONTROL: a hook write to a read-only column still lands', async () => { + await engine.update('kpi_entry_line', { id: 'kpi_1', actual_value: 380 }); + expect(row().reviewed_at).toBe('2026-09-08T00:00:00.000Z'); + }); + + it('#5591/#14088 CONTROL: a hook write lands even when the caller echoed the same key', async () => { + // The whole-record write-back idiom: the caller echoes `reviewed_at` back + // as it read it. The hook overwrites it, and the hook's value is the one + // that commits. + await engine.update('kpi_entry_line', { + id: 'kpi_1', actual_value: 380, reviewed_at: null, + }); + expect(row().reviewed_at).toBe('2026-09-08T00:00:00.000Z'); + }); + + it('the PREDICATE branch is fixed on the same terms', async () => { + // Both update branches run the strip off one snapshot, so the multi path + // must not need its own fix — pinned, because "both call sites" is the + // #3106 / #4441 shape that gets missed. + await engine.update( + 'kpi_entry_line', + { actual_value: 380, target_value: 1, weight: 1 }, + { where: { target_value: 400 }, multi: true } as any, + ); + expect(sightings.length).toBeGreaterThan(0); + expect(sightings.every((s) => s.readonlyKeyPresent === false)).toBe(true); + expect(sightings.every((s) => s.writableValue === 380)).toBe(true); + expect(row('kpi_1').target_value).toBe(400); + expect(row('kpi_1').score).toBe(9.5); + expect(row('kpi_1').calc_trace).not.toContain('目标 1'); + }); + + it('an isSystem caller is UNCHANGED: the exemption is not narrowed by this card', async () => { + // `isSystem` legitimately writes read-only columns, and it must still see + // its own payload in the hook — the strip's gate is untouched. + await engine.update( + 'kpi_entry_line', + { id: 'kpi_1', actual_value: 380, target_value: 1000 }, + { context: { isSystem: true } } as any, + ); + expect(sightings[0]!.readonlyKeyPresent).toBe(true); + expect(sightings[0]!.readonlyValue).toBe(1000); + expect(row().target_value).toBe(1000); + }); + + // ── The ruling's SECOND half: `ctx.submitted` ───────────────────────────── + + it('`ctx.submitted` carries the caller submission as sent, including what was hidden', async () => { + // The whole point of the channel: what half 1 takes out of `input.data` + // has to remain READABLE somewhere, or every guard that reports on the + // caller's submission degrades silently. Asserted in the SAME dispatch as + // the hidden reading above, so the pair cannot drift. + await engine.update('kpi_entry_line', { + id: 'kpi_1', actual_value: 380, target_value: 1, weight: 1, + }); + + const seen = sightings[0]!; + expect(seen.readonlyKeyPresent).toBe(false); // half 1 + expect(seen.submittedKeys).toEqual(['id', 'actual_value', 'target_value', 'weight']); + expect(seen.submittedReadonlyValue).toBe(1); // half 2 + }); + + it('`ctx.submitted` is frozen — "diagnostics only" is enforced, not merely documented', async () => { + // `previous` is the only other read-only-by-contract member and it is a + // live driver row, so nothing would enforce this if the producer did not. + let threw: unknown; + let payloadAfter: Record | undefined; + engine.registerHook('beforeUpdate', async (ctx: any) => { + expect(Object.isFrozen(ctx.submitted)).toBe(true); + // ES modules are strict by construction, so the assignment THROWS rather + // than failing silently — which is the half that makes the freeze a + // guarantee an author can rely on instead of a convention. + try { ctx.submitted.target_value = 999; } catch (e) { threw = e; } + payloadAfter = { ...(ctx.input.data as Record) }; + }, { object: 'kpi_entry_line', priority: 20 }); + + await engine.update('kpi_entry_line', { + id: 'kpi_1', actual_value: 380, target_value: 1, + }); + + expect(threw).toBeInstanceOf(TypeError); + // ⛔ And the attempt reached the payload through no other door: writing to + // the diagnostics record must never be a way to reinstate a refused value. + expect(payloadAfter).not.toHaveProperty('target_value'); + expect(row().target_value).toBe(400); + }); + + it('`ctx.submitted` is NOT the payload object — a hook rewriting the payload does not rewrite it', async () => { + // The #5591 aliasing hazard, asked of the new member: a snapshot that + // aliased `input.data` would answer every question about "what the caller + // sent" with the POST-hook payload, which is the failure this key exists + // to make impossible. + const submittedAtEnd: unknown[] = []; + engine.registerHook('beforeUpdate', async (ctx: any) => { + ctx.input.data.actual_value = 999; + ctx.input.data.calc_trace = 'rewritten by a hook'; + }, { object: 'kpi_entry_line', priority: 20 }); + engine.registerHook('beforeUpdate', async (ctx: any) => { + submittedAtEnd.push({ ...(ctx.submitted as Record) }); + }, { object: 'kpi_entry_line', priority: 90 }); + + await engine.update('kpi_entry_line', { id: 'kpi_1', actual_value: 380 }); + + expect(submittedAtEnd).toEqual([{ id: 'kpi_1', actual_value: 380 }]); + }); + + it('`ctx.submitted` is bound on the PREDICATE path too — one write, one submission', async () => { + // Per-row contexts are spread from the batch context, so this is a + // statement about that spread rather than a second binding site: every + // matched row's dispatch sees the SAME submission, because one caller + // write has one submission however many rows it matches. + await engine.update( + 'kpi_entry_line', + { actual_value: 380, target_value: 1, weight: 1 }, + { where: { target_value: 400 }, multi: true } as any, + ); + + expect(sightings.length).toBeGreaterThan(1); + expect(sightings.every((s) => s.submittedIsFrozen === true)).toBe(true); + expect(sightings.every((s) => s.submittedReadonlyValue === 1)).toBe(true); + expect(sightings.every((s) => s.readonlyKeyPresent === false)).toBe(true); + }); + + it('`ctx.submitted` reaches the AFTER phase on the same write', async () => { + // The by-id path reuses one context across the before/after pair, so the + // member is present in both — pinned so a later refactor that rebuilds the + // after context cannot drop it unnoticed. + const afterSubmitted: unknown[] = []; + engine.registerHook('afterUpdate', async (ctx: any) => { + afterSubmitted.push(ctx.submitted ? { ...(ctx.submitted as object) } : undefined); + }, { object: 'kpi_entry_line', priority: 10 }); + + await engine.update('kpi_entry_line', { + id: 'kpi_1', actual_value: 380, target_value: 1, + }); + + expect(afterSubmitted).toEqual([{ id: 'kpi_1', actual_value: 380, target_value: 1 }]); + }); +}); diff --git a/packages/objectql/src/engine-readonly-strip-caller-values.test.ts b/packages/objectql/src/engine-readonly-strip-caller-values.test.ts index 986d3b0aaf..c9910d741e 100644 --- a/packages/objectql/src/engine-readonly-strip-caller-values.test.ts +++ b/packages/objectql/src/engine-readonly-strip-caller-values.test.ts @@ -4,11 +4,11 @@ // the CALLER SUBMITTED, never whatever value happens to sit on the key at the // moment the strip runs. // -// The strip executes AFTER `beforeUpdate`, so those two are different facts the -// instant a hook writes to a read-only column. The guard used to be a key SET -// snapshotted at engine entry, which can answer only "did the caller name this -// key?" — so `delete data[name]` took the hook's value with it whenever the -// caller's payload happened to carry the same key. +// The ENFORCEMENT pass executes AFTER `beforeUpdate`, so those two are different +// facts the instant a hook writes to a read-only column. The guard used to be a +// key SET snapshotted at engine entry, which can answer only "did the caller +// name this key?" — so `delete data[name]` took the hook's value with it +// whenever the caller's payload happened to carry the same key. // // The measured downstream shape (objectstack#5591, from hotcrm#788, reproduced // below verbatim): "read the whole record → change one field → write the whole @@ -29,6 +29,40 @@ // What this suite is NOT: a relaxation of #2948 / #3003 / #3015. A // caller-supplied read-only value that no hook overwrote is still stripped, and // the case is pinned here next to the fix so the two verdicts are read together. +// +// ─────────────────────────────────────────────────────────────────────────── +// SUPERSEDED IN WRITING (#16344, maintainer ruling, decision batch #87, +// 2026-09-08) — this file's #5591 reasoning is NOT deleted, and the part of it +// that still stands is the part this note names. +// +// #5591 argued for comparing VALUES instead of stripping before the hooks, and +// gave a second reason for the ordering beyond its own: "a `beforeUpdate` guard +// that rejects or reports on what the caller submitted (plugin-auth's ADR-0092 +// identity write guard is the in-repo instance — its error text NAMES the +// non-whitelisted keys it found) reads `ctx.input.data`. Stripping ahead of the +// hooks would empty that out and silently degrade every such diagnostic." +// +// ⭐ That cost was REAL and was measured again on #16344 — the guard's 403 did +// degrade to `(—)` on a strip-before-hooks build with no other change. What the +// ruling rejected is the CONCLUSION that the ordering was therefore the only +// way to pay it, because the ordering has a cost of its own that #5591 never +// weighed: a hook handed a value the engine has already refused can derive a +// column that IS persisted. Measured on a real app (17.2.0): a KPI row +// committed `target_value = 400` beside a hook-derived `calc_trace` reading +// `目标 1`, with no error, no warning and a 200 — a record whose own audit +// trail cites values it does not hold. +// +// So both are paid: the caller-forged read-only values are HIDDEN from the +// hooks (they cannot reach a derivation), and the caller's submission as sent +// travels on its own named channel, `ctx.submitted`, which the ADR-0092 guard +// now reads — so its message is byte-identical to the one #5591 was protecting. +// The suite below is unchanged except for the one case that pinned the +// diagnostic's OLD channel, which is re-pinned on the new one. +// +// ⛔ What did NOT move is the ENFORCEMENT pass. It is still after the hooks, +// because it is the only point that can tell a hook's stamp from a caller's +// forgery (`hookWrittenKeys`, #14088) — which is this file's own subject, and +// the reason a hook write to a read-only column still lands. import { describe, it, expect, beforeEach } from 'vitest'; import type { EngineQueryOptionsParsed } from '@objectstack/spec/data'; @@ -290,23 +324,31 @@ describe('update strip acts on CALLER-submitted values (#5591)', () => { expect(ka().published_at).toBe(NOW); }); - it('a hook that reads the caller-submitted read-only value can still SEE it', async () => { - // Why the fix compares values instead of stripping before the hooks: a - // `beforeUpdate` guard that rejects or reports on what the caller - // submitted (plugin-auth's ADR-0092 identity write guard is the in-repo - // instance — its error text NAMES the non-whitelisted keys it found) reads - // `ctx.input.data`. Stripping ahead of the hooks would empty that out and - // silently degrade every such diagnostic, so the caller's payload still - // reaches the hooks unchanged. - const seen: unknown[] = []; + it('a hook that reads the caller-submitted read-only value can still SEE it — on `ctx.submitted`', async () => { + // [#16344] The diagnostic this case has always defended, re-pinned on the + // channel the ruling gave it. A `beforeUpdate` guard that reports on what + // the caller submitted (plugin-auth's ADR-0092 identity write guard is the + // in-repo instance — its error text NAMES the non-whitelisted keys it + // found) must still be able to name a caller-forged read-only key. + // + // BOTH readings are asserted, and the pair IS the contract: + // - `input.data` no longer carries it — that is #16344's whole fix, and + // an assertion here is what stops the leak coming back; + // - `submitted` does — that is what keeps the guard whole, and an + // assertion here is what stops the channel being quietly dropped as + // unused. + const seenInput: unknown[] = []; + const seenSubmitted: unknown[] = []; engine.registerHook('beforeUpdate', async (ctx: any) => { - seen.push(Object.keys(ctx.input.data)); + seenInput.push(Object.keys(ctx.input.data)); + seenSubmitted.push(Object.keys(ctx.submitted ?? {})); }, { object: 'crm_knowledge_article', priority: 1 }); await engine.update('crm_knowledge_article', { id: 'ka_1', title: 'B', published_at: '1999-01-01T00:00:00.000Z', }); - expect(seen).toEqual([['id', 'title', 'published_at']]); + expect(seenInput).toEqual([['id', 'title']]); + expect(seenSubmitted).toEqual([['id', 'title', 'published_at']]); }); it('an isSystem caller is untouched by any of this', async () => { @@ -716,7 +758,7 @@ describe('the strip reads hook-write PROVENANCE, not value equality (#14088)', ( // ── The measured consequence of "an assignment ran", stated out loud ─────── - it('MEASURED: a lone self-assigning hook leaves the CALLER value on the key', async () => { + it('MEASURED: a lone self-assigning hook is a NO-OP on a hidden key — the STORED value stands (#16344)', async () => { // ⚠️ RECORDING BEHAVIOUR, NOT BLESSING IT. The direct consequence of the // mechanism #14088 chose: the record says an ASSIGNMENT RAN and is // deliberately blind to the VALUE (that blindness is the whole repair — it @@ -748,6 +790,37 @@ describe('the strip reads hook-write PROVENANCE, not value equality (#14088)', ( // // A future ruling that reverses this INVERTS both pins together; it never // deletes either. `isCallerSuppliedValue`'s docblock carries the argument. + // + // ⭐ [#16344] THE VERDICT MOVED, and this is the record of it rather than a + // deletion. Everything above still describes the recording mechanism + // exactly; what changed is what a `beforeUpdate` hook is SHOWN. The + // caller's forged `completed_at` is hidden from the hook, so + // `ctx.input.data.completed_at` reads `undefined` and the self-assign + // re-creates the key holding THAT. + // + // ⛔ And a hook write of `undefined` is NOT persisted as one. The + // confluence treats set-to-undefined of a HIDDEN key as the no-op the line + // actually is: the key is deleted, its entry leaves the record, and the + // ordinary hand-back puts the caller's value back for the strip to judge. + // The write then reads EXACTLY as it would have with no hook at all — + // stripped, reported, warned, refused under `strictReadonlyWrites`. + // + // The direction matters, and it is why this is not `toBeUndefined()`: + // - persisting the `undefined` ERASES the stored value on the memory + // driver and hands knex an undefined binding on a SQL one (a bare + // compile-time Error, outside the ADR-0112 envelope). Neither is "the + // record the engine intends to persist", which is this card's whole + // subject. + // - the laundering route this case existed to make VISIBLE stays CLOSED. + // A no-op-looking line still cannot confer hook provenance on a value + // the caller minted — the FORGED negative below is what pins that, and + // it must stay red-able: drop the record-narrowing at the confluence and + // the caller's timestamp is handed back onto a key the record still + // calls hook-owned, and it commits. + // + // ⚠ The blindness to VALUE stated above is unchanged for every key the + // hook can actually SEE. This narrowing reaches only keys hidden by this + // card, and only the one value — `undefined` — that no driver can store. const FORGED = '1999-01-01T00:00:00.000Z'; engine.registerHook('beforeUpdate', async (ctx: any) => { ctx.input.data.completed_at = ctx.input.data.completed_at; @@ -758,36 +831,77 @@ describe('the strip reads hook-write PROVENANCE, not value equality (#14088)', ( id: 't_20', status: 'in_progress', completed_at: FORGED, }); - expect(task('t_20').completed_at).toBe(FORGED); - expect(warns).toEqual([]); + // The FORGED negative, KEPT: the laundering route stays closed. + expect(task('t_20').completed_at).not.toBe(FORGED); + // ⭐ The re-pin. The stored value STANDS — not erased, not `undefined`. + expect(task('t_20').completed_at).toBe(STAMPED); + // ...and the write is reported exactly as an un-hooked one would be. + expect(warns.some((w) => w.includes("Field 'completed_at'"))).toBe(true); }); it('the recording is transparent to a hook reading its own payload', async () => { - // Hooks read `ctx.input.data` for diagnostics (plugin-auth's identity write - // guard NAMES the keys it found). The recording view must be indistinguishable - // from the payload for every read shape a hook uses. + // The recording view must be indistinguishable from the payload for every + // read shape a hook uses. + // + // [#16344] Written on WRITABLE keys, and that is the repair rather than an + // accommodation. This case's subject is the recording PROXY; using a + // read-only key the engine now hides made it assert two mechanisms at once + // and fail on the one it does not test. The hidden key gets its own + // assertion below — where it says what it means. const seen: any[] = []; engine.registerHook('beforeUpdate', async (ctx: any) => { seen.push({ keys: Object.keys(ctx.input.data), spread: { ...ctx.input.data }, json: JSON.stringify(ctx.input.data), - has: 'completed_at' in ctx.input.data, + has: 'status' in ctx.input.data, own: Object.prototype.hasOwnProperty.call(ctx.input.data, 'title'), }); }, { object: 'duly_task', priority: 1 }); seedDone('t_19'); - await engine.update('duly_task', { id: 't_19', title: 'T', completed_at: null }); + await engine.update('duly_task', { id: 't_19', title: 'T', status: 'in_progress' }); expect(seen).toHaveLength(1); - expect(seen[0].keys).toEqual(['id', 'title', 'completed_at']); - expect(seen[0].spread).toEqual({ id: 't_19', title: 'T', completed_at: null }); - expect(seen[0].json).toBe(JSON.stringify({ id: 't_19', title: 'T', completed_at: null })); + expect(seen[0].keys).toEqual(['id', 'title', 'status']); + expect(seen[0].spread).toEqual({ id: 't_19', title: 'T', status: 'in_progress' }); + expect(seen[0].json).toBe(JSON.stringify({ id: 't_19', title: 'T', status: 'in_progress' })); expect(seen[0].has).toBe(true); expect(seen[0].own).toBe(true); }); + it('[#16344] the hide is CONSISTENT across every read shape, not just Object.keys', async () => { + // A key hidden from enumeration but reachable by `in`, by a direct read or + // through `JSON.stringify` would be worse than no hide at all: the derived + // write this card exists to stop is written by hooks that read the key + // DIRECTLY, and a partial hide would leave exactly those working while the + // audit of them came back clean. Asked of all five shapes, in one dispatch, + // through the recording view — so this also pins that the hide and the + // #14088 Proxy compose rather than one masking the other. + const seen: any[] = []; + engine.registerHook('beforeUpdate', async (ctx: any) => { + seen.push({ + keys: Object.keys(ctx.input.data), + ownNames: Object.getOwnPropertyNames(ctx.input.data), + spread: { ...ctx.input.data }, + json: JSON.stringify(ctx.input.data), + inOperator: 'completed_at' in ctx.input.data, + direct: ctx.input.data.completed_at, + }); + }, { object: 'duly_task', priority: 1 }); + seedDone('t_21'); + + await engine.update('duly_task', { id: 't_21', title: 'T', completed_at: null }); + + expect(seen).toHaveLength(1); + expect(seen[0].keys).toEqual(['id', 'title']); + expect(seen[0].ownNames).toEqual(['id', 'title']); + expect(seen[0].spread).toEqual({ id: 't_21', title: 'T' }); + expect(seen[0].json).toBe(JSON.stringify({ id: 't_21', title: 'T' })); + expect(seen[0].inOperator).toBe(false); + expect(seen[0].direct).toBeUndefined(); + }); + it('no recording view reaches the driver — the seal puts the RAW payload back', async () => { // A driver handed the recording view would be writing into a recorder the // engine has stopped reading, and on a driver that keeps the object it is @@ -799,6 +913,13 @@ describe('the strip reads hook-write PROVENANCE, not value equality (#14088)', ( // stripped, every pass returns the SAME reference, so `updateMany` receives // `hookContext.input.data` verbatim and the identity is decisive rather // than laundered through one of the copies the by-id path makes. + // + // [#16344] "Nothing stripped" is a PRECONDITION of the identity argument, + // and the caller payload is now written to satisfy it: it no longer echoes + // the read-only `completed_at`, because the pre-hook hide would copy the + // payload for exactly the reason every other strip does. The hook still + // stamps the column, so the write under test is unchanged; what the caller + // echoes is not this case's subject, and the echo is covered above. const seenByDriver: any[] = []; const d2 = makeDriver(); const e2 = new ObjectQL({}); @@ -823,7 +944,7 @@ describe('the strip reads hook-write PROVENANCE, not value equality (#14088)', ( }, { object: 'duly_task', priority: 50 }); d2.storeFor('duly_task').set('t_20', { id: 't_20', title: 'T', status: 'open', completed_at: null }); - const payload: Record = { status: 'done', completed_at: null }; + const payload: Record = { status: 'done' }; await e2.update('duly_task', payload as any, { where: { status: 'open' }, multi: true } as any); expect(seenByDriver).toHaveLength(1); diff --git a/packages/objectql/src/engine-readonly-strip-signal.test.ts b/packages/objectql/src/engine-readonly-strip-signal.test.ts index 811e00fa89..0ea55c47ae 100644 --- a/packages/objectql/src/engine-readonly-strip-signal.test.ts +++ b/packages/objectql/src/engine-readonly-strip-signal.test.ts @@ -253,16 +253,25 @@ describe('static `readonly` write strip — caller-facing signal (#4903)', () => expect(att().work_duration).toBe(480); }); - it('[#5591] a hook OVERWRITING a key the caller supplied now survives the strip', async () => { + it('[#5591] a hook OVERWRITING a key the caller supplied still survives the strip', async () => { // The mechanism, restated: the entry snapshot carries the caller's VALUES, // and the strip deletes a read-only key only while it still holds the // caller's own value. A hook that writes over it has replaced a caller // write with a platform write, and platform writes to read-only columns // are legitimate. Before #5591 the snapshot was a key SET, so the hook's // 999 was deleted and the column kept its stored null. + // + // [#16344] The hook is now UNCONDITIONAL, and that edit is the point + // rather than an accommodation. This case's subject is what happens to a + // hook's OWN write to a read-only column the caller also named, and that + // verdict is unchanged. The version before #16344 reached it through a + // guard — `if (ctx.input.data.work_duration !== undefined)` — which + // silently made the case depend on a SECOND fact: that the caller's + // forged value is visible to the hook. It no longer is, deliberately, and + // the two facts now get one assertion each (the sibling case below). storeFor('attendance').set('att_2', { id: 'att_2', status: 'open', work_duration: null }); engine.registerHook('beforeUpdate', async (ctx: any) => { - if (ctx.input.data.work_duration !== undefined) ctx.input.data.work_duration = 999; + ctx.input.data.work_duration = 999; }, { object: 'attendance' }); await engine.update('attendance', { id: 'att_2', status: 'closed', work_duration: 480 }); @@ -271,6 +280,35 @@ describe('static `readonly` write strip — caller-facing signal (#4903)', () => expect(storeFor('attendance').get('att_2').work_duration).toBe(999); }); + it('[#16344] a hook that GATES on seeing the caller-forged value does not fire — and the column keeps its stored value', async () => { + // The other half of the case above, and the behaviour change this card + // ships, asked directly rather than left implicit inside a guard. + // + // A `beforeUpdate` that keys on "did the caller send this read-only + // field" is reading a value the engine has ALREADY refused. Before + // #16344 it saw the forgery and could act on it — which is the whole + // defect: whatever such a hook derives IS persisted, so the caller + // steered a stored value through a column it was never allowed to write. + // The value is now hidden, the guard is false, and nothing is derived. + // + // ⚠️ Read `att_3`'s stored `work_duration` as the verdict: it keeps the + // value it had. The caller's 480 did not land (the strip never stopped + // working) and no hook-derived value landed either, because the hook + // correctly saw nothing to derive from. A hook that WANTS to know what + // the caller submitted reads `ctx.submitted`. + storeFor('attendance').set('att_3', { id: 'att_3', status: 'open', work_duration: 111 }); + const gateSaw: unknown[] = []; + engine.registerHook('beforeUpdate', async (ctx: any) => { + gateSaw.push({ input: ctx.input.data.work_duration, submitted: ctx.submitted?.work_duration }); + if (ctx.input.data.work_duration !== undefined) ctx.input.data.work_duration = 999; + }, { object: 'attendance' }); + + await engine.update('attendance', { id: 'att_3', status: 'closed', work_duration: 480 }); + expect(gateSaw).toEqual([{ input: undefined, submitted: 480 }]); + expect(storeFor('attendance').get('att_3')).toMatchObject({ status: 'closed' }); + expect(storeFor('attendance').get('att_3').work_duration).toBe(111); + }); + it('[#5591] with NO hook on the key, the caller-supplied value is still stripped', async () => { // The #2948 verdict, unchanged: this is the case the strip exists for, // and it must not have moved a millimetre. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index cfbbff531a..e54df038b6 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -11369,13 +11369,97 @@ export class ObjectQL implements IObjectQLEngine { // Single-row by construction: `update()` takes one payload, so there is // no partial-row mode to carry the verdict into — unlike `insert()`, the // refusal is simply thrown. One element in, one verdict out. + // + // [#16344] Hoisted from its old site below the dispatch ladder: the + // declared-field door and the pre-hook read-only pass both need this + // schema, and ONE lookup answering both is one fact rather than two that + // can drift. + const updateSchema = this._registry.getObject(object); const undeclared = undeclaredWriteFieldErrors( object, - this._registry.getObject(object) as { fields?: unknown } | undefined, + updateSchema as { fields?: unknown } | undefined, [opCtx.data], )[0]; if (undeclared) throw undeclared; + // ── [#16344] HIDE caller-forged read-only values from the hooks ────── + // + // The invariant, in the maintainer-confirmed ruling's words:「交给生命 + // 周期钩子的记录,就是它打算持久化的那条记录。」A hook handed a value + // the engine has ALREADY decided will not be stored can derive a column + // that IS stored, and that derived write is the HOOK's own — so nothing + // downstream takes it back. + // + // Measured on this change's base, with a control, in + // `engine-readonly-hook-input.test.ts`: one + // `PATCH { actual_value: 380, target_value: 1, weight: 1 }` against a + // `readonly` `target_value` committed `target_value = 400` — the strip + // DID work — beside a hook-derived trace reading `目标 1`. A row whose + // own audit trail cites a value it does not contain, with no error, no + // warning and a 200. The strip was never the defect; its POSITION was. + // + // ⭐ HIDE, not strip, and that word is the whole design. The enforcement + // point stays exactly where #2948 / #5591 / #14088 put it — after the + // hooks, where it is the only pass that can tell a hook's stamp from a + // caller's forgery (`hookWrittenKeys`). Moving it here instead would + // delete every server-side stamp a `beforeUpdate` makes to a read-only + // column, which is #5591 reintroduced. So this pass takes the caller's + // values out of the HOOKS' view only, and the confluence below hands + // them straight back before anything engine-owned reads the payload. + // + // What that buys: the post-hook declared-field door, + // `normalizeMultiValueFields`, `validateRecord`, the `readonlyWhen` + // strip, the static strip and therefore `onFieldsDropped`, the WARN and + // `strictReadonlyWrites` all see the payload they see today, and say the + // identical thing about it. ⛔ Feeding those channels from HERE instead + // is the shape to avoid: it would report — and under strict REFUSE — a + // whole-record write-back whose read-only key a hook goes on to restamp + // (the #5591 idiom), turning a write that succeeds today into a 400. + // + // ⛔ A hook that legitimately needs the caller's submission reads + // `ctx.submitted` (bound below) — the ruling's second half, and the + // reason nothing degrades: plugin-auth's ADR-0092 identity write guard + // names the non-whitelisted keys from THERE, so its 403 still says which + // field it refused. + // + // Placed BEFORE the recording is armed: a pass running inside that + // window is an ENGINE write recorded as a HOOK write, which is the + // laundering the seal note below exists to make impossible. + // + // ⛔ `id` is excluded — ADDRESSING IS NOT PAYLOAD (#8093), and the + // non-scalar case has its own owner further down (#6435), which reads + // the key where it already looks. Excluded through `supplied`, the one + // input that decides eligibility, so this pass never forms a second + // opinion about what an address is. + // + // ⛔ `readonlyWhen` is NOT hidden and must not be: a conditional lock is + // judged against the prior record — per ROW on the predicate path, where + // one shared payload cannot carry a per-row verdict — and #9107 + // deliberately leaves it hook-writable. + let readonlyHiddenFromHooks: Record | undefined; + if (!opCtx.context?.isSystem) { + const preHookPayload = opCtx.data as Record | null | undefined; + if (preHookPayload && typeof preHookPayload === 'object') { + const suppliedDataOnly: Record = { ...suppliedValues }; + delete suppliedDataOnly.id; + // No logger, deliberately: this pass is SILENT by construction. The + // strip below owns every word said about these keys. + const hidden = stripReadonlyFields( + updateSchema as any, preHookPayload, suppliedDataOnly, undefined, + { preserveAudit: opCtx.context?.preserveAudit === true }, + ) as Record; + if (hidden !== preHookPayload) { + readonlyHiddenFromHooks = {}; + for (const k of Object.keys(preHookPayload)) { + if (!(k in hidden)) readonlyHiddenFromHooks[k] = preHookPayload[k]; + } + // Kept in step with the hook payload, which the recording below + // arms over: the two must not name different objects. + opCtx.data = hidden as any; + } + } + } + // ── [#14088] ARM the hook-write recording ──────────────────────────── // // `suppliedValues` above answers "what did the caller send". This @@ -11416,6 +11500,25 @@ export class ObjectQL implements IObjectQLEngine { object, event: 'beforeUpdate', input: { id, data: hookWrites?.payload ?? opCtx.data, options: opCtx.options }, + // [#16344] The caller's submission AS SENT — the other half of the + // ruling, and the channel that keeps a submission-reading guard whole + // now that `input.data` is the persist image. It is the #5591 + // snapshot, which is already a COPY taken at engine entry before any + // middleware or hook stamp, so nothing a hook does can rewrite it and + // no hook write can leak back through it into the payload. + // + // Frozen at the boundary rather than trusted: `previous` is the only + // other read-only-by-contract member and it is a live driver row, so + // "diagnostics only" would otherwise be enforced by nothing. A hook + // that assigns here fails loudly in strict mode instead of silently + // editing a record of what the caller sent. + // + // Bound ONCE, on the batch context: `dispatchPerRowBeforeHooks`, + // `dispatchUnscopedMultiWriteHooks` and `buildPerRowAfterContexts` + // all build their contexts by spreading this one, so every dispatch + // of this write — both phases, every matched row — carries the same + // submission, which is what it is: one caller write, one submission. + submitted: Object.freeze({ ...suppliedValues }) as Record, session: this.buildSession(opCtx.context), provenance: this.buildProvenance(opCtx.context), // [#13644] The declared referential-cleanup marker. Conditional @@ -11497,7 +11600,6 @@ export class ObjectQL implements IObjectQLEngine { scope: {}, }; - const updateSchema = this._registry.getObject(object); // Pre-update snapshot. Exposed to hooks via `hookContext.previous` in // BOTH phases now (the HookContext contract documents `previous` for // update/delete) and reused for object-level validation rules and the @@ -11731,7 +11833,78 @@ export class ObjectQL implements IObjectQLEngine { // deliberately keeps the pre-#14088 over-strip instead. const sealedHookWrites = hookWrites?.seal(hookContext.input.data); if (sealedHookWrites) hookContext.input.data = sealedHookWrites.data as any; - const hookWrittenKeys = sealedHookWrites?.hookWrittenKeys; + let hookWrittenKeys = sealedHookWrites?.hookWrittenKeys; + + // ── [#16344] HAND BACK what was hidden from the hooks ──────────────── + // + // The other end of the pre-hook pass above, on the SAME confluence and + // for the same reason: this is the line at which the payload has stopped + // being the hooks' and has not yet been read by anything engine-owned. + // Restoring HERE rather than at each branch's strip is what keeps the + // change invisible below — one site covers both branches, so the two can + // never end up with different notions of what the hooks were shown. + // + // ⛔ Only keys the payload does not already hold. A hook that wrote one + // of these columns owns the value standing on it, and putting the + // caller's back over it is precisely the forgery the recording refuses. + // + // Placed AFTER the seal, deliberately: a hand-back inside the recording + // window would enter the record as a hook write, and the static strip + // below reads that record for provenance — so the caller's own forgery + // would be handed the one credential (`hookWrittenKeys`) that stops it + // being stripped. The exact laundering #14088 exists to prevent. + // + // ⛔ ...and SET-TO-UNDEFINED of a hidden key is a NO-OP, not a hook + // write. A hook that assigns a hidden key from the payload it was shown + // (`data.x = data.x`, the shape #14088's own pin names) reads + // `undefined` and RE-CREATES the key holding it. Left alone, three + // mechanisms agree the wrong way: the recorder's `set` trap counts it as + // a hook write, the hand-back below skips the key because `k in target`, + // and the strip keeps it on that record — so a driver is handed + // `{ x: undefined }`. On the memory driver that ERASES the stored + // read-only value; on a knex-backed one `formatInput` does not drop + // `undefined` and `builder.update(payload)` hands knex an undefined + // binding, a bare compile-time `Error` OUTSIDE the ADR-0112 envelope. + // Neither is "the record the engine intends to persist", which is the + // whole subject of this card. + // + // Undoing it here — delete the key, drop it from the record, let the + // ordinary hand-back put the caller's value back for the strip to judge + // — makes the write read EXACTLY as it would have with no hook at all: + // stripped, `onFieldsDropped` reporting it, the WARN said, and + // `strictReadonlyWrites` refusing. That identity IS the invariant this + // hide/hand-back pair exists to hold. + // + // ⛔ Dropping the key from `hookWrittenKeys` is NOT optional and is not + // tidiness: leaving it there while handing the caller's value back over + // it would credit the caller's forgery with hook provenance — the exact + // laundering the note above refuses, arrived at from the other side. The + // narrowing reaches only keys THIS pass hid, and only the one value no + // driver can store; a hook write of any real value is untouched, so the + // recorder's deliberate blindness to VALUE (#14088) is unchanged for + // every key a hook can actually see. + if (readonlyHiddenFromHooks) { + const restoreTargets = new Set | null | undefined>([ + hookContext.input.data as Record | null | undefined, + opCtx.data as Record | null | undefined, + ]); + const undoneSelfAssigns = new Set(); + for (const target of restoreTargets) { + if (!target || typeof target !== 'object') continue; + for (const [k, v] of Object.entries(readonlyHiddenFromHooks)) { + if (k in target && target[k] === undefined) { + delete target[k]; + undoneSelfAssigns.add(k); + } + if (!(k in target)) target[k] = v; + } + } + if (undoneSelfAssigns.size > 0 && hookWrittenKeys !== undefined) { + const narrowed = new Set(hookWrittenKeys); + for (const k of undoneSelfAssigns) narrowed.delete(k); + hookWrittenKeys = narrowed; + } + } // ── [#13657] The POST-hook half of the declared-field door ────────── // diff --git a/packages/plugins/plugin-auth/src/identity-write-guard.test.ts b/packages/plugins/plugin-auth/src/identity-write-guard.test.ts index d79487fc2f..cfa3847f8f 100644 --- a/packages/plugins/plugin-auth/src/identity-write-guard.test.ts +++ b/packages/plugins/plugin-auth/src/identity-write-guard.test.ts @@ -242,6 +242,152 @@ describe('identity write guard — update whitelist (ADR-0092 D2)', () => { }); }); +// ─────────────────────────────────────────────────────────────────────────── +// #16344 — the guard's DIAGNOSTICS against an engine whose `input.data` is the +// persist image. +// +// Since the maintainer ruling of decision batch #87 (2026-09-08) the engine +// hides a caller-forged statically-`readonly` value from `beforeUpdate`: what +// this guard is handed on `input.data` is what the engine intends to persist, +// and the caller's submission travels separately on `ctx.submitted`. +// +// ⚠️ The cases below are the ones the #5591 docblock predicted would degrade, +// and they are pinned AT THEIR PRE-#16344 TEXT — measured on the unfixed engine +// and reproduced verbatim here. That is this block's whole contract: the strip +// moved, and the caller still gets told which field was refused. A regression +// shows up as `(—)` in place of a field name, which is exactly the wording a +// reader of this file is meant to recognise. +// +// The fixture builds the contexts the POST-fix engine builds — `input.data` +// already missing the read-only key, `submitted` carrying the submission as +// sent — because this guard is unit-tested against a fake engine throughout. +// The end-to-end leg (a real ObjectQL engine dispatching a real `beforeUpdate`) +// lives in objectql's `engine-readonly-hook-input.test.ts`; this file pins the +// half that is plugin-auth's. +describe('identity write guard — caller submission channel (#16344)', () => { + let engine: ReturnType; + let warns: string[]; + + beforeEach(() => { + warns = []; + engine = makeEngine(SCHEMAS); + registerManagedUpdateWhitelist('sys_user', SYS_USER_PROFILE_EDIT_FIELDS); + registerIdentityWriteGuard(engine, { + packageId: 'test.identity-write-guard', + logger: { info() {}, warn: (m: string) => warns.push(String(m)) }, + }); + }); + + it('CASE B — a read-only-only payload still 403s NAMING the field, not `(—)`', async () => { + // Pre-#16344 reading, reproduced: `update sys_user { id, role: 'admin' }` + // answered `None of the submitted fields (role) are editable on + // 'sys_user'`. `role` is read-only, so the post-fix engine hides it before + // this hook runs and `input.data` arrives as `{ id }` alone. + await expect( + guardOn(engine, 'beforeUpdate')({ + object: 'sys_user', + session: USER_SESSION, + input: { id: 'u1', data: { id: 'u1' } }, + submitted: Object.freeze({ id: 'u1', role: 'admin' }), + }), + ).rejects.toThrow(/None of the submitted fields \(role\) are editable on 'sys_user'/); + }); + + it('CASE B, the regression shape — without the channel it degrades to `(—)`', async () => { + // The SAME request with no `submitted` member: this is what an engine that + // moved the strip and skipped the ruling's second half produces. Pinned as + // the failure it is, so the two readings sit side by side and neither can + // be mistaken for the other. It is also the honest statement of what this + // guard does on an older engine: refusal intact, field list empty. + await expect( + guardOn(engine, 'beforeUpdate')({ + object: 'sys_user', + session: USER_SESSION, + input: { id: 'u1', data: { id: 'u1' } }, + }), + ).rejects.toThrow(/None of the submitted fields \(—\) are editable/); + }); + + it('CASE A — a smuggled read-only field still WARNS by name while the legit half commits', async () => { + // Pre-#16344 reading, reproduced: `[IdentityWriteGuard] stripped + // non-whitelisted field(s) from user-context update to 'sys_user': role + // (ADR-0092)`. The write itself succeeds — `name` is whitelisted — so + // without the channel this warn simply disappears: a security diagnostic + // vanishing on a write that reports success. + const data: any = { id: 'u1', name: 'B' }; + await guardOn(engine, 'beforeUpdate')({ + object: 'sys_user', + session: USER_SESSION, + input: { id: 'u1', data }, + submitted: Object.freeze({ id: 'u1', name: 'B', role: 'admin' }), + }); + + expect(data).toEqual({ id: 'u1', name: 'B' }); + expect(warns).toEqual([ + "[IdentityWriteGuard] stripped non-whitelisted field(s) from user-context update to 'sys_user': role (ADR-0092)", + ]); + }); + + it('the two sources UNION — a guard-stripped key and an engine-hidden key are both named', async () => { + // `email` is not read-only, so it reaches this hook and THIS guard strips + // it; `role` is read-only, so the engine hid it and only `submitted` knows. + // Either source alone under-reports, which is why the guard reads both. + const data: any = { id: 'u1', name: 'B', email: 'evil@x' }; + await guardOn(engine, 'beforeUpdate')({ + object: 'sys_user', + session: USER_SESSION, + input: { id: 'u1', data }, + submitted: Object.freeze({ id: 'u1', name: 'B', email: 'evil@x', role: 'admin' }), + }); + + expect(data).toEqual({ id: 'u1', name: 'B' }); + expect(warns).toHaveLength(1); + expect(warns[0]).toContain('email'); + expect(warns[0]).toContain('role'); + }); + + it('⛔ the channel is READ, never written back — a refused value never reaches the payload', async () => { + // The one way this migration could turn a diagnostic into a privilege + // escalation: re-applying what `submitted` names, under the hook's own + // provenance, past the strip that refused it (#14088). Asked directly. + const data: any = { id: 'u1', name: 'B' }; + const submitted = Object.freeze({ id: 'u1', name: 'B', role: 'admin' }); + await guardOn(engine, 'beforeUpdate')({ + object: 'sys_user', session: USER_SESSION, input: { id: 'u1', data }, submitted, + }); + expect(data).not.toHaveProperty('role'); + expect(submitted).toEqual({ id: 'u1', name: 'B', role: 'admin' }); + }); + + it('`id` and the lifecycle stamps are not "refused fields" on this channel either', async () => { + // The submission carries the REST ingress fold's `id` (#6479) and the data + // routes' `updated_at`. Neither is a field the caller lost, so neither may + // appear in a message about fields that are not editable — the same two + // exclusions the payload loop already makes, asked of the new source. + const data: any = { id: 'u1', name: 'B', updated_at: '2026-09-09T00:00:00Z' }; + await guardOn(engine, 'beforeUpdate')({ + object: 'sys_user', + session: USER_SESSION, + input: { id: 'u1', data }, + submitted: Object.freeze({ id: 'u1', name: 'B', updated_at: '2026-09-09T00:00:00Z' }), + }); + expect(warns).toEqual([]); + expect(data).toEqual({ id: 'u1', name: 'B', updated_at: '2026-09-09T00:00:00Z' }); + }); + + it('an isSystem caller is untouched — the guard does not run, whatever the channel says', async () => { + const data: any = { id: 'u1', role: 'admin' }; + await guardOn(engine, 'beforeUpdate')({ + object: 'sys_user', + session: SYSTEM_SESSION, + input: { id: 'u1', data }, + submitted: Object.freeze({ id: 'u1', role: 'admin' }), + }); + expect(data).toEqual({ id: 'u1', role: 'admin' }); + expect(warns).toEqual([]); + }); +}); + describe('identity write guard — session snapshot refresh (ADR-0092 D6)', () => { const NOW = Date.now(); const EXPIRES = new Date(NOW + 3600_000).toISOString(); diff --git a/packages/plugins/plugin-auth/src/identity-write-guard.ts b/packages/plugins/plugin-auth/src/identity-write-guard.ts index 99252fbd80..a70ce0e7b4 100644 --- a/packages/plugins/plugin-auth/src/identity-write-guard.ts +++ b/packages/plugins/plugin-auth/src/identity-write-guard.ts @@ -210,18 +210,61 @@ export function registerIdentityWriteGuard(engine: any, opts: IdentityWriteGuard } } + // ── [#16344] What the CALLER submitted, for the DIAGNOSTIC half only ──── + // + // `input.data` is the record the engine intends to persist, and since + // #16344 that excludes a statically `readonly` field the caller supplied a + // value for: the engine strips it before this hook runs. ENFORCEMENT is + // unaffected — a field that never reaches the row cannot be written, and + // the loop above still deletes every non-whitelisted key that DID reach it. + // + // What WOULD have degraded is this guard's whole reason for naming things. + // Measured on the pre-#16344 engine and pinned in + // `identity-write-guard.test.ts`: `update sys_user { id, role: 'admin' }` + // — `role` read-only and not whitelisted — answered + // `403 None of the submitted fields (role) are editable`. With the strip + // moved ahead of the hooks and this guard still reading only `input.data`, + // the identical request answers `(—)`: as strong a refusal, saying nothing + // about what was refused. That degradation is the reason the ruling paired + // the strip move with a channel, and this is the migration onto it. + // + // So the field LIST is composed from `ctx.submitted` — the caller's + // payload as sent, which the engine publishes for exactly this + // (`HookContextSchema.submitted`: "diagnostics only, never the persist + // image"). ⛔ Never written back into `data`: re-applying a value the + // engine refused, from inside a hook, is the forgery #14088's write + // provenance exists to make impossible. + // + // UNION, not replacement, and both halves are load-bearing: `submitted` + // carries fields the ENGINE refused before this hook ever saw them, + // `stripped` carries the ones THIS guard refused, and only their union is + // "what you sent that is not editable here". Absent `submitted` — a + // non-update event, or an engine that does not publish it — degrades to + // today's list rather than to nothing. + const submitted = ctx.submitted as Record | undefined; + const refused = new Set(stripped); + if (submitted && typeof submitted === 'object') { + for (const key of Object.keys(submitted)) { + if (key === 'id') continue; + if (LIFECYCLE_PASSTHROUGH.has(key)) continue; + if (whitelist.has(key)) continue; + refused.add(key); + } + } + const refusedFields = [...refused]; + if (editableRemaining === 0) { throw forbidden( ctx.object, - `None of the submitted fields (${stripped.join(', ') || '—'}) are editable on ` + + `None of the submitted fields (${refusedFields.join(', ') || '—'}) are editable on ` + `'${ctx.object}' via the data API (ADR-0092). Editable fields: ` + `${[...whitelist].join(', ')}. For anything else, ${DEDICATED_SURFACE_HINT}.`, ); } - if (stripped.length > 0) { + if (refusedFields.length > 0) { logger?.warn( `[IdentityWriteGuard] stripped non-whitelisted field(s) from user-context update to ` + - `'${ctx.object}': ${stripped.join(', ')} (ADR-0092)`, + `'${ctx.object}': ${refusedFields.join(', ')} (ADR-0092)`, ); } }; diff --git a/packages/runtime/src/sandbox/hook-input-writeback-readonly-provenance.integration.test.ts b/packages/runtime/src/sandbox/hook-input-writeback-readonly-provenance.integration.test.ts index 0f74a9ee09..84a1d784e1 100644 --- a/packages/runtime/src/sandbox/hook-input-writeback-readonly-provenance.integration.test.ts +++ b/packages/runtime/src/sandbox/hook-input-writeback-readonly-provenance.integration.test.ts @@ -145,7 +145,15 @@ describe('#14760 — an untouched readonly key is not laundered by the sandbox w booted = null; }); - async function boot(source: string): Promise { + /** + * `opts.preHook` [#16344] registers a CODE hook ahead of the body, on the raw + * envelope (`ctx.input.data`). It exists so a case can put an object-valued + * readonly key on the payload the way the platform is allowed to — a hook's + * own write, which #5591/#14088 keep and this card did not touch — instead of + * the way a caller no longer can. See the write-THROUGH control for why that + * distinction became load-bearing. + */ + async function boot(source: string, opts?: { preHook?: (ctx: any) => void }): Promise { const dir = mkdtempSync(join(tmpdir(), 'os-14760-')); const driver = new SqlDriver({ client: 'better-sqlite3', @@ -175,6 +183,18 @@ describe('#14760 — an untouched readonly key is not laundered by the sandbox w body: { language: 'js', source, capabilities: ['log'] }, } as any], { packageId: 'guard' }); + // ⛔ Registered AFTER `bindHooksToEngine` and under a DIFFERENT packageId: + // the binder is hot-reload friendly and opens by calling + // `unregisterHooksByPackage(opts.packageId)`, so a pre-hook registered + // before it under `'guard'` is silently dropped — measured, as the body + // faulting on an absent key. Ordering within the event is by PRIORITY + // (lower first, default 100), never by registration order, so priority 1 + // still puts this ahead of the body. + if (opts?.preHook) { + engine.registerHook('beforeUpdate', async (ctx: any) => { opts.preHook!(ctx); }, + { object: 'guard_task', priority: 1, packageId: 'guard-prehook' }); + } + booted = { engine, driver, seen, dir }; return booted; } @@ -295,7 +315,29 @@ describe('#14760 — an untouched readonly key is not laundered by the sandbox w }, 60000); it('write-THROUGH control: leg 2 still carries an object the body mutated in place', async () => { - const { engine, driver, seen } = await boot(WRITES_THROUGH_SOURCE); + // [#16344] RE-ROUTED, and the subject is unchanged. This control asks one + // thing: can leg 2 carry a mutation made THROUGH an object-valued readonly + // key, which leg 1 (the `set` trap on `ctx.input`) structurally cannot see? + // It used to reach that question by having the CALLER put the object on the + // payload. Since #16344 a caller cannot: a caller-supplied static `readonly` + // value is hidden from `beforeUpdate`, so `ctx.input.locked_meta` would be + // `undefined` and the body would fault on the dereference — measuring the + // hide, not the write-back. + // + // So the object arrives the way the platform is still allowed to put it + // there: a code hook's own write, ahead of the body. That is #5591/#14088 + // semantics, which this card deliberately did not move, and it leaves the + // control strictly sharper — the value under test is now unambiguously + // hook-authored, so a pass cannot be explained by a caller value leaking + // through. Leg 1 still cannot see the body's in-place mutation (the body + // assigns no readonly key), so leg 2 is still the only thing that can carry + // it, which is the whole assertion. + // + // The old path — a caller supplying it — is not lost: it is pinned as its + // own case below, with the verdict #16344 gives it. + const { engine, driver, seen } = await boot(WRITES_THROUGH_SOURCE, { + preHook: (ctx) => { ctx.input.data.locked_meta = { who: 'platform' }; }, + }); await seed(driver); const seeded = await row(engine); seen.splice(0); @@ -303,7 +345,6 @@ describe('#14760 — an untouched readonly key is not laundered by the sandbox w await engine.update('guard_task', { id: seeded.id, status: 'done', - locked_meta: { who: 'caller' }, locked_note: 'CALLER', } as any); @@ -313,10 +354,53 @@ describe('#14760 — an untouched readonly key is not laundered by the sandbox w // `ctx.input.locked_meta.who = 'hook'` trips no trap on `ctx.input`, so leg // 1 cannot list it: only the normalised leg-2 comparison can carry it, and // only a carried key is recorded as hook-written and kept by the strip. + // `'hook'` rather than `'platform'` is what proves the body's in-place + // mutation survived — a write-back that had gone silent would leave the + // pre-hook value standing and is caught here. expect(asJson(after.locked_meta)).toEqual({ who: 'hook' }); expect(after.locked_note).toBe('SEEDED'); expect(after.touched_by).toBe('hook'); }, 60000); + + it('[#16344] a body that reads a CALLER-supplied readonly key no longer sees it — and says so instead of deriving from it', async () => { + // The old path of the control above, kept and re-judged rather than + // deleted, because the behaviour change is the point of the card and this + // is the one place in the repo that measures it end to end through a REAL + // sandbox: QuickJS, the flat-input proxy, the #14088 recorder and the strip. + // + // The caller forges the readonly `locked_meta`. Before #16344 the body saw + // it, mutated it in place, and the mutation PERSISTED — a stored value the + // caller steered through a column it may not write, which is this card's + // defect wearing its sandbox costume. Now the key is simply not on the + // hook's record, so `ctx.input.locked_meta.who = 'hook'` faults. + // + // ⚠️ That fault is a REFUSAL, not a silent no-op: a `body` hook's default + // `onError` is `abort`, so the caller's whole write is rejected and the row + // is untouched. Loud beats silent — but the message is a raw `TypeError` + // from the app's own dereference, which names nothing an author can act on. + // Recorded here rather than smoothed over: a body that needs the caller's + // submission has no `ctx.submitted` (it is deliberately not marshalled onto + // the sandbox face), and its supported source for a derived column is + // `ctx.previous`. + const { engine, driver } = await boot(WRITES_THROUGH_SOURCE); + await seed(driver); + const seeded = await row(engine); + + await expect(engine.update('guard_task', { + id: seeded.id, + status: 'done', + locked_meta: { who: 'caller' }, + locked_note: 'CALLER', + } as any)).rejects.toThrow(/locked_meta|cannot set property 'who' of undefined/); + + const after = await row(engine); + // ⭐ The verdict that matters: NOTHING the caller sent reached the row — + // not the forged readonly value, and not the writable `status` either, + // because the write was refused whole. + expect(asJson(after.locked_meta)).toEqual({ seeded: true }); + expect(after.locked_note).toBe('SEEDED'); + expect(after.status).toBe('open'); + }, 60000); }); /** diff --git a/packages/spec/authorable-surface/data.json b/packages/spec/authorable-surface/data.json index 5eb5fd5920..ce9a8eafbc 100644 --- a/packages/spec/authorable-surface/data.json +++ b/packages/spec/authorable-surface/data.json @@ -511,6 +511,7 @@ "data/HookContext:referentialFieldClear", "data/HookContext:result", "data/HookContext:session", + "data/HookContext:submitted", "data/HookContext:transaction", "data/HookContext:user", "data/ImportFieldMapping:params", diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index c92bd21baa..b8a1edd9dc 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -451,6 +451,33 @@ export const HookContextSchema = lazySchema(() => z.object({ * - delete (bulk, multi:true) — before, PER MATCHED ROW: { id: ID, options: EngineDeleteOptions } * - delete (bulk, multi:true) — after, PER MATCHED ROW: { id: ID, options: DriverOptions } * + * WHICH RECORD `data` IS, on `beforeUpdate` (#16344, maintainer ruling, + * decision batch #87, 2026-09-08) + * + * On the UPDATE verb `input.data` is **the record the engine intends to + * persist**, not the caller's submission. A `readonly` field the caller + * supplied a value for is stripped from it BEFORE the before phase is + * dispatched, so no hook can derive a persisted column from a value the row + * will never contain. What a hook that needs the caller's own words reads is + * {@link HookContext.submitted} — the same payload as sent, diagnostics only. + * + * WHICH fields, exactly: the subject set is the update strip's own + * (`stripReadonlyFields`), which is author-declared `readonly: true` AND the + * types whose value the runtime owns end to end — `autonumber` today + * (#5503), implicitly read-only whether or not the author wrote the flag. + * Deliberately the same set rather than a second opinion: a pass that hid a + * different set from the one enforced below would put the two out of step, + * which is the whole failure this ordering exists to remove. + * + * ⛔ NOT `readonlyWhen`. A conditional lock is judged against the prior + * record, per row on the predicate path, and #9107 leaves it hook-writable + * on purpose. + * + * Two things this deliberately does NOT change: a hook's OWN write to a + * read-only column still lands (#5591 / #14088 — the enforcement pass stays + * after the hooks, where provenance is knowable), and `beforeInsert` is + * untouched (ruling C, #14147, keeps the create-side strip post-hook). + * * DECLARATIVE SURFACE — what an app author is actually handed * * `bindHooksToEngine` wraps every metadata `Hook` in `wrapDeclarativeHook`, @@ -615,6 +642,89 @@ export const HookContextSchema = lazySchema(() => z.object({ */ previous: z.record(z.string(), z.unknown()).optional().describe('Record state before operation'), + /** + * Caller Submission (#16344) + * + * What the CALLER submitted — **diagnostics only, never the persist image**. + * + * ## The two records a `beforeUpdate` handler now has, and why they differ + * + * `input.data` is **the record the engine intends to persist**. A statically + * `readonly` field the caller supplied a value for is not in it: the engine + * has already decided that value will not be stored, so a hook deriving a + * column from it would derive a persisted column from a number the row will + * never contain. That is not hypothetical — it is the measured defect this + * key was ruled for (maintainer, decision batch #87, 2026-09-08): a KPI row + * committed `target_value = 400` beside a hook-derived `calc_trace` reading + * `目标 1`, with no error, no warning and a 200. A record whose own audit + * trail cites values it does not hold. + * + * `submitted` is the OTHER record: the caller's payload exactly as it + * arrived at the engine, snapshotted before any middleware or hook stamp + * (the #5591 entry snapshot). It exists because moving the strip ahead of + * the hooks would otherwise have DEGRADED, silently, every guard that + * reports on what the caller sent — plugin-auth's ADR-0092 identity write + * guard is the in-repo instance, and its 403 NAMES the non-whitelisted keys + * it found. It reads them from here, so the message is unchanged. + * + * ## ⛔ Never the persist image, and the boundary is mechanical + * + * Assigning to this record, or to a key on it, changes NOTHING about the + * write — the engine reads `input.data` and nothing else on the way to the + * driver. The object is SHALLOW-frozen by the producer, so an assignment to + * one of ITS OWN keys throws in strict mode rather than silently editing a + * record of what a caller sent. A handler that wants to change what is + * written writes `input.data`; a handler that wants to REFUSE a write + * throws; a handler that wants to know what the caller asked for reads this. + * + * ⚠️ SHALLOW is the honest word and the depth matters. The snapshot is a + * shallow spread of the caller's payload, so a NESTED object reached through + * a key here is the caller's own reference and is mutable — `Object.freeze` + * does not travel. That is not a laundering route (a nested mutation on a + * hidden read-only key is handed back and stripped; the recorder never saw a + * hook write), but it is not a deep guarantee either, and a handler must not + * treat a nested read from here as tamper-proof. Deep-freezing was not + * chosen: it costs a full walk of every payload on every update, to harden a + * face documented as diagnostics-only. + * + * ⚠️ A value HERE and no matching key in `input.data` means precisely one + * thing: the engine refused that field. It does not mean the field is + * unknown, absent from the object, or safe to re-apply — re-applying it from + * a hook is writing the value the engine just refused, under the hook's own + * provenance, which is the forgery `hookWrittenKeys` (#14088) exists to + * make impossible. + * + * ## Where it is bound + * + * The UPDATE verb, both phases, every dispatch of one caller write — the + * batch context and every per-row context spread from it carry the SAME + * submission, because one caller write has one submission however many rows + * it matches. `id` is present when the caller sent one (including the REST + * ingress fold, #6479), because this is what the caller submitted rather + * than a payload the engine curated. + * + * ⛔ NOT bound on `insert` or `delete`, and that is a scope statement rather + * than an omission: the create side's strip position is settled POST-hook by + * ruling C (#14147, "one semantics, one enforcement point"), so `beforeInsert` + * still receives the caller's own values in `input.data` and needs no second + * channel to see them. Whether it should is a separate measurement, raised + * against #14147 if a create-side leak is ever measured. + * + * ⛔ NOT marshalled into the sandboxed `body` face, for the same reason + * `dispatch.scope` is not: the body surface is assembled key by key + * (`buildSandboxContext`), and a key added there is a second published + * contract with its own compatibility story. A `body` needing it is the + * signal to raise that question, not to add it silently. + * + * OPTIONAL for the reason `api` and `dispatch` are: making it required would + * start rejecting the partial contexts `HookContextSchema.parse` accepts + * today. Read it as `ctx.submitted?.[field]` — an ABSENT record reads as + * "nothing known about the submission", which is the back-compatible + * direction and what every non-update event carries. + */ + submitted: z.record(z.string(), z.unknown()).optional() + .describe('What the caller submitted, as sent (update only) — diagnostics only, never the persist image'), + /** * Dispatch Marker * How THIS hook call relates to the caller's write — and the one place a