diff --git a/.changeset/readonly-strip-hook-write-provenance.md b/.changeset/readonly-strip-hook-write-provenance.md new file mode 100644 index 0000000000..8948294c9a --- /dev/null +++ b/.changeset/readonly-strip-hook-write-provenance.md @@ -0,0 +1,63 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): decide the read-only strip on hook-write PROVENANCE, so a hook can clear a field the caller also sent (#14088) + +`stripReadonlyFields` decided whether a `readonly` field's value came from a +before-phase hook or from the caller with `Object.is(payload[name], +supplied[name])`. Value equality cannot carry that question, and this is it +failing: when both sides hold the same value the comparison cannot separate +*the hook deliberately wrote it* from *the hook never touched it*, and the two +demand opposite verdicts. The hook's write was deleted along with the caller's. + +**Measured downstream** (published 17.2.0, `duly_task`): a `readonly` +`completed_at` stamped by a `beforeUpdate` hook on the transition into `done` +and cleared on the transition out. Reopening a completed task works — unless +the caller also sends `completed_at: null`, which is exactly what a form +round-trip of the whole record does. `Object.is(null, null)` is `true`, the +hook's clear is stripped, and the row commits `status = in_progress` still +carrying its **old completion timestamp**, with no error. That row is the one +a validation rule structurally cannot catch ("a completed task must carry a +completion timestamp" has no purchase on its inverse), nothing downstream can +tell it from a genuinely completed one, and every on-time metric reading +`completed_at` counts it. + +This is the second failure of one sentence, not a new defect. #5591 / #6339 +retired the key-SET judgement because it made the strip's own written contract +— "hook-written keys are NOT caller-supplied" — true only *by accident*. Value +equality is accidental in precisely the same way; `null == null` is just its +most common collision. + +**The repair is provenance, recorded rather than inferred.** A new +`recordHookPayloadWrites` view is armed over the update payload after the +caller's entry snapshot and sealed at the engine's post-hook confluence, where +`hookContext.input.data` is final on **both** update branches. It records only +the fact that an assignment executed — never the payload's contents — and +`stripReadonlyFields` now keeps a key a hook demonstrably assigned. Both +branches consume the one sealed record, so a bulk write and a by-id write can +never reach different verdicts about who wrote a key. + +⛔ **Not a `null` special case.** `0`, `''`, `false` and a shared object +reference collide identically, and all of them are echoed back by the same +whole-record write-back idiom. A sentinel fix would have left every one of them +open. + +⛔ **Not a relaxation of #2948 / #3003 / #5503.** A caller-supplied read-only +value that no hook wrote is still dropped, still warns with the same text, and +still reports through `onFieldsDropped` / `strictReadonlyWrites`. The +discriminator is pinned in both directions: the same caller payload +(`completed_at: null` over a stored timestamp) now **clears** when a hook wrote +the null and is still **stripped** when no hook did. A caller cannot enter the +record — echoing a key or a value is not an assignment — so no caller write can +become hook-owned. + +**Known limit, deliberately fail-safe and pinned as a test.** A hook that +*replaces* the payload object (`ctx.input.data = { ...ctx.input.data, x: 1 }`) +rather than mutating it leaves no attributable record, and that call falls back +to the previous value comparison — i.e. it keeps the old over-strip. Reading a +replacement's keys as hook-owned would launder a caller's forged `created_by` +into a platform write, so the fallback direction is the only safe one. A hook +that means to own a read-only column should ASSIGN to it. The pre-existing +shallow-snapshot limit (a hook mutating a caller-supplied object *in place*) is +unchanged for the same reason. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 7fe8c239f8..d5f735c117 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:10914` | -| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11076` | -| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9772` | +| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:10981` | +| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:11149` | +| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9777` | | 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:9809`, `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:5730` | -| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3599`, `:3609`, `:3636` | +| 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:9814`, `readonly-strict-errors.ts:66` | +| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5735` | +| 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3604`, `:3614`, `:3641` | | 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:6428` | -| 28 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:11662` | -| 29 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:11591` | +| 27 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6433` | +| 28 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:11742` | +| 29 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:11671` | ### 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:3406` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | -| 63 | `objectql/src/engine.ts:14011` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | +| 62 | `objectql/src/engine.ts:3411` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | +| 63 | `objectql/src/engine.ts:14091` | 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:1909` (rationale at `:1819`–`1821`, #3760), `flow.zod.ts:685` | | "It skips the state machine" | **No.** That is `skipStateMachine`, carried by seed replay and by `treatAsHistorical` imports | `objectql/src/engine.ts` FSM gate; see [State Machine](/docs/protocol/objectql/state-machine) | -| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:9755`–`9772` | +| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:9760`–`9777` | | "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-readonly-strip-caller-values.test.ts b/packages/objectql/src/engine-readonly-strip-caller-values.test.ts index 34feb711b3..189a345111 100644 --- a/packages/objectql/src/engine-readonly-strip-caller-values.test.ts +++ b/packages/objectql/src/engine-readonly-strip-caller-values.test.ts @@ -329,3 +329,458 @@ describe('update strip acts on CALLER-submitted values (#5591)', () => { expect(row.case_number).not.toBe('FORGED-9'); }); }); + +// ─────────────────────────────────────────────────────────────────────────── +// #14088 — the SAME sentence failing a second time, one rung further along. +// +// #5591 (above) retired the key SET because it made the contract +// ("hook-written keys are NOT caller-supplied") true only BY ACCIDENT. Value +// equality is accidental in precisely the same way, and this is the accident: +// `Object.is(payload[k], supplied[k])` cannot separate *the hook deliberately +// wrote the value the caller also sent* from *the hook never touched the key*. +// +// Measured downstream (published 17.2.0, `duly_task`): a `readonly` +// `completed_at` stamped by a `beforeUpdate` hook on the transition INTO `done` +// and CLEARED on the transition out. Reopening works — until the caller ALSO +// sends `completed_at: null`, which is what a form round-trip of the whole +// record does. `Object.is(null, null)` is true, the hook's clear dies with the +// caller's null, and the row commits `status = in_progress` still carrying its +// OLD completion timestamp. No error. Nothing downstream can tell that row from +// one that really was completed — the corruption is the exact inverse of what a +// validation rule can express ("a completed task must carry a timestamp" has no +// purchase on a NON-completed task that carries one), so every on-time metric +// reading `completed_at` counts it. +// +// ⛔ THE FIX IS PROVENANCE, NOT A `null` CASE, and this suite is written to +// fail if anyone narrows it to one: the `0` case below collides identically, +// and so would `''`, `false` and a shared object reference. The strip now reads +// a RECORD of the keys the hook chain actually assigned +// (`recordHookPayloadWrites`, sealed at the engine's post-hook confluence). +// +// ⛔ AND IT IS NOT "STOP STRIPPING READONLY FIELDS". The discriminator pair is +// the point of this suite: the same payload (`completed_at: null` over a stored +// timestamp) must CLEAR when a hook wrote the null and must be STRIPPED when no +// hook did. Two opposite verdicts on byte-identical caller input — which is +// exactly what value equality cannot deliver and a record can. +describe('the strip reads hook-write PROVENANCE, not value equality (#14088)', () => { + let engine: ObjectQL; + let storeFor: ReturnType['storeFor']; + let warns: string[]; + /** The completion instant already ON the stored row — the value a lost clear leaves behind. */ + const STAMPED = '2026-08-01T09:00:00.000Z'; + + const registerTransitionHook = (e: ObjectQL) => { + // `duly_task`'s hook, reproduced: stamp on the way INTO `done`, clear on + // the way out. The clear is the direction the card measured. + e.registerHook('beforeUpdate', async (ctx: any) => { + const next = ctx.input.data.status; + if (next === undefined) return; + const wasDone = ctx.previous?.status === 'done'; + if (next === 'done' && !wasDone) { + ctx.input.data.completed_at = NOW; + ctx.input.data.elapsed_minutes = 42; + } else if (next !== 'done' && wasDone) { + ctx.input.data.completed_at = null; + ctx.input.data.elapsed_minutes = 0; + } + }, { object: 'duly_task', priority: 50 }); + }; + + beforeEach(async () => { + warns = []; + 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(); + + engine.registry.registerObject({ + name: 'duly_task', + fields: { + title: { type: 'text' }, + status: { type: 'text' }, + completed_at: { type: 'datetime', readonly: true }, + // The `0` twin of the same collision — proof the repair is provenance + // and not a `null` sentinel. + elapsed_minutes: { type: 'number', readonly: true }, + }, + } as any); + + // The kernel `previous` binder (`object: '*'`, priority 5), replicated — + // a transition cannot be expressed in a hook without it. + engine.registerHook('beforeUpdate', async (ctx: any) => { + if (!ctx.previous && ctx.input?.id) { + const priorQuery: EngineQueryOptionsParsed = { where: { id: ctx.input.id }, limit: 1 }; + ctx.previous = await engine.findOne(ctx.object, priorQuery); + } + }, { priority: 5 }); + }); + + const task = (id: string) => storeFor('duly_task').get(id); + const seedDone = (id: string) => storeFor('duly_task').set(id, { + id, title: 'T', status: 'done', completed_at: STAMPED, elapsed_minutes: 42, + }); + const seedOpen = (id: string) => storeFor('duly_task').set(id, { + id, title: 'T', status: 'in_progress', completed_at: null, elapsed_minutes: null, + }); + + // ── The card's exact failure ────────────────────────────────────────────── + + it("THE REPORT: a reopen that also sends completed_at: null lands the hook's CLEAR", async () => { + // The reported call, byte for byte: reopen a completed task while echoing + // the field the form round-trips. Before the repair the row kept STAMPED + // and nothing errored. + registerTransitionHook(engine); + seedDone('t_1'); + + await engine.update('duly_task', { + id: 't_1', title: 'T', status: 'in_progress', completed_at: null, + }); + + expect(task('t_1').status).toBe('in_progress'); + // The regression, stated as the value it must NOT be. + expect(task('t_1').completed_at).not.toBe(STAMPED); + expect(task('t_1').completed_at).toBeNull(); + }); + + it('the same collision on `0` — so the repair cannot be a `null` sentinel', async () => { + // `elapsed_minutes` is reset to 0 by the same hook while the caller also + // sent 0. `Object.is(0, 0)` is true for exactly the reason + // `Object.is(null, null)` is, and a fix that reads `null` specially leaves + // this one corrupt. (`-0` is why the test uses a plain 0: `Object.is` + // separates the two, and relying on that would be the same accident again.) + registerTransitionHook(engine); + seedDone('t_2'); + + await engine.update('duly_task', { + id: 't_2', status: 'in_progress', completed_at: null, elapsed_minutes: 0, + }); + + expect(task('t_2').elapsed_minutes).toBe(0); + expect(task('t_2').completed_at).toBeNull(); + }); + + // ── The STAMP direction — the card measures only the CLEAR ──────────────── + + it('the STAMP direction is broken by the same mechanism, and is fixed with it', async () => { + // The card measures the clear. The stamp collides identically whenever the + // caller's echoed value happens to equal what the hook writes — the ordinary + // way being a whole-record write-back of a value some other client already + // stamped, or an idempotent retry of the very same request. Same verdict, + // opposite direction: before the repair the hook's stamp was deleted and + // the row committed `status = done` with `completed_at = null`. + registerTransitionHook(engine); + seedOpen('t_3'); + + await engine.update('duly_task', { + id: 't_3', title: 'T', status: 'done', completed_at: NOW, elapsed_minutes: 42, + }); + + expect(task('t_3').status).toBe('done'); + expect(task('t_3').completed_at).toBe(NOW); + expect(task('t_3').elapsed_minutes).toBe(42); + }); + + // ── Both update branches, off the one record ────────────────────────────── + + it('the PREDICATE branch clears on the same terms (both call sites, one record)', async () => { + // `stripReadonlyFields` runs on both update branches, so a repair that + // reaches only the by-id one is a divergence, not a fix — the #3106 / #4441 + // shape. Both matched rows are already `done`, so the batch's single + // payload is correct for every row it touches and this measures the strip + // rather than the batch-hook question (#14099). + registerTransitionHook(engine); + seedDone('t_4'); + seedDone('t_5'); + + await engine.update( + 'duly_task', + { status: 'in_progress', completed_at: null }, + { where: { status: 'done' }, multi: true } as any, + ); + + expect(task('t_4').completed_at).toBeNull(); + expect(task('t_5').completed_at).toBeNull(); + expect(task('t_4').status).toBe('in_progress'); + }); + + it('the PREDICATE branch stamps on the same terms', async () => { + registerTransitionHook(engine); + seedOpen('t_6'); + + await engine.update( + 'duly_task', + { status: 'done', completed_at: NOW }, + { where: { status: 'in_progress' }, multi: true } as any, + ); + + expect(task('t_6').completed_at).toBe(NOW); + }); + + // ── The discriminator: same payload, no hook write, OPPOSITE verdict ────── + + it('⛔ THE FORGERY FACE: the identical payload with NO hook write is still STRIPPED', async () => { + // The one test that separates "provenance" from "stopped stripping". Byte + // for byte the caller input of THE REPORT above — `completed_at: null` over + // a stored timestamp — but no hook writes the key (no transition: the task + // is already `in_progress`). Nobody authorised the clear, so the stored + // timestamp must survive and the caller must be told. + registerTransitionHook(engine); + storeFor('duly_task').set('t_7', { + id: 't_7', title: 'T', status: 'in_progress', completed_at: STAMPED, elapsed_minutes: 42, + }); + + await engine.update('duly_task', { + id: 't_7', title: 'T2', status: 'in_progress', completed_at: null, + }); + + expect(task('t_7').title).toBe('T2'); + expect(task('t_7').completed_at).toBe(STAMPED); + expect(warns.some((w) => w.includes("Field 'completed_at'"))).toBe(true); + }); + + it('⛔ a hook that runs but writes some OTHER key confers nothing on this one', async () => { + // Provenance is per KEY, never "a hook ran on this write". A hook touching + // `title` must not make a caller's forged `completed_at` hook-owned. + engine.registerHook('beforeUpdate', async (ctx: any) => { + ctx.input.data.title = 'rewritten-by-hook'; + }, { object: 'duly_task', priority: 50 }); + seedDone('t_8'); + + await engine.update('duly_task', { + id: 't_8', title: 'T', completed_at: '1999-01-01T00:00:00.000Z', + }); + + expect(task('t_8').title).toBe('rewritten-by-hook'); + expect(task('t_8').completed_at).toBe(STAMPED); + }); + + it('⛔ #2948 UNCHANGED: a plain forge with no hook at all is still stripped', async () => { + seedDone('t_9'); + await engine.update('duly_task', { + id: 't_9', completed_at: '1999-01-01T00:00:00.000Z', + }); + expect(task('t_9').completed_at).toBe(STAMPED); + expect(warns.some((w) => w.includes("Field 'completed_at'"))).toBe(true); + }); + + it('⛔ the PREDICATE branch strips a forge no hook wrote, too', async () => { + registerTransitionHook(engine); + storeFor('duly_task').set('t_10', { + id: 't_10', title: 'T', status: 'archived', completed_at: STAMPED, elapsed_minutes: 42, + }); + await engine.update( + 'duly_task', + { title: 'T2', completed_at: null }, + { where: { status: 'archived' }, multi: true } as any, + ); + expect(task('t_10').title).toBe('T2'); + expect(task('t_10').completed_at).toBe(STAMPED); + }); + + // ── The two "common paths" the card says are unaffected — negative controls ─ + + it('NEGATIVE CONTROL: the bare { status } reopen is unaffected', async () => { + // The card's own explanation for why this survived casual testing: the hook + // ADDS the key, so it was never in the caller's snapshot and the old + // key/value test already kept it. It must still be kept, and for a reason + // the repair did not have to invent. + registerTransitionHook(engine); + seedDone('t_11'); + + await engine.update('duly_task', { id: 't_11', status: 'in_progress' }); + + expect(task('t_11').status).toBe('in_progress'); + expect(task('t_11').completed_at).toBeNull(); + expect(task('t_11').elapsed_minutes).toBe(0); + }); + + it('NEGATIVE CONTROL: a partial patch that touches no transition is unaffected', async () => { + registerTransitionHook(engine); + seedDone('t_12'); + + await engine.update('duly_task', { id: 't_12', title: 'renamed' }); + + expect(task('t_12').title).toBe('renamed'); + expect(task('t_12').status).toBe('done'); + expect(task('t_12').completed_at).toBe(STAMPED); + expect(task('t_12').elapsed_minutes).toBe(42); + }); + + it('NEGATIVE CONTROL: an isSystem caller still bypasses the strip entirely', async () => { + seedDone('t_13'); + await engine.update( + 'duly_task', + { id: 't_13', completed_at: '1999-01-01T00:00:00.000Z' }, + { context: { isSystem: true } } as any, + ); + expect(task('t_13').completed_at).toBe('1999-01-01T00:00:00.000Z'); + }); + + // ── The observability seams move with the verdict, not against it ────────── + + it('a hook-written clear is NOT reported to onFieldsDropped', async () => { + // `DroppedFieldsEvent` means "dropped, and the write completed WITHOUT + // them" (#3407). The column IS written now — with the platform's null — so + // reporting it would make the seam lie, exactly as #5591 argued for the + // differing-value case one describe up. + registerTransitionHook(engine); + seedDone('t_14'); + const events: any[] = []; + + await engine.update( + 'duly_task', + { id: 't_14', status: 'in_progress', completed_at: null }, + { onFieldsDropped: (e: any) => events.push(e) } as any, + ); + + expect(events).toEqual([]); + expect(task('t_14').completed_at).toBeNull(); + }); + + it('strictReadonlyWrites does not REFUSE a write whose only "drop" was a hook clear', async () => { + // The loud half of the same seam (#5126). Before the repair this write was + // refused outright for a field the caller never successfully wrote — the + // strict caller's punishment for its own hook's clear. + registerTransitionHook(engine); + seedDone('t_15'); + + await engine.update( + 'duly_task', + { id: 't_15', status: 'in_progress', completed_at: null }, + { strictReadonlyWrites: true } as any, + ); + + expect(task('t_15').completed_at).toBeNull(); + }); + + it('strictReadonlyWrites still REFUSES a real forge', async () => { + registerTransitionHook(engine); + seedDone('t_16'); + + await expect(engine.update( + 'duly_task', + { id: 't_16', completed_at: '1999-01-01T00:00:00.000Z' }, + { strictReadonlyWrites: true } as any, + )).rejects.toThrow(); + expect(task('t_16').completed_at).toBe(STAMPED); + }); + + // ── The declared limit, pinned so nobody "fixes" it into an escalation ───── + + it('KNOWN LIMIT: a hook that REPLACES the payload leaves no record, and falls back', async () => { + // `ctx.input.data = { …ctx.input.data, completed_at: null }` produces a + // fresh object whose keys are indistinguishable from the caller's — because + // most of them ARE the caller's, spread across. So there is no record for + // this call and the pre-#14088 value test decides, i.e. the clear is still + // lost here. + // + // Pinned as the FAIL-SAFE direction on purpose. The alternative — reading a + // replacement's keys as hook-owned — would launder a caller's forged + // `created_by` into a platform write on any object carrying such a hook. + // Keeping the old over-strip is strictly better than opening the lock, and + // this test exists so that trade is re-argued rather than quietly inverted. + engine.registerHook('beforeUpdate', async (ctx: any) => { + if (ctx.input.data.status !== 'done' && ctx.previous?.status === 'done') { + ctx.input.data = { ...ctx.input.data, completed_at: null }; + } + }, { object: 'duly_task', priority: 50 }); + seedDone('t_17'); + + await engine.update('duly_task', { + id: 't_17', status: 'in_progress', completed_at: null, + }); + + expect(task('t_17').status).toBe('in_progress'); + expect(task('t_17').completed_at).toBe(STAMPED); + }); + + it('...but a REPLACING hook still lands a key the caller did NOT send', async () => { + // The fallback is the pre-#14088 behaviour in full, not a new hole: with no + // record, a replaced payload is judged by the #5591 test, and a key absent + // from the caller's snapshot is kept exactly as it always was. + engine.registerHook('beforeUpdate', async (ctx: any) => { + ctx.input.data = { ...ctx.input.data, completed_at: null }; + }, { object: 'duly_task', priority: 50 }); + seedDone('t_18'); + + await engine.update('duly_task', { id: 't_18', status: 'in_progress' }); + + expect(task('t_18').completed_at).toBeNull(); + }); + + 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. + 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, + 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 }); + + 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].has).toBe(true); + expect(seen[0].own).toBe(true); + }); + + 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 + // given, engine-internal machinery ends up on a row. + // + // A `Proxy` is invisible to `typeof` and `instanceof`, so the assertion has + // to be IDENTITY against the caller's own object — the recorder's target. + // The predicate branch is chosen deliberately: with no `id` key and nothing + // 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. + const seenByDriver: any[] = []; + const d2 = makeDriver(); + const e2 = new ObjectQL({}); + const wrapped: any = { + ...d2.driver, + async updateMany(object: string, ast: any, data: Record) { + seenByDriver.push(data); + return d2.driver.updateMany(object, ast, data, undefined as any); + }, + }; + e2.registerDriver(wrapped, true); + await e2.init(); + e2.registry.registerObject({ + name: 'duly_task', + fields: { + title: { type: 'text' }, status: { type: 'text' }, + completed_at: { type: 'datetime', readonly: true }, + }, + } as any); + e2.registerHook('beforeUpdate', async (ctx: any) => { + ctx.input.data.completed_at = NOW; + }, { 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 }; + await e2.update('duly_task', payload as any, { where: { status: 'open' }, multi: true } as any); + + expect(seenByDriver).toHaveLength(1); + expect(seenByDriver[0]).toBe(payload); + expect(seenByDriver[0].completed_at).toBe(NOW); + expect(d2.storeFor('duly_task').get('t_20').completed_at).toBe(NOW); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 07c5edbe5a..3864e5b226 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -188,6 +188,11 @@ import { bindHooksToEngine } from './hook-binder.js'; import { validateRecord, normalizeMultiValueFields, coerceBooleanFields, ValidationError, buildFieldError, resolveFieldLabel, valueShapePostureSetByEnv, mediaPostureSetByEnv, isScannableValueShapeField, valueShapeStrictEffective, mediaStrictEffective } from './validation/record-validator.js'; import type { AdmittedValueShapeViolation, AdmittedValueShapeViolationSink } from './validation/record-validator.js'; import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, hasParentScopedReadonlyWhenInPayload, hasParentScopedRequiredWhen, stripReadonlyFields, stripRuntimeOwnedFields } from './validation/rule-validator.js'; +// [#14088] The before-phase write recorder — the provenance channel the static +// `readonly` strip needs to tell a hook's write from a caller's echo of the +// 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 { 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 @@ -10346,10 +10351,46 @@ export class ObjectQL implements IObjectQLEngine { )[0]; if (undeclared) throw undeclared; + // ── [#14088] ARM the hook-write recording ──────────────────────────── + // + // `suppliedValues` above answers "what did the caller send". This + // answers the other half — "which keys did a HOOK assign" — and it must + // be RECORDED, because it cannot be recovered from the values + // afterwards: `Object.is(payload[k], supplied[k])` reads a hook that + // deliberately wrote the value the caller also sent as a hook that never + // touched the key, and the static `readonly` strip below then deletes + // the hook's write. Measured: a `readonly` `completed_at` CLEARED by a + // reopen hook against a caller that round-tripped the whole record and + // so also sent `completed_at: null` — the row committed `in_progress` + // with its old completion timestamp and no error. + // + // The two ends of this window are load-bearing, not stylistic (the + // module's own header carries the full argument): + // + // - ARMED HERE, after the entry snapshot and after the caller's payload + // has stopped being written by anything the caller controls. A + // caller cannot execute an assignment on this object; echoing a key, + // a value, a `null` or a `Proxy` back is not a `set`. So no key a + // caller supplied can enter the record, and a hook-owned key is a key + // the strip stops defending. + // - SEALED at the confluence below, BEFORE `encryptSecretFields` / + // `normalizeMultiValueFields` / the strips write to the payload. A + // recorder still armed for those would report ENGINE writes as HOOK + // writes, which on a caller-forged secret column is precisely the + // escalation this is built to make impossible. + // + // Writes through the recording land on the SAME object, so a hook + // mutating `ctx.input.data.x` in place is mutating the engine's payload + // exactly as it always has, and `opCtx.data` stays in step. + const hookWrites = + opCtx.data !== null && typeof opCtx.data === 'object' + ? recordHookPayloadWrites(opCtx.data as Record) + : undefined; + const hookContext: HookContext = { object, event: 'beforeUpdate', - input: { id, data: opCtx.data, options: opCtx.options }, + input: { id, data: hookWrites?.payload ?? opCtx.data, options: opCtx.options }, session: this.buildSession(opCtx.context), provenance: this.buildProvenance(opCtx.context), // [#13644] The declared referential-cleanup marker. Conditional @@ -10641,6 +10682,32 @@ export class ObjectQL implements IObjectQLEngine { } } + // ── [#14088] SEAL the hook-write recording ─────────────────────────── + // + // The same CONFLUENCE #13657 uses one comment down, and for the same + // reason: on either branch this is the line at which + // `hookContext.input.data` is the final POST-hook payload and nothing + // engine-owned has written to it yet. One seal covers both branches, so + // the by-id and predicate paths can never end up with different notions + // of who wrote a key — the divergence "both call sites" (#3106 / #4441) + // is the standing shape for. + // + // Sealing does two things, and the write is wrong without either: it + // freezes the record before the engine's own passes can be mis-recorded + // as hook writes, and it puts the RAW payload back in `input.data` so no + // recording view reaches a driver. + // + // `hookWrittenKeys` is `undefined` — not empty — when the recording + // cannot speak for this call, which happens when a hook REPLACED the + // payload object (`ctx.input.data = { …ctx.input.data }`) rather than + // mutating it. Consumers must read that as "fall back to the #5591 value + // test", never as "no hook wrote anything": treating a replacement's + // keys as hook-owned would launder a caller's forgery, so the fallback + // 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; + // ── [#13657] The POST-hook half of the declared-field door ────────── // // The insert path's twin, applied to the second write verb — same @@ -10921,7 +10988,13 @@ export class ObjectQL implements IObjectQLEngine { // WITHOUT IT" while `driverWrites` was 0. The seam that // composes the sentence has to know the mode the sentence // describes; nothing else here can tell it. - hookContext.input.data = stripReadonlyFields(updateSchema as any, preRo, suppliedValues, this.logger, { preserveAudit: opCtx.context?.preserveAudit === true, addressKey: idAddressesThisRow ? 'id' : undefined, strictReadonlyWrites }) as any; + // [#14088] `hookWrittenKeys` — the other half of the same + // question `suppliedValues` answers, and the half no + // comparison of values can reach. Without it a hook CLEARING + // a read-only column loses its write to any caller that + // echoed the same value back, which on `null` is every + // whole-record form round-trip. + hookContext.input.data = stripReadonlyFields(updateSchema as any, preRo, suppliedValues, this.logger, { preserveAudit: opCtx.context?.preserveAudit === true, addressKey: idAddressesThisRow ? 'id' : undefined, strictReadonlyWrites, hookWrittenKeys }) as any; reportDroppedFields(preRo, hookContext.input.data as Record, 'readonly'); } // [#5126] Both strip passes are done; refuse now if the caller @@ -11079,7 +11152,14 @@ export class ObjectQL implements IObjectQLEngine { // branch still passes no `addressKey` (nothing addresses a // row by key here), which is what keeps it byte-identical // to #8141 in every other respect. - hookContext.input.data = stripReadonlyFields(updateSchema as any, preRoMulti, suppliedValues, this.logger, { preserveAudit: opCtx.context?.preserveAudit === true, strictReadonlyWrites }) as any; + // [#14088] The SAME record the by-id branch consumes, + // sealed once at the shared confluence. Not a second + // derivation: two notions of "a hook wrote this key" that + // disagree in one edge case would be a worse defect than the + // one they were each added to close, and a bulk write that + // reached a different verdict about authorship than a by-id + // write is the #3106 / #4441 divergence verbatim. + hookContext.input.data = stripReadonlyFields(updateSchema as any, preRoMulti, suppliedValues, this.logger, { preserveAudit: opCtx.context?.preserveAudit === true, strictReadonlyWrites, hookWrittenKeys }) as any; reportDroppedFields(preRoMulti, hookContext.input.data as Record, 'readonly'); } // [#5126] Same refusal on the predicate path. A bulk strip is diff --git a/packages/objectql/src/hook-write-provenance.test.ts b/packages/objectql/src/hook-write-provenance.test.ts new file mode 100644 index 0000000000..72ac4c8c16 --- /dev/null +++ b/packages/objectql/src/hook-write-provenance.test.ts @@ -0,0 +1,154 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #14088 — the provenance recorder itself, at the unit. +// +// The engine-level suite (`engine-readonly-strip-caller-values.test.ts`, the +// `#14088` describe) pins what a caller and a hook observe. This one pins the +// two properties that make the whole repair safe, where they are decidable +// without a write path in the way: +// +// 1. a key enters the record ONLY through an assignment executed against the +// payload — never through its contents, and never through a caller's echo; +// 2. the record is FROZEN at the seal, and the seal hands back the RAW object +// so nothing engine-owned is ever attributed to a hook. +// +// Property 1 is the forgery boundary. A key in this set is a key the read-only +// strip stops defending, so "can caller data reach it?" is the question that +// decides whether this is a fix or a privilege escalation. + +import { describe, it, expect } from 'vitest'; +import { recordHookPayloadWrites } from './hook-write-provenance.js'; + +describe('recordHookPayloadWrites (#14088)', () => { + it('records a plain assignment, and writes through to the target', () => { + const target: Record = { title: 'T', completed_at: null }; + const rec = recordHookPayloadWrites(target); + + rec.payload.completed_at = null; // the same value that is already there + + const sealed = rec.seal(rec.payload); + expect(sealed.data).toBe(target); + expect([...(sealed.hookWrittenKeys ?? [])]).toEqual(['completed_at']); + expect(target.completed_at).toBeNull(); + }); + + it('⛔ THE FORGERY BOUNDARY: the caller\'s CONTENTS put nothing in the record', () => { + // Everything a caller controls arrives as data on the object BEFORE the + // recorder is armed. Echoing a key, echoing a value, sending `null`, + // sending nested objects — none of it is an assignment, so the record is + // empty and the strip's own two-part test decides every key, unchanged. + const target: Record = { + updated_by: 'attacker', created_by: 'attacker', + completed_at: null, elapsed_minutes: 0, flag: false, note: '', + nested: { x: 1 }, arr: [1, 2], + }; + const rec = recordHookPayloadWrites(target); + + // Every read shape a hook (or the engine) performs, none of them a write. + void Object.keys(rec.payload); + void JSON.stringify(rec.payload); + void { ...rec.payload }; + void ('updated_by' in rec.payload); + void Object.entries(rec.payload); + (rec.payload.nested as Record).x = 2; // in-place, on a CHILD + + expect([...(rec.seal(rec.payload).hookWrittenKeys ?? [])]).toEqual([]); + }); + + it('records `Object.defineProperty` too — the second write verb', () => { + const target: Record = { completed_at: null }; + const rec = recordHookPayloadWrites(target); + + Object.defineProperty(rec.payload, 'completed_at', { + value: null, writable: true, enumerable: true, configurable: true, + }); + + expect([...(rec.seal(rec.payload).hookWrittenKeys ?? [])]).toEqual(['completed_at']); + }); + + it('a delete removes the key from the record; a later assignment puts it back', () => { + const target: Record = { completed_at: null }; + const rec = recordHookPayloadWrites(target); + + rec.payload.completed_at = 'x'; + delete rec.payload.completed_at; + expect(target).not.toHaveProperty('completed_at'); + + const rec2 = recordHookPayloadWrites({ completed_at: null } as Record); + rec2.payload.completed_at = 'x'; + delete rec2.payload.completed_at; + rec2.payload.completed_at = null; + + expect([...(rec.seal(rec.payload).hookWrittenKeys ?? [])]).toEqual([]); + expect([...(rec2.seal(rec2.payload).hookWrittenKeys ?? [])]).toEqual(['completed_at']); + }); + + it('symbol keys are not field names and never enter the record', () => { + const target: Record = {}; + const rec = recordHookPayloadWrites(target); + const stash = Symbol('stash'); + + (rec.payload as any)[stash] = 1; + + expect([...(rec.seal(rec.payload).hookWrittenKeys ?? [])]).toEqual([]); + expect((target as any)[stash]).toBe(1); + }); + + it('the seal FREEZES the record — a later write cannot grow it', () => { + // The seal is what keeps engine-owned passes (secret encryption, + // multi-value normalisation, the strips) from being attributed to a hook. + // A hook that stashed the view and writes to it afterwards must not be able + // to reopen the record either. + const target: Record = {}; + const rec = recordHookPayloadWrites(target); + rec.payload.a = 1; + + const sealed = rec.seal(rec.payload); + rec.payload.b = 2; + + expect([...(sealed.hookWrittenKeys ?? [])]).toEqual(['a']); + expect(target.b).toBe(2); // still a write-through view, just no longer recorded + }); + + it('KNOWN LIMIT: a REPLACED payload yields NO record — undefined, not empty', () => { + // `undefined` and `new Set()` mean opposite things to the strip: the first + // says "this call cannot say, fall back to the value test", the second says + // "no hook wrote anything, strip freely". Conflating them is how a + // replacement's keys would become hook-owned, which is the escalation. + const target: Record = { completed_at: null }; + const rec = recordHookPayloadWrites(target); + rec.payload.completed_at = null; + + const replacement = { ...target, completed_at: null }; + const sealed = rec.seal(replacement); + + expect(sealed.hookWrittenKeys).toBeUndefined(); + expect(sealed.data).toBe(replacement); + }); + + it('the view is transparent for every read shape', () => { + const target: Record = { id: 'r1', title: 'T', completed_at: null }; + const rec = recordHookPayloadWrites(target); + + expect(Object.keys(rec.payload)).toEqual(['id', 'title', 'completed_at']); + expect({ ...rec.payload }).toEqual(target); + expect(JSON.stringify(rec.payload)).toBe(JSON.stringify(target)); + expect('completed_at' in rec.payload).toBe(true); + expect(Object.prototype.hasOwnProperty.call(rec.payload, 'title')).toBe(true); + expect(rec.payload.title).toBe('T'); + expect(Object.getOwnPropertyNames(rec.payload)).toEqual(['id', 'title', 'completed_at']); + }); + + it('an assignment that FAILS is not recorded', () => { + // A non-writable own property rejects the write in sloppy mode. Recording + // it would claim a hook owns a value it never managed to set. + const target: Record = {}; + Object.defineProperty(target, 'locked', { value: 1, writable: false, enumerable: true }); + const rec = recordHookPayloadWrites(target); + + try { (rec.payload as any).locked = 2; } catch { /* strict mode throws; either way, no write */ } + + expect([...(rec.seal(rec.payload).hookWrittenKeys ?? [])]).toEqual([]); + expect(target.locked).toBe(1); + }); +}); diff --git a/packages/objectql/src/hook-write-provenance.ts b/packages/objectql/src/hook-write-provenance.ts new file mode 100644 index 0000000000..10fb16e6b0 --- /dev/null +++ b/packages/objectql/src/hook-write-provenance.ts @@ -0,0 +1,192 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #14088 — WHO WROTE THIS KEY, recorded rather than inferred. +// +// ## The question, and why every value-shaped answer to it is wrong +// +// The static `readonly` strip (`stripReadonlyFields`) and its `readonlyWhen` +// sibling both run AFTER the before-phase hooks, so at the moment they look at +// a key, "the caller sent this key" and "this key still holds what the caller +// sent" are different facts. #5591 / #6339 moved the judgement from the first +// to the second, on the argument that a key SET made the contract +// (`runtimeOwnedStripWarning`: "hook-written keys are NOT caller-supplied") +// true only BY ACCIDENT. +// +// The same sentence is true of value equality, and #14088 is it failing the +// second time. `Object.is(payload[name], supplied[name])` cannot separate +// +// - the hook deliberately wrote the value the caller happened to send, from +// - the hook never touched the key at all, +// +// and the two demand opposite verdicts. The measured downstream row: a +// `readonly` `completed_at` cleared by a `beforeUpdate` hook on the transition +// OUT of `done`, against a caller that round-tripped the whole record and so +// also sent `completed_at: null`. `Object.is(null, null)` is `true`, the key is +// deleted with the caller's null, and the row commits `status = in_progress` +// carrying its OLD completion timestamp. No error. Nothing downstream can tell +// that row from a genuinely completed one. +// +// ⛔ A `null` special case is not the fix, and this module exists so nobody +// reaches for one: the identical collision is available on `0`, `''`, `false`, +// a shared object reference, and on any value a form round-trip echoes back. +// The distinction the strip needs is PROVENANCE — which keys the hook chain +// actually ASSIGNED — and provenance cannot be recovered from the values +// afterwards. It has to be RECORDED WHILE THE WRITES HAPPEN. +// +// ## The forgery boundary — the one thing here that must never be got wrong +// +// A hook-owned key is a key the strip STOPS DEFENDING, so a record that a +// caller could influence would be a privilege escalation, not a fix. This +// recorder cannot be reached by caller data, and the reason is structural +// rather than careful: +// +// * it records nothing about the object's CONTENTS — only the fact that an +// assignment executed against it; +// * it is armed AFTER the caller's payload has arrived and been snapshotted, +// and SEALED before the engine's own normalisation passes touch the payload +// (`encryptSecretFields`, `normalizeMultiValueFields`, the strips +// themselves). Between those two points the only code that runs is +// before-phase hook code — server code, by definition; +// * a caller cannot execute an assignment. Echoing a key back, echoing a +// value back, sending `null`, sending a `Proxy`, sending a getter — none of +// them is a `set` on this object, so none of them adds a key to the record. +// +// The record therefore only ever converts a "strip" verdict into a "keep" one, +// for keys a hook demonstrably assigned. Every other verdict is left to the +// two-part test that was already there — which stays, unchanged, as the answer +// for every key this recorder has nothing to say about. +// +// ## KNOWN LIMIT: a hook that REPLACES the payload object +// +// `ctx.input.data.x = 1` is recorded. `ctx.input.data = { …ctx.input.data, x: 1 }` +// is not: the replacement is a fresh object whose keys are indistinguishable +// from the caller's, because most of them ARE the caller's, spread across. So +// {@link HookWriteRecording.seal} returns NO record at all for that call and +// the strip falls back to the pre-#14088 value comparison. +// +// That fallback direction is deliberate and is the only safe one: the fallback +// OVER-strips (the pre-existing defect) where the alternative — treating a +// replacement's keys as hook-owned — would launder a caller's forged +// `created_by` into a platform write on any object with such a hook. Fail-safe +// here means "keep the old bug", and keeping the old bug is strictly better +// than opening the lock. Same shape, and the same argument, as the SHALLOW +// snapshot limit already documented on `stripReadonlyFields`: a hook that means +// to own a read-only column should ASSIGN to it. + +/** + * A live recording of the keys a hook chain assigns on one write payload. + * + * Produced by {@link recordHookPayloadWrites}, handed to the before-phase as + * `hookContext.input.data`, and closed by {@link HookWriteRecording.seal} + * before anything else in the engine touches the payload. + */ +export interface HookWriteRecording { + /** + * The object to hand the hook phase INSTEAD of the raw payload. + * + * It is a transparent write-through view of the raw payload — reads, spreads, + * `Object.keys`, `JSON.stringify` and in-place mutation all behave exactly as + * they do on the payload itself, and every write lands on the SAME underlying + * object, so a hook that mutates in place is mutating the engine's payload + * exactly as it always has. + */ + readonly payload: Record; + /** + * Close the recording and hand back the payload the rest of the write must + * use. + * + * `current` is whatever the hook phase left in `hookContext.input.data`. + * Pass it in rather than assuming: a hook is allowed to REPLACE the payload, + * and that case is exactly the one with no attributable record (see the + * KNOWN LIMIT above). + * + * Sealing is what keeps the record honest about its own boundary: the engine + * writes to this payload too (secret encryption, multi-value normalisation, + * the strips' own shallow copies), and a recorder still armed for those would + * report ENGINE writes as HOOK writes — which, on a caller-forged secret + * field, is the privilege escalation this module is built to make + * impossible. The returned set is a snapshot; later writes through a stashed + * reference cannot grow it. + */ + seal(current: unknown): SealedHookWrites; +} + +/** What {@link HookWriteRecording.seal} hands back. */ +export interface SealedHookWrites { + /** + * The payload the rest of the write must use — the RAW object when the + * recording survived (never the recording view, which must not reach a + * driver), else `current` untouched. + */ + data: Record | undefined; + /** + * The keys a hook assigned, or `undefined` when this call has no attributable + * record (a hook replaced the payload object). `undefined` is not "no hook + * wrote anything" — it is "this call cannot say", and every consumer must + * treat it as the pre-#14088 fallback rather than as an empty set. + */ + hookWrittenKeys?: ReadonlySet; +} + +/** + * Arm a recording of hook writes over `target`. + * + * ⚠️ Arm AFTER the caller's entry snapshot and SEAL before any engine-owned + * mutation of the payload — see the forgery-boundary note at the top of this + * file. Both ends are load-bearing; neither is a style choice. + */ +export function recordHookPayloadWrites(target: Record): HookWriteRecording { + const written = new Set(); + let sealed = false; + + const record = (key: string | symbol): void => { + // Symbols are not field names (a field is `^[a-z_][a-z0-9_]*$`), so a + // symbol write can never be about a column the strip judges. Ignored rather + // than stringified, so nothing a hook stashes under a symbol can ever + // collide with a real key's provenance. + if (sealed || typeof key !== 'string') return; + written.add(key); + }; + + const payload = new Proxy(target, { + set(t, key, value) { + // Three-argument `Reflect.set`: the receiver defaults to the TARGET, not + // to the proxy. Passing the proxy through as the receiver re-enters this + // trap for any accessor property and recurses until the stack goes. + const ok = Reflect.set(t, key, value); + if (ok) record(key); + return ok; + }, + defineProperty(t, key, descriptor) { + // `Object.defineProperty(ctx.input.data, k, …)` is a hook write too, and + // it does NOT route through the `set` trap. Rare in hook code; recorded + // because a provenance channel with a second, unwatched write verb is a + // provenance channel that answers wrongly on exactly the writes someone + // took care over. + const ok = Reflect.defineProperty(t, key, descriptor); + if (ok) record(key); + return ok; + }, + deleteProperty(t, key) { + const ok = Reflect.deleteProperty(t, key); + // A deleted key holds no value for the strip to keep, and re-adding it + // records it again. Dropping it here keeps the set meaning "a hook + // assigned the value standing on this key" rather than "a hook once + // touched this name". + if (ok && typeof key === 'string') written.delete(key); + return ok; + }, + }); + + return { + payload, + seal(current: unknown): SealedHookWrites { + sealed = true; + if (current !== payload) { + // Replaced wholesale — no attributable record. KNOWN LIMIT above. + return { data: current as Record | undefined }; + } + return { data: target, hookWrittenKeys: new Set(written) }; + }, + }; +} diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 52b08b30fe..1f20551486 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -1023,6 +1023,34 @@ export function isRuntimeOwnedField(def: { type?: string } | undefined | null): * verdict as before. What changed is exclusively the case where a hook already * overwrote the key — where the value being deleted was never the caller's. * + * ### ...and why VALUES are not enough either (#14088) + * + * #5591's own argument applied one turn further, and it is the argument rather + * than a new one: it retired the key SET because that made the contract + * ("hook-written keys are NOT caller-supplied") true only BY ACCIDENT. Value + * equality is true by accident in exactly the same way. `Object.is` 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. + * + * Measured downstream (objectstack#14088, from a `duly_task` on published + * 17.2.0): a `readonly` `completed_at` stamped by a `beforeUpdate` hook on the + * transition INTO `done` and CLEARED on the transition out. Reopening works — + * until the caller also sends `completed_at: null`, which is exactly what a + * form round-trip of the whole record does. `Object.is(null, null)` is `true`, + * the hook's clear is deleted along with the caller's null, and the row commits + * `status = in_progress` still carrying its OLD completion timestamp, with no + * error. The corrupted row is the one a validation rule cannot catch — "a + * completed task must carry a completion timestamp" has no purchase on its + * inverse — and nothing downstream can tell it from a genuinely completed one. + * + * ⛔ Not a `null` bug, and `null` must not be special-cased: `0`, `''`, `false` + * and a shared object reference collide identically, and every one of them is + * echoed back by the same whole-record write-back idiom. The distinction is + * PROVENANCE, it cannot be recovered from the values after the fact, and so it + * is RECORDED while the hook writes happen — `options.hookWrittenKeys`, fed by + * `recordHookPayloadWrites` at the engine's before-phase seam. The value test + * above stays as the fallback for every key the record cannot speak to. + * * KNOWN LIMIT, deliberately not papered over: the snapshot is SHALLOW, so a hook * that mutates a caller-supplied object or array IN PLACE * (`data.some_json.x = 1`) is indistinguishable from a hook that did nothing — @@ -1122,13 +1150,33 @@ export function stripReadonlyFields( data: Record | undefined | null, supplied: Readonly>, logger?: EvaluateRulesOptions['logger'], - options?: { preserveAudit?: boolean; addressKey?: string; strictReadonlyWrites?: boolean }, + options?: { + preserveAudit?: boolean; + addressKey?: string; + strictReadonlyWrites?: boolean; + /** + * [#14088] The keys the before-phase hook chain ACTUALLY ASSIGNED on this + * payload, recorded while the writes happened — see + * `recordHookPayloadWrites`. OPTIONAL, and absent means "this call cannot + * say", never "no hook wrote anything": a call site with no recording + * (every direct caller of this function, and any write whose hook replaced + * the payload object) falls back to the two-part test below, exactly as + * before this option existed. + * + * ⚠️ It may only ever turn a STRIP into a KEEP, and only for a key a hook + * assigned. A caller cannot put a key in here — see the forgery-boundary + * note on the recorder — and any future producer of this set owes the same + * proof, because a key in this set is a key this strip stops defending. + */ + hookWrittenKeys?: ReadonlySet; + }, ): Record | undefined | null { const fields = objectSchema?.fields; if (!fields || !data) return data; const preserveAudit = options?.preserveAudit === true; const addressKey = options?.addressKey; const strict = options?.strictReadonlyWrites === true; + const hookWrittenKeys = options?.hookWrittenKeys; let result = data; for (const [name, def] of Object.entries(fields)) { // [#5503] `readonly: true` is the AUTHOR-declared lock; a runtime-owned @@ -1142,12 +1190,29 @@ export function stripReadonlyFields( // any plain snapshot, so `in` would call a hook stamp caller-supplied and // strip it. if (!Object.prototype.hasOwnProperty.call(supplied, name)) continue; // server-stamped, not caller-supplied — keep + // [#14088] ...or a hook ASSIGNED it. Asked BEFORE the value comparison + // because it answers the question the comparison is only a proxy for, and + // answers it by RECORD rather than by inference: `Object.is` collapses "the + // hook deliberately wrote the value the caller also sent" into "the hook + // never touched it", and the two demand opposite verdicts. `null` is where + // that collision was measured (a hook CLEARING a readonly column against a + // caller that round-tripped the whole record, committing `status = + // in_progress` beside a stale `completed_at`, with no error), but it is not + // a `null` bug — `0`, `''`, `false` and a shared object reference collide + // identically, which is why the fix is provenance and not a sentinel. + if (hookWrittenKeys?.has(name)) continue; // the hook wrote this value — keep // [#5591] ...and it must still BE the caller's value. A hook that // overwrote this key wrote a PLATFORM value; deleting that is what put // `status = published` rows in the database with `published_at = null`. // `Object.is`, not `===`, on purpose: `===` 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. + // + // [#14088] STAYS, and stays as the fallback for every key the record above + // has nothing to say about: a call site that passes no `hookWrittenKeys` + // gets byte-identical behaviour, and a hook that REPLACED the payload + // object leaves no record, so this test is what still separates its + // overwrite from a forgery. 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 };