diff --git a/.changeset/hook-provenance-sibling-seams.md b/.changeset/hook-provenance-sibling-seams.md new file mode 100644 index 0000000000..962e825a17 --- /dev/null +++ b/.changeset/hook-provenance-sibling-seams.md @@ -0,0 +1,50 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): decide the insert-side runtime-owned strip by hook-write PROVENANCE, not `Object.is` (#14259) + +#14088 replaced `Object.is(payload[k], supplied[k])` inside `stripReadonlyFields` +with a recording of the keys the before-phase hook chain actually assigned +(`recordHookPayloadWrites`). Its argument was never about `null`: value equality +cannot separate *the hook deliberately wrote the value the caller also sent* from +*the hook never touched the key*, and those two demand opposite verdicts. + +`stripRuntimeOwnedFields` — the INSERT-side twin — was left on the comparison +that argument retired, and #6339's own prose is the finding: it argued a key SET +made the contract true "only BY ACCIDENT" and moved to values, which is +accidental in the identical way. A `beforeInsert` hook that re-issues or +normalises a record number therefore still lost its write to any caller that +submitted the same value — the caller who omitted the key kept the hook's number, +the caller who echoed it got the sequence value, and the two differed in nothing +else. + +`engine.insert` now arms one recording **per row** at hook-context construction +and seals each immediately after that row's `beforeInsert` chain, and +`stripRuntimeOwnedFields` consults the sealed record before the value test. Per +row, never per call, so a hook stamping one row of a batch confers nothing on the +next. + +⛔ **Not a relaxation of #5503, and the accept set for callers does not move.** A +caller-seeded record number that no hook assigned is still stripped, still warns +with the same text, and still reports through `onFieldsDropped` / +`strictReadonlyWrites`; `isSystem` and `preserveAudit` are untouched. What +changed is only the EVIDENCE for the hook-write exemption that already existed — +a record of which keys were assigned, instead of an inference from the values +afterwards. + +The forgery boundary is inherited verbatim: a caller-supplied value must never +become hook-owned. The new insert-side recording is armed after the caller's +payload has arrived and been snapshotted, sealed before any engine-owned pass +touches the row, and records that an assignment ran rather than anything about +the payload's contents — a caller cannot execute an assignment, so no key it +sends can enter the record. A hook that REPLACES the payload object leaves no +attributable record and falls back to the pre-existing value test, which +over-strips: keeping the old bug is the only safe direction, because reading a +replacement's keys as hook-owned would launder a caller's forgery. + +The `readonlyWhen` sibling seam #14259 also names (`isCallerSuppliedValue`, +behind `stripReadonlyWhenFields` / `stripReadonlyWhenFieldsMulti`) is **not** +included: threading the record there was measured to let a caller's value survive +a TRUE `readonlyWhen` predicate, which is a maintainer decision rather than a +mechanical follow-through. Nothing about that seam's behaviour changes here. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index a6fd7f3811..b8856cbf46 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -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:11049` | -| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11217` | -| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9827` | +| 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:11128` | +| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11296` | +| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9895` | | 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1746` | -| 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:9864`, `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:5761` | -| 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:3605`, `:3615`, `:3642` | +| 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:9943`, `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:5762` | +| 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:3606`, `:3616`, `:3643` | | 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:6459` | -| 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:11810` | -| 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:11739` | +| 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:6460` | +| 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:11889` | +| 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:11818` | ### 3. Sharing (`plugin-sharing`) @@ -179,8 +179,8 @@ a reader tracing where elevation travels needs them. | # | Site | Package | What it does | |:--|:---|:---|:---| -| 62 | `objectql/src/engine.ts:3412` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | -| 63 | `objectql/src/engine.ts:14159` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | +| 62 | `objectql/src/engine.ts:3413` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | +| 63 | `objectql/src/engine.ts:14238` | 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 | @@ -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:1971` (rationale at `:1881`–`1883`, #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:9810`–`9827` | +| "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:9878`–`9895` | | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1516` (#3493 / #6640) | | "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` | diff --git a/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts b/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts new file mode 100644 index 0000000000..c0b3fd4908 --- /dev/null +++ b/packages/objectql/src/engine-hook-provenance-sibling-seams.test.ts @@ -0,0 +1,383 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #14259 — the INSERT-side runtime-owned strip must decide hook-vs-caller by +// the same RECORD `stripReadonlyFields` uses, not by `Object.is`. +// +// #14088 replaced `Object.is(payload[k], supplied[k])` inside +// `stripReadonlyFields` with a recording of the keys the before-phase hook +// chain actually assigned (`recordHookPayloadWrites`). Its argument was never +// about `null`: value equality cannot separate +// +// - the hook deliberately wrote the value the caller also sent, from +// - the hook never touched the key at all, +// +// and the two demand opposite verdicts. `stripRuntimeOwnedFields` — the +// INSERT-side twin — was left on the comparison that argument retired, and +// #6339's own prose is the finding: it argued a key SET made the contract true +// "only BY ACCIDENT" and moved to VALUES, which is accidental in the identical +// way. So a `beforeInsert` hook that re-issues or normalises a record number +// still loses its write to the one caller that submitted the same value. +// +// ⛔ THE SIBLING SEAM IS DELIBERATELY NOT HERE. #14259 named a second one — +// `isCallerSuppliedValue`, behind the two `readonlyWhen` strips — and it is +// WITHHELD pending a maintainer ruling, not forgotten. Measured on this branch: +// threading the record into that predicate turns the existing #9107 pin +// `LOCK 3b` red, because a hook spelled `ctx.input.data.x = ctx.input.data.x` +// is a `set` on the recorded object, so the CALLER's forged value becomes +// hook-owned and survives a TRUE `readonlyWhen` predicate (measured: +// `closed_note` committed `'1999-01-01'` where the lock had stripped it to +// `null`). That is the card's own fork clause — a caller value surviving a TRUE +// predicate — and it goes to the decision inbox, never resolved here. +// +// ⛔ WHAT THIS SUITE IS NOT, and is written to fail if anyone reads it that +// way: it is NOT a relaxation of #5503. The DISCRIMINATOR PAIRS are the +// deliverable's proof — the same caller payload, byte for byte, on the same +// runtime-owned key, reaching OPPOSITE verdicts depending on whether a hook +// assigned it. A caller-seeded record number that no hook wrote is still +// stripped, still warns with the same text, and still reports through +// `onFieldsDropped` / `strictReadonlyWrites`. That is what value equality +// cannot deliver and a record can. +// +// ⛔ And the forgery boundary is inherited unchanged: A CALLER-SUPPLIED VALUE +// MUST NEVER BECOME HOOK-OWNED. The insert-side recording this card arms is new +// (the update path's already existed), so it owes the same three properties, +// and they are pinned below: armed after the caller's payload has arrived, +// sealed before any engine-owned pass touches it, and recording that an +// assignment ran rather than anything about the payload's contents. + +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 more rows than the engine asked for, and a test written + // against it passes for a reason the real driver does not share. + 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[]) { + const out = []; + for (const r of rows) out.push(await this.create(object, r, undefined)); + return out; + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, storeFor }; +} + +const makeLogger = (sink: string[]) => { + const logger: any = { + warn: (m: string) => sink.push(String(m)), + debug() {}, info() {}, error() {}, trace() {}, fatal() {}, + child() { return logger; }, + }; + return logger; +}; + +// ───────────────────────────────────────────────────────────────────────────── +// SEAM 2 — runtime-owned (`autonumber`), INSERT path +// ───────────────────────────────────────────────────────────────────────────── + +describe('seam 2 — the insert-side runtime-owned strip reads PROVENANCE (#14259)', () => { + let engine: ObjectQL; + let storeFor: ReturnType['storeFor']; + let warns: string[]; + let hookSaw: Array>; + + beforeEach(async () => { + warns = []; + hookSaw = []; + engine = new ObjectQL({ logger: makeLogger(warns) }); + const d = makeDriver(); + storeFor = d.storeFor; + engine.registerDriver(d.driver, true); + await engine.init(); + + engine.registry.registerObject({ + name: 'prov_ticket', + fields: { title: { type: 'text' }, code: { type: 'autonumber' } }, + } as any, 'test'); + + // #6339's own hook, unchanged: a `beforeInsert` that OWNS the record number + // — it re-issues or normalises it rather than letting the sequence decide. + // `title: 'no-hook'` short-circuits it, which is how a test tells "the hook + // did not fire" from "the hook fired and lost". + engine.registerHook('beforeInsert', async (ctx: any) => { + hookSaw.push({ ...(ctx.input.data as Record) }); + if (ctx.input.data.title === 'no-hook') return; + ctx.input.data.code = `HOOK-${String(ctx.input.data.title)}`; + }, { object: 'prov_ticket', priority: 50 }); + }); + + // ── THE DEFECT ──────────────────────────────────────────────────────────── + + it('THE DEFECT: a hook write the caller ECHOED now LANDS on a runtime-owned field', async () => { + // The whole-record POST: read a record, edit a field, submit everything + // back — so the payload necessarily echoes the record-number column it just + // read, and the hook re-issues the SAME number (an idempotent normalise, or + // a retried submit). `Object.is('HOOK-B', 'HOOK-B')` is true, so the + // pre-#14259 strip deleted the hook's deliberate write and the sequence + // value went to the database instead. + const row: any = await engine.insert('prov_ticket', { title: 'B', code: 'HOOK-B' }); + + // The regression, stated as the value it must NOT be. The sequence renders + // through the contract default `{0000}` since #6555 / #7262. + expect(row.code).not.toBe('0001'); + expect(row.code).toBe('HOOK-B'); + expect(warns).toEqual([]); + }); + + it('the echoing caller and the omitting caller now AGREE — the difference was the accident', async () => { + // #6339's own proof shape, re-run on the input its fix could not see. The + // two calls differ in nothing but whether the caller's payload happened to + // carry the value the hook was going to write. + const omitted: any = await engine.insert('prov_ticket', { title: 'X' }); + const echoed: any = await engine.insert('prov_ticket', { title: 'X', code: 'HOOK-X' }); + expect(omitted.code).toBe(echoed.code); + expect(echoed.code).toBe('HOOK-X'); + }); + + // ── ⛔ THE NEGATIVE CONTROL — the deliverable's proof ────────────────────── + + it('⛔ NEGATIVE CONTROL: the IDENTICAL caller payload with NO hook write is still STRIPPED', async () => { + // `title: 'no-hook'` makes the hook return without assigning, so the value + // standing on `code` is the caller's seed and #5503 takes it. Byte-identical + // key and value shape to THE DEFECT above; opposite verdict. + const row: any = await engine.insert('prov_ticket', { title: 'no-hook', code: 'HOOK-no-hook' }); + + expect(row.code).not.toBe('HOOK-no-hook'); + expect(row.code).toBe('0001'); + // The same warning text as before this card — contract of the text, not its + // wording, per #5503's own pin discipline. + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("Field 'code' on 'prov_ticket'"); + expect(warns[0]).toContain('runtime-owned'); + expect(warns[0]).toContain('COMMITTED WITHOUT IT'); + expect(warns[0]).toContain('hook-written keys are not caller-supplied'); + }); + + it('⛔ NEGATIVE CONTROL: the drop still reaches onFieldsDropped as readonly', async () => { + const events: any[] = []; + await engine.insert( + 'prov_ticket', + { title: 'no-hook', code: 'CALLER-SEEDED' }, + { onFieldsDropped: (e: any) => events.push(e) } as any, + ); + expect(events.some((e) => e.reason === 'readonly' && e.fields.includes('code'))).toBe(true); + }); + + it('⛔ NEGATIVE CONTROL: strictReadonlyWrites still REFUSES the unauthorised seed', async () => { + let refused: any; + await engine.insert( + 'prov_ticket', + { title: 'no-hook', code: 'CALLER-SEEDED' }, + { strictReadonlyWrites: true } as any, + ).catch((e: unknown) => { refused = e; }); + + // The refusal ENVELOPE, not merely "it threw" (see the seam 1 twin for why + // `status` is not asserted: this class carries `code` and `name` only). + expect(refused).toBeDefined(); + expect(refused.name).toBe('ReadonlyFieldRejectedError'); + expect(refused.code).toBe('ERR_READONLY_FIELD_REJECTED'); + expect(refused.fields).toContain('code'); + expect(refused.operation).toBe('insert'); + // Refused BEFORE any driver dispatch. + expect(storeFor('prov_ticket').size).toBe(0); + }); + + it('⛔ a hook that runs but writes some OTHER key confers nothing on `code`', async () => { + engine.registerHook('beforeInsert', async (ctx: any) => { + ctx.input.data.title = `${String(ctx.input.data.title)}!`; + }, { object: 'prov_ticket', priority: 60 }); + + const row: any = await engine.insert('prov_ticket', { title: 'no-hook', code: 'CALLER-SEEDED' }); + + expect(row.title).toBe('no-hook!'); + expect(row.code).toBe('0001'); + }); + + // ── PER ROW, never per call ─────────────────────────────────────────────── + + it('a BATCH records per ROW: a hook stamping row 0 does not exempt row 1', async () => { + // The property a single shared recording would break, and the reason the + // insert path arms one recorder per row rather than one per call. + const rows: any = await engine.insert('prov_ticket', [ + { title: 'A', code: 'HOOK-A' }, + { title: 'no-hook', code: 'HOOK-A' }, + ] as any); + + expect(rows[0].code).toBe('HOOK-A'); + // Row 1's hook returned without assigning, so its caller seed is stripped + // and the sequence issues the number — even though the value is the very + // one row 0's hook legitimately wrote. + expect(rows[1].code).not.toBe('HOOK-A'); + // `0001`, not `0002`: `applyAutonumbers` fills only an EMPTY slot, and row + // 0 kept its hook-written code, so row 1 is the batch's first draw. Pinned + // at the measured value rather than at the row index — reading the counter + // as "one per row" would be a guess this very fix makes wrong. + expect(rows[1].code).toBe('0001'); + expect(warns).toHaveLength(1); + expect(warns[0]).toContain("Field 'code' on 'prov_ticket'"); + }); + + // ── The forgery boundary the NEW recording owes (card, verbatim) ─────────── + + it('FORGERY BOUNDARY: the recording is armed AFTER the caller payload arrives', async () => { + // "armed after the caller's payload has arrived": the hook sees the caller's + // own keys and values, and the caller's object is not the recorded one — so + // nothing the caller sent is in the record and the strip still judges + // against what the caller actually sent (`suppliedPerRow`, an explicit + // shallow copy taken ahead of the hooks, #6339's P3 invariant). + const payload: Record = { title: 'no-hook', code: 'CALLER-SEEDED' }; + const row: any = await engine.insert('prov_ticket', payload); + + expect(hookSaw[0]).toEqual({ title: 'no-hook', code: 'CALLER-SEEDED' }); + expect(hookSaw[0]).not.toBe(payload); + expect(payload).toEqual({ title: 'no-hook', code: 'CALLER-SEEDED' }); + // The caller's echo did not become hook-owned. + expect(row.code).toBe('0001'); + }); + + it('FORGERY BOUNDARY: the recording is SEALED before any engine-owned pass', async () => { + // "sealed before any engine-owned pass touches it". `applyAutonumbers` is + // the engine-owned writer of exactly this column and it runs AFTER the + // strip; if the recorder were still armed for it, the sequence value would + // register as a hook write. Read as an outcome: on the `no-hook` row the + // engine's own autonumber write does NOT exempt the caller's seed, so the + // strip still fires and still warns. + const row: any = await engine.insert('prov_ticket', { title: 'no-hook', code: 'CALLER-SEEDED' }); + expect(row.code).toBe('0001'); + expect(warns).toHaveLength(1); + }); + + it('FORGERY BOUNDARY: the record says an assignment RAN, not what the value was', async () => { + // "it records that an assignment ran rather than anything about the + // payload's contents". A hook that assigns the key the caller's OWN value, + // deliberately, is a hook write — that is the whole ruling — and a hook that + // assigns a different one is equally a hook write. Same verdict, two + // values: the record is blind to contents. + const same: any = await engine.insert('prov_ticket', { title: 'S', code: 'HOOK-S' }); + const diff: any = await engine.insert('prov_ticket', { title: 'D', code: 'CALLER-SEEDED' }); + expect(same.code).toBe('HOOK-S'); + expect(diff.code).toBe('HOOK-D'); + expect(warns).toEqual([]); + }); + + it('a hook that REPLACES ctx.input.data falls back to the value test, not to "keep everything"', async () => { + // The recorder's KNOWN LIMIT on the insert path. The replacement carries the + // caller's own `code`, and reading a replacement's keys as hook-owned would + // launder a caller-seeded record number into a platform write — so the + // fallback deliberately keeps the pre-#14259 over-strip. + engine.registerHook('beforeInsert', async (ctx: any) => { + if (ctx.input.data.title !== 'replace') return; + ctx.input.data = { ...ctx.input.data }; + }, { object: 'prov_ticket', priority: 60 }); + + const row: any = await engine.insert('prov_ticket', { title: 'replace', code: 'HOOK-replace' }); + + // The priority-50 hook assigned `HOOK-replace` on the RECORDING view; the + // priority-60 hook then replaced the object, so there is no attributable + // record and the value test judges the result. The caller sent the same + // value, so it is stripped — the old bug, kept on purpose. + expect(row.code).not.toBe('HOOK-replace'); + expect(row.code).toBe('0001'); + }); + + // ── 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 () => { + // ⚠️ RECORDING BEHAVIOUR, NOT BLESSING IT. This is the direct consequence + // of the mechanism the card mandates: the record says an ASSIGNMENT RAN and + // is deliberately blind to the value, so `ctx.input.data.code = + // ctx.input.data.code` — which computes nothing — is a `set`, and the + // caller's seed becomes hook-owned and survives. `title: 'no-hook'` + // short-circuits the priority-50 hook so the self-assignment is the ONLY + // write to `code`, which is what makes the surviving value the caller's. + // + // Pinned so the consequence is visible rather than discovered later, and + // pinned rather than argued: it is the exact shape that forked the + // `readonlyWhen` seam out of this PR, whose #9107 pin `LOCK 3b` pins the + // OPPOSITE verdict for a STATE lock ("a hook that writes the caller value + // BACK is the caller value, and goes"). #14259's fork clause sends that one + // to the decision inbox; nothing here resolves it. + // + // Why the same mechanism ships on THIS seam: `stripRuntimeOwnedFields` + // guards a runtime-owned COLUMN (#5503) — the same class of protection + // #14088 already moved to provenance on the update side, and the class + // whose exemption `runtimeOwnedStripWarning` promises hook authors in prose + // — not a STATE lock whose whole purpose is that no caller write survives a + // TRUE predicate. INSERT is exempt from `readonlyWhen` entirely, so no lock + // of that class exists on this path to open. A ruling that self-assignment + // must NOT count would move this pin and #14088's seam together; that is a + // deliberate follow-up, not silent drift. + engine.registerHook('beforeInsert', async (ctx: any) => { + ctx.input.data.code = ctx.input.data.code; + }, { object: 'prov_ticket', priority: 60 }); + + const row: any = await engine.insert('prov_ticket', { title: 'no-hook', code: 'CALLER-SEEDED' }); + + expect(row.code).toBe('CALLER-SEEDED'); + expect(warns).toEqual([]); + }); + + // ── The exemptions above this seam are untouched ────────────────────────── + + it('#5503 UNCHANGED: an isSystem caller still bypasses the whole pass', async () => { + const row: any = await engine.insert( + 'prov_ticket', { title: 'no-hook', code: 'SEEDED' }, + { context: { isSystem: true } } as any, + ); + expect(row.code).toBe('SEEDED'); + expect(warns).toEqual([]); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 3c1fa3da9a..0c1b225bfc 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -194,6 +194,7 @@ import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, str // SAME value. Armed and sealed in `update()`; the module owns the argument for // why neither end may move. import { recordHookPayloadWrites } from './hook-write-provenance.js'; +import type { HookWriteRecording } from './hook-write-provenance.js'; import { resolveMasterDetailRelation } from './master-detail.js'; // [#6457] The master-detail header a `parent`-scoped predicate reads is made // total over the MASTER's declared fields before it leaves this engine — the @@ -9621,19 +9622,62 @@ export class ObjectQL implements IObjectQLEngine { // is created once per CALL and shared by every row's context, before and // after — see `HookContext.dispatch`. const insertScope: Record = {}; + // ── [#14259] ARM the hook-write recording, one PER ROW ─────────────── + // + // The insert-side twin of the update path's single recording (#14088), + // and the reason it has to exist here at all: `stripRuntimeOwnedFields` + // below judges hook-vs-caller by `Object.is`, which cannot separate "the + // hook re-issued the record number the caller also submitted" from "the + // hook never touched the key". #6339 moved that judgement from a key SET + // to values on the argument that the key set made the contract true "only + // BY ACCIDENT" — and values are accidental in the identical way. + // + // PER ROW, not per call: a batch runs `beforeInsert` once per row against + // its own payload object, so one shared record would let a hook that + // stamps row 3 exempt row 4's caller-seeded value. One recording per row + // is what makes the answer mean "a hook assigned THIS key on THIS row". + // + // The two ends of the window are load-bearing, exactly as on the update + // path (the recorder module's own header carries the full argument): + // + // - ARMED HERE, over `defaultedData` — after the caller's payload has + // arrived, after `suppliedPerRow` snapshotted it, and after the + // engine's own defaulting (`applyFieldDefaults` / + // `initializeSummaryFields`) has run, so no engine default and no + // caller key can enter the record. A caller cannot execute an + // assignment; echoing a key, a value, a `null` or a `Proxy` back is + // not a `set` on this object. + // - SEALED immediately below, before the post-hook declared-field door + // reads the payload and long before `encryptSecretFields` writes to + // it. A recorder still armed for an engine-owned pass would report + // ENGINE writes as HOOK writes, which on a caller-forged secret column + // is precisely the escalation this is built to make impossible. + // + // Between those two points the only code that runs is the `beforeInsert` + // dispatch — server code, by definition. Writes through the recording + // land on the SAME row object, so a hook mutating `ctx.input.data.x` in + // place is mutating the engine's row exactly as it always has. + const rowHookWrites: Array = []; const rowHookContexts: HookContext[] = (isBatch ? (defaultedData as any[]) : [defaultedData]).map( - (row, rowIndex) => ({ - object, - event: 'beforeInsert', - input: { data: row, options: opCtx.options }, - dispatch: { mode: isBatch ? 'per-row' : 'record', index: rowIndex, scope: insertScope }, - session: this.buildSession(opCtx.context), - provenance: this.buildProvenance(opCtx.context), - user: this.buildUser(opCtx.context), - api: this.buildHookApi(opCtx.context), - transaction: opCtx.context?.transaction, - ql: this, - }), + (row, rowIndex) => { + const recording = + row !== null && typeof row === 'object' + ? recordHookPayloadWrites(row as Record) + : undefined; + rowHookWrites[rowIndex] = recording; + return { + object, + event: 'beforeInsert', + input: { data: recording?.payload ?? row, options: opCtx.options }, + dispatch: { mode: isBatch ? 'per-row' : 'record', index: rowIndex, scope: insertScope }, + session: this.buildSession(opCtx.context), + provenance: this.buildProvenance(opCtx.context), + user: this.buildUser(opCtx.context), + api: this.buildHookApi(opCtx.context), + transaction: opCtx.context?.transaction, + ql: this, + }; + }, ); // [#8682] A row the declared-field door refused runs NO hook. In // non-partial mode the throw above already returned, so this skip only @@ -9644,6 +9688,30 @@ export class ObjectQL implements IObjectQLEngine { if (undeclaredPerRow[i] !== undefined) continue; await this.triggerHooks('beforeInsert', rowHookContexts[i]); } + // ── [#14259] SEAL, before anything engine-owned reads or writes a row ── + // + // Sealing does two things and the write is wrong without either: it + // freezes each record before the engine's own passes can be mis-recorded + // as hook writes, and it puts the RAW row back in `input.data` so no + // recording view reaches a driver. + // + // Every row is sealed, including one the declared-field door culled: that + // row ran no hook, so its record is legitimately EMPTY, and restoring its + // raw payload keeps `rows[i]` a plain object on every branch rather than + // only on the live ones. + // + // `hookWrittenKeys` is `undefined` — not empty — for a row whose hook + // REPLACED the payload object (`ctx.input.data = { …ctx.input.data }`). + // `stripRuntimeOwnedFields` must read that as "fall back to the #6339 + // value test", never as "no hook wrote anything": a replacement's keys + // are mostly the CALLER's, so treating them as hook-owned would launder a + // caller-seeded record number into a platform write. + const rowHookWrittenKeys: Array | undefined> = []; + for (let i = 0; i < rowHookContexts.length; i++) { + const sealed = rowHookWrites[i]?.seal(rowHookContexts[i]!.input.data); + if (sealed) rowHookContexts[i]!.input.data = sealed.data as any; + rowHookWrittenKeys[i] = sealed?.hookWrittenKeys; + } // ── [#13657] The POST-hook half of the declared-field door ─────────── // @@ -9834,9 +9902,20 @@ export class ObjectQL implements IObjectQLEngine { // `origin/main`: `driverCreates 0` while the line said the write // was "COMMITTED WITHOUT IT". The card marked this half UNVERIFIED; // it reproduces, so the flag is threaded here too. + // [#14259] `hookWrittenKeys` — THIS row's sealed record, the other + // half of the same question `suppliedPerRow[i]` answers. #6339 + // handed values over instead of a key set because "the caller named + // this key" and "this key still holds the caller's value" are + // different facts; the record closes the case values cannot reach, + // where the hook wrote the value the caller also sent. The value + // test stays as the fallback for any row with no record. const stripped = stripRuntimeOwnedFields( schemaForValidation as any, rows[i], suppliedPerRow[i] ?? {}, this.logger, - { preserveAudit, strictReadonlyWrites: options?.strictReadonlyWrites === true }, + { + preserveAudit, + strictReadonlyWrites: options?.strictReadonlyWrites === true, + hookWrittenKeys: rowHookWrittenKeys[i], + }, ) as Record; if (stripped === rows[i]) continue; for (const k of Object.keys(rows[i])) { diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 2aaaa242b0..a75f6c74d6 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -1300,25 +1300,63 @@ export function stripReadonlyFields( * `onFieldsDropped` / `strictReadonlyWrites`. What changed is exclusively the * case where the value being deleted was never the caller's. * + * ### ...and why VALUES were not enough either (#14259) + * + * #6339's own sentence is the finding, one iteration on: it argued that a key + * SET made the contract true "only BY ACCIDENT", and value equality is + * accidental in the identical way. `Object.is(row[name], supplied[name])` + * cannot separate *the hook re-issued the record number the caller happened to + * submit* from *the hook never touched the key*, and those demand opposite + * verdicts — so #6339's own measured row (a `beforeInsert` hook that re-issues + * or normalises `code`) still loses its write to the one caller who submitted + * the same value. `options.hookWrittenKeys` answers by RECORD what the + * comparison could only infer; the comparison stays as the fallback for every + * key no record covers. + * * KNOWN LIMIT, identical to the update side's and deliberately not papered over: - * the snapshot is SHALLOW, so a hook that mutates a caller-supplied object IN - * PLACE is indistinguishable from a hook that did nothing, and the field is - * still stripped. That fallback is the pre-#6339 behaviour, i.e. fail-safe; a - * hook meaning to own a runtime-owned column should ASSIGN to it. (An - * `autonumber` value is a scalar in every supported shape, so this limit is - * theoretical here in a way it is not for the update path's `json` columns.) + * without a record the snapshot is SHALLOW, so a hook that mutates a + * caller-supplied object IN PLACE is indistinguishable from a hook that did + * nothing, and the field is still stripped. That fallback is the pre-#6339 + * behaviour, i.e. fail-safe; a hook meaning to own a runtime-owned column + * should ASSIGN to it. (An `autonumber` value is a scalar in every supported + * shape, so this limit is theoretical here in a way it is not for the update + * path's `json` columns.) */ export function stripRuntimeOwnedFields( objectSchema: { name?: string; fields?: Record } | undefined | null, data: Record | undefined | null, supplied: Readonly>, logger?: EvaluateRulesOptions['logger'], - options?: { preserveAudit?: boolean; strictReadonlyWrites?: boolean }, + options?: { + preserveAudit?: boolean; + strictReadonlyWrites?: boolean; + /** + * [#14259] The keys the `beforeInsert` hook chain ACTUALLY ASSIGNED on THIS + * ROW, recorded while the writes happened (`recordHookPayloadWrites`, armed + * per row at `engine.insert`'s hook-context construction and sealed + * immediately after that row's dispatch). The insert-side twin of the + * option #14088 gave {@link stripReadonlyFields}, and read by the same + * rules: OPTIONAL, and absent means "this call cannot say", never "no hook + * wrote anything" — a direct caller, and any row whose hook REPLACED the + * payload object, fall back to the `Object.is` test below exactly as before + * this option existed. + * + * ⚠️ Per ROW, never per call: one recording is armed for each row of a + * batch, so a hook that stamps row 3 cannot exempt row 4's caller-seeded + * record number. And a caller cannot put a key in here — see the + * forgery-boundary note on the recorder — so #5503 is untouched: a + * caller-seeded record number that no hook assigned is still dropped, still + * warns with the same text, and still reports through `onFieldsDropped` / + * `strictReadonlyWrites`. + */ + hookWrittenKeys?: ReadonlySet; + }, ): Record | undefined | null { const fields = objectSchema?.fields; if (!fields || !data) return data; const preserveAudit = options?.preserveAudit === true; const strict = options?.strictReadonlyWrites === true; + const hookWrittenKeys = options?.hookWrittenKeys; let result = data; for (const [name, def] of Object.entries(fields)) { if (!isRuntimeOwnedField(def)) continue; @@ -1328,12 +1366,24 @@ export function stripRuntimeOwnedFields( // any plain snapshot, so `in` would call a hook stamp caller-supplied and // strip it. if (!Object.prototype.hasOwnProperty.call(supplied, name)) continue; // hook/middleware stamp — keep + // [#14259] ...or a hook ASSIGNED it on THIS row. Asked BEFORE the value + // comparison because it answers by RECORD the question the comparison is + // only a proxy for: `Object.is` reads "the hook re-issued the record number + // the caller also submitted" as "the hook never touched the key", and the + // two demand opposite verdicts. That is #6339's own argument against the + // key SET, applied to the values that replaced it. + if (hookWrittenKeys?.has(name)) continue; // the hook wrote this value — keep // [#6339] ...and it must still BE the caller's value. A hook that overwrote // this key wrote a PLATFORM value, and deleting that is what sent records // to the database holding a sequence number the hook had just replaced. // `Object.is`, not `===`: `===` reports NaN !== NaN, which would read a // caller-forged NaN as "a hook rewrote it" and KEEP the forgery — the one // input where the loose operator inverts the verdict. + // + // [#14259] STAYS as the fallback for every key the record above has nothing + // to say about — a call site passing no `hookWrittenKeys`, and a row whose + // hook replaced the payload object. ⛔ Not "keep everything": the fallback + // over-strips, which is the only safe direction. if (!Object.is((result as Record)[name], supplied[name])) continue; if (preserveAudit && isPreservableUnderAudit(name, def)) continue; // historical import reinstates it if (result === data) result = { ...data };