From 3390cd1a949019b7859ff3e98c6972888dc9f359 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 20:30:27 +0000 Subject: [PATCH 1/2] fix(objectql): the read-only strip stops calling the addressed row's own id a forged caller write (#8141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WIP scaffold — implementation in, tests and changeset to follow. `stripReadonlyFields` gains an `addressKey` option: the named key is still stripped, it just no longer logs. The by-id update branch passes the SAME `idAddressesThisRow` predicate #8093 wired to the report channel, so log and report cannot disagree about what an address is. Every other call site passes nothing and is unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014C8pAprWdmtecFsEprZax4 --- packages/objectql/src/engine.ts | 19 ++++++- .../objectql/src/validation/rule-validator.ts | 54 ++++++++++++++++++- 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 7979c9b413..58b1f34f5d 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -8198,6 +8198,14 @@ export class ObjectQL implements IObjectQLEngine { // about what the CALLER submitted, and the answer must survive a hook // rewriting the key mid-write — the same reason that snapshot carries // values at all (#5591). + // + // [#8141] #8093 wired this to the REPORT channel only, so the strip's own + // WARN went on calling the address a forged caller write on every + // single-record PATCH of every platform object. It now feeds BOTH channels + // — `reportDroppedFields` below and `stripReadonlyFields`' `addressKey` + // argument — from this ONE predicate. Deliberately not a second derivation: + // two notions of "this id is the address" that disagree in one edge case + // would be a worse defect than the log line either of them silences. const onFieldsDropped = options?.onFieldsDropped; const strictReadonlyWrites = options?.strictReadonlyWrites === true; const strictDrops: DroppedFieldsEvent[] = []; @@ -8690,9 +8698,18 @@ export class ObjectQL implements IObjectQLEngine { // 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. + // + // [#8141] `addressKey` carries the SAME fact `reportDroppedFields` + // is already keyed on — `idAddressesThisRow`, one predicate, both + // channels — so the log and the report can never disagree about + // what is an address. The strip is unchanged: `id` still leaves + // the SET clause, and the driver receives the identical payload; + // only the WARN that called the address a caller forgery is gone. + // Undefined on every other path (the multi branch below, and the + // insert-side sibling), which is what keeps those byte-identical. if (!opCtx.context?.isSystem) { const preRo = hookContext.input.data as Record; - hookContext.input.data = stripReadonlyFields(updateSchema as any, preRo, suppliedValues, 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, addressKey: idAddressesThisRow ? 'id' : undefined }) as any; reportDroppedFields(preRo, hookContext.input.data as Record, 'readonly'); } // [#5126] Both strip passes are done; refuse now if the caller diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index d7fd06c706..c2d3d49b8a 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -957,17 +957,61 @@ export function isRuntimeOwnedField(def: { type?: string } | undefined | null): * silent-drop. `warn` + a message that names both remedies is the honest single * level. Changing this means giving `ExecutionContext` a real origin marker * first — not guessing from the absence of a principal. + * + * ### ...and the one key it does NOT log: the write's ADDRESS (#8141) + * + * `options.addressKey` names the key that carries the ADDRESS of the row this + * call is already writing — the caller passes it only when it has established + * that fact, and only the by-id update branch can (`idAddressesThisRow` in + * `engine.ts`, #8093: the payload `id` equals the bound row's key). That key is + * still STRIPPED, byte for byte as before; what it no longer does is log. + * + * The three claims the message makes are all false for that key, and #8093 + * measured them: on `PATCH /data/sys_user_preference/rec_1` with the body + * `{"value":[…]}` — no `id` key in it at all — the REST ingress folds the path + * id into the payload (`metadata-protocol`'s `updateData`, #6479, so a body + * `id` cannot bind a row other than the one the URL, the OCC check and the + * receipt all name), the fold lands before the `suppliedValues` snapshot, and + * this strip then logged that a caller-supplied `id` was DROPPED and the update + * COMMITTED WITHOUT IT. The caller supplied nothing, lost nothing, and the + * update carried out everything it asked for. Worse, the message's remedy told + * that caller to pass `{ context: { isSystem: true } }` — which would exempt it + * from this strip ENTIRELY, a strictly worse posture adopted to silence a line + * that should never have printed. + * + * Why this is a fix and not a mute: it fires on every single-record `PATCH` of + * every object declaring `id` as `readonly: true`, i.e. every platform object + * (the console's recents trace alone emits one per org switch), and the level + * argument directly above is that `warn` is worth keeping so REAL forgery + * attempts stay visible. A per-call flood of a line that cannot be acted on is + * what trains a reader to skip the channel — the log-side twin of the amber + * toast #8093 stopped, and of #3431 / #3794 one field over. + * + * Deliberately scoped to the LOG and to ONE key: + * - any other read-only key in the same payload still logs, unchanged in + * wording and level — a caller that forged `created_by` alongside the + * address is still named; + * - an `id` that does NOT equal the bound row is not an address, so the caller + * never passes it here and it still logs; + * - the multi branch and the insert-side sibling pass no `addressKey` at all + * (nothing addresses a row by key there), so their behaviour is identical to + * before this option existed; + * - and the payload handed to the driver is unchanged everywhere. Removing the + * address BEFORE the read-only pass would change what drivers receive for + * objects whose `id` is not readonly, which #6435 ruled an explicitly + * separate decision. */ export function stripReadonlyFields( objectSchema: { name?: string; fields?: Record } | undefined | null, data: Record | undefined | null, supplied: Readonly>, logger?: EvaluateRulesOptions['logger'], - options?: { preserveAudit?: boolean }, + options?: { preserveAudit?: boolean; addressKey?: string }, ): Record | undefined | null { const fields = objectSchema?.fields; if (!fields || !data) return data; const preserveAudit = options?.preserveAudit === true; + const addressKey = options?.addressKey; let result = data; for (const [name, def] of Object.entries(fields)) { // [#5503] `readonly: true` is the AUTHOR-declared lock; a runtime-owned @@ -991,6 +1035,14 @@ export function stripReadonlyFields( if (preserveAudit && isPreservableUnderAudit(name, def)) continue; // historical import reinstates it if (result === data) result = { ...data }; delete (result as Record)[name]; + // [#8141] STRIPPED, then not logged: this key is the write's address, and + // every claim the message makes about it is false — the caller did not + // supply it, lost nothing, and the update committed everything it asked + // for. Placed AFTER the delete, never as an early `continue` above it: the + // strip must stay (a same-value primary-key write is a no-op on SQL but a + // rejection on stores with immutable primary keys, #6435), and only the + // caller can know a key is an address — this helper never guesses one. + if (addressKey !== undefined && name === addressKey) continue; logger?.warn?.( def?.readonly ? readonlyStripWarning(name, objectSchema?.name) From fe4ffe7552c1652bfeb9ee4a9591112ce00a29d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 20:50:10 +0000 Subject: [PATCH 2/2] test(objectql): pin the address-key log exemption and the tripwire it must not silence (#8141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Engine-level acceptance pair on a real ObjectQL + recording driver/logger: the card's measured repro (by-id PATCH, body carrying no `id`) emits no WARN and the driver's SET clause is unchanged; a really-forged read-only field in the same write still WARNs byte-identically against the exported message, at `warn`. Plus the multi branch, the ruled-non-id `primary_key` diagnostic, a non-readonly `id`, and the `isSystem` exemption, all unmoved. Helper-level cases cover `addressKey` opt-in-ness (no option ⇒ the line still prints, which is what keeps the other two call sites unchanged), composition with `preserveAudit`, and the runtime-owned message. Changeset: patch, this changes observable log behaviour. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014C8pAprWdmtecFsEprZax4 --- .changeset/readonly-strip-address-warn.md | 46 +++ ...ngine-update-addressing-id-no-warn.test.ts | 281 ++++++++++++++++++ .../src/validation/rule-validator.test.ts | 137 +++++++++ 3 files changed, 464 insertions(+) create mode 100644 .changeset/readonly-strip-address-warn.md create mode 100644 packages/objectql/src/engine-update-addressing-id-no-warn.test.ts diff --git a/.changeset/readonly-strip-address-warn.md b/.changeset/readonly-strip-address-warn.md new file mode 100644 index 0000000000..808d68270a --- /dev/null +++ b/.changeset/readonly-strip-address-warn.md @@ -0,0 +1,46 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): the read-only strip stops warning that the addressed row's own `id` was a forged caller write (#8141) + +Every single-record update of every platform object emitted a server WARN whose +three claims were all false for that write: + +``` +Field 'id' on 'sys_user_preference' is read-only: the caller-supplied value was +DROPPED and the update is being COMMITTED WITHOUT IT — … +``` + +Measured with `updateData({ object: 'sys_user_preference', id: 'rec_1', data: { value: ['x'] } })` +— a body carrying no `id` key at all. The value was not caller-supplied: the +REST ingress folds the path id into the payload so a body `id` cannot bind a row +other than the one the URL, the OCC check and the receipt all name (#6479), and +that fold lands before the engine's caller snapshot. Nothing the caller wanted +was dropped, and nothing it asked for was left out of the commit. Worse, the +message's remedy told that caller to pass `{ context: { isSystem: true } }` — +which would exempt it from the read-only strip **entirely**, a strictly worse +posture bought to silence a line that should never have printed. + +Volume is the damage: it fired on every single-record `PATCH` of every object +declaring `id` as `readonly: true`, which is every platform object (the console's +recents trace alone emits one per org switch). The line is deliberately kept at +`warn` so **real** forgery attempts stay visible, and a warning that fires on +every ordinary write trains its reader to skip the channel. This is the log-side +half of the amber toast #8093 removed. + +`stripReadonlyFields` now takes an `addressKey` option naming the key that +carries the write's address; that key is still **stripped**, it just no longer +logs. The by-id update branch supplies it from the same `idAddressesThisRow` +predicate #8093 wired to `droppedFields` / `onFieldsDropped`, so the log and the +report channel cannot disagree about what an address is. + +**Unchanged, deliberately:** the strip itself — `id` still never reaches the +driver's SET clause, and the payload handed to every driver is byte-identical +(a same-value primary-key write is a no-op on SQL but an outright rejection on +stores with immutable primary keys). Any other read-only field a caller really +did forge still warns, byte-identical in wording and still at `warn`. A payload +`id` the engine has ruled is not a primary key still gets its own `primary_key` +diagnostic. Predicate/multi updates and the insert-side strip pass no address at +all and behave exactly as before. `strictReadonlyWrites` refusals are untouched: +they are derived from the report channel, which already excluded the address. diff --git a/packages/objectql/src/engine-update-addressing-id-no-warn.test.ts b/packages/objectql/src/engine-update-addressing-id-no-warn.test.ts new file mode 100644 index 0000000000..7e72ee7972 --- /dev/null +++ b/packages/objectql/src/engine-update-addressing-id-no-warn.test.ts @@ -0,0 +1,281 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// objectstack#8141 — the read-only strip's WARN must stop calling the addressed +// row's OWN primary key a forged caller write. +// +// ## What was wrong +// +// #8093 fixed the REPORT channel (`droppedFields` / `onFieldsDropped`): on a +// by-id update the payload `id` that equals the bound row is the write's +// ADDRESS, not payload, so it is not a drop. It deliberately left the strip — +// and the strip's own log — alone, so `stripReadonlyFields` went on emitting +// `readonlyStripWarning('id', …)` on every single-record update of every object +// declaring `id` as `readonly: true`, i.e. every platform object. +// +// Measured by the card on a real ObjectQL + a real protocol, with +// `updateData({ object: 'sys_user_preference', id: 'rec_1', data: { value: ['x'] } })` +// — a body carrying no `id` key at all: +// +// droppedFields : null ← #8093's fix, correct +// warn count : 1 +// WARN: Field 'id' on 'sys_user_preference' is read-only: the caller-supplied +// value was DROPPED and the update is being COMMITTED WITHOUT IT — … +// +// All three claims are false for that write. The value was not caller-supplied +// (`metadata-protocol`'s `updateData` folds the path id into the body, #6479); +// nothing the caller wanted was dropped; nothing it asked for was left out of +// the commit. And the message's remedy tells that caller to pass +// `{ context: { isSystem: true } }` — which would exempt it from the static +// read-only strip ENTIRELY: a strictly worse posture bought to silence a line +// that should never have printed. +// +// The console's recents trace alone emits one per org switch. The cost is the +// same one #3431 / #3794 and #8093 were about, one channel over: a warning that +// fires on every ordinary write trains its reader to skip the channel, and the +// forgery this line exists to surface is skipped with it. +// +// ## What the fix is, and what it is NOT +// +// `stripReadonlyFields` gains `options.addressKey`: the named key is still +// STRIPPED, it just no longer LOGS. The by-id branch passes the key from the +// SAME `idAddressesThisRow` predicate #8093 wired to the report channel — one +// notion of "this id is the address", feeding both channels, so they cannot +// disagree. NOT "stop stripping `id`" (a same-value primary-key write is a +// no-op on SQL but a rejection on stores with immutable primary keys), and NOT +// "remove the address before the read-only pass" (that changes what the driver +// receives for objects whose `id` is not readonly — #6435's explicitly separate +// decision). The driver's SET clause is byte-identical before and after, which +// is why half the cases below assert it. +// +// ## Predicted table, written BEFORE the first run +// +// | case | warn lines | driver SET | +// |---------------------------------------------------------------|-------------------------------------|------------------| +// | REST ingress shape: `{value,id}` + `where.id`, readonly `id` | NONE | no `id` | +// | canonical `update(obj,{id,…})`, readonly `id` | NONE | no `id` | +// | address + a really-forged `locked_note` | ONE — `locked_note`, byte-identical | no `id`, no note | +// | ruled-non-id `data.id` + forged `locked_note` (by-id branch) | TWO — `primary_key` + `locked_note` | neither | +// | MULTI branch, forged `locked_note` | ONE — `locked_note`, byte-identical | no note | +// | object whose `id` is NOT readonly | NONE (as before) | `id` PRESENT | +// | `isSystem` caller writing `locked_note` | NONE (as before) | note PRESENT | +// +// Rows 3–5 are the ones that make this file able to fail: the fix NARROWS what +// is logged, so without a case that still DEMANDS the line — asserted `toBe` +// against the exported message, so a reworded or downgraded one is red — +// "stop calling the address a forgery" and "delete the tripwire" look alike. +// +// ## Reverse verification — prediction, then what actually happened +// +// PREDICTED, before the run: drop the `addressKey` argument at the `engine.ts` +// by-id call site (leave everything else, including #8093's report exclusion, +// in place) and rows 1, 2 and 3 go RED — 1 and 2 gain the `id` line, 3 sees two +// warns instead of one — while rows 4, 5, 6 and 7 stay GREEN, because none of +// them has an address to exclude. +// +// MEASURED: exactly that — 3 failed / 6 passed, and the three failures are rows +// 1, 2 and 3, each reporting the extra `readonlyStripWarning('id', 'pref')` +// line. Recorded verbatim in the PR body. The direction was NOT the "more +// diagnostics, not fewer" inversion #8093 hit on its strict rows: this option +// feeds no counter and no derived refusal — `strictReadonlyWrites` is keyed on +// `reportDroppedFields`, which #8093 already excluded the address from, so the +// loud half cannot move when only the log line does. Rows 4–7 confirmed it did +// not. + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { readonlyStripWarning } from './validation/rule-validator.js'; +import type { DroppedFieldsEvent } from '@objectstack/spec/data'; + +/** A logger that records every line, with its level. */ +function makeCapturingLogger() { + const lines: Array<{ level: string; msg: string }> = []; + const logger: any = { + lines, + trace() {}, fatal() {}, + debug(msg: string) { lines.push({ level: 'debug', msg: String(msg) }); }, + info(msg: string) { lines.push({ level: 'info', msg: String(msg) }); }, + warn(msg: string) { lines.push({ level: 'warn', msg: String(msg) }); }, + error(msg: string) { lines.push({ level: 'error', msg: String(msg) }); }, + child() { return logger; }, + }; + return logger; +} + +interface DriverWrite { + readonly fn: 'update' | 'updateMany'; + readonly id?: unknown; + /** A COPY — the engine keeps mutating its own payload after the call. */ + readonly data: Record; +} + +function makeRecordingDriver() { + const writes: DriverWrite[] = []; + const row = { id: 'rec_1', value: 'v0', locked_note: 'n0', title: 't0' }; + const driver: any = { + name: 'recording', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find() { return [{ ...row }]; }, + async findOne() { return { ...row }; }, + async create(_o: string, data: Record) { return { id: 'rec_1', ...data }; }, + async update(_o: string, id: string, data: Record) { + writes.push({ fn: 'update', id, data: { ...data } }); + return { ...row, ...data, id }; + }, + async updateMany(_o: string, _ast: unknown, data: Record) { + writes.push({ fn: 'updateMany', data: { ...data } }); + return 2; + }, + async delete() { return true; }, + async deleteMany() { return 0; }, + async count() { return 1; }, + async bulkCreate() { return []; }, async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, writes }; +} + +/** + * `readonlyId: true` mirrors every platform object — `sys_user_preference`'s + * `id` is `Field.text({ label: 'Preference ID', required: true, readonly: true })`. + */ +async function makeEngine(readonlyId: boolean) { + const logger = makeCapturingLogger(); + const engine = new ObjectQL({ logger }); + const { driver, writes } = makeRecordingDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject({ + name: 'pref', + fields: { + id: { name: 'id', label: 'Preference ID', type: 'text', primaryKey: true, ...(readonlyId ? { readonly: true } : {}) }, + value: { name: 'value', type: 'text' }, + locked_note: { name: 'locked_note', type: 'text', readonly: true }, + title: { name: 'title', type: 'text' }, + }, + } as any, 'test'); + return { engine, writes, logger }; +} + +/** Run one write; return the WARN lines it emitted plus the driver's writes. */ +async function observe( + data: unknown, + options: Record = {}, + opts: { readonlyId?: boolean } = {}, +) { + const { engine, writes, logger } = await makeEngine(opts.readonlyId !== false); + const events: DroppedFieldsEvent[] = []; + await engine.update('pref', data as any, { + ...options, + onFieldsDropped: (e: DroppedFieldsEvent) => events.push(e), + } as any); + const warns = logger.lines.filter((l: any) => l.level === 'warn').map((l: any) => l.msg); + return { warns, writes, events, lines: logger.lines }; +} + +describe('#8141 — the addressed row\'s primary key is not logged as a forged caller write', () => { + it('THE REPRO: the REST ingress shape logs NOTHING, and the strip is unchanged', async () => { + // Byte-for-byte what `metadata-protocol`'s `updateData` builds for + // `PATCH /data/pref/rec_1` with the body `{ value: 'v1' }` — no `id` key: + // `{ ...request.data, id: request.id }` plus `where: { id: request.id }`. + const { warns, writes, events } = await observe( + { value: 'v1', id: 'rec_1' }, + { where: { id: 'rec_1' } }, + ); + expect(warns).toEqual([]); + // …and #8093's channel still agrees — one predicate, two channels. + expect(events).toEqual([]); + // The strip itself did NOT move: `id` still never reaches the SET clause. + expect(writes.map((w) => w.fn)).toEqual(['update']); + expect(writes[0].data).toEqual({ value: 'v1' }); + expect(writes[0].id).toBe('rec_1'); + }); + + it('the canonical ObjectQL by-id spelling `update(obj, { id, ...fields })` logs nothing either', async () => { + const { warns, writes } = await observe({ id: 'rec_1', value: 'v1' }); + expect(warns).toEqual([]); + expect(writes[0].data).toEqual({ value: 'v1' }); + }); + + it('THE TRIPWIRE: a read-only field the caller REALLY forged still WARNs, byte-identical', async () => { + // Without this the fix is indistinguishable from "stop warning about + // dropped fields". `toBe` against the exported message pins the wording, + // and the level assertion pins that it is still `warn` — the level its own + // docblock defends precisely so real forgery attempts stay visible. + const { warns, writes, events, lines } = await observe( + { value: 'v1', locked_note: 'forged', id: 'rec_1' }, + { where: { id: 'rec_1' } }, + ); + expect(warns).toEqual([readonlyStripWarning('locked_note', 'pref')]); + expect(lines.filter((l: any) => l.level !== 'debug' && l.level !== 'info').map((l: any) => l.level)) + .toEqual(['warn']); + // The line still carries the consequence and both remedies (#4903), and + // the remedy is TRUE here: this caller really did write a read-only column. + expect(warns[0]).toContain('COMMITTED WITHOUT IT'); + expect(warns[0]).toContain('{ context: { isSystem: true } }'); + expect(warns[0]).toContain('onFieldsDropped'); + // …and it names the forged field alone, never the address. + expect(warns[0]).not.toContain("Field 'id'"); + expect(events).toEqual([{ object: 'pref', fields: ['locked_note'], reason: 'readonly' }]); + expect(writes[0].data).toEqual({ value: 'v1' }); + }); + + it('a caller-supplied `id` that does NOT address the bound row is still called out', async () => { + // The engine rules a non-scalar `data.id` "not a primary key" and strips it + // on the by-id branch as `primary_key` (#6262 / #6435 / #6437) BEFORE the + // read-only pass — so this is the shape a non-addressing `id` actually + // takes on this branch, and its own WARN must survive untouched. The + // exclusion cannot reach it: it is keyed on equality with the bound key, + // which a ruled-non-id value never has. + const { warns, writes, events } = await observe( + { id: { $in: ['a', 'b'] }, value: 'v1', locked_note: 'forged' }, + { where: { id: 'rec_1' } }, + ); + expect(warns).toHaveLength(2); + expect(warns.some((w: string) => w.includes("dropped 'id' from the write payload"))).toBe(true); + expect(warns).toContain(readonlyStripWarning('locked_note', 'pref')); + expect(events).toEqual([ + { object: 'pref', fields: ['id'], reason: 'primary_key' }, + { object: 'pref', fields: ['locked_note'], reason: 'readonly' }, + ]); + expect(writes[0].data).toEqual({ value: 'v1' }); + }); + + it('the MULTI branch is untouched — it passes no address at all', async () => { + // Nothing addresses a row by key on a predicate write, so that call site + // hands `stripReadonlyFields` no `addressKey` and its behaviour is + // identical to before the option existed. + const { warns, writes } = await observe( + { locked_note: 'forged', value: 'v1' }, + { multi: true, where: { title: 't0' } }, + ); + expect(warns).toEqual([readonlyStripWarning('locked_note', 'pref')]); + expect(writes.map((w) => w.fn)).toEqual(['updateMany']); + expect(writes[0].data).toEqual({ value: 'v1' }); + }); + + it('an object whose `id` is NOT readonly is unaffected — no strip existed to silence', async () => { + const { warns, writes } = await observe( + { value: 'v1', id: 'rec_1' }, + { where: { id: 'rec_1' } }, + { readonlyId: false }, + ); + expect(warns).toEqual([]); + // Unchanged: the `id` still rides into the SET clause. Removing it here is + // #6435's explicitly separate decision, not this card's. + expect(writes[0].data).toEqual({ value: 'v1', id: 'rec_1' }); + }); + + it('an `isSystem` caller still skips the whole strip, silently, as before', async () => { + const { engine, writes, logger } = await makeEngine(true); + await engine.update( + 'pref', + { id: 'rec_1', value: 'v1', locked_note: 'system-write' } as any, + { where: { id: 'rec_1' }, context: { isSystem: true } } as any, + ); + expect(logger.lines.filter((l: any) => l.level === 'warn')).toEqual([]); + // The exemption is the whole pass — including the address, which is why + // this case cannot be confused with the one above it. + expect(writes[0].data).toEqual({ id: 'rec_1', value: 'v1', locked_note: 'system-write' }); + }); +}); diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index a608a60f67..5bef6e471e 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -14,6 +14,7 @@ import { stripRuntimeOwnedFields, isRuntimeOwnedField, runtimeOwnedStripWarning, + readonlyStripWarning, } from './rule-validator.js'; import { ValidationError } from './record-validator.js'; @@ -900,6 +901,142 @@ describe('stripReadonlyFields — implicit readonly on autonumber (#5503)', () = }); }); +// #8141 — `options.addressKey`: the key that carries the write's ADDRESS is +// still stripped, it just stops LOGGING. Every claim `readonlyStripWarning` +// makes is false for that key (the caller did not supply it — the REST ingress +// folded the path id in, #6479; nothing it wanted was dropped; nothing it asked +// for was left out of the commit), and its remedy prose sends that caller to +// `{ context: { isSystem: true } }`, which would exempt it from this strip +// entirely — a strictly worse posture bought to silence a line that should +// never have printed. +// +// The trap this block exists to catch is silencing too much: the line's own +// docblock keeps it at `warn` so REAL forgery attempts stay visible, so every +// case below has a counter-case where the WARN must survive BYTE-IDENTICAL. +const addressedFields = { + name: 'pref', + fields: { + id: { type: 'text', primaryKey: true, readonly: true }, + value: { type: 'text' }, + locked_note: { type: 'text', readonly: true }, + }, +}; + +/** Collect the warn lines a single strip call emits. */ +function stripWithWarns( + schema: unknown, + data: Record, + supplied: Record, + options?: { preserveAudit?: boolean; addressKey?: string }, +) { + const warns: string[] = []; + const levels: string[] = []; + const logger: any = { + warn: (m: string) => { warns.push(m); levels.push('warn'); }, + error: (m: string) => { warns.push(m); levels.push('error'); }, + info: (m: string) => { warns.push(m); levels.push('info'); }, + debug: (m: string) => { warns.push(m); levels.push('debug'); }, + }; + const out = stripReadonlyFields(schema as any, data, supplied, logger, options); + return { out, warns, levels }; +} + +describe('stripReadonlyFields — addressKey silences the LOG, never the strip (#8141)', () => { + it('STILL STRIPS the address key — the payload handed on is byte-identical', () => { + // The half that must not move. A same-value primary-key write is a no-op + // on SQL but an outright rejection on stores with immutable primary keys, + // and widening/narrowing the strip is #6435's explicitly separate + // decision. This fix is about the log line, and only the log line. + const supplied = { id: 'rec_1', value: 'v1' }; + const { out, warns } = stripWithWarns(addressedFields, { ...supplied }, supplied, { addressKey: 'id' }); + expect(out).toEqual({ value: 'v1' }); + expect(warns).toEqual([]); + }); + + it('with NO addressKey the very same call still WARNs — the option is opt-in', () => { + // The regression pin for the two call sites this card does not touch (the + // multi branch and the insert-side sibling pass no `addressKey` at all). + const supplied = { id: 'rec_1', value: 'v1' }; + const { out, warns, levels } = stripWithWarns(addressedFields, { ...supplied }, supplied); + expect(out).toEqual({ value: 'v1' }); + expect(warns).toEqual([readonlyStripWarning('id', 'pref')]); + expect(levels).toEqual(['warn']); + }); + + it('a DIFFERENT read-only field in the same payload still WARNs, unchanged in wording and level', () => { + // The tripwire. Silencing the address must not silence the forgery riding + // along with it — asserted with `toBe` against the exported message so a + // reworded or downgraded line fails here. + const supplied = { id: 'rec_1', value: 'v1', locked_note: 'forged' }; + const { out, warns, levels } = stripWithWarns( + addressedFields, { ...supplied }, supplied, { addressKey: 'id' }, + ); + expect(out).toEqual({ value: 'v1' }); + expect(warns).toEqual([readonlyStripWarning('locked_note', 'pref')]); + expect(levels).toEqual(['warn']); + expect(warns[0]).toContain('COMMITTED WITHOUT IT'); + expect(warns[0]).toContain('{ context: { isSystem: true } }'); + expect(warns[0]).toContain('onFieldsDropped'); + }); + + it('an addressKey naming a key the caller did NOT supply changes nothing', () => { + // A server stamp on the named key is kept, exactly as before — the option + // is consulted only where a strip would otherwise have logged. + const d = { id: 'rec_1', value: 'v1' }; + const { out, warns } = stripWithWarns(addressedFields, d, { value: 'v1' }, { addressKey: 'id' }); + expect(out).toBe(d); // nothing stripped ⇒ same reference + expect(warns).toEqual([]); + }); + + it('an addressKey naming a field that is not read-only changes nothing', () => { + const supplied = { value: 'v1', locked_note: 'forged' }; + const { out, warns } = stripWithWarns( + addressedFields, { ...supplied }, supplied, { addressKey: 'value' }, + ); + expect(out).toEqual({ value: 'v1' }); + expect(warns).toEqual([readonlyStripWarning('locked_note', 'pref')]); + }); + + it('composes with preserveAudit rather than overriding it', () => { + const supplied = { id: 'rec_1', created_at: '2020-01-01T00:00:00Z', organization_id: 'org_forged' }; + const { out, warns } = stripWithWarns( + { name: 'pref', fields: { ...addressedFields.fields, ...historicalFields.fields } }, + { ...supplied }, + supplied, + { preserveAudit: true, addressKey: 'id' }, + ); + // Measured, and NOT what the first draft of this case predicted: under a + // historical import the address never reaches the address rule at all. A + // `readonly` field that is not `system` is an author-declared business + // field to {@link isPreservableUnderAudit}, so `preserveAudit` KEEPS `id` + // one line earlier — which is pre-#8141 behaviour, unchanged here, and + // #6435's separate decision if anyone wants it different. `created_at` is + // reinstated by the same whitelist; `organization_id` is a non-audit system + // column, so it is still stripped and still loud. + expect(out).toEqual({ id: 'rec_1', created_at: '2020-01-01T00:00:00Z' }); + expect(warns).toEqual([readonlyStripWarning('organization_id', 'pref')]); + }); + + it('silences the RUNTIME-OWNED message for the same key, on the same ground', () => { + // Why the exemption keys on the key's ROLE and not on which lock caught it: + // an `autonumber` primary key addressed by id is the same write, and the + // reason the line is untrue there is identical. The record number of a + // DIFFERENT field still logs. + const numberedId = { + name: 'pref', + fields: { + id: { type: 'autonumber', primaryKey: true }, + account_number: { type: 'autonumber' }, + value: { type: 'text' }, + }, + }; + const supplied = { id: 'ACC-1', account_number: 'ACC-888888', value: 'v1' }; + const { out, warns } = stripWithWarns(numberedId, { ...supplied }, supplied, { addressKey: 'id' }); + expect(out).toEqual({ value: 'v1' }); + expect(warns).toEqual([runtimeOwnedStripWarning('account_number', 'autonumber', 'pref')]); + }); +}); + describe('stripRuntimeOwnedFields — the INSERT-side strip (#5503)', () => { it('drops a caller-supplied record number', () => { const supplied = { title: 'x', account_number: 'ACC-777777' };