From 581b86eee65d7d10c43a35cde08f9ec8447b6e76 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 14:11:35 +0000 Subject: [PATCH 1/2] fix(objectql): update readonly strip acts on caller-submitted values (#5591) The static-`readonly` write strip runs after `beforeUpdate`, but decided what to delete from a snapshot of the caller's KEY NAMES. Those are different facts the moment a hook writes to a read-only column: `delete data[name]` took the hook's value with it whenever the caller's payload happened to carry the same key. Measured downstream (hotcrm#788): a REST caller reads a whole `crm_knowledge_article`, flips `status` to `published`, and PUTs the whole record back -- `published_at: null` included, because that is what it read. The publish hook stamped `published_at` on the transition; the strip then deleted the stamp, and the row committed as `status = "published"` with `published_at = null`. The same hook's `last_reviewed_at` -- equally read-only, but not echoed by the caller -- landed in that same write. The entry snapshot now carries the caller's values (a copy: hooks mutate `opCtx.data` in place), and a read-only key is stripped only while it still holds the caller's own value. A key a hook overwrote is a platform write and survives -- the same verdict #4903 already pins for a read-only key a hook ADDS. Route chosen by measurement, not preference: stripping BEFORE the hooks would also work, but a `beforeUpdate` guard that reports on what the caller submitted reads `ctx.input.data` -- plugin-auth's ADR-0092 identity write guard NAMES the non-whitelisted keys it finds -- and pre-hook stripping empties that out. The caller's payload therefore still reaches the hooks unchanged; only the strip's verdict narrowed. Not a relaxation of #2948 / #3003 / #3015: a caller-supplied read-only value no hook overwrote is dropped exactly as before, on both the single-id and predicate paths, and `isSystem` / `preserveAudit` are untouched. INSERT is unchanged (its own narrower strip carries the same defect -- filed as #6339, not fixed here). Reverse-verified: restoring the pre-fix reading turns exactly the 8 predicted pins red -- including `expected null not to be null` on the reported scenario -- and leaves every #2948 and #4903 case green. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .changeset/eighty-jars-tickle.md | 35 ++ ...ngine-readonly-strip-caller-values.test.ts | 324 ++++++++++++++++++ .../src/engine-readonly-strip-signal.test.ts | 65 ++-- packages/objectql/src/engine.ts | 33 +- .../src/validation/rule-validator.test.ts | 110 ++++-- .../objectql/src/validation/rule-validator.ts | 68 +++- .../dogfood/test/authz-conformance.matrix.ts | 2 +- 7 files changed, 577 insertions(+), 60 deletions(-) create mode 100644 .changeset/eighty-jars-tickle.md create mode 100644 packages/objectql/src/engine-readonly-strip-caller-values.test.ts diff --git a/.changeset/eighty-jars-tickle.md b/.changeset/eighty-jars-tickle.md new file mode 100644 index 0000000000..f63c737e93 --- /dev/null +++ b/.changeset/eighty-jars-tickle.md @@ -0,0 +1,35 @@ +--- +'@objectstack/objectql': patch +--- + +fix(objectql): the update-path `readonly` strip now drops the value the CALLER submitted, not whatever value the key holds when it runs + +The static-`readonly` write strip runs after `beforeUpdate`, but decided what to +delete from a snapshot of the caller's KEY NAMES. Those are different facts the +moment a hook writes to a read-only column: `delete data[name]` took the hook's +value with it whenever the caller's payload happened to carry the same key. + +Behaviour change — a whole-record write-back no longer erases hook writes. The +reported shape: a REST caller reads a record, flips `status` to `published`, and +PUTs the whole record back — `published_at: null` included, because that is what +it read. The publish hook stamped `published_at` on the transition; the strip +then deleted the stamp, and the row committed as `status = "published"` with +`published_at = null`, which every view sorting or filtering by `published_at` is +undefined on. The same hook's `last_reviewed_at` — equally read-only, but not +echoed by the caller — landed in that same write. Two hook-derived writes, one +alive and one dead, decided by nothing but a key name collision. + +The entry snapshot now carries the caller's values, and a read-only key is +stripped only while it still holds the caller's own value. A key a hook +overwrote is a platform write and survives — the same verdict the runtime +already gave a read-only key a hook ADDS. + +Not a relaxation of the read-only write rule: a caller-supplied read-only value +that no hook overwrote is dropped exactly as before, on both the single-id and +predicate update paths, and `isSystem` / `preserveAudit` are untouched. The +insert path is unchanged. + +Known limit, by design: the snapshot is shallow, so a hook that mutates a +caller-supplied object or array IN PLACE is indistinguishable from a hook that +did nothing, and the field is still stripped. A hook that means to write a +read-only column should assign to it. diff --git a/packages/objectql/src/engine-readonly-strip-caller-values.test.ts b/packages/objectql/src/engine-readonly-strip-caller-values.test.ts new file mode 100644 index 0000000000..66ce5a4785 --- /dev/null +++ b/packages/objectql/src/engine-readonly-strip-caller-values.test.ts @@ -0,0 +1,324 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #5591 — the static `readonly` strip on the UPDATE path must delete the value +// the CALLER SUBMITTED, never whatever value happens to sit on the key at the +// moment the strip runs. +// +// The strip executes AFTER `beforeUpdate`, so those two are different facts the +// instant a hook writes to a read-only column. The guard used to be a key SET +// snapshotted at engine entry, which can answer only "did the caller name this +// key?" — so `delete data[name]` took the hook's value with it whenever the +// caller's payload happened to carry the same key. +// +// The measured downstream shape (objectstack#5591, from hotcrm#788, reproduced +// below verbatim): "read the whole record → change one field → write the whole +// record back" is an ordinary REST/integration idiom, and a whole-record +// write-back necessarily echoes the read-only columns it just read. A publish +// hook stamped `published_at` on the draft→published transition; the strip then +// deleted the stamp because `published_at` was in the caller's payload. The row +// committed as `status = "published"` with `published_at = null` — a state every +// view that sorts or filters by `published_at` is undefined on. The console UI +// never triggered it because its forms do not submit read-only fields. +// +// The asymmetry that proves it was never deliberate is in ONE write: the same +// hook also stamped `last_reviewed_at`, an equally read-only column the caller +// had NOT echoed — and that one landed. Two hook-derived writes in one +// transaction, one alive and one dead, decided by nothing but whether the +// caller's payload carried a same-named key. +// +// What this suite is NOT: a relaxation of #2948 / #3003 / #3015. A +// caller-supplied read-only value that no hook overwrote is still stripped, and +// the case is pinned here next to the fix so the two verdicts are read together. + +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]) => 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) { + return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + n += 1; + const id = (data.id as string) ?? `r_${n}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const row = { ...s.get(id), ...data, id }; + s.set(id, row); + return row; + }, + async updateMany(object: string, ast: any, data: Record) { + const s = storeFor(object); + let count = 0; + for (const row of [...s.values()]) { + if (!matches(row, ast?.where)) continue; + s.set(row.id, { ...row, ...data, id: row.id }); + count += 1; + } + return count; + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r, undefined))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, storeFor }; +} + +const NOW = '2026-08-05T10:00:00.000Z'; + +describe('update strip acts on CALLER-submitted values (#5591)', () => { + let engine: ObjectQL; + let storeFor: ReturnType['storeFor']; + let warns: string[]; + + 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(); + + // The downstream object, trimmed to the fields the report turns on. + engine.registry.registerObject({ + name: 'crm_knowledge_article', + fields: { + title: { type: 'text' }, + status: { type: 'text' }, + published_at: { type: 'datetime', readonly: true }, + last_reviewed_at: { type: 'datetime', readonly: true }, + }, + } as any); + storeFor('crm_knowledge_article').set('ka_1', { + id: 'ka_1', title: 'A', status: 'draft', published_at: null, last_reviewed_at: null, + }); + + // `sys_fetch_previous_update` (the kernel builtin, `object: '*'`, + // priority 5) replicated: it binds `previous` before any authored + // before-hook runs, which is how a transition is expressed in one. + engine.registerHook('beforeUpdate', async (ctx: any) => { + if (!ctx.previous && ctx.input?.id) { + ctx.previous = await engine.findOne(ctx.object, { where: { id: ctx.input.id } } as any); + } + }, { priority: 5 }); + + // `knowledge_article_publish_timestamps`: stamp both read-only columns on + // the draft→published transition. + engine.registerHook('beforeUpdate', async (ctx: any) => { + if (ctx.input.data.status === 'published' && ctx.previous?.status !== 'published') { + ctx.input.data.published_at = NOW; + ctx.input.data.last_reviewed_at = NOW; + } + }, { object: 'crm_knowledge_article', priority: 50 }); + }); + + const ka = (id = 'ka_1') => storeFor('crm_knowledge_article').get(id); + + it('THE REPORT: a whole-record write-back lands the hook stamp, not null', async () => { + // Exactly the reported call: the caller read the record, flipped `status`, + // and PUT everything back — `published_at: null` included, because that is + // what it had just read. Non-system context. + await engine.update('crm_knowledge_article', { + id: 'ka_1', title: 'A', status: 'published', + published_at: null, last_reviewed_at: null, + }); + + expect(ka().status).toBe('published'); + // The regression, stated as the value it must NOT be. + expect(ka().published_at).not.toBeNull(); + expect(ka().published_at).toBe(NOW); + // ...and the field that always worked still works, so the fix closed the + // asymmetry rather than inverting it. + expect(ka().last_reviewed_at).toBe(NOW); + }); + + it('the two read-only columns of one write now agree (the asymmetry is gone)', async () => { + // The proof the old behaviour was accidental: echo ONE of the two keys and + // watch only that one die. After the fix both stamps land either way. + await engine.update('crm_knowledge_article', { + id: 'ka_1', status: 'published', published_at: null, // last_reviewed_at NOT echoed + }); + expect(ka().published_at).toBe(ka().last_reviewed_at); + expect(ka().published_at).toBe(NOW); + }); + + it('#2948 UNCHANGED: an explicit forge with no hook overwrite is still stripped', async () => { + // No transition, so the publish hook does not fire and nothing overwrites + // the key — the caller's value is the value on the key, and it goes. + await engine.update('crm_knowledge_article', { + id: 'ka_1', title: 'B', published_at: '1999-01-01T00:00:00.000Z', + }); + expect(ka().title).toBe('B'); + expect(ka().published_at).toBeNull(); + expect(warns.some((w) => w.includes("Field 'published_at'") && w.includes('COMMITTED WITHOUT IT'))).toBe(true); + }); + + it('#2948 UNCHANGED: a forge on a field the hook stamps but for a DIFFERENT record state', async () => { + // The article is already published, so the transition guard is false and + // the hook writes nothing. A caller forging `published_at` in that state + // gets it stripped — the hook's existence is not a blanket exemption for + // the column, only for the writes it actually makes. + storeFor('crm_knowledge_article').set('ka_2', { + id: 'ka_2', title: 'B', status: 'published', published_at: NOW, last_reviewed_at: NOW, + }); + await engine.update('crm_knowledge_article', { + id: 'ka_2', status: 'published', published_at: '1999-01-01T00:00:00.000Z', + }); + expect(ka('ka_2').published_at).toBe(NOW); + }); + + it('#4903 CONTROL still holds: a read-only key the hook ADDS lands', async () => { + // The control face the report names. It passed before the fix and must + // keep passing — the fix makes the overwrite case agree with it, and is + // worthless if it moved this one. + await engine.update('crm_knowledge_article', { id: 'ka_1', status: 'published' }); + expect(ka().published_at).toBe(NOW); + expect(ka().last_reviewed_at).toBe(NOW); + }); + + it('the BULK path is fixed on the same terms', async () => { + // `stripReadonlyFields` runs on both update branches off one snapshot, so + // the predicate write must not need its own fix — pinned, because "both + // call sites" is exactly the #3106 / #4441 shape that gets missed. + storeFor('crm_knowledge_article').set('ka_3', { + id: 'ka_3', title: 'C', status: 'draft', published_at: null, last_reviewed_at: null, + }); + await engine.update( + 'crm_knowledge_article', + { status: 'published', published_at: null }, + { where: { status: 'draft' }, multi: true } as any, + ); + expect(ka('ka_3').status).toBe('published'); + expect(ka('ka_3').published_at).toBe(NOW); + }); + + it('the BULK path still strips a forge no hook overwrote', async () => { + storeFor('crm_knowledge_article').set('ka_4', { + id: 'ka_4', title: 'D', status: 'archived', published_at: null, last_reviewed_at: null, + }); + await engine.update( + 'crm_knowledge_article', + { title: 'D2', published_at: '1999-01-01T00:00:00.000Z' }, + { where: { status: 'archived' }, multi: true } as any, + ); + expect(ka('ka_4').title).toBe('D2'); + expect(ka('ka_4').published_at).toBeNull(); + }); + + it('a hook-overwritten key is NOT reported as dropped to onFieldsDropped', async () => { + // `DroppedFieldsEvent` is contracted as "dropped, and the write completed + // WITHOUT them" (#3407). After the fix the column IS written — with the + // platform's value — so reporting it as dropped would make the observability + // seam lie. The forge case below proves the listener still fires when a + // value really is discarded. + const events: any[] = []; + await engine.update( + 'crm_knowledge_article', + { id: 'ka_1', status: 'published', published_at: null }, + { onFieldsDropped: (e: any) => events.push(e) } as any, + ); + expect(events).toEqual([]); + expect(ka().published_at).toBe(NOW); + }); + + it('onFieldsDropped still fires for a value that really is discarded', async () => { + const events: any[] = []; + await engine.update( + 'crm_knowledge_article', + { id: 'ka_1', title: 'B', published_at: '1999-01-01T00:00:00.000Z' }, + { onFieldsDropped: (e: any) => events.push(e) } as any, + ); + expect(events).toEqual([ + { object: 'crm_knowledge_article', fields: ['published_at'], reason: 'readonly' }, + ]); + }); + + it('strictReadonlyWrites refuses the forge and admits the hook write', async () => { + // #5126 refuses a write rather than committing it without the stripped + // columns. A hook-overwritten key is not stripped, so there is nothing to + // refuse — the strict caller's contract is about columns that would be + // MISSING, and none are. + await expect(engine.update( + 'crm_knowledge_article', + { id: 'ka_1', title: 'B', published_at: '1999-01-01T00:00:00.000Z' }, + { strictReadonlyWrites: true } as any, + )).rejects.toThrow(); + + await engine.update( + 'crm_knowledge_article', + { id: 'ka_1', status: 'published', published_at: null }, + { strictReadonlyWrites: true } as any, + ); + expect(ka().published_at).toBe(NOW); + }); + + it('a hook that reads the caller-submitted read-only value can still SEE it', async () => { + // Why the fix compares values instead of stripping before the hooks: a + // `beforeUpdate` guard that rejects or reports on what the caller + // submitted (plugin-auth's ADR-0092 identity write guard is the in-repo + // instance — its error text NAMES the non-whitelisted keys it found) reads + // `ctx.input.data`. Stripping ahead of the hooks would empty that out and + // silently degrade every such diagnostic, so the caller's payload still + // reaches the hooks unchanged. + const seen: unknown[] = []; + engine.registerHook('beforeUpdate', async (ctx: any) => { + seen.push(Object.keys(ctx.input.data)); + }, { object: 'crm_knowledge_article', priority: 1 }); + + await engine.update('crm_knowledge_article', { + id: 'ka_1', title: 'B', published_at: '1999-01-01T00:00:00.000Z', + }); + expect(seen).toEqual([['id', 'title', 'published_at']]); + }); + + it('an isSystem caller is untouched by any of this', async () => { + await engine.update( + 'crm_knowledge_article', + { id: 'ka_1', published_at: '1999-01-01T00:00:00.000Z' }, + { context: { isSystem: true } } as any, + ); + expect(ka().published_at).toBe('1999-01-01T00:00:00.000Z'); + }); + + it('INSERT is unaffected — a caller-seeded runtime-owned field still goes', async () => { + // The insert path keeps its own, narrower strip (`stripRuntimeOwnedFields`, + // #5503) and its own snapshot; #5591 did not touch either. Pinned as a + // regression boundary, not as a claim about insert semantics. + engine.registry.registerObject({ + name: 'crm_case', fields: { title: { type: 'text' }, case_number: { type: 'autonumber' } }, + } as any); + const row: any = await engine.insert('crm_case', { title: 'x', case_number: 'FORGED-9' }); + expect(row.case_number).not.toBe('FORGED-9'); + }); +}); diff --git a/packages/objectql/src/engine-readonly-strip-signal.test.ts b/packages/objectql/src/engine-readonly-strip-signal.test.ts index 74d41dba39..4a47a83836 100644 --- a/packages/objectql/src/engine-readonly-strip-signal.test.ts +++ b/packages/objectql/src/engine-readonly-strip-signal.test.ts @@ -15,10 +15,13 @@ // This suite pins the three things that decide how expensive that is to // diagnose: // -// 1. WHY the hook path differs (`suppliedKeys` is snapshotted at engine entry, -// BEFORE middleware and beforeUpdate hooks run) — the asymmetry the issue -// reports is pinned as EXISTING behaviour so the next change to it is -// deliberate. Nothing here endorses it. +// 1. WHY the hook path differs (the caller's payload is snapshotted at engine +// entry, BEFORE middleware and beforeUpdate hooks run, so a hook write is +// not a caller write). #5591 narrowed that snapshot's reading from a key +// SET to the keys AND VALUES: a hook that OVERWRITES a read-only key the +// caller also sent now survives too, where before it was deleted along +// with the caller's value. See the block at the bottom of this file — the +// case that pinned the old behaviour was replaced there, not re-spelled. // 2. The strip's log states the CONSEQUENCE and both REMEDIES, so the log is // actionable on its own (#4632's second-class shape: caller believes // persisted, database disagrees, log is the only trace). @@ -210,7 +213,7 @@ describe('static `readonly` write strip — caller-facing signal (#4903)', () => stripReadonlyFields( { name: 'attendance', fields: { work_duration: { type: 'number', readonly: true } } } as any, { work_duration: 480 }, - new Set(['work_duration']), + { work_duration: 480 }, probe, ); expect(calls.map(([level]) => level)).toEqual(['warn']); @@ -221,14 +224,22 @@ describe('static `readonly` write strip — caller-facing signal (#4903)', () => expect(readonlyStripWarning('work_duration')).toContain("Field 'work_duration' is read-only"); }); - // ── 4. the hook/plugin asymmetry, PINNED as-is ────────────────────────── + // ── 4. hook write vs. caller supply — the two are now judged alike ────── + // + // This block used to pin the OPPOSITE of its second case, under the heading + // "the hook/plugin asymmetry, PINNED as-is", explicitly as a mechanism record + // and explicitly not as an endorsement ("a future change is made on purpose + // instead of by accident"). #5591 is that change, so the case is REPLACED + // rather than re-spelled: it asserted that a hook could not rescue a key the + // caller had supplied, which is exactly the limb that was removed. // - // NOT an endorsement. The issue asks whether a beforeUpdate hook SHOULD be - // able to write a column a plugin cannot; that question is open. This pins - // the current answer and — more usefully — the MECHANISM, so a future change - // is made on purpose instead of by accident. + // What survives unchanged is the principle underneath: a value a HOOK wrote + // is a platform write and lands; a value the CALLER submitted to a read-only + // column does not. #4903 pins that from the "hook adds a new key" side, and + // the old asymmetry was that the same hook write died instead whenever the + // caller's payload happened to carry the same key name. - describe('beforeUpdate backfill vs. caller supply (pinned mechanism)', () => { + describe('beforeUpdate write vs. caller supply', () => { it('a hook-written readonly field LANDS while the same field supplied by the caller is stripped', async () => { engine.registerHook('beforeUpdate', async (ctx: any) => { ctx.input.data.work_duration = 480; @@ -239,12 +250,13 @@ describe('static `readonly` write strip — caller-facing signal (#4903)', () => expect(att().work_duration).toBe(480); }); - it('a hook cannot rescue a key the CALLER supplied — the snapshot is taken first', async () => { - // The mechanism, stated: `suppliedKeys` is `new Set(Object.keys(data))` - // captured at engine entry, BEFORE middleware and beforeUpdate hooks run. - // A key the hook ADDS is absent from that snapshot and survives; a key the - // caller sent is in it and is stripped no matter what the hook does to the - // value afterwards. + it('[#5591] a hook OVERWRITING a key the caller supplied now survives the strip', async () => { + // The mechanism, restated: the entry snapshot carries the caller's VALUES, + // and the strip deletes a read-only key only while it still holds the + // caller's own value. A hook that writes over it has replaced a caller + // write with a platform write, and platform writes to read-only columns + // are legitimate. Before #5591 the snapshot was a key SET, so the hook's + // 999 was deleted and the column kept its stored null. storeFor('attendance').set('att_2', { id: 'att_2', status: 'open', work_duration: null }); engine.registerHook('beforeUpdate', async (ctx: any) => { if (ctx.input.data.work_duration !== undefined) ctx.input.data.work_duration = 999; @@ -252,20 +264,29 @@ describe('static `readonly` write strip — caller-facing signal (#4903)', () => await engine.update('attendance', { id: 'att_2', status: 'closed', work_duration: 480 }); expect(storeFor('attendance').get('att_2')).toMatchObject({ status: 'closed' }); - expect(storeFor('attendance').get('att_2').work_duration).toBeNull(); + // The HOOK's value — never the caller's 480. + expect(storeFor('attendance').get('att_2').work_duration).toBe(999); + }); + + it('[#5591] with NO hook on the key, the caller-supplied value is still stripped', async () => { + // The #2948 verdict, unchanged: this is the case the strip exists for, + // and it must not have moved a millimetre. + storeFor('attendance').set('att_3', { id: 'att_3', status: 'open', work_duration: null }); + await engine.update('attendance', { id: 'att_3', status: 'closed', work_duration: 480 }); + expect(storeFor('attendance').get('att_3').work_duration).toBeNull(); }); it('the engine-stamped audit column is the same exemption, not a special case', () => { // `updated_by` survives a user write for exactly one reason: the audit - // hook writes it, so it is not in `suppliedKeys`. Supplied explicitly, it - // is dropped like any other readonly field. + // hook writes it, so it is not in the caller's snapshot. Supplied + // explicitly, it is dropped like any other readonly field. const schema = { name: 'attendance', fields: { updated_by: { type: 'text', readonly: true, system: true } }, } as any; - const stamped = stripReadonlyFields(schema, { updated_by: 'hook-stamp' }, new Set()); + const stamped = stripReadonlyFields(schema, { updated_by: 'hook-stamp' }, {}); expect(stamped).toEqual({ updated_by: 'hook-stamp' }); - const forged = stripReadonlyFields(schema, { updated_by: 'attacker' }, new Set(['updated_by'])); + const forged = stripReadonlyFields(schema, { updated_by: 'attacker' }, { updated_by: 'attacker' }); expect(forged).toEqual({}); }); }); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 40f08541ae..2c4754b4e8 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -5690,13 +5690,28 @@ export class ObjectQL implements IObjectQLEngine { opCtx.ast = { object, ...(options?.where !== undefined ? { where: options.where } : {}) } as QueryAST; } - // [#2948] Snapshot the keys the CALLER supplied, BEFORE any middleware / + // [#2948] Snapshot what the CALLER supplied, BEFORE any middleware / // beforeUpdate hook stamps server-managed columns (owner/tenant stamp, // `updated_by`/`updated_at`). The static-`readonly` strip below drops only // caller-supplied read-only writes, so hook/middleware stamps survive. - const suppliedKeys: ReadonlySet = new Set( - Object.keys((opCtx.data ?? {}) as Record), - ); + // + // [#5591] KEYS ARE NOT ENOUGH — this snapshot carries the VALUES too, and + // it must be a COPY. Hooks mutate `opCtx.data` IN PLACE + // (`ctx.input.data.x = …` — `hookContext.input.data` starts as this very + // reference), so a snapshot that aliased it would track those mutations + // and answer every question about "what the caller sent" with the + // post-hook payload. A key-only snapshot was already immune to that; a + // value snapshot only stays immune because of the spread. + // + // Why values: the strip runs AFTER the hooks (below), so "this key is + // caller-supplied" and "this key still holds the caller's value" are + // different facts, and only the second one licenses a delete. Reading the + // first as the second deleted hook-written timestamps whenever the caller + // had echoed the key back — see `stripReadonlyFields` for the measured + // downstream row (`status = published`, `published_at = null`). + const suppliedValues: Readonly> = { + ...((opCtx.data ?? {}) as Record), + }; // [#3407] Structured strip observability. The readonly/readonlyWhen strips // below are LEGAL semantics (the write still succeeds without the locked @@ -5921,11 +5936,13 @@ export class ObjectQL implements IObjectQLEngine { // [#2948] Enforce STATIC `readonly` on the write path for // non-system callers (system writes legitimately set read-only // columns and are exempt). Runs AFTER hooks/middleware stamped - // their columns; `suppliedKeys` ensures only caller-forged - // read-only writes are dropped, never the server stamps. + // their columns; `suppliedValues` ensures only caller-forged + // read-only writes are dropped, never the server stamps — and + // (#5591) never a stamp a hook wrote OVER a key the caller + // happened to echo back. if (!opCtx.context?.isSystem) { const preRo = hookContext.input.data as Record; - hookContext.input.data = stripReadonlyFields(updateSchema as any, preRo, suppliedKeys, this.logger, { preserveAudit: opCtx.context?.preserveAudit === true }) as any; + hookContext.input.data = stripReadonlyFields(updateSchema as any, preRo, suppliedValues, this.logger, { preserveAudit: opCtx.context?.preserveAudit === true }) as any; reportDroppedFields(preRo, hookContext.input.data as Record, 'readonly'); } // [#5126] Both strip passes are done; refuse now if the caller @@ -6017,7 +6034,7 @@ export class ObjectQL implements IObjectQLEngine { // rejected upstream by the tenant write wall, #2946). if (!opCtx.context?.isSystem) { const preRoMulti = hookContext.input.data as Record; - hookContext.input.data = stripReadonlyFields(updateSchema as any, preRoMulti, suppliedKeys, this.logger, { preserveAudit: opCtx.context?.preserveAudit === true }) as any; + hookContext.input.data = stripReadonlyFields(updateSchema as any, preRoMulti, suppliedValues, this.logger, { preserveAudit: opCtx.context?.preserveAudit === true }) 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/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index 9e3f664318..84865f867e 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -289,26 +289,26 @@ const stampedFields = { describe('stripReadonlyFields (#2948)', () => { it('drops a caller-supplied write to a static readonly field', () => { - const supplied = new Set(['title', 'created_by']); - const out = stripReadonlyFields(stampedFields, { title: 'x', created_by: 'attacker' }, supplied); + const supplied = { title: 'x', created_by: 'attacker' }; + const out = stripReadonlyFields(stampedFields, { ...supplied }, supplied); expect(out).toEqual({ title: 'x' }); }); it('KEEPS a readonly field the caller did NOT supply (server stamp survives)', () => { // `updated_by` was written into `data` by the audit-stamp hook, not the // caller — it must not be stripped. - const supplied = new Set(['title']); + const supplied = { title: 'x' }; const out = stripReadonlyFields(stampedFields, { title: 'x', updated_by: 'u1' }, supplied); expect(out).toEqual({ title: 'x', updated_by: 'u1' }); }); it('returns the SAME object when nothing is stripped', () => { const d = { title: 'x' }; - expect(stripReadonlyFields(stampedFields, d, new Set(['title']))).toBe(d); + expect(stripReadonlyFields(stampedFields, d, { title: 'x' })).toBe(d); }); it('drops a caller-forged readonly field even when it also carries a server stamp key', () => { - const supplied = new Set(['title', 'created_by']); + const supplied = { title: 'x', created_by: 'attacker' }; const out = stripReadonlyFields( stampedFields, { title: 'x', created_by: 'attacker', updated_by: 'u1' }, @@ -318,6 +318,64 @@ describe('stripReadonlyFields (#2948)', () => { }); }); +// #5591 — the strip must delete the value the CALLER SUBMITTED, never the value +// that happens to sit on the key when the strip runs. The strip executes after +// `beforeUpdate`, so those two differ exactly when a hook overwrote a key the +// caller had also named — which is what silently deleted hook-written publish +// timestamps on whole-record write-backs (hotcrm#788). +describe('stripReadonlyFields — supplied VALUE identity, not just key presence (#5591)', () => { + it('KEEPS a readonly key a hook OVERWROTE, even though the caller supplied it', () => { + // The caller echoed `created_by` back; a beforeUpdate hook then wrote its + // own value over it. What is on the key now is a PLATFORM write. + const supplied = { title: 'x', created_by: 'attacker' }; + const afterHooks = { title: 'x', created_by: 'hook-resolved-owner' }; + const out = stripReadonlyFields(stampedFields, afterHooks, supplied); + expect(out).toEqual({ title: 'x', created_by: 'hook-resolved-owner' }); + expect(out).toBe(afterHooks); // nothing dropped ⇒ same reference + }); + + it('STILL drops it when the hook wrote the caller value back unchanged', () => { + // Identity is the whole test: an unchanged value is indistinguishable from + // "no hook touched it", and the fail-safe direction is to strip. + const supplied = { created_by: 'attacker' }; + const out = stripReadonlyFields(stampedFields, { created_by: 'attacker' }, supplied); + expect(out).toEqual({}); + }); + + it('drops a caller-forged NaN — `Object.is`, not `===`', () => { + // `===` reports NaN !== NaN, which would read a forged NaN as "a hook + // rewrote this" and KEEP it. The one input where the loose operator + // inverts the verdict, so it is pinned rather than left to a reviewer. + const numeric = { fields: { score: { type: 'number', readonly: true } } }; + const supplied = { score: Number.NaN }; + const out = stripReadonlyFields(numeric, { score: Number.NaN }, supplied); + expect(out).toEqual({}); + }); + + it('does not read an inherited `Object.prototype` key as caller-supplied', () => { + // `constructor` matches the machine-name regex, so it is a legal field + // name; `name in supplied` would be TRUE for it on any plain object and + // would strip a hook stamp. Own-property check, pinned. + const oddly = { fields: { constructor: { type: 'text', readonly: true } } }; + const out = stripReadonlyFields(oddly, { constructor: 'hook-stamp' }, {}); + expect(out).toEqual({ constructor: 'hook-stamp' }); + }); + + it('KNOWN LIMIT: a hook that mutates a caller-supplied object IN PLACE is still stripped', () => { + // The snapshot is shallow, so an in-place mutation leaves identity + // unchanged and is invisible to any comparison short of a deep clone — + // which this path will not pay for on every write. Documented and pinned + // so the limit is a decision, not a surprise: a hook meaning to write a + // read-only column should ASSIGN to it. + const jsonish = { fields: { payload: { type: 'json', readonly: true } } }; + const shared: Record = { a: 1 }; + const supplied = { payload: shared }; + shared.a = 2; // the "hook" mutates in place — same reference + const out = stripReadonlyFields(jsonish, { payload: shared }, supplied); + expect(out).toEqual({}); + }); +}); + // #3493 — a "historical" import (preserveAudit) reinstates the original // timeline: the audit/timestamp family and author-declared business `readonly` // fields survive the strip, while platform-managed `system` columns outside the @@ -337,22 +395,21 @@ const historicalFields = { describe('stripReadonlyFields — preserveAudit whitelist (#3493)', () => { it('KEEPS the caller-supplied audit/timestamp family under preserveAudit', () => { - const supplied = new Set(['created_at', 'created_by', 'updated_at', 'updated_by']); const data = { created_at: '2020-01-01T00:00:00Z', created_by: 'u_creator', updated_at: '2021-03-01T00:00:00Z', updated_by: 'u_old', }; - const out = stripReadonlyFields(historicalFields, { ...data }, supplied, undefined, { preserveAudit: true }); + const out = stripReadonlyFields(historicalFields, { ...data }, data, undefined, { preserveAudit: true }); expect(out).toEqual(data); }); it('KEEPS an author-declared business readonly field (closed_at) under preserveAudit', () => { - const supplied = new Set(['closed_at']); + const supplied = { closed_at: '2021-03-01T00:00:00Z' }; const out = stripReadonlyFields( historicalFields, - { closed_at: '2021-03-01T00:00:00Z' }, + { ...supplied }, supplied, undefined, { preserveAudit: true }, @@ -361,10 +418,10 @@ describe('stripReadonlyFields — preserveAudit whitelist (#3493)', () => { }); it('STILL strips a non-audit system column (organization_id) under preserveAudit — no tenancy backdoor', () => { - const supplied = new Set(['organization_id', 'closed_at']); + const supplied = { organization_id: 'org_forged', closed_at: '2021-03-01T00:00:00Z' }; const out = stripReadonlyFields( historicalFields, - { organization_id: 'org_forged', closed_at: '2021-03-01T00:00:00Z' }, + { ...supplied }, supplied, undefined, { preserveAudit: true }, @@ -373,13 +430,12 @@ describe('stripReadonlyFields — preserveAudit whitelist (#3493)', () => { }); it('strips the whole family as before when preserveAudit is NOT set (regression)', () => { - const supplied = new Set(['updated_at', 'closed_at', 'organization_id']); - const out = stripReadonlyFields(historicalFields, { - title: 'x', + const supplied = { updated_at: '2021-03-01T00:00:00Z', closed_at: '2021-03-01T00:00:00Z', organization_id: 'o1', - }, supplied); + }; + const out = stripReadonlyFields(historicalFields, { title: 'x', ...supplied }, supplied); expect(out).toEqual({ title: 'x' }); }); }); @@ -414,31 +470,41 @@ describe('isRuntimeOwnedField (#5503)', () => { describe('stripReadonlyFields — implicit readonly on autonumber (#5503)', () => { it('drops a caller-supplied record number even with no `readonly: true` flag', () => { - const supplied = new Set(['title', 'account_number']); - const out = stripReadonlyFields(numberedFields, { title: 'x', account_number: 'ACC-888888' }, supplied); + const supplied = { title: 'x', account_number: 'ACC-888888' }; + const out = stripReadonlyFields(numberedFields, { ...supplied }, supplied); expect(out).toEqual({ title: 'x' }); }); it('KEEPS a hook-stamped record number the caller did not supply', () => { - const supplied = new Set(['title']); + const supplied = { title: 'x' }; + const out = stripReadonlyFields(numberedFields, { title: 'x', account_number: 'HOOK-1' }, supplied); + expect(out).toEqual({ title: 'x', account_number: 'HOOK-1' }); + }); + + it('KEEPS a hook-REWRITTEN record number the caller DID supply (#5591)', () => { + // The #5503 limb read through the same key-only guard #5591 replaced, so + // it inherited the same defect: a hook that re-issues the record number + // lost its value to a caller that had echoed the old one back. + const supplied = { title: 'x', account_number: 'ACC-888888' }; const out = stripReadonlyFields(numberedFields, { title: 'x', account_number: 'HOOK-1' }, supplied); expect(out).toEqual({ title: 'x', account_number: 'HOOK-1' }); }); it('KEEPS it under preserveAudit — a migration reinstates legacy record numbers', () => { - const supplied = new Set(['account_number']); + const supplied = { account_number: 'LEGACY-7' }; const out = stripReadonlyFields( - numberedFields, { account_number: 'LEGACY-7' }, supplied, undefined, { preserveAudit: true }, + numberedFields, { ...supplied }, supplied, undefined, { preserveAudit: true }, ); expect(out).toEqual({ account_number: 'LEGACY-7' }); }); it('logs the runtime-owned message, not the author-declared readonly one', () => { const warns: string[] = []; + const supplied = { account_number: 'ACC-888888', closed_at: '2021-01-01T00:00:00Z' }; stripReadonlyFields( numberedFields, - { account_number: 'ACC-888888', closed_at: '2021-01-01T00:00:00Z' }, - new Set(['account_number', 'closed_at']), + { ...supplied }, + supplied, { warn: (m: string) => warns.push(m) } as any, ); expect(warns).toHaveLength(2); diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 289d917df1..b5883035ea 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -633,17 +633,60 @@ export function isRuntimeOwnedField(def: { type?: string } | undefined | null): * number: the same defect as #4447 (`created_at` forgeable by a normal * PATCH), except the field carried no flag for this loop to notice. * - * Two guards keep every legitimate write intact: - * - `suppliedKeys` — only keys the CALLER sent are candidates. Server stamps - * applied by beforeUpdate hooks or write middleware (e.g. `updated_by` / - * `updated_at`, plugin.ts) land in `data` but are NOT in `suppliedKeys`, so - * they survive. A caller that *explicitly* forges e.g. `updated_by` simply + * Three guards keep every legitimate write intact: + * - `supplied` KEY presence — only keys the CALLER sent are candidates. Server + * stamps applied by beforeUpdate hooks or write middleware (e.g. `updated_by` + * / `updated_at`, plugin.ts) land in `data` but are absent from `supplied`, + * so they survive. A caller that *explicitly* forges e.g. `updated_by` simply * has it dropped for that request (the last-modified stamp is left unchanged * — safe). + * - `supplied` VALUE identity (#5591) — the key must still hold THE CALLER'S + * OWN VALUE. This is the half that used to be missing, and its absence is + * what made the guard above conditional on an accident. See below. * - system context — the caller passes this strip only for NON-system writes; * system-context writes (import, seed replay, approvals, lifecycle hooks — * all `isSystem: true`) legitimately set read-only columns and skip it. * + * ### Why `supplied` carries VALUES, not just keys (#5591) + * + * This strip runs AFTER `beforeUpdate`, so by the time it looks at a key, the + * value sitting there may no longer be the caller's — a hook may have written + * its own. The guard used to be a key SET, which cannot tell those apart, so + * `delete data[name]` deleted whatever was there: on a payload that merely + * ECHOED a read-only key back, the hook's write died with it. + * + * Measured downstream (objectstack#5591, from hotcrm#788): a REST caller reads a + * whole `crm_knowledge_article`, flips `status` to `published`, and PUTs the + * whole record back — `published_at: null` included, because that is what it + * read. The publish hook stamps `published_at` on the draft→published + * transition; the strip then deleted the hook's timestamp because the caller had + * echoed the key. The row committed as `status = "published"` with + * `published_at = null`, which every list and report ordering by `published_at` + * is undefined on. The same hook's `last_reviewed_at` — a read-only field the + * caller had NOT echoed — landed normally in the same write. Two hook-derived + * writes, one alive and one dead, decided by nothing but whether the caller's + * payload happened to carry a same-named key. + * + * So the rule is: **strip the value the CALLER SUBMITTED, never the value that + * happens to be there when the strip runs.** A key whose value a hook has + * replaced is a PLATFORM write, and platform writes to read-only columns are + * legitimate by construction — that is the same contract #4903 pins from the + * other side (a read-only key a hook ADDS lands, because it is not caller + * supplied). #5591 only makes the two agree. + * + * This does NOT relax #2948 / #3003 / #3015 in any direction: a caller-supplied + * read-only value that no hook touched is still dropped, byte for byte the same + * 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. + * + * 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 — + * identity is unchanged — and the field is still stripped. No comparison can see + * that without deep-cloning every write payload, which this path will not pay + * for. The fallback is the pre-#5591 behaviour (strip), i.e. fail-safe; a hook + * that means to write a read-only column should ASSIGN a value to it. + * * `options.preserveAudit` (#3493) relaxes the strip for an opt-in "historical" * import that reinstates the original timeline: a caller-supplied read-only * field is KEPT when {@link isPreservableUnderAudit} allows it — the @@ -682,7 +725,7 @@ export function isRuntimeOwnedField(def: { type?: string } | undefined | null): export function stripReadonlyFields( objectSchema: { name?: string; fields?: Record } | undefined | null, data: Record | undefined | null, - suppliedKeys: ReadonlySet, + supplied: Readonly>, logger?: EvaluateRulesOptions['logger'], options?: { preserveAudit?: boolean }, ): Record | undefined | null { @@ -697,7 +740,18 @@ export function stripReadonlyFields( const runtimeOwned = isRuntimeOwnedField(def); if (!def?.readonly && !runtimeOwned) continue; if (!(name in (result as Record))) continue; - if (!suppliedKeys.has(name)) continue; // server-stamped, not caller-supplied — keep + // Own-property, never `in`: a field name is `^[a-z_][a-z0-9_]*$`, which + // admits `constructor` / `valueOf` — inherited from `Object.prototype` on + // 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 + // [#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. + 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 }; delete (result as Record)[name]; diff --git a/packages/qa/dogfood/test/authz-conformance.matrix.ts b/packages/qa/dogfood/test/authz-conformance.matrix.ts index 82d34f142a..a543bc8daa 100644 --- a/packages/qa/dogfood/test/authz-conformance.matrix.ts +++ b/packages/qa/dogfood/test/authz-conformance.matrix.ts @@ -134,7 +134,7 @@ export const AUTHZ_CONFORMANCE: AuthzPrimitive[] = [ { id: 'default-profile', summary: 'app-declared default profile (isDefault)', state: 'enforced', enforcement: 'plugin-security/security-plugin.ts fallback resolution', proof: 'showcase-default-profile.dogfood.test.ts' }, { id: 'readonly-static-write', summary: 'static `readonly: true` stripped from non-system UPDATE (#2948 / #3003) AND INSERT (#3043) payloads — neither a direct PATCH nor a direct POST can forge approval/status/amount columns the UI never renders', state: 'enforced', - enforcement: 'UPDATE: objectql/engine.ts stripReadonlyFields on the single-id + multi-row paths (#2948, caller-supplied keys only so server stamps survive). INSERT: metadata-protocol/protocol.ts strips read-only keys at the DataProtocol create INGRESS (createData / createManyData / batchData / cloneData) — the single seam every external REST/GraphQL/MCP create funnels through, while trusted internal engine.insert writers (better-auth adapter, metadata repo, seed loader) bypass it; stripped before the engine so the field re-derives its defaultValue. isSystem exempt on both; symmetric with the readonlyWhen strip', + enforcement: 'UPDATE: objectql/engine.ts stripReadonlyFields on the single-id + multi-row paths (#2948, caller-supplied VALUES only — the entry snapshot carries the caller payload, so a server stamp survives whether the hook ADDED the key or OVERWROTE one the caller also sent, #5591). INSERT: metadata-protocol/protocol.ts strips read-only keys at the DataProtocol create INGRESS (createData / createManyData / batchData / cloneData) — the single seam every external REST/GraphQL/MCP create funnels through, while trusted internal engine.insert writers (better-auth adapter, metadata repo, seed loader) bypass it; stripped before the engine so the field re-derives its defaultValue. isSystem exempt on both; symmetric with the readonlyWhen strip', proof: 'showcase-static-readonly.dogfood.test.ts', note: 'The #3003 field report: `readonly: true` used to be UI-only, so a logged-in non-admin self-approved a 4-stage approval (approval_status/approval_stage/confirmed_total) with one same-session REST PATCH on a draft record — RECORD_LOCKED only guards pending flows, and the draft never entered one. #3043 is the INSERT face: the same non-admin could skip the draft entirely and POST a record already `approval_status:"approved"` — a step SHORTER than #3003, and one the UPDATE strip never reached. Enforced at the DATA-WRITE INGRESS (not the engine) so it covers every external caller — REST, the GraphQL/MCP dispatcher, bulk import — without stripping the internal writers that legitimately seed readonly columns on create (identity provisioning, provenance, event-log cursors). The strip is SILENT on both paths (HTTP 2xx, forged value dropped; a stripped INSERT field falls back to its defaultValue). `readonlyWhen` stays INSERT-exempt (a conditional lock needs a prior record). System-context writes (import, seed replay, migration) still seed readonly columns. Ingress unit proof in metadata-protocol protocol.readonly-insert.test.ts (forge stripped, default re-seeded, system context allowed, batch rows covered, internal engine.insert unaffected).' }, From 54b09ed1265d629f6d627ecc10b77a5121f3e662 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 14:39:00 +0000 Subject: [PATCH 2/2] test(objectql): type the prior-record query instead of erasing it with `as any` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #4918 query-options-erasure ratchet counts an erased engine query bag in test code too (test surface is outside the BLOCKING rule, not outside the count), and the new suite's `sys_fetch_previous_update` replica pushed it 267 -> 268. The bag is not deliberately off-contract here — it is an ordinary by-id lookup — so the remedy is the typed one, not `as unknown as`. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .../src/engine-readonly-strip-caller-values.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 66ce5a4785..08688f8350 100644 --- a/packages/objectql/src/engine-readonly-strip-caller-values.test.ts +++ b/packages/objectql/src/engine-readonly-strip-caller-values.test.ts @@ -31,6 +31,7 @@ // the case is pinned here next to the fix so the two verdicts are read together. import { describe, it, expect, beforeEach } from 'vitest'; +import type { EngineQueryOptions } from '@objectstack/spec/data'; import { ObjectQL } from './engine.js'; function makeDriver() { @@ -129,7 +130,10 @@ describe('update strip acts on CALLER-submitted values (#5591)', () => { // before-hook runs, which is how a transition is expressed in one. engine.registerHook('beforeUpdate', async (ctx: any) => { if (!ctx.previous && ctx.input?.id) { - ctx.previous = await engine.findOne(ctx.object, { where: { id: ctx.input.id } } as any); + // Typed, not `as any`: the #4918 ratchet counts an erased engine + // query-options bag even in test code. + const priorQuery: EngineQueryOptions = { where: { id: ctx.input.id }, limit: 1 }; + ctx.previous = await engine.findOne(ctx.object, priorQuery); } }, { priority: 5 });