diff --git a/.changeset/ingress-resolved-id-wins-over-payload-id.md b/.changeset/ingress-resolved-id-wins-over-payload-id.md new file mode 100644 index 0000000000..09316d043c --- /dev/null +++ b/.changeset/ingress-resolved-id-wins-over-payload-id.md @@ -0,0 +1,42 @@ +--- +'@objectstack/service-settings': patch +'@objectstack/metadata': patch +--- + +Make the settings engine facade and the metadata database loader bind the row +they resolved, not a row the payload names + +Two ingresses resolved an authoritative row id and then folded it into the +write payload with the **losing** spread order — `{ id, ...data }` — so a +caller-supplied `data.id` spread over the id the ingress had just resolved and +silently retargeted the write: + +- `wrapEngineAsSettingsEngine`'s by-id `update` branch + (`@objectstack/service-settings`), whose id comes from the caller's + `where.id`. +- `DatabaseLoader._update` (`@objectstack/metadata`), whose id arrives as a + separate parameter every caller resolves first (`existing.id`, from the read + immediately above). + +Both now spell it `{ ...data, id }` — the operation's id **after** the spread, +so it wins. That is the convention the repo's other two ingresses already +document: `rest-server.ts`'s batch update arm ("the operation's id AFTER the +spread, so it wins") and `protocol.updateData`'s #6479 fix +(`{ ...request.data, id: request.id }`). + +**No wrong write is known to have been reachable.** Both sites' current callers +build fresh field literals and never put an `id` inside `data`, so this is +hardening a fragile pattern rather than repairing a measured defect. What makes +it worth the three characters is that neither site can be caught downstream: +both pass **no `where`** to the engine, so the payload is the only id the engine +ever sees, and the engine's conflicting-id refusal (`UPDATE_ID_MISMATCH`, 400) +needs two disagreeing declarations before it can fire. The fold is the entire +trust boundary at both sites, and it is one refactor — a caller handing back a +row copy, and rows carry `id` — from the #6479 shape. + +Both are pinned with a payload whose `id` names a **different** row than the +one the ingress resolved, asserting the resolved row is still the row bound. A +pin exercising a payload without an `id` would have passed against both +spellings. The doubles answer "which row does this bind?" with the producer's +own `assertEngineUpdateDispatch`, so they cannot be kinder about it than a +running server. diff --git a/packages/metadata/src/loaders/database-loader-update-id-fold-wins.test.ts b/packages/metadata/src/loaders/database-loader-update-id-fold-wins.test.ts new file mode 100644 index 0000000000..6e767561bb --- /dev/null +++ b/packages/metadata/src/loaders/database-loader-update-id-fold-wins.test.ts @@ -0,0 +1,149 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11231] `DatabaseLoader._update` folds its authoritative `id` PARAMETER + * into the write payload — and that id outranks a payload naming another row. + * + * ## The shape + * + * `_update(table, id, data)` takes the row address as a separate parameter — + * every caller resolves it first (`existing.id`, from the `baseFilter(type, + * name)` read just above) and then hands it here. The method folds it into the + * payload and passes **no `where`** to the engine, so the payload is the only + * id the engine sees; the engine's conflicting-id refusal + * (`UPDATE_ID_MISMATCH`, 400 — #11142/#11230) needs two disagreeing + * declarations and therefore cannot cover this site. The fold IS the trust + * boundary. + * + * Spelled `{ id, ...data }` the fold LOSES: a `data.id` spreads over the + * resolved id and the write silently retargets to a row the loader never read + * and never version-checked. Spelled `{ ...data, id }` — the convention + * documented at `rest-server.ts`'s batch arm ("the operation's id AFTER the + * spread, so it wins") and at `protocol.updateData` (#6479) — the parameter + * wins, which is what a separate id parameter MEANS. + * + * ## Why this drives `_update` directly + * + * Today's in-repo callers (`save`'s update arm, `registerRollback`) build a + * fresh field literal — `{ metadata, version, checksum, updated_at, state }` — + * so none of them can currently produce the conflict, and no public path can + * reach it. That is exactly what makes this a fragile-pattern pin rather than + * a repro: the guarded fact is the METHOD's contract, "the id parameter names + * the row", which a future caller passing a row copy (rows carry `id`) would + * otherwise break silently. Pinning it through a caller that cannot express + * the conflict would pin nothing. + * + * ## Why every case carries a CONFLICT + * + * A payload with no `id` produces the same write under BOTH spellings — such a + * case passes against the defect and measures nothing. Each case below hands + * `_update` a payload whose `id` names a DIFFERENT row than the id parameter. + * + * The bound row is not re-derived here: the double's `update` asks + * `assertEngineUpdateDispatch`, the predicate `ObjectQL.update` itself + * dispatches on, so it cannot be kinder or stricter than a running server + * (#4550/#5480, the contract `check:engine-double-contract` keeps). + */ + +import { describe, expect, it } from 'vitest'; +import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import type { IDataEngine } from '@objectstack/spec/contracts'; +import { DatabaseLoader } from './database-loader.js'; + +/** The row the loader resolved and passed as the `id` parameter. */ +const RESOLVED = 'meta_resolved'; +/** The row a payload `id` claims instead. Never the row that should be written. */ +const CLAIMED = 'meta_claimed'; + +interface SeenUpdate { + objectName: string; + data: Record; + dispatch: ReturnType; +} + +function makeRecordingEngine(seen: SeenUpdate[]): IDataEngine { + return { + async update( + objectName: string, + data: Record, + options?: Record, + ) { + const dispatch = assertEngineUpdateDispatch(data, options); + seen.push({ objectName, data, dispatch }); + return { ...data }; + }, + } as unknown as IDataEngine; +} + +/** Reach the private fold the way the loader's own callers do. */ +function updateVia( + loader: DatabaseLoader, + table: string, + id: string, + data: Record, +): Promise> { + return ( + loader as unknown as { + _update(t: string, i: string, d: Record): Promise>; + } + )._update(table, id, data); +} + +describe('[#11231] DatabaseLoader._update — the id parameter outranks a payload id', () => { + it('binds the row named by the id parameter, not the row the payload claims', async () => { + const seen: SeenUpdate[] = []; + const loader = new DatabaseLoader({ engine: makeRecordingEngine(seen) }); + + await updateVia(loader, 'sys_metadata', RESOLVED, { + id: CLAIMED, + version: 7, + state: 'active', + }); + + expect(seen).toHaveLength(1); + const [call] = seen; + + // The load-bearing assertion: which row the engine BINDS. On the losing + // spread order this reads `CLAIMED`. + expect(call.dispatch).toEqual({ kind: 'by-id', id: RESOLVED }); + expect(call.data.id).toBe(RESOLVED); + expect(call.data.id).not.toBe(CLAIMED); + }); + + it('keeps the payload’s other fields while overriding only its id', async () => { + const seen: SeenUpdate[] = []; + const loader = new DatabaseLoader({ engine: makeRecordingEngine(seen) }); + + await updateVia(loader, 'sys_metadata', RESOLVED, { + id: CLAIMED, + metadata: '{"a":1}', + version: 7, + checksum: 'abc', + state: 'active', + }); + + expect(seen[0].objectName).toBe('sys_metadata'); + expect(seen[0].data).toEqual({ + id: RESOLVED, + metadata: '{"a":1}', + version: 7, + checksum: 'abc', + state: 'active', + }); + }); + + it('writes the resolved row for the history table too', async () => { + const seen: SeenUpdate[] = []; + const loader = new DatabaseLoader({ engine: makeRecordingEngine(seen) }); + + // The fold is per-call, not per-table: the same method serves every table + // the loader writes, so the guarantee cannot be table-specific. + await updateVia(loader, 'sys_metadata_history', RESOLVED, { + id: CLAIMED, + event_seq: 3, + }); + + expect(seen[0].objectName).toBe('sys_metadata_history'); + expect(seen[0].dispatch).toEqual({ kind: 'by-id', id: RESOLVED }); + }); +}); diff --git a/packages/metadata/src/loaders/database-loader.ts b/packages/metadata/src/loaders/database-loader.ts index 70fd1b6552..761d1a6f17 100644 --- a/packages/metadata/src/loaders/database-loader.ts +++ b/packages/metadata/src/loaders/database-loader.ts @@ -273,7 +273,14 @@ export class DatabaseLoader implements MetadataLoader { private async _update(table: string, id: string, data: Record): Promise> { if (this.engine) { - return this.engine.update(table, { id, ...data }); + // [#11231] The resolved id AFTER the spread, so it WINS — the same + // convention as `rest-server.ts`'s batch arm and `protocol.updateData` + // (#6479). No `where` is passed, so the payload is the only id the + // engine sees and its conflicting-id refusal (`UPDATE_ID_MISMATCH`) has + // nothing to compare against: a `data.id` spread over the id parameter + // would retarget the write to a row no caller resolved. The separate + // `id` parameter is the row address — do not let a payload outrank it. + return this.engine.update(table, { ...data, id }); } return this.driver!.update(table, id, data); } diff --git a/packages/services/service-settings/src/settings-engine-id-fold-wins.test.ts b/packages/services/service-settings/src/settings-engine-id-fold-wins.test.ts new file mode 100644 index 0000000000..2c30f8e6f0 --- /dev/null +++ b/packages/services/service-settings/src/settings-engine-id-fold-wins.test.ts @@ -0,0 +1,157 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11231] The settings engine facade folds the id it RESOLVED into the write + * payload — and that id outranks a payload naming a different row. + * + * ## The shape + * + * `wrapEngineAsSettingsEngine`'s `update` is an ingress: it reads the row + * address out of the caller's `where.id`, then hands ObjectQL a payload with + * that id folded in. On that branch it passes **no `where`** to the engine, so + * the payload is the ONLY id the engine ever sees — which is also why the + * engine's conflicting-id refusal (`UPDATE_ID_MISMATCH`, 400 — #11142/#11230) + * cannot cover this site: a refusal needs two declarations to disagree, and + * the fold leaves exactly one. The fold IS the trust boundary here. + * + * Spelled `{ id, ...data }` the fold LOSES — a caller-supplied `data.id` + * spreads over the resolved id and silently retargets the write to a row the + * ingress never resolved, never authorised and never read. Spelled + * `{ ...data, id }` — the convention the repo's other two ingresses already + * document (`rest-server.ts`'s batch arm, "the operation's id AFTER the + * spread, so it wins", and `protocol.updateData`'s #6479 fix) — the resolved + * id wins. + * + * ## Why every case below carries a CONFLICT + * + * A payload with no `id` in it produces the same write under BOTH spellings, + * so a case shaped that way passes against the defect and measures nothing. + * Each case here hands the facade a payload whose `id` names a DIFFERENT row + * than `where.id` and asserts the engine still binds the resolved one. That is + * the ingress-level fact a future refactor would break — a caller handing back + * a row copy (rows carry `id`) is the whole population this guards. + * + * ## The bound row is not re-derived here + * + * The double's `update` asks `assertEngineUpdateDispatch` — the predicate + * `ObjectQL.update` itself dispatches on — which row a call binds, so it can + * be neither kinder nor stricter than a running server about which id wins + * (#4550/#5480, and the contract `check:engine-double-contract` keeps). + */ + +import { describe, expect, it } from 'vitest'; +import { assertEngineUpdateDispatch } from '@objectstack/objectql'; +import type { IDataEngine } from '@objectstack/spec/contracts'; +import { wrapEngineAsSettingsEngine } from './settings-service-plugin.js'; + +/** The row the ingress resolved from `where.id` — the only legitimate target. */ +const RESOLVED = 'sys_setting_resolved'; +/** The row a payload `id` claims instead. Never the row that should be written. */ +const CLAIMED = 'sys_setting_claimed'; + +interface SeenUpdate { + objectName: string; + data: Record; + options: Record | undefined; + /** Which row the REAL engine would bind for this call. */ + dispatch: ReturnType; +} + +/** + * An engine double that records the call and answers "which row?" with the + * producer's verdict rather than a hand-written re-reading of the ladder. + */ +function makeRecordingEngine(seen: SeenUpdate[]): IDataEngine { + return { + async update( + objectName: string, + data: Record, + options?: Record, + ) { + const dispatch = assertEngineUpdateDispatch(data, options); + seen.push({ objectName, data, options, dispatch }); + return { ...data }; + }, + } as unknown as IDataEngine; +} + +describe('[#11231] wrapEngineAsSettingsEngine — the resolved id outranks a payload id', () => { + it('binds the row `where.id` resolved, not the row the payload claims', async () => { + const seen: SeenUpdate[] = []; + const wrapped = wrapEngineAsSettingsEngine(makeRecordingEngine(seen)); + + await wrapped.update('sys_setting', { + where: { id: RESOLVED }, + data: { id: CLAIMED, value: 'rotated' }, + }); + + expect(seen).toHaveLength(1); + const [call] = seen; + + // The load-bearing assertion: the row the engine BINDS. On the losing + // spread order this reads `CLAIMED`. + expect(call.dispatch).toEqual({ kind: 'by-id', id: RESOLVED }); + + // …and the payload the facade actually handed over carries the resolved + // id, so nothing downstream of the engine can re-derive the claimed one. + expect(call.data.id).toBe(RESOLVED); + expect(call.data.id).not.toBe(CLAIMED); + }); + + it('keeps the payload’s other fields while overriding only its id', async () => { + const seen: SeenUpdate[] = []; + const wrapped = wrapEngineAsSettingsEngine(makeRecordingEngine(seen)); + + await wrapped.update('sys_setting', { + where: { id: RESOLVED }, + data: { id: CLAIMED, value: 'rotated', updated_by: 'admin' }, + }); + + // The fix overrides the id and nothing else — a fold that dropped caller + // fields would be a different defect wearing this one's fix. + expect(seen[0].data).toEqual({ + id: RESOLVED, + value: 'rotated', + updated_by: 'admin', + }); + }); + + it('forwards context and bypassTenantAudit alongside the winning id (#8030)', async () => { + const seen: SeenUpdate[] = []; + const wrapped = wrapEngineAsSettingsEngine(makeRecordingEngine(seen)); + + await wrapped.update('sys_setting', { + where: { id: RESOLVED }, + data: { id: CLAIMED, value_enc: 'new-handle' }, + bypassTenantAudit: true, + context: { isSystem: true }, + }); + + expect(seen[0].dispatch).toEqual({ kind: 'by-id', id: RESOLVED }); + // The by-id branch still forwards both driver options — the #8030 fix is + // on the same three lines this card edits, so it is pinned beside it. + expect(seen[0].options).toEqual({ + bypassTenantAudit: true, + context: { isSystem: true }, + }); + }); + + it('leaves the multi branch addressing by `where`, with no id folded in', async () => { + const seen: SeenUpdate[] = []; + const wrapped = wrapEngineAsSettingsEngine(makeRecordingEngine(seen)); + + // The settings row write takes this branch in practice: its `where` is the + // composite (namespace, key, scope, user_id) and carries no id at all. + await wrapped.update('sys_setting', { + where: { namespace: 'mail', key: 'smtp_host', scope: 'global' }, + data: { value: 'smtp.example.com' }, + }); + + expect(seen[0].dispatch).toEqual({ kind: 'multi' }); + expect(seen[0].data).not.toHaveProperty('id'); + expect(seen[0].options).toMatchObject({ + where: { namespace: 'mail', key: 'smtp_host', scope: 'global' }, + multi: true, + }); + }); +}); diff --git a/packages/services/service-settings/src/settings-service-plugin.ts b/packages/services/service-settings/src/settings-service-plugin.ts index 5be6a27110..daaa2b21c1 100644 --- a/packages/services/service-settings/src/settings-service-plugin.ts +++ b/packages/services/service-settings/src/settings-service-plugin.ts @@ -466,7 +466,14 @@ export function wrapEngineAsSettingsEngine(engine: IDataEngine): SettingsEngine }; const id = (where as any)?.id; if (id !== undefined && id !== null) { - return eng.update(objectName, { id, ...data }, driverOpts); + // [#11231] The operation's id AFTER the spread, so it WINS — the same + // convention as `rest-server.ts`'s batch arm and `protocol.updateData` + // (#6479). This branch passes no `where` to the engine, so the payload + // is the only id the engine sees and its conflicting-id refusal + // (`UPDATE_ID_MISMATCH`) has nothing to compare against: a + // caller-supplied `data.id` spread over the resolved id would retarget + // the write silently. Do not flip this back to `{ id, ...data }`. + return eng.update(objectName, { ...data, id }, driverOpts); } return eng.update(objectName, data, { where,