diff --git a/.changeset/autonumber-runtime-owned-write-path.md b/.changeset/autonumber-runtime-owned-write-path.md new file mode 100644 index 0000000000..db8e7a1567 --- /dev/null +++ b/.changeset/autonumber-runtime-owned-write-path.md @@ -0,0 +1,83 @@ +--- +"@objectstack/objectql": patch +"@objectstack/metadata-protocol": patch +--- + +fix(objectql): `autonumber` 是运行时拥有的字段,写路径不再接受调用者提交的单号 (#5503) + +`autonumber` 的值一直被文档声明为运行时所有 —— `applyAutonumbers` 的注释写着 +"the runtime owns the value, not the client",两个记录校验器也正是因此在 insert +与 update 上都豁免了 `required` 检查。缺的是另一半:**没有任何一层写路径阻止客户端 +自己填这个值**。于是一个普通的 REST 调用者可以: + +- `POST /data/:object` 携带显式单号 → 原样落库,序列被绕过; +- `PATCH /data/:object/:id` 携带该字段 → 200 且改写落库,业务单号被篡改。 + +这与已修复的 #4447(`created_at` 可被普通 PATCH 伪造)是同一缺陷族。区别在于: +声明了 `readonly: true` 的字段早已被 #2948 / #3043 的剥离机制保护,而 `autonumber` +字段身上根本没有这个标记,剥离循环从它旁边直接走过去了。 + +**修法:在引擎/校验层把 `type: 'autonumber'` 视为隐含 readonly,insert 与 update +同权。** 非 system 上下文提交的单号,在派发给任何驱动之前就被剥离: + +- **UPDATE** —— `stripReadonlyFields`(`packages/objectql`)的判定从"作者声明的 + `readonly: true`"扩展为"作者声明的 **或** 运行时拥有的字段类型" + (`isRuntimeOwnedField`,当前恰好只有 `autonumber`)。单行更新与 `multi` 批量更新 + 共用这一个剥离点,因此两条路径同时被覆盖。 +- **INSERT** —— 引擎新增一个更窄的 `stripRuntimeOwnedFields`,只剥离运行时拥有的 + 字段。它**不**接管作者声明的 `readonly` 在 insert 上的语义:那条防线按 #3413 的 + 设计留在 DataProtocol 入口(#3043),因为 create 确实可能合法地写入只读列,而直接 + 调用 `engine.insert` 的可信内部写入者(身份预置、元数据仓库、事件游标)必须不受影响。 + 单号没有这种两可性 —— 谁都不该在 create 时自带单号。 + +剥离发生在引擎里、派发之前,这正是修复**与驱动无关**的原因:声明 +`supports.autonumber === true` 的 SQL 驱动(持久序列)拿到的行里根本没有这个键, +所以它的序列必然胜出 —— 没有任何驱动需要改动一行代码。测试直接断言递交给 +`driver.create` 的负载,而不是打补丁到驱动上。 + +**豁免语义保持不变**,与 update 侧原有的白名单完全一致: + +- `isSystem` 写入(seed 回放、迁移、内部预置)整体跳过剥离; +- `preserveAudit`(#3493)的"历史数据导入"仍可写入原始单号 —— 把遗留系统的历史 + 单号迁移进来正是这个白名单存在的业务场景,而 `autonumber` 属于作者声明的业务字段 + (`system !== true`),恰好落在 `isPreservableUnderAudit` 允许的范围内; +- `beforeInsert` / `beforeUpdate` 钩子计算出的值不受影响 —— 只有**调用者提交**的键 + 才是剥离候选。 + +**这是一次静默剥离,所以它被上报而不是被吞掉。** 引擎 insert 路径上的 +`onFieldsDropped`(#3407)此前只是为了与 `update()` 对称而存在、从不触发,并留了一 +句"若 insert 将来出现静默剥离,必须在剥离点接上监听器"——现在正是那个剥离点。 +事件沿用既有的 `readonly` 原因码(对调用者而言,隐含只读与声明只读被丢弃的理由完全 +相同,不值得为一个没有消费者会区分的差别在 `packages/spec` 里分叉词表)。 +`createManyData` 与 `insertManyData` 也补上了监听器转发:后者保持**逐行精度**—— +引擎事件是整批的并集,但剥离只会移除**行自身提交过**的键,因此可以准确归属回具体行。 +导入器优先走的正是 `insertManyData` 这条部分成功路径。 + +**与 `strictReadonlyWrites`(#5126 / #5610)叠加。** 该开关是"剥离即拒绝"的进程内出路, +本次改动使它自然覆盖单号,两条路径同权: + +- **UPDATE 无需新代码** —— autonumber 限肢走的正是 `stripReadonlyFields` → + `reportDroppedFields` → `assertNoStrictDrops` 这条 #5126 已经铺好的接缝,因此 strict + 开启时,调用者提交的单号与声明 `readonly` 的字段一样被拒绝,整笔写入不落库; +- **INSERT 需要接上** —— #5126 当时把该开关在 insert 上留作惰性,并写下条件:"insert + 一旦有了剥离,两个成员就在那个剥离点一起接上"。本次正是那个剥离点,于是 + `onFieldsDropped` 与 `strictReadonlyWrites` 一并兑现:默认剥离+上报,strict 开启则在 + 任何驱动调用之前抛 `ERR_READONLY_FIELD_REJECTED`,且**监听器不触发**(被拒绝的写入 + 并未完成,这是 #5126 自己的设计要点)。 + +接缝处**没有新增任何策略**:#5126 明确写着 strict "不引入第二套策略,它只是把既有策略 +报出来",且"剥离拿不走的字段也不会被拒绝"。照此逐字适用,`isSystem` 与 `preserveAudit` +两个豁免在 strict 下依旧被接受(它们根本不会走到剥离分支)。 + +`ReadonlyFieldRejectedError` 新增可选的 `operation`(默认 `'update'`,#5126 的 UPDATE +文案逐字节不变):动词与补救办法确实因操作而异 —— INSERT 的拒绝必然关于运行时拥有的值, +其合法写入者是 `isSystem` 与历史导入 `preserveAudit`,而 `readonlyWhen` 在 create 上 +根本锁不住任何东西。 + +**升级影响。** 普通(非历史)导入若把遗留单号列映射到 `autonumber` 字段,该值现在会 +被丢弃并改由序列发号,同时在响应的 `droppedFields` 里上报、在服务端日志里留下一条 +带补救办法的 `warn`。要保留原始单号,请把导入标记为历史导入 +(`treat_as_historical` → `preserveAudit`),这与 #3493 为只读业务字段确立的划分一致。 + +`packages/spec` 未改动:`autonumber` builder 是否应当直接注入 `readonly: true` 是 +spec 层的独立议题,与这条引擎侧防线不冲突。 diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 8961c23074..f1034aedb1 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -6066,9 +6066,12 @@ export class ObjectStackProtocolImplementation implements ); // [#3455] Surface the #3043 ingress strip, symmetric with single-write // createData. Diff each supplied row against its stripped form, then - // AGGREGATE — the `{ records, count }` response has no per-row slot, and - // the insert-time strip is static-`readonly` only (schema-uniform), so a - // union view is faithful rather than lossy. + // AGGREGATE — the `{ records, count }` response has no per-row slot, so + // a union is the only representable view here. (It used to be lossless + // as well, the ingress strip being static-`readonly` and therefore + // schema-uniform; the engine strip #5503 adds is per-row, so the union + // now genuinely aggregates. `insertManyData`, which HAS a per-row slot, + // keeps row precision for both sources.) const dropped: DroppedFieldsEvent[] = []; if (Array.isArray(request.records)) { for (let i = 0; i < request.records.length; i++) { @@ -6076,12 +6079,15 @@ export class ObjectStackProtocolImplementation implements if (ev) dropped.push(ev); } } + // [#5503] The engine gained an INSERT-side strip of its own (runtime-owned + // `autonumber` values a non-system caller supplied). Forward the listener + // here as `createData` already does, so a bulk create / import learns + // which record numbers were refused instead of only the server log seeing + // it. Merging AFTER the write is what lets both sources land in one list. + const opts: any = { onFieldsDropped: (e: DroppedFieldsEvent) => { dropped.push(e); } }; + if (request.context !== undefined) opts.context = request.context; + const records = await this.engine.insert(request.object, rows, opts); const merged = mergeDroppedFieldEvents(dropped); - const records = await this.engine.insert( - request.object, - rows, - request.context !== undefined ? { context: request.context } as any : undefined, - ); return { object: request.object, records, @@ -6120,16 +6126,33 @@ export class ObjectStackProtocolImplementation implements const perRowDropped: Array = Array.isArray(request.records) ? request.records.map((rec, i) => diffDroppedFields(request.object, rec, rowsArr[i], 'readonly')) : []; + // [#5503] The ENGINE now strips too (runtime-owned `autonumber` values a + // non-system caller supplied), and its `onFieldsDropped` event is the + // UNION over the batch — the listener signature carries no row index. Row + // precision is recoverable without one: the engine strip only removes + // keys the ROW ITSELF supplied, so a dropped name belongs to exactly the + // rows whose supplied payload carried it. Without this the import + // surface (which prefers this partial-success path over createManyData) + // would drop record numbers with nothing but a server log to show for it. + const engineDropped = new Set(); + const opts: any = { onFieldsDropped: (e: DroppedFieldsEvent) => { for (const f of e.fields) engineDropped.add(f); } }; + if (request.context !== undefined) opts.context = request.context; const outcomes: Array<{ ok: boolean; record?: any; error?: unknown; droppedFields?: DroppedFieldsEvent[] }> = await engineInsertMany.call( this.engine, request.object, rows, - request.context !== undefined ? { context: request.context } as any : undefined, + opts, ); if (Array.isArray(outcomes)) { for (let i = 0; i < outcomes.length; i++) { - const ev = perRowDropped[i]; - if (ev && outcomes[i]) outcomes[i].droppedFields = [ev]; + if (!outcomes[i]) continue; + const supplied = (request.records?.[i] ?? {}) as Record; + const mine = [...engineDropped].filter((f) => f in supplied); + const events: DroppedFieldsEvent[] = []; + if (perRowDropped[i]) events.push(perRowDropped[i]!); + if (mine.length > 0) events.push({ object: request.object, fields: mine, reason: 'readonly' }); + const merged = mergeDroppedFieldEvents(events); + if (merged.length > 0) outcomes[i].droppedFields = merged; } } return { object: request.object, outcomes }; diff --git a/packages/objectql/src/engine-autonumber-runtime-owned.test.ts b/packages/objectql/src/engine-autonumber-runtime-owned.test.ts new file mode 100644 index 0000000000..57be2578f1 --- /dev/null +++ b/packages/objectql/src/engine-autonumber-runtime-owned.test.ts @@ -0,0 +1,539 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5503 — `autonumber` is RUNTIME-owned on the write path, not caller-owned. + * + * The engine has always documented that "the runtime owns the value, not the + * client" (`applyAutonumbers`), and required-validation exempts `autonumber` + * on both verbs because of it. But no write layer enforced it: a plain REST + * caller could POST an explicit record number (bypassing the sequence) and + * PATCH an existing one (forging a business identifier). Same defect family as + * #4447 (`created_at` forgeable by a normal PATCH); the difference is that + * `readonly: true` fields were already stripped (#2948 / #3043) while + * `type: 'autonumber'` carries no such flag. + * + * The fix treats `type: 'autonumber'` as IMPLICITLY read-only in the engine / + * validation layer — insert and update at equal rank — so the strip happens + * BEFORE the payload is dispatched to any driver. That placement is what makes + * it driver-agnostic: the SQL driver's `supports.autonumber` sequence path is + * covered without the driver changing a line, which the "native driver" cases + * below assert directly (the row handed to `driver.create` carries no + * autonumber key at all). + * + * Exemptions keep their existing semantics: + * - `isSystem` writes (seed replay, migration, internal provisioning); + * - an opt-in "historical" import (`preserveAudit`, #3493) — a data migration + * reinstating legacy record numbers is the business case the whitelist + * exists for, and `autonumber` is an author-declared business field + * (`system !== true`), exactly what `isPreservableUnderAudit` admits; + * - server-side stamps: only keys the CALLER supplied are candidates, so a + * `beforeInsert` / `beforeUpdate` hook that computes the value survives. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { ObjectQL } from './engine.js'; +import type { DroppedFieldsEvent } from '@objectstack/spec/data'; + +const ACCOUNT = { + name: 'an_account', + label: 'Account', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const, required: true }, + // The forgeable business identifier: engine-generated, never author-flagged + // `readonly`. `required` on purpose — the runtime owns it, so required + // validation must keep exempting it after the strip removes the value. + account_number: { + name: 'account_number', + label: 'Account No.', + type: 'autonumber' as const, + required: true, + autonumberFormat: 'ACC-{0000}', + }, + }, +}; + +function makeMemoryDriver(opts: { nativeAutonumber?: boolean } = {}) { + const stores = new Map>>(); + const createdRows: Array> = []; + const updatedPayloads: Array> = []; + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + let nativeSeq = 0; + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + const a = row[k] === undefined ? null : row[k]; + const b = expected === undefined ? null : expected; + if (a !== b) return false; + } + return true; + }; + const driver: any = { + name: opts.nativeAutonumber ? 'sql' : 'memory', + version: '0.0.0', + // `supports.autonumber: true` is the SQL driver's contract (#1603): the + // engine defers generation entirely to the driver's persistent sequence. + supports: opts.nativeAutonumber ? ({ autonumber: true } as any) : ({} as any), + 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) => matchesWhere(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + createdRows.push({ ...data }); + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row: Record = { ...data, id }; + if (opts.nativeAutonumber) { + // Mirrors `fillAutoNumberFields`: fill ONLY a slot the caller left + // empty. With the engine strip in place that slot is always empty for + // a non-system caller, so the persistent sequence always wins. + if (row.account_number === undefined || row.account_number === null || row.account_number === '') { + nativeSeq += 1; + row.account_number = `SEQ-${String(nativeSeq).padStart(4, '0')}`; + } + } + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + updatedPayloads.push({ ...data }); + const s = storeFor(object); + const cur = s.get(id); + if (!cur) throw new Error(`not found: ${object}/${id}`); + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return updated; + }, + async updateMany(object: string, ast: any, data: Record) { + updatedPayloads.push({ ...data }); + const s = storeFor(object); + let n = 0; + for (const [id, row] of s) { + if (!matchesWhere(row, ast?.where)) continue; + s.set(id, { ...row, ...data, id }); + n += 1; + } + return n; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + const out = []; + for (const r of rows) out.push(await this.create(object, r)); + return out; + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, + async rollback() {}, + }; + return { driver, stores, createdRows, updatedPayloads }; +} + +async function makeEngine(opts: { nativeAutonumber?: boolean } = {}) { + const engine = new ObjectQL(); + const rig = makeMemoryDriver(opts); + engine.registerDriver(rig.driver, true); + await engine.init(); + engine.registry.registerObject(ACCOUNT as any); + const protocol = new ObjectStackProtocolImplementation(engine); + return { engine, protocol, ...rig }; +} + +describe('#5503 — autonumber is runtime-owned: INSERT', () => { + let rig: Awaited>; + beforeEach(async () => { rig = await makeEngine(); }); + + it('strips a caller-supplied record number and issues the sequence value instead', async () => { + const created = await rig.protocol.createData({ + object: 'an_account', + data: { name: 'AN forge', account_number: 'ACC-777777' }, + }); + // The forged value never reached the store... + expect(created.record.account_number).not.toBe('ACC-777777'); + // ...and the engine's own sequence issued the real one. + expect(created.record.account_number).toBe('ACC-0001'); + // The row handed to the driver carried no forged value either. + expect(rig.createdRows[0]?.account_number).toBe('ACC-0001'); + }); + + it('reports the strip instead of dropping it silently (#3407 / #3431)', async () => { + const created = await rig.protocol.createData({ + object: 'an_account', + data: { name: 'AN forge', account_number: 'ACC-777777' }, + }); + const dropped = (created as { droppedFields?: DroppedFieldsEvent[] }).droppedFields ?? []; + expect(dropped.flatMap((e) => e.fields)).toContain('account_number'); + }); + + it('a `required` autonumber still passes insert validation after the strip', async () => { + // The strip empties a REQUIRED field — required-validation exempts + // `autonumber` either way, so this must not become a 400. + await expect( + rig.protocol.createData({ object: 'an_account', data: { name: 'ok', account_number: 'ACC-9' } }), + ).resolves.toBeTruthy(); + }); + + it('sequence continuity: a forged insert does not steal or skip a number', async () => { + await rig.protocol.createData({ object: 'an_account', data: { name: 'a' } }); + await rig.protocol.createData({ object: 'an_account', data: { name: 'b', account_number: 'ACC-9999' } }); + const third = await rig.protocol.createData({ object: 'an_account', data: { name: 'c' } }); + expect(third.record.account_number).toBe('ACC-0003'); + }); + + it('strips per row in a batch insert', async () => { + const rows = await rig.engine.insert('an_account', [ + { name: 'a', account_number: 'ACC-111111' }, + { name: 'b' }, + ]); + expect(rows.map((r: any) => r.account_number)).toEqual(['ACC-0001', 'ACC-0002']); + }); + + it('keeps an explicit value for a system write (seed replay / migration)', async () => { + const row = await rig.engine.insert( + 'an_account', + { name: 'seeded', account_number: 'ACC-000042' }, + { context: { isSystem: true } } as any, + ); + expect(row.account_number).toBe('ACC-000042'); + }); + + it('keeps an explicit value for a `preserveAudit` historical import (#3493)', async () => { + const row = await rig.engine.insert( + 'an_account', + { name: 'legacy', account_number: 'LEGACY-0007' }, + { context: { preserveAudit: true } } as any, + ); + expect(row.account_number).toBe('LEGACY-0007'); + }); + + it('keeps a beforeInsert hook stamp — only CALLER-supplied keys are candidates', async () => { + rig.engine.registerHook('beforeInsert', async (ctx: any) => { + ctx.input.data.account_number = 'HOOK-0001'; + }, { object: 'an_account' }); + const row = await rig.engine.insert('an_account', { name: 'hooked' }); + expect(row.account_number).toBe('HOOK-0001'); + }); +}); + +describe('#5503 — autonumber is runtime-owned: INSERT on a native-autonumber driver', () => { + it('hands the driver NO autonumber key, so the persistent sequence wins', async () => { + // The strip lives in the engine, BEFORE dispatch — which is precisely why + // the SQL driver's `supports.autonumber` path is covered without the + // driver changing. Asserted here on the driver-facing payload, not by + // patching a driver. + const rig = await makeEngine({ nativeAutonumber: true }); + const created = await rig.protocol.createData({ + object: 'an_account', + data: { name: 'AN forge', account_number: 'ACC-777777' }, + }); + expect('account_number' in (rig.createdRows[0] ?? {})).toBe(false); + expect(created.record.account_number).toBe('SEQ-0001'); + }); + + it('a system write still reaches the driver with its explicit value', async () => { + const rig = await makeEngine({ nativeAutonumber: true }); + const row = await rig.engine.insert( + 'an_account', + { name: 'seeded', account_number: 'ACC-000042' }, + { context: { isSystem: true } } as any, + ); + expect(rig.createdRows[0]?.account_number).toBe('ACC-000042'); + expect(row.account_number).toBe('ACC-000042'); + }); +}); + +describe('#5503 — autonumber is runtime-owned: bulk-create surfaces', () => { + it('createManyData strips per row and reports the union', async () => { + const rig = await makeEngine(); + const res: any = await rig.protocol.createManyData({ + object: 'an_account', + records: [ + { name: 'a', account_number: 'ACC-111111' }, + { name: 'b' }, + ], + }); + expect(res.records.map((r: any) => r.account_number)).toEqual(['ACC-0001', 'ACC-0002']); + expect((res.droppedFields ?? []).flatMap((e: DroppedFieldsEvent) => e.fields)).toContain('account_number'); + }); + + it('insertManyData keeps ROW precision — only the forging row is reported', async () => { + // The import runner prefers this partial-success surface, so it is the one + // that has to stay honest about which row lost its record number. + const rig = await makeEngine(); + const res: any = await rig.protocol.insertManyData({ + object: 'an_account', + records: [ + { name: 'a' }, + { name: 'b', account_number: 'ACC-111111' }, + ], + }); + expect(res.outcomes.map((o: any) => o.record.account_number)).toEqual(['ACC-0001', 'ACC-0002']); + expect(res.outcomes[0].droppedFields).toBeUndefined(); + expect(res.outcomes[1].droppedFields.flatMap((e: DroppedFieldsEvent) => e.fields)).toEqual(['account_number']); + }); +}); + +/** + * #5503 x #5126 — the two features superposed. + * + * #5126 gave the readonly strip a LOUD half: `strictReadonlyWrites` refuses the + * write instead of committing it without the stripped columns. #5503 makes + * `autonumber` an implicitly-readonly field. The correct joint semantics fall + * straight out of #5126's own stated rule — "strict adds no second policy, it + * refuses exactly what the strip would have taken" — so: + * + * - strict ON → a caller-supplied record number is REFUSED, at equal rank + * with a declared `readonly` field, on insert AND update; + * - strict OFF → unchanged: stripped, committed, reported via + * `onFieldsDropped`; + * - a value the strip does NOT take is not rejected either, so the `isSystem` + * and `preserveAudit` exemptions survive strict untouched. That is the + * rule applied verbatim, not a new decision. + * + * On UPDATE this needed no new code — the autonumber limb rides + * `stripReadonlyFields` → `reportDroppedFields` → `assertNoStrictDrops`, the + * seam #5126 already built. On INSERT it did: #5126 left `strictReadonlyWrites` + * inert there with the standing note "if insert ever gains a strip, both + * members wire up together at that site", and #5503 is that strip. + */ +describe('#5503 x #5126 — strictReadonlyWrites covers runtime-owned fields', () => { + it('INSERT: refuses a caller-supplied record number and writes NOTHING', async () => { + const rig = await makeEngine(); + const err = await rig.engine + .insert( + 'an_account', + { name: 'AN forge', account_number: 'ACC-777777' }, + { strictReadonlyWrites: true }, + ) + .then(() => null, (e) => e); + + expect(err?.code).toBe('ERR_READONLY_FIELD_REJECTED'); + expect(err.fields).toEqual(['account_number']); + expect(err.operation).toBe('insert'); + expect(err.message).toContain('account_number'); + // The refusal lands BEFORE the driver — not even the legitimate `name`. + expect(rig.createdRows).toHaveLength(0); + // …and no sequence value was burned on the refused attempt. + const ok = await rig.engine.insert('an_account', { name: 'clean' }); + expect(ok.account_number).toBe('ACC-0001'); + }); + + it('INSERT: the listener does NOT fire under strict — a refused write did not complete', async () => { + const rig = await makeEngine(); + const events: DroppedFieldsEvent[] = []; + await rig.engine + .insert( + 'an_account', + { name: 'x', account_number: 'ACC-777777' }, + { strictReadonlyWrites: true, onFieldsDropped: (e) => { events.push(e); } }, + ) + .catch(() => {}); + expect(events).toEqual([]); + }); + + it('INSERT: strict is inert when nothing would be stripped', async () => { + const rig = await makeEngine(); + const row = await rig.engine.insert('an_account', { name: 'clean' }, { strictReadonlyWrites: true }); + expect(row.account_number).toBe('ACC-0001'); + }); + + it('INSERT: the exemptions survive strict — strict refuses only what the strip takes', async () => { + // #5126's rule, applied verbatim rather than re-decided: an exempt writer's + // value is never stripped, so there is nothing for strict to refuse. + const rig = await makeEngine(); + const seeded = await rig.engine.insert( + 'an_account', + { name: 'seeded', account_number: 'ACC-000042' }, + { strictReadonlyWrites: true, context: { isSystem: true } }, + ); + expect(seeded.account_number).toBe('ACC-000042'); + + const legacy = await rig.engine.insert( + 'an_account', + { name: 'legacy', account_number: 'LEGACY-0007' }, + { strictReadonlyWrites: true, context: { preserveAudit: true } }, + ); + expect(legacy.account_number).toBe('LEGACY-0007'); + }); + + it('UPDATE: refuses a caller-supplied record number, at equal rank with a declared readonly field', async () => { + const rig = await makeEngine(); + const created = await rig.protocol.createData({ object: 'an_account', data: { name: 'Acme' } }); + const id = created.id as string; + + const err = await rig.engine + .update( + 'an_account', + { id, name: 'renamed', account_number: 'ACC-888888' }, + { where: { id }, strictReadonlyWrites: true }, + ) + .then(() => null, (e) => e); + + expect(err?.code).toBe('ERR_READONLY_FIELD_REJECTED'); + expect(err.fields).toEqual(['account_number']); + expect([...err.drops].map((d: DroppedFieldsEvent) => d.reason)).toEqual(['readonly']); + // Nothing was written — not even the legitimate rename. + const readback = await rig.engine.findOne('an_account', { where: { id } }); + expect(readback.name).toBe('Acme'); + expect(readback.account_number).toBe('ACC-0001'); + }); + + it('UPDATE: strict OFF keeps the default — stripped, committed, reported', async () => { + const rig = await makeEngine(); + const created = await rig.protocol.createData({ object: 'an_account', data: { name: 'Acme' } }); + const id = created.id as string; + const events: DroppedFieldsEvent[] = []; + + await rig.engine.update( + 'an_account', + { id, name: 'renamed', account_number: 'ACC-888888' }, + { where: { id }, onFieldsDropped: (e) => { events.push(e); } }, + ); + + expect(events.flatMap((e) => e.fields)).toContain('account_number'); + const readback = await rig.engine.findOne('an_account', { where: { id } }); + expect(readback.name).toBe('renamed'); // the legitimate half landed + expect(readback.account_number).toBe('ACC-0001'); // the forged half did not + }); + + it('UPDATE: the preserveAudit exemption survives strict', async () => { + const rig = await makeEngine(); + const created = await rig.protocol.createData({ object: 'an_account', data: { name: 'Acme' } }); + const id = created.id as string; + const res = await rig.engine.update( + 'an_account', + { id, account_number: 'LEGACY-0007' }, + { where: { id }, strictReadonlyWrites: true, context: { preserveAudit: true } }, + ); + expect(res.account_number).toBe('LEGACY-0007'); + }); + + it('a strict BATCH insert is refused whole — consistent with "NOTHING was written"', async () => { + // `insertMany`'s partial-success mode culls bad ROWS; strict is a refusal of + // the WRITE. When a caller asks for both, the refusal wins: the error + // contract says nothing was written, and degrading to "we kept the other + // rows" would make that false. Pinned so the corner is a decision, not an + // accident. + const rig = await makeEngine(); + const err = await rig.engine + .insertMany( + 'an_account', + [{ name: 'a' }, { name: 'b', account_number: 'ACC-111111' }], + { strictReadonlyWrites: true }, + ) + .then(() => null, (e) => e); + expect(err?.code).toBe('ERR_READONLY_FIELD_REJECTED'); + expect(rig.createdRows).toHaveLength(0); + }); +}); + +describe('#5503 — autonumber is runtime-owned: UPDATE', () => { + let rig: Awaited>; + let id: string; + + beforeEach(async () => { + rig = await makeEngine(); + const created = await rig.protocol.createData({ object: 'an_account', data: { name: 'Acme' } }); + id = created.id as string; + expect(created.record.account_number).toBe('ACC-0001'); + }); + + it('strips a caller-supplied record number — the stored number is unchanged', async () => { + const res = await rig.protocol.updateData({ + object: 'an_account', + id, + data: { account_number: 'ACC-888888' }, + }); + expect(res.record.account_number).toBe('ACC-0001'); + const readback = await rig.engine.findOne('an_account', { where: { id } }); + expect(readback.account_number).toBe('ACC-0001'); + }); + + it('the driver never sees the forged column', async () => { + await rig.protocol.updateData({ + object: 'an_account', + id, + data: { name: 'Acme Renamed', account_number: 'ACC-888888' }, + }); + const payload = rig.updatedPayloads.at(-1) ?? {}; + expect('account_number' in payload).toBe(false); + // The legitimate part of the same PATCH still landed. + expect(payload.name).toBe('Acme Renamed'); + }); + + it('reports the strip through droppedFields', async () => { + const res = await rig.protocol.updateData({ + object: 'an_account', + id, + data: { account_number: 'ACC-888888' }, + }); + const dropped = (res as { droppedFields?: DroppedFieldsEvent[] }).droppedFields ?? []; + expect(dropped.flatMap((e) => e.fields)).toContain('account_number'); + }); + + it('strips on a multi-row update too (the #3106 call-site shape)', async () => { + // A second row sharing the predicate value, so the bulk branch really + // rewrites BOTH rows — otherwise "the numbers did not change" would pass + // for the empty reason that nothing was matched at all. + await rig.protocol.createData({ object: 'an_account', data: { name: 'Acme' } }); + await rig.engine.update( + 'an_account', + { account_number: 'ACC-888888', name: 'bulk' }, + { where: { name: 'Acme' }, multi: true } as any, + ); + const payload = rig.updatedPayloads.at(-1) ?? {}; + expect('account_number' in payload).toBe(false); + expect(payload.name).toBe('bulk'); // the legitimate half of the same write landed + const rows = await rig.engine.find('an_account', {}); + expect(rows.map((r: any) => r.name)).toEqual(['bulk', 'bulk']); // both rows were rewritten + expect(rows.map((r: any) => r.account_number).sort()).toEqual(['ACC-0001', 'ACC-0002']); + }); + + it('keeps an explicit value for a system write', async () => { + const res = await rig.engine.update( + 'an_account', + { id, account_number: 'ACC-000042' }, + { where: { id }, context: { isSystem: true } } as any, + ); + expect(res.account_number).toBe('ACC-000042'); + }); + + it('keeps an explicit value for a `preserveAudit` historical import / undo (#3493)', async () => { + const res = await rig.engine.update( + 'an_account', + { id, account_number: 'LEGACY-0007' }, + { where: { id }, context: { preserveAudit: true } } as any, + ); + expect(res.account_number).toBe('LEGACY-0007'); + }); + + it('keeps a beforeUpdate hook stamp — only CALLER-supplied keys are candidates', async () => { + rig.engine.registerHook('beforeUpdate', async (ctx: any) => { + ctx.input.data.account_number = 'HOOK-0002'; + }, { object: 'an_account' }); + const res = await rig.engine.update('an_account', { id, name: 'x' }, { where: { id } } as any); + expect(res.account_number).toBe('HOOK-0002'); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 2f55aa38d5..173b3e7c46 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -88,7 +88,7 @@ import { isAggregatedViewContainer, expandViewContainer } from '@objectstack/spe import { bindHooksToEngine } from './hook-binder.js'; import { validateRecord, normalizeMultiValueFields, coerceBooleanFields, ValidationError, buildFieldError, valueShapePostureSetByEnv, mediaPostureSetByEnv, isScannableValueShapeField } from './validation/record-validator.js'; import type { AdmittedValueShapeViolation, AdmittedValueShapeViolationSink } from './validation/record-validator.js'; -import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, hasParentScopedReadonlyWhenInPayload, stripReadonlyFields } from './validation/rule-validator.js'; +import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, hasParentScopedReadonlyWhenInPayload, stripReadonlyFields, stripRuntimeOwnedFields } from './validation/rule-validator.js'; import { resolveMasterDetailRelation } from './master-detail.js'; import { applyInMemoryAggregation } from './in-memory-aggregation.js'; import { @@ -1839,6 +1839,16 @@ export class ObjectQL implements IObjectQLEngine { * a `required` record number is never rejected for "missing" — the runtime * owns the value, not the client. * + * That ownership is now ENFORCED rather than merely asserted (#5503): the + * insert path strips a caller-supplied value before this runs, so the "respect + * explicit value" skip below is reached only by writers the strip exempts — + * `isSystem` (seed replay, migration) and an opt-in historical import + * (`preserveAudit`), plus values a `beforeInsert` hook computed server-side. + * For every other caller the slot arrives empty and the sequence issues the + * number. Do NOT "fix" a forged value here by overwriting it: the strip must + * stay upstream of this method, because a driver with + * `supports.autonumber === true` returns above without ever entering the loop. + * * In the fallback path the next value is `max(existing) + 1`, seeded once per * `object.field.` from the store then incremented in memory (monotonic * within the process, resilient to deletions). The shared `autonumberFormat` @@ -1861,7 +1871,10 @@ export class ObjectQL implements IObjectQLEngine { for (const [name, def] of Object.entries(fields)) { if ((def as any)?.type !== 'autonumber') continue; const current = record[name]; - if (current != null && current !== '') continue; // respect explicit value + // Respect an explicit value — reachable only for an EXEMPT writer now + // (isSystem / preserveAudit / a hook stamp): #5503's strip removed every + // other caller's value before this method was called. + if (current != null && current !== '') continue; // Honor either the spec-canonical `autonumberFormat` or the shorthand // `format` (both appear in metadata; the driver reads both too) — #1603. const fmt = (def as any).autonumberFormat ?? (def as any).format; @@ -4825,17 +4838,22 @@ export class ObjectQL implements IObjectQLEngine { * validation passes, so a doomed attempt no longer consumes a sequence value * (no number-range gaps from a rejected batch). */ - // [#3407] `WriteObservabilityOptions.onFieldsDropped` is accepted for - // signature symmetry with `update()` but never fires here: INSERT is - // deliberately exempt from the readonly/readonlyWhen strips (a create may - // legitimately seed read-only columns), and the FLS write gate throws - // instead of stripping. If insert ever gains a silent strip, wire the - // listener at that strip site — do not let it go silent. - // [#5126] `strictReadonlyWrites` is inert here for the SAME reason and by the - // same rule: it refuses a write that would be stripped, and insert strips - // nothing. It is not silently ignored in the #4371 sense — there is no - // behaviour to execute. If insert ever gains a strip, both members wire up - // together at that site. + // [#3407 / #5126] BOTH members of `WriteObservabilityOptions` are live here, + // for exactly ONE strip: the runtime-owned (`autonumber`) strip added by + // #5503, wired at its strip site below. Each arrived carrying the same + // standing condition — #3407's "if insert ever gains a silent strip, wire the + // listener at that strip site", #5126's "it is inert here only because insert + // strips nothing; if insert ever gains a strip, both members wire up together + // at that site". #5503 is that strip, so both are discharged together: + // quiet-and-observable by default (`onFieldsDropped`), refused outright under + // `strictReadonlyWrites` — the same one-per-call choice update offers. + // + // INSERT remains deliberately exempt from the AUTHOR-declared + // readonly/readonlyWhen strips (a create may legitimately seed read-only + // columns; the #3043 ingress strip covers external callers instead), and the + // FLS write gate throws rather than stripping. So neither member reports on + // those here — only on what this path actually strips. Any FURTHER strip added + // here must wire both members at its own site too. async insert(object: string, data: any | any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise { object = this.resolveObjectName(object); this.logger.debug('Insert operation starting', { object, isBatch: Array.isArray(data) }); @@ -4946,6 +4964,74 @@ export class ObjectQL implements IObjectQLEngine { (isBatch ? (opCtx.data as any[]) : [opCtx.data]).map( (row) => (row ?? {}) as Record, ); + // [#5503] `autonumber` is RUNTIME-owned: the engine (or the driver's + // persistent sequence) issues the value, so a non-system caller does not + // get to supply or rewrite it. Until now nothing enforced that — a POST + // carrying an explicit record number was stored verbatim, bypassing the + // sequence, and the SQL driver's `supports.autonumber` path adopted it + // too (it only fills a slot left empty). Stripping HERE, in the engine + // and before `applyAutonumbers`, is what makes the fix driver-agnostic: + // every driver — native-sequence or not — is handed a row with no + // caller-supplied record number, so no driver had to change. + // + // Runs BEFORE validation on purpose: a value the caller was never + // allowed to send must not be judged by the object's rules either (a + // `format` rule on the field would otherwise 400 on a payload we are + // about to discard). Symmetric with the UPDATE strip, which likewise + // runs before `evaluateValidationRules`. Exemptions are the update + // path's, unchanged: `isSystem` (seed replay, migration) skips the whole + // pass, and `preserveAudit` (#3493) lets a historical import reinstate + // legacy record numbers. + const autonumberDropped: string[] = []; + if (!opCtx.context?.isSystem) { + const preserveAudit = opCtx.context?.preserveAudit === true; + for (let i = 0; i < rows.length; i++) { + if (rowErrors[i] !== undefined) continue; + const supplied = new Set(Object.keys(suppliedPerRow[i] ?? {})); + const stripped = stripRuntimeOwnedFields( + schemaForValidation as any, rows[i], supplied, this.logger, { preserveAudit }, + ) as Record; + if (stripped === rows[i]) continue; + for (const k of Object.keys(rows[i])) { + if (!(k in stripped) && !autonumberDropped.includes(k)) autonumberDropped.push(k); + } + rows[i] = stripped; + rowHookContexts[i].input.data = stripped; + } + } + // [#3407 / #5126] This is the strip site both standing notes on + // `insert()` pointed at, so both members of `WriteObservabilityOptions` + // discharge here — the same one-per-call choice `update` offers, and by + // the same rule #5126 wrote down: strict adds NO second policy, it + // refuses exactly what the strip would have taken. A value the strip + // does not take is not rejected either, so an `isSystem` write and a + // `preserveAudit` historical import stay accepted under strict — they + // never reach this branch at all. + // + // Reported under the existing `readonly` reason: from the caller's side + // an implicitly read-only field is dropped for exactly the reason a + // declared one is, and inventing a parallel reason code would fork the + // vocabulary (`packages/spec`) for a distinction no consumer acts on. + if (autonumberDropped.length > 0) { + const drop: DroppedFieldsEvent = { object, fields: autonumberDropped, reason: 'readonly' }; + if (options?.strictReadonlyWrites === true) { + // Before the driver write and before validation — nothing is + // written, and "you sent a runtime-owned field" should not depend on + // whether some other field also failed a business rule (#5126's + // ordering on the update path, mirrored). + throw new ReadonlyFieldRejectedError(object, autonumberDropped, [drop], 'insert'); + } + if (typeof options?.onFieldsDropped === 'function') { + // Under strict the listener deliberately does NOT fire (above): + // `DroppedFieldsEvent` is contracted as "dropped, and the write + // completed without them", and a refused write did not complete. + try { + options.onFieldsDropped(drop); + } catch (err) { + this.logger.warn('onFieldsDropped listener threw — ignored', { object, error: err }); + } + } + } for (let i = 0; i < rows.length; i++) { if (rowErrors[i] !== undefined) continue; try { @@ -5085,8 +5171,14 @@ export class ObjectQL implements IObjectQLEngine { * A summary-recompute failure after retries still throws * {@link SummaryRecomputeError} (framework#3147) with `written` set to the * outcome array — the records ARE written. + * + * `onFieldsDropped` (#3407) is forwarded to `insert`, so the runtime-owned + * strip (#5503) reports here too. The event carries no row index — it is the + * UNION over the batch — but the strip only ever removes keys the row itself + * supplied, so a caller holding the input rows can attribute each name back to + * the rows that carried it (`insertManyData` does exactly that). */ - async insertMany(object: string, rows: any[], options?: DataEngineInsertOptions): Promise { + async insertMany(object: string, rows: any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise { if (!Array.isArray(rows)) throw new Error('insertMany expects an array of rows'); return this.insert(object, rows, { ...(options ?? {}), __partialRowErrors: true } as any); } diff --git a/packages/objectql/src/readonly-strict-errors.ts b/packages/objectql/src/readonly-strict-errors.ts index 31d25323f6..d3f316086b 100644 --- a/packages/objectql/src/readonly-strict-errors.ts +++ b/packages/objectql/src/readonly-strict-errors.ts @@ -3,8 +3,9 @@ import type { DroppedFieldsEvent } from '@objectstack/spec/data'; /** - * Thrown by `engine.update` when the caller passed - * `options.strictReadonlyWrites: true` (`WriteObservabilityOptions`, #5126) and + * Thrown by `engine.update` — and, since #5503, by `engine.insert` — when the + * caller passed `options.strictReadonlyWrites: true` + * (`WriteObservabilityOptions`, #5126) and * the payload contained caller-supplied fields the engine would have STRIPPED. * * The write did NOT happen — nothing was sent to the driver, so neither the @@ -13,9 +14,12 @@ import type { DroppedFieldsEvent } from '@objectstack/spec/data'; * payload the caller never wrote, and a caller that opts out of that does not * want a smaller version of it. * - * `fields` is the union across BOTH strip passes — static `readonly` (#2948) - * and a TRUE `readonlyWhen` predicate (#3042) — so one error names everything - * wrong with the payload instead of forcing a round-trip per field. `drops` + * `fields` is the union across every strip pass the operation runs — on UPDATE + * static `readonly` (#2948), a TRUE `readonlyWhen` predicate (#3042), and the + * implicitly-readonly runtime-owned types (#5503); on INSERT only the last of + * those, because a create is deliberately exempt from the author-declared + * strips (#3413). One error names everything wrong with the payload instead of + * forcing a round-trip per field. `drops` * keeps the per-reason breakdown (the same `DroppedFieldsEvent` shape * `onFieldsDropped` would have received, had the write been allowed to * complete), so a caller can tell a schema-level lock from a state-dependent @@ -32,16 +36,31 @@ export class ReadonlyFieldRejectedError extends Error { public readonly object: string, public readonly fields: string[], public readonly drops: readonly DroppedFieldsEvent[], + /** + * Which verb refused (#5503). Defaults to `'update'` so the UPDATE message + * — and every caller written against it — is byte-identical to #5126's. + * The remedies genuinely differ per operation, so they are not shared: an + * INSERT refusal can only ever be about a runtime-owned value, whose exempt + * writers are `isSystem` and the `preserveAudit` historical import, while + * `readonlyWhen` cannot lock anything on a create at all. + */ + public readonly operation: 'insert' | 'update' = 'update', ) { super( - `Update on '${object}' was REFUSED: ${fields.length} caller-supplied field(s) ` + + `${operation === 'insert' ? 'Insert' : 'Update'} on '${object}' was REFUSED: ` + + `${fields.length} caller-supplied field(s) ` + `(${fields.join(', ')}) are read-only and would have been stripped, and this write ` + `passed options.strictReadonlyWrites — so NOTHING was written, including the fields ` + `that would have survived. Remove the read-only field(s) from the payload; or, for ` + `server-side code that legitimately writes read-only columns, pass ` + - `{ context: { isSystem: true } } (this exempts statically 'readonly' fields, but NOT ` + - `fields locked by a TRUE 'readonlyWhen' predicate — those stay locked for every ` + - `caller). To let the strip happen and merely observe it, drop ` + + (operation === 'insert' + ? `{ context: { isSystem: true } } — or, for a data migration reinstating legacy ` + + `values for a runtime-owned field (a record number), the historical-import ` + + `context { context: { preserveAudit: true } } (#3493). ` + : `{ context: { isSystem: true } } (this exempts statically 'readonly' fields, but NOT ` + + `fields locked by a TRUE 'readonlyWhen' predicate — those stay locked for every ` + + `caller). `) + + `To let the strip happen and merely observe it, drop ` + `strictReadonlyWrites and pass options.onFieldsDropped instead (#3407).`, ); this.name = 'ReadonlyFieldRejectedError'; diff --git a/packages/objectql/src/validation/record-validator.test.ts b/packages/objectql/src/validation/record-validator.test.ts index 8d08d1f389..4712572bca 100644 --- a/packages/objectql/src/validation/record-validator.test.ts +++ b/packages/objectql/src/validation/record-validator.test.ts @@ -28,6 +28,13 @@ describe('validateRecord — required + autonumber exemption', () => { }); it('accepts an explicitly-provided autonumber value', () => { + // NOT a licence for clients to supply record numbers — since #5503 the + // engine strips a non-system caller's value BEFORE this validator runs, so + // the only writes that still arrive here carrying one are the exempt ones + // (`isSystem` seed replay / migration, a `preserveAudit` historical import, + // or a hook-computed stamp). This pins that those are not then rejected by + // the validator instead. The write-path ownership itself is pinned in + // `engine-autonumber-runtime-owned.test.ts`. expect(() => validateRecord(schema, { title: 'Hello', record_no: 'REC-0042' }, 'insert'), ).not.toThrow(); diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index 84a3860520..9e3f664318 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -10,6 +10,9 @@ import { hasReadonlyWhenInPayload, hasParentScopedReadonlyWhenInPayload, stripReadonlyFields, + stripRuntimeOwnedFields, + isRuntimeOwnedField, + runtimeOwnedStripWarning, } from './rule-validator.js'; import { ValidationError } from './record-validator.js'; @@ -381,6 +384,109 @@ describe('stripReadonlyFields — preserveAudit whitelist (#3493)', () => { }); }); +// #5503 — `autonumber` is IMPLICITLY read-only: the runtime issues the value, +// so a caller may neither seed it on create nor rewrite it on update. The field +// carries no `readonly: true` flag (and the spec builder does not inject one), +// which is exactly why the #2948 loop used to walk straight past it. +const numberedFields = { + name: 'an_account', + fields: { + title: { type: 'text' }, + // the forgeable business identifier + account_number: { type: 'autonumber', autonumberFormat: 'ACC-{0000}' }, + // a formula field: computed on READ from a plan, never persisted from the + // write payload — deliberately NOT runtime-owned for strip purposes. + total: { type: 'formula' }, + // an author-declared readonly field, to prove both sources coexist + closed_at: { type: 'datetime', readonly: true }, + }, +}; + +describe('isRuntimeOwnedField (#5503)', () => { + it('is true for autonumber and false for every other type in the fixture', () => { + expect(isRuntimeOwnedField({ type: 'autonumber' })).toBe(true); + expect(isRuntimeOwnedField({ type: 'text' })).toBe(false); + expect(isRuntimeOwnedField({ type: 'formula' })).toBe(false); + expect(isRuntimeOwnedField({ type: 'summary' })).toBe(false); + expect(isRuntimeOwnedField(undefined)).toBe(false); + }); +}); + +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); + expect(out).toEqual({ title: 'x' }); + }); + + it('KEEPS a hook-stamped record number the caller did not supply', () => { + const supplied = new Set(['title']); + 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 out = stripReadonlyFields( + numberedFields, { account_number: 'LEGACY-7' }, 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[] = []; + stripReadonlyFields( + numberedFields, + { account_number: 'ACC-888888', closed_at: '2021-01-01T00:00:00Z' }, + new Set(['account_number', 'closed_at']), + { warn: (m: string) => warns.push(m) } as any, + ); + expect(warns).toHaveLength(2); + expect(warns.some((m) => m === runtimeOwnedStripWarning('account_number', 'autonumber', 'an_account'))).toBe(true); + // The message must say WHY (runtime-issued) and name BOTH exempt writer + // paths — an author who never wrote `readonly: true` gets no help from a + // bare "this field is read-only". + const rt = warns.find((m) => m.includes('runtime-owned'))!; + expect(rt).toContain('isSystem'); + expect(rt).toContain('preserveAudit'); + expect(rt).toContain('COMMITTED WITHOUT IT'); + }); +}); + +describe('stripRuntimeOwnedFields — the INSERT-side strip (#5503)', () => { + it('drops a caller-supplied record number', () => { + const out = stripRuntimeOwnedFields( + numberedFields, { title: 'x', account_number: 'ACC-777777' }, new Set(['title', 'account_number']), + ); + expect(out).toEqual({ title: 'x' }); + }); + + it('leaves AUTHOR-declared readonly fields alone — insert keeps its #3413 exemption', () => { + // The engine is deliberately NOT the place the static-`readonly` insert + // strip lives (that is the #3043 protocol ingress); this narrower helper + // must not quietly take over that job and start stripping columns the + // trusted internal writers legitimately seed on create. + const out = stripRuntimeOwnedFields( + numberedFields, + { title: 'x', closed_at: '2021-01-01T00:00:00Z' }, + new Set(['title', 'closed_at']), + ); + expect(out).toEqual({ title: 'x', closed_at: '2021-01-01T00:00:00Z' }); + }); + + it('KEEPS a hook-stamped value and returns the SAME object when nothing is stripped', () => { + const d = { title: 'x', account_number: 'HOOK-1' }; + expect(stripRuntimeOwnedFields(numberedFields, d, new Set(['title']))).toBe(d); + }); + + it('KEEPS it under preserveAudit', () => { + const out = stripRuntimeOwnedFields( + numberedFields, { account_number: 'LEGACY-7' }, new Set(['account_number']), undefined, { preserveAudit: true }, + ); + expect(out).toEqual({ account_number: 'LEGACY-7' }); + }); +}); + describe('needsPriorRecord — field conditional rules (B2)', () => { it('is true when a field declares requiredWhen / readonlyWhen', () => { expect(needsPriorRecord(invoiceFields as any)).toBe(true); diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 54c3fdbfb5..11300af60a 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -551,14 +551,54 @@ export function stripReadonlyWhenFieldsMulti( } /** - * Strip CALLER-SUPPLIED writes to statically `readonly: true` fields from an - * UPDATE payload (#2948). Unlike `readonlyWhen` (conditional, handled above), a + * Field types whose VALUE the runtime owns end to end, whether or not the + * author ever wrote `readonly: true` on them (#5503). + * + * Exactly one member today: `autonumber`. The engine has always documented the + * ownership (`applyAutonumbers`: "the runtime owns the value, not the client") + * and both record validators act on it — a `required` autonumber is exempt from + * the missing-value check on insert AND update precisely because the client is + * not supposed to supply it. What was missing was the other half: nothing on + * the write path stopped a client from supplying it anyway, so a plain REST + * caller could POST a record number of their choosing (bypassing the sequence) + * and PATCH an existing one (forging a business identifier). Declaring the + * ownership here makes it enforced rather than merely asserted — the same + * `declared ≠ enforced` correction as #4447 (`created_at`), one type over. + * + * Deliberately NOT `formula` / `summary`: those are computed on read from a + * plan, never stored from the write payload, so there is no caller value to + * strip. Keep this set to types whose value is (a) persisted and (b) issued by + * the runtime. + */ +const RUNTIME_OWNED_FIELD_TYPES: ReadonlySet = new Set(['autonumber']); + +/** + * Whether the runtime owns this field's value outright — i.e. the field is + * IMPLICITLY read-only on the write path even with no `readonly: true` flag. + * See {@link RUNTIME_OWNED_FIELD_TYPES}. #5503. + */ +export function isRuntimeOwnedField(def: { type?: string } | undefined | null): boolean { + return def?.type != null && RUNTIME_OWNED_FIELD_TYPES.has(String(def.type)); +} + +/** + * Strip CALLER-SUPPLIED writes to read-only fields from an UPDATE payload + * (#2948). Unlike `readonlyWhen` (conditional, handled above), a * static `readonly` field was never enforced on the server write path: the * record validator only SKIPS it from validation, so a user-context update * could overwrite audit stamps, provenance, or any other read-only column. We * STRIP the change (symmetric with `readonlyWhen`) rather than reject it, for * compatibility. * + * "Read-only" here has TWO sources, at equal rank (#5503): + * - the AUTHOR-declared `readonly: true` flag; and + * - an IMPLICITLY read-only, RUNTIME-OWNED field type + * ({@link isRuntimeOwnedField}) — today exactly `autonumber`, whose value the + * engine or the driver's persistent sequence issues. Before #5503 only the + * first source was read here, so a plain PATCH could rewrite any record + * 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` / @@ -573,7 +613,9 @@ export function stripReadonlyWhenFieldsMulti( * `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 - * audit/timestamp family or any author-declared business `readonly` field. + * audit/timestamp family or any author-declared business field (including a + * runtime-owned `autonumber`, so a migration may reinstate the legacy record + * numbers it is carrying over — #5503). * Platform-managed `system` columns outside that family (`organization_id` / * tenancy, generated columns) stay stripped, so the relaxation reinstates facts * without becoming a tenancy-forging backdoor. It is a WHITELIST, deliberately @@ -615,17 +657,99 @@ export function stripReadonlyFields( const preserveAudit = options?.preserveAudit === true; let result = data; for (const [name, def] of Object.entries(fields)) { - if (!def?.readonly) continue; + // [#5503] `readonly: true` is the AUTHOR-declared lock; a runtime-owned + // type (`autonumber`) is the IMPLICIT one. Both mean the same thing to a + // caller: you do not get to write this column. + 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 if (preserveAudit && isPreservableUnderAudit(name, def)) continue; // historical import reinstates it if (result === data) result = { ...data }; delete (result as Record)[name]; - logger?.warn?.(readonlyStripWarning(name, objectSchema?.name)); + logger?.warn?.( + def?.readonly + ? readonlyStripWarning(name, objectSchema?.name) + : runtimeOwnedStripWarning(name, String(def?.type), objectSchema?.name), + ); } return result; } +/** + * Strip CALLER-SUPPLIED writes to RUNTIME-OWNED fields ({@link + * isRuntimeOwnedField}) only — the INSERT-side counterpart of + * {@link stripReadonlyFields} (#5503). + * + * Why a separate, narrower function rather than reusing the one above: INSERT is + * deliberately exempt from the author-declared static-`readonly` strip inside + * the engine (#3413). A create may legitimately seed read-only columns, and the + * trusted internal writers (identity provisioning, the metadata repository, the + * event-log cursor) call `engine.insert` DIRECTLY — which is why that strip + * lives at the DataProtocol ingress instead (`stripReadonlyForInsert`, #3043). + * Runtime-owned fields carry none of that ambiguity: nobody may seed a record + * number on create, because the engine (or the driver's persistent sequence) + * issues it. So this one CAN live in the engine, and living there is the point — + * it runs before the payload is dispatched, which covers the SQL driver's + * `supports.autonumber` path without the driver participating at all. + * + * Same two guards as the update strip: only keys the CALLER supplied are + * candidates (a `beforeInsert` hook that computes the value survives), and the + * `preserveAudit` whitelist is honoured so a historical import may reinstate the + * legacy record numbers it is migrating. `isSystem` writes never reach here — + * the caller gates on that, exactly as it does for the update strip. + */ +export function stripRuntimeOwnedFields( + objectSchema: { name?: string; fields?: Record } | undefined | null, + data: Record | undefined | null, + suppliedKeys: ReadonlySet, + logger?: EvaluateRulesOptions['logger'], + options?: { preserveAudit?: boolean }, +): Record | undefined | null { + const fields = objectSchema?.fields; + if (!fields || !data) return data; + const preserveAudit = options?.preserveAudit === true; + let result = data; + for (const [name, def] of Object.entries(fields)) { + if (!isRuntimeOwnedField(def)) continue; + if (!(name in (result as Record))) continue; + if (!suppliedKeys.has(name)) continue; // hook/middleware stamp — keep + if (preserveAudit && isPreservableUnderAudit(name, def)) continue; // historical import reinstates it + if (result === data) result = { ...data }; + delete (result as Record)[name]; + logger?.warn?.(runtimeOwnedStripWarning(name, String(def?.type), objectSchema?.name)); + } + return result; +} + +/** + * The message the runtime-owned strip logs per dropped field (#5503). Exported + * so the pin test asserts the CONTRACT of this text rather than its wording. + * + * Deliberately distinct from {@link readonlyStripWarning}: the author never + * wrote `readonly: true` on an `autonumber` field, so "this field is read-only" + * alone reads as a bug report against their own metadata. The message has to + * name WHY the value is refused (the runtime issues it) and WHICH legitimate + * writer paths still may set it. Same `warn` level, for the same reason spelled + * out on {@link readonlyStripWarning}: this seam cannot tell a hostile forged + * body from a trusted server-side writer that simply forgot to declare itself. + */ +export function runtimeOwnedStripWarning(field: string, type: string, object?: string): string { + const on = object ? ` on '${object}'` : ''; + return ( + `Field '${field}'${on} is a runtime-owned '${type}' field: the caller-supplied value was ` + + `DROPPED and the write is being COMMITTED WITHOUT IT — the runtime issues this value from its ` + + `sequence, so the call returns success while the column holds the generated number, not the one ` + + `sent (#5503). Server-side code that legitimately sets it (seed replay, a migration) must ` + + `declare itself trusted by passing { context: { isSystem: true } }; a data import reinstating ` + + `legacy record numbers uses the historical-import context ({ context: { preserveAudit: true } }, ` + + `#3493). A beforeInsert/beforeUpdate hook does NOT need either — hook-written keys are not ` + + `caller-supplied. To detect drops programmatically instead of reading this log, pass ` + + `options.onFieldsDropped (#3407). Forged record numbers from untrusted client input are ` + + `expected here and need no action.` + ); +} + /** * The message {@link stripReadonlyFields} logs per dropped field (#4903). * Exported so the pin test asserts the CONTRACT of this text — consequence,