From cb325c62203fc0af272c17382938458d9687b6a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 00:49:13 +0000 Subject: [PATCH 1/2] feat(objectql,plugin-audit): refuse an unscoped multi-UPDATE on the shape (#9974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend #9719's whole-operation dispatch to `beforeUpdate`'s predicate path, per the maintainer's option-A ruling of 2026-08-19, and generalize the flag (`dispatchUnscopedMultiDelete` -> `dispatchUnscopedMultiWrite`) instead of adding a sibling: it is already per-registration and per-event, so a delete-only guard still says "delete only". The #4630 refusal in `resolveTargetRows` now fires on the SHAPE — no id and no `where` — on both write verbs. Previously an unscoped multi-update was refused only when it happened to sweep a row the caller lacked rights to, with the per-row message, and resolved silently when the caller owned every row. The three MEASURED-GAP pins are REPLACED with refusal assertions, not relaxed. Objects whose guards do not declare the flag are unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM --- ...ine-unscoped-multi-delete-dispatch.test.ts | 381 ---------- ...gine-unscoped-multi-write-dispatch.test.ts | 716 ++++++++++++++++++ packages/objectql/src/engine.ts | 205 +++-- .../objectql/src/hook-target-rebind-errors.ts | 24 +- .../src/comment-access-hooks.test.ts | 191 ++++- .../plugin-audit/src/comment-access-hooks.ts | 78 +- .../src/attachment-access-hooks.test.ts | 2 +- .../src/attachment-access-hooks.ts | 15 +- .../src/attachment-lifecycle.ts | 14 +- 9 files changed, 1083 insertions(+), 543 deletions(-) delete mode 100644 packages/objectql/src/engine-unscoped-multi-delete-dispatch.test.ts create mode 100644 packages/objectql/src/engine-unscoped-multi-write-dispatch.test.ts diff --git a/packages/objectql/src/engine-unscoped-multi-delete-dispatch.test.ts b/packages/objectql/src/engine-unscoped-multi-delete-dispatch.test.ts deleted file mode 100644 index fa748edac8..0000000000 --- a/packages/objectql/src/engine-unscoped-multi-delete-dispatch.test.ts +++ /dev/null @@ -1,381 +0,0 @@ -// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. - -/** - * [#9719] The opt-in whole-operation `beforeDelete` dispatch for an UNSCOPED - * predicate delete — `multi: true` carrying no caller `where` at all. - * - * Why the engine needs it, measured on #9719: the per-row contract - * (#5038 / #5574) binds `input.id` on every predicate dispatch, so a handler - * guarding the OPERATION SHAPE (the #4757 predicate-less multi-delete refusal - * on `sys_attachment`) always took its by-id branch — and a zero-match - * predicate dispatches nothing at all ([D1]), so the guard never ran on - * exactly the shape it refuses. Both unreachability limbs need one dispatch - * that happens BEFORE the matched-row read, keyed on the operation's shape. - * - * Pinned here, against the REAL engine: - * 1. the dispatch CONDITION — `where` absent or `null` is unscoped; - * `where: {}` and a real predicate are scoped and get NO extra dispatch; - * 2. the dispatched SHAPE — whole-operation: `input.id` undefined, the - * caller's raw `options` (the `hook.zod.ts` upper-bound read), - * `dispatch.mode === 'record'`, the batch `scope` identity-shared; - * 3. ORDERING — a refusal from the flagged handler rejects the delete - * before the doomed-row read and before `deleteMany`: zero driver calls; - * 4. the ZERO-MATCH limb — an unscoped delete of an EMPTY table still - * dispatches (with a positive control proving the measurement is not - * vacuous); - * 5. NEUTRALITY — undeclared registrations and by-id deletes see no new - * dispatch: today's behaviour for every other object is unchanged; - * 6. the retired-lever rule — binding `input.id` on the whole-operation - * context is refused (`HookTargetRebindError`, path `'unscoped-multi'`), - * never silently ignored; - * 7. the registration-time refusal of the flag on any event whose dispatch - * never reads it (ADR-0078: no silently inert declaration). - * - * The consumer half — the #4757 refusal itself, wired end-to-end — is pinned - * in `packages/services/service-storage/src/attachment-access-hooks.test.ts`. - */ - -import { describe, it, expect } from 'vitest'; -import { ObjectQL } from './engine.js'; -import { HOOK_TARGET_REBIND_ERROR_CODE } from './hook-target-rebind-errors.js'; -import type { HookContext } from '@objectstack/spec/data'; - -const FIELDS = { - id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, - status: { name: 'status', label: 'Status', type: 'text' as const }, - owner: { name: 'owner', label: 'Owner', type: 'text' as const }, -}; -const attObject = { name: 'att', label: 'Att', fields: FIELDS }; -const taskObject = { name: 'task', label: 'Task', fields: FIELDS }; - -/** - * Minimal in-memory driver. Its WHERE matcher REFUSES combinators and - * operator objects by throwing (the conforming shape - * `check-where-matcher-conformance.mjs` documents): a double that answers an - * operator silently wrong makes a suite green on a different query. - */ -function makeStubDriver() { - const stores = new Map>>(); - const storeFor = (o: string) => { - let s = stores.get(o); - if (!s) { s = new Map(); stores.set(o, s); } - return s; - }; - const matches = (row: Record, where: unknown): boolean => { - if (!where || typeof where !== 'object') return true; - for (const [k, v] of Object.entries(where)) { - if (k.startsWith('$')) throw new Error(`stub driver: unsupported combinator ${k}`); - if (v !== null && typeof v === 'object') throw new Error(`stub driver: unsupported operator value on ${k}`); - if ((row[k] ?? null) !== (v ?? null)) return false; - } - return true; - }; - const d: any = { - name: 'memory', version: '0.0.0', supports: {}, - stores, - /** Every read/write the engine issued, in order — the ordering pins read it. */ - calls: [] as string[], - async connect() {}, async disconnect() {}, async checkHealth() { return true; }, - async execute() { return null; }, async syncSchema() {}, - async find(o: string, ast: any) { - d.calls.push(`find:${o}`); - return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); - }, - async findOne(o: string, ast: any) { - d.calls.push(`findOne:${o}`); - for (const r of storeFor(o).values()) if (matches(r, ast?.where)) return r; - return null; - }, - async create(o: string, data: Record) { - const id = String(data.id); - const row = { ...data, id }; - storeFor(o).set(id, row); - return row; - }, - async update() { return null; }, - async delete(o: string, id: string) { d.calls.push(`delete:${o}`); return storeFor(o).delete(String(id)); }, - async count(o: string, ast: any) { - return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)).length; - }, - async deleteMany(o: string, ast: any) { - d.calls.push(`deleteMany:${o}`); - const doomed = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); - for (const r of doomed) storeFor(o).delete(String(r.id)); - return doomed.length; - }, - async updateMany() { return 0; }, - }; - return d; -} - -async function boot() { - const engine = new ObjectQL(); - const driver = makeStubDriver(); - engine.registerDriver(driver, true); - await engine.init(); - engine.registry.registerObject(attObject as any, 'app:test'); - engine.registry.registerObject(taskObject as any, 'app:test'); - return { engine, driver }; -} - -/** Seed rows straight into the driver store — no insert hooks involved. */ -function seed(driver: any, object: string, rows: Array>) { - if (!driver.stores.get(object)) driver.stores.set(object, new Map()); - for (const row of rows) driver.stores.get(object)!.set(String(row.id), { ...row }); -} - -const rowsIn = (driver: any, object: string): number => driver.stores.get(object)?.size ?? 0; - -type Seen = { id: unknown; where: unknown; multi: unknown; mode: unknown; index: unknown }; -const snapshot = (ctx: HookContext): Seen => ({ - id: (ctx.input as any).id, - where: (ctx.input as any).options?.where, - multi: (ctx.input as any).options?.multi, - mode: (ctx.dispatch as any)?.mode, - index: (ctx.dispatch as any)?.index, -}); - -describe('[#9719] registration-time validation of dispatchUnscopedMultiDelete', () => { - it("refuses the flag on any event other than 'beforeDelete'", async () => { - const { engine } = await boot(); - for (const event of ['beforeUpdate', 'afterDelete', 'beforeInsert']) { - expect(() => - engine.registerHook(event, async () => {}, { - object: 'att', - dispatchUnscopedMultiDelete: true, - }), - ).toThrow(/dispatchUnscopedMultiDelete/); - } - }); - - it("accepts the flag on 'beforeDelete'", async () => { - const { engine } = await boot(); - expect(() => - engine.registerHook('beforeDelete', async () => {}, { - object: 'att', - dispatchUnscopedMultiDelete: true, - }), - ).not.toThrow(); - }); -}); - -describe('[#9719] the whole-operation dispatch on an unscoped predicate delete', () => { - it('dispatches ONCE, whole-operation-shaped, before the per-row fan-out', async () => { - const { engine, driver } = await boot(); - seed(driver, 'att', [ - { id: 'a1', status: 'x', owner: 'u1' }, - { id: 'a2', status: 'y', owner: 'u1' }, - ]); - const seen: Seen[] = []; - engine.registerHook( - 'beforeDelete', - async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, - { object: 'att', dispatchUnscopedMultiDelete: true }, - ); - - await engine.delete('att', { multi: true, context: { userId: 'u1' } } as any); - - // 1 whole-operation dispatch + 2 per-row dispatches, in that order. - expect(seen).toHaveLength(3); - expect(seen[0]).toMatchObject({ id: undefined, where: undefined, multi: true, mode: 'record', index: 0 }); - expect([seen[1]!.id, seen[2]!.id].sort()).toEqual(['a1', 'a2']); - expect(seen[1]!.mode).toBe('per-row'); - // The handler that let it pass did not stop the wipe — engine policy stays - // with the handler, not the dispatch. - expect(rowsIn(driver, 'att')).toBe(0); - }); - - it('a refusal from the flagged handler rejects the delete BEFORE any driver call', async () => { - const { engine, driver } = await boot(); - seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); - engine.registerHook( - 'beforeDelete', - async (ctx: HookContext) => { - if ((ctx.input as any).id === undefined) { - const err: any = new Error('unscoped delete refused by test guard'); - err.code = 'TEST_UNSCOPED_REFUSED'; - err.status = 403; - throw err; - } - }, - { object: 'att', dispatchUnscopedMultiDelete: true }, - ); - - await expect(engine.delete('att', { multi: true, context: { userId: 'u1' } } as any)) - .rejects.toMatchObject({ code: 'TEST_UNSCOPED_REFUSED', status: 403 }); - // No doomed-row read, no deleteMany: the refusal cost nothing downstream. - expect(driver.calls).toEqual([]); - expect(rowsIn(driver, 'att')).toBe(1); - }); - - it('the ZERO-MATCH limb: an unscoped delete of an EMPTY table still dispatches', async () => { - const { engine, driver } = await boot(); - // No rows at all — the [D1] per-row gate would dispatch nothing. - let dispatched = 0; - engine.registerHook( - 'beforeDelete', - async (ctx: HookContext) => { - if ((ctx.input as any).id === undefined) { - dispatched += 1; - const err: any = new Error('unscoped delete refused by test guard'); - err.code = 'TEST_UNSCOPED_REFUSED'; - err.status = 403; - throw err; - } - }, - { object: 'att', dispatchUnscopedMultiDelete: true }, - ); - - await expect(engine.delete('att', { multi: true, context: { userId: 'u1' } } as any)) - .rejects.toMatchObject({ code: 'TEST_UNSCOPED_REFUSED' }); - expect(dispatched).toBe(1); - expect(driver.calls).toEqual([]); - }); - - it('positive control for the zero-match limb: WITHOUT the flag, the same shape dispatches nothing', async () => { - const { engine, driver } = await boot(); - let dispatched = 0; - engine.registerHook( - 'beforeDelete', - async () => { dispatched += 1; }, - { object: 'att' }, // same handler, same object — flag withheld - ); - - // Resolves: zero rows matched, zero per-row dispatches ([D1]), and no - // whole-operation dispatch without the declaration. This is exactly the - // pre-#9719 wired behaviour, so the zero-match measurement above is a - // measurement of the flag, not of the harness. - await expect(engine.delete('att', { multi: true, context: { userId: 'u1' } } as any)) - .resolves.toBeDefined(); - expect(dispatched).toBe(0); - expect(driver.calls).toContain('deleteMany:att'); - }); - - it("dispatches on `where: null` — the handler contract's other unscoped spelling", async () => { - const { engine, driver } = await boot(); - seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); - const seen: Seen[] = []; - engine.registerHook( - 'beforeDelete', - async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, - { object: 'att', dispatchUnscopedMultiDelete: true }, - ); - - await engine.delete('att', { multi: true, where: null, context: { userId: 'u1' } } as any); - expect(seen[0]).toMatchObject({ id: undefined, where: null, multi: true, mode: 'record' }); - }); - - it('the whole-operation `scope` is identity-shared with the per-row contexts', async () => { - const { engine, driver } = await boot(); - seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); - const perRowSawMarker: unknown[] = []; - engine.registerHook( - 'beforeDelete', - async (ctx: HookContext) => { - const scope = (ctx.dispatch as any)?.scope as Record; - if ((ctx.input as any).id === undefined) scope.marker = 'from-whole-op'; - else perRowSawMarker.push(scope.marker); - }, - { object: 'att', dispatchUnscopedMultiDelete: true }, - ); - - await engine.delete('att', { multi: true, context: { userId: 'u1' } } as any); - expect(perRowSawMarker).toEqual(['from-whole-op']); - }); -}); - -describe('[#9719] scoped deletes and undeclared registrations see NO new dispatch', () => { - it('a real `where` predicate gets only the per-row fan-out', async () => { - const { engine, driver } = await boot(); - seed(driver, 'att', [ - { id: 'a1', status: 'x', owner: 'u1' }, - { id: 'a2', status: 'y', owner: 'u1' }, - ]); - const seen: Seen[] = []; - engine.registerHook( - 'beforeDelete', - async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, - { object: 'att', dispatchUnscopedMultiDelete: true }, - ); - - await engine.delete('att', { multi: true, where: { status: 'x' }, context: { userId: 'u1' } } as any); - expect(seen).toHaveLength(1); - expect(seen[0]).toMatchObject({ id: 'a1', mode: 'per-row' }); - expect(rowsIn(driver, 'att')).toBe(1); - }); - - it('`where: {}` is a REAL match-all query, not an unscoped delete', async () => { - const { engine, driver } = await boot(); - seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); - const seen: Seen[] = []; - engine.registerHook( - 'beforeDelete', - async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, - { object: 'att', dispatchUnscopedMultiDelete: true }, - ); - - await engine.delete('att', { multi: true, where: {}, context: { userId: 'u1' } } as any); - // Per-row only: the guard's whole-operation branch is not summoned for a - // query that really ran and really matched. - expect(seen).toHaveLength(1); - expect(seen[0]).toMatchObject({ id: 'a1', mode: 'per-row' }); - expect(rowsIn(driver, 'att')).toBe(0); - }); - - it('a by-id delete is untouched', async () => { - const { engine, driver } = await boot(); - seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); - const seen: Seen[] = []; - engine.registerHook( - 'beforeDelete', - async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, - { object: 'att', dispatchUnscopedMultiDelete: true }, - ); - - await engine.delete('att', { where: { id: 'a1' }, context: { userId: 'u1' } } as any); - expect(seen).toHaveLength(1); - expect(seen[0]!.id).toBe('a1'); - expect(seen[0]!.mode).toBe('record'); - expect(rowsIn(driver, 'att')).toBe(0); - }); - - it('an object whose registration does NOT declare the flag keeps exactly today\'s dispatches', async () => { - const { engine, driver } = await boot(); - seed(driver, 'task', [ - { id: 't1', status: 'x', owner: 'u1' }, - { id: 't2', status: 'y', owner: 'u1' }, - ]); - const seen: Seen[] = []; - engine.registerHook( - 'beforeDelete', - async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, - { object: 'task' }, - ); - - await engine.delete('task', { multi: true, context: { userId: 'u1' } } as any); - // Per-row dispatches only — no whole-operation call arrived, and the - // unscoped wipe proceeds as it does today for undeclared objects. - expect(seen).toHaveLength(2); - expect(seen.every((s) => s.id !== undefined && s.mode === 'per-row')).toBe(true); - expect(rowsIn(driver, 'task')).toBe(0); - }); -}); - -describe('[#9719] the id slot is not a lever on the whole-operation context', () => { - it('binding `input.id` is refused with HookTargetRebindError, nothing deleted', async () => { - const { engine, driver } = await boot(); - seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); - engine.registerHook( - 'beforeDelete', - async (ctx: HookContext) => { - if ((ctx.input as any).id === undefined) (ctx.input as any).id = 'a1'; - }, - { object: 'att', dispatchUnscopedMultiDelete: true }, - ); - - await expect(engine.delete('att', { multi: true, context: { userId: 'u1' } } as any)) - .rejects.toMatchObject({ code: HOOK_TARGET_REBIND_ERROR_CODE, path: 'unscoped-multi' }); - expect(driver.calls).toEqual([]); - expect(rowsIn(driver, 'att')).toBe(1); - }); -}); diff --git a/packages/objectql/src/engine-unscoped-multi-write-dispatch.test.ts b/packages/objectql/src/engine-unscoped-multi-write-dispatch.test.ts new file mode 100644 index 0000000000..acbcd5a53e --- /dev/null +++ b/packages/objectql/src/engine-unscoped-multi-write-dispatch.test.ts @@ -0,0 +1,716 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#9719, both write verbs since #9974] The opt-in whole-operation + * `beforeUpdate` / `beforeDelete` dispatch for an UNSCOPED predicate write — + * `multi: true` carrying no caller `where` at all. + * + * Why the engine needs it, measured on #9719: the per-row contract + * (#5038 / #5574) binds `input.id` on every predicate dispatch, so a handler + * guarding the OPERATION SHAPE (the #4757 predicate-less multi-delete refusal + * on `sys_attachment`) always took its by-id branch — and a zero-match + * predicate dispatches nothing at all ([D1]), so the guard never ran on + * exactly the shape it refuses. Both unreachability limbs need one dispatch + * that happens BEFORE the matched-row read, keyed on the operation's shape. + * + * #9974 (maintainer ruling, option A, 2026-08-19) extended the mechanism to + * `beforeUpdate`'s predicate path and RENAMED the flag accordingly + * (`dispatchUnscopedMultiDelete` → `dispatchUnscopedMultiWrite`). One flag + * rather than a sibling: it is already per-registration and per-event, so a + * delete-only guard still says "delete only" by declaring it on `beforeDelete` + * alone. The update half is pinned below with the SAME seven properties, and + * with its own neutrality controls — widening what an engine REFUSES is the + * half a suite must not take on trust. + * + * Pinned here, against the REAL engine, for EACH write verb: + * 1. the dispatch CONDITION — `where` absent or `null` is unscoped; + * `where: {}` and a real predicate are scoped and get NO extra dispatch; + * 2. the dispatched SHAPE — whole-operation: `input.id` undefined, the + * caller's raw `options` (the `hook.zod.ts` upper-bound read), + * `dispatch.mode === 'record'`, the batch `scope` identity-shared; + * 3. ORDERING — a refusal from the flagged handler rejects the write + * before the matched-row read and before `deleteMany`/`updateMany`: zero + * driver calls; + * 4. the ZERO-MATCH limb — an unscoped write against an EMPTY table still + * dispatches (with a positive control proving the measurement is not + * vacuous); + * 5. NEUTRALITY — undeclared registrations and by-id writes see no new + * dispatch: today's behaviour for every other object is unchanged; + * 6. the retired-lever rule — binding `input.id` on the whole-operation + * context is refused (`HookTargetRebindError`, path `'unscoped-multi'`), + * never silently ignored; + * 7. the registration-time refusal of the flag on any event whose dispatch + * never reads it (ADR-0078: no silently inert declaration). + * + * The consumer halves — the #4757 refusal wired end-to-end — are pinned in + * `packages/services/service-storage/src/attachment-access-hooks.test.ts` + * (delete) and, for #4630's `sys_comment` twin on BOTH verbs, in + * `packages/plugins/plugin-audit/src/comment-access-hooks.test.ts`. + */ + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { HOOK_TARGET_REBIND_ERROR_CODE } from './hook-target-rebind-errors.js'; +import type { HookContext } from '@objectstack/spec/data'; + +const FIELDS = { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + status: { name: 'status', label: 'Status', type: 'text' as const }, + owner: { name: 'owner', label: 'Owner', type: 'text' as const }, +}; +const attObject = { name: 'att', label: 'Att', fields: FIELDS }; +const taskObject = { name: 'task', label: 'Task', fields: FIELDS }; + +/** + * Minimal in-memory driver. Its WHERE matcher REFUSES combinators and + * operator objects by throwing (the conforming shape + * `check-where-matcher-conformance.mjs` documents): a double that answers an + * operator silently wrong makes a suite green on a different query. + */ +function makeStubDriver() { + const stores = new Map>>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + const matches = (row: Record, where: unknown): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) throw new Error(`stub driver: unsupported combinator ${k}`); + if (v !== null && typeof v === 'object') throw new Error(`stub driver: unsupported operator value on ${k}`); + if ((row[k] ?? null) !== (v ?? null)) return false; + } + return true; + }; + const d: any = { + name: 'memory', version: '0.0.0', supports: {}, + stores, + /** Every read/write the engine issued, in order — the ordering pins read it. */ + calls: [] as string[], + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async find(o: string, ast: any) { + d.calls.push(`find:${o}`); + return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(o: string, ast: any) { + d.calls.push(`findOne:${o}`); + for (const r of storeFor(o).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(o: string, data: Record) { + const id = String(data.id); + const row = { ...data, id }; + storeFor(o).set(id, row); + return row; + }, + async update(o: string, id: string, data: Record) { + d.calls.push(`update:${o}`); + const row = storeFor(o).get(String(id)); + if (!row) return null; + const next = { ...row, ...data, id: String(id) }; + storeFor(o).set(String(id), next); + return next; + }, + async delete(o: string, id: string) { d.calls.push(`delete:${o}`); return storeFor(o).delete(String(id)); }, + async count(o: string, ast: any) { + return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)).length; + }, + async deleteMany(o: string, ast: any) { + d.calls.push(`deleteMany:${o}`); + const doomed = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + for (const r of doomed) storeFor(o).delete(String(r.id)); + return doomed.length; + }, + async updateMany(o: string, ast: any, data: Record) { + d.calls.push(`updateMany:${o}`); + const hit = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + for (const r of hit) storeFor(o).set(String(r.id), { ...r, ...data, id: String(r.id) }); + return hit.length; + }, + }; + return d; +} + +async function boot() { + const engine = new ObjectQL(); + const driver = makeStubDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(attObject as any, 'app:test'); + engine.registry.registerObject(taskObject as any, 'app:test'); + return { engine, driver }; +} + +/** Seed rows straight into the driver store — no insert hooks involved. */ +function seed(driver: any, object: string, rows: Array>) { + if (!driver.stores.get(object)) driver.stores.set(object, new Map()); + for (const row of rows) driver.stores.get(object)!.set(String(row.id), { ...row }); +} + +const rowsIn = (driver: any, object: string): number => driver.stores.get(object)?.size ?? 0; + +type Seen = { id: unknown; where: unknown; multi: unknown; mode: unknown; index: unknown }; +const snapshot = (ctx: HookContext): Seen => ({ + id: (ctx.input as any).id, + where: (ctx.input as any).options?.where, + multi: (ctx.input as any).options?.multi, + mode: (ctx.dispatch as any)?.mode, + index: (ctx.dispatch as any)?.index, +}); + +describe('[#9719/#9974] registration-time validation of dispatchUnscopedMultiWrite', () => { + it('refuses the flag on every event whose dispatch does not read it', async () => { + const { engine } = await boot(); + // `beforeUpdate` is deliberately ABSENT from this list since #9974 — it + // moved to the accept case below by ruling, not by relaxing the assert. + // Everything still here would register "successfully" and never fire. + for (const event of ['afterDelete', 'afterUpdate', 'beforeInsert', 'beforeFind']) { + expect(() => + engine.registerHook(event, async () => {}, { + object: 'att', + dispatchUnscopedMultiWrite: true, + }), + ).toThrow(/dispatchUnscopedMultiWrite/); + } + }); + + it("accepts the flag on both write verbs — 'beforeDelete' and 'beforeUpdate'", async () => { + const { engine } = await boot(); + for (const event of ['beforeDelete', 'beforeUpdate']) { + expect(() => + engine.registerHook(event, async () => {}, { + object: 'att', + dispatchUnscopedMultiWrite: true, + }), + ).not.toThrow(); + } + }); + + it('names the events it DOES read in the refusal, so the message is actionable', async () => { + const { engine } = await boot(); + expect(() => + engine.registerHook('afterInsert', async () => {}, { + object: 'att', + dispatchUnscopedMultiWrite: true, + }), + ).toThrow(/'beforeUpdate' \/ 'beforeDelete'/); + }); +}); + +describe('[#9719] the whole-operation dispatch on an unscoped predicate delete', () => { + it('dispatches ONCE, whole-operation-shaped, before the per-row fan-out', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [ + { id: 'a1', status: 'x', owner: 'u1' }, + { id: 'a2', status: 'y', owner: 'u1' }, + ]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.delete('att', { multi: true, context: { userId: 'u1' } } as any); + + // 1 whole-operation dispatch + 2 per-row dispatches, in that order. + expect(seen).toHaveLength(3); + expect(seen[0]).toMatchObject({ id: undefined, where: undefined, multi: true, mode: 'record', index: 0 }); + expect([seen[1]!.id, seen[2]!.id].sort()).toEqual(['a1', 'a2']); + expect(seen[1]!.mode).toBe('per-row'); + // The handler that let it pass did not stop the wipe — engine policy stays + // with the handler, not the dispatch. + expect(rowsIn(driver, 'att')).toBe(0); + }); + + it('a refusal from the flagged handler rejects the delete BEFORE any driver call', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { + if ((ctx.input as any).id === undefined) { + const err: any = new Error('unscoped delete refused by test guard'); + err.code = 'TEST_UNSCOPED_REFUSED'; + err.status = 403; + throw err; + } + }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await expect(engine.delete('att', { multi: true, context: { userId: 'u1' } } as any)) + .rejects.toMatchObject({ code: 'TEST_UNSCOPED_REFUSED', status: 403 }); + // No doomed-row read, no deleteMany: the refusal cost nothing downstream. + expect(driver.calls).toEqual([]); + expect(rowsIn(driver, 'att')).toBe(1); + }); + + it('the ZERO-MATCH limb: an unscoped delete of an EMPTY table still dispatches', async () => { + const { engine, driver } = await boot(); + // No rows at all — the [D1] per-row gate would dispatch nothing. + let dispatched = 0; + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { + if ((ctx.input as any).id === undefined) { + dispatched += 1; + const err: any = new Error('unscoped delete refused by test guard'); + err.code = 'TEST_UNSCOPED_REFUSED'; + err.status = 403; + throw err; + } + }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await expect(engine.delete('att', { multi: true, context: { userId: 'u1' } } as any)) + .rejects.toMatchObject({ code: 'TEST_UNSCOPED_REFUSED' }); + expect(dispatched).toBe(1); + expect(driver.calls).toEqual([]); + }); + + it('positive control for the zero-match limb: WITHOUT the flag, the same shape dispatches nothing', async () => { + const { engine, driver } = await boot(); + let dispatched = 0; + engine.registerHook( + 'beforeDelete', + async () => { dispatched += 1; }, + { object: 'att' }, // same handler, same object — flag withheld + ); + + // Resolves: zero rows matched, zero per-row dispatches ([D1]), and no + // whole-operation dispatch without the declaration. This is exactly the + // pre-#9719 wired behaviour, so the zero-match measurement above is a + // measurement of the flag, not of the harness. + await expect(engine.delete('att', { multi: true, context: { userId: 'u1' } } as any)) + .resolves.toBeDefined(); + expect(dispatched).toBe(0); + expect(driver.calls).toContain('deleteMany:att'); + }); + + it("dispatches on `where: null` — the handler contract's other unscoped spelling", async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.delete('att', { multi: true, where: null, context: { userId: 'u1' } } as any); + expect(seen[0]).toMatchObject({ id: undefined, where: null, multi: true, mode: 'record' }); + }); + + it('the whole-operation `scope` is identity-shared with the per-row contexts', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + const perRowSawMarker: unknown[] = []; + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { + const scope = (ctx.dispatch as any)?.scope as Record; + if ((ctx.input as any).id === undefined) scope.marker = 'from-whole-op'; + else perRowSawMarker.push(scope.marker); + }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.delete('att', { multi: true, context: { userId: 'u1' } } as any); + expect(perRowSawMarker).toEqual(['from-whole-op']); + }); +}); + +describe('[#9719] scoped deletes and undeclared registrations see NO new dispatch', () => { + it('a real `where` predicate gets only the per-row fan-out', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [ + { id: 'a1', status: 'x', owner: 'u1' }, + { id: 'a2', status: 'y', owner: 'u1' }, + ]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.delete('att', { multi: true, where: { status: 'x' }, context: { userId: 'u1' } } as any); + expect(seen).toHaveLength(1); + expect(seen[0]).toMatchObject({ id: 'a1', mode: 'per-row' }); + expect(rowsIn(driver, 'att')).toBe(1); + }); + + it('`where: {}` is a REAL match-all query, not an unscoped delete', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.delete('att', { multi: true, where: {}, context: { userId: 'u1' } } as any); + // Per-row only: the guard's whole-operation branch is not summoned for a + // query that really ran and really matched. + expect(seen).toHaveLength(1); + expect(seen[0]).toMatchObject({ id: 'a1', mode: 'per-row' }); + expect(rowsIn(driver, 'att')).toBe(0); + }); + + it('a by-id delete is untouched', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.delete('att', { where: { id: 'a1' }, context: { userId: 'u1' } } as any); + expect(seen).toHaveLength(1); + expect(seen[0]!.id).toBe('a1'); + expect(seen[0]!.mode).toBe('record'); + expect(rowsIn(driver, 'att')).toBe(0); + }); + + it('an object whose registration does NOT declare the flag keeps exactly today\'s dispatches', async () => { + const { engine, driver } = await boot(); + seed(driver, 'task', [ + { id: 't1', status: 'x', owner: 'u1' }, + { id: 't2', status: 'y', owner: 'u1' }, + ]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'task' }, + ); + + await engine.delete('task', { multi: true, context: { userId: 'u1' } } as any); + // Per-row dispatches only — no whole-operation call arrived, and the + // unscoped wipe proceeds as it does today for undeclared objects. + expect(seen).toHaveLength(2); + expect(seen.every((s) => s.id !== undefined && s.mode === 'per-row')).toBe(true); + expect(rowsIn(driver, 'task')).toBe(0); + }); +}); + +describe('[#9719] the id slot is not a lever on the whole-operation context', () => { + it('binding `input.id` is refused with HookTargetRebindError, nothing deleted', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { + if ((ctx.input as any).id === undefined) (ctx.input as any).id = 'a1'; + }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await expect(engine.delete('att', { multi: true, context: { userId: 'u1' } } as any)) + .rejects.toMatchObject({ code: HOOK_TARGET_REBIND_ERROR_CODE, path: 'unscoped-multi' }); + expect(driver.calls).toEqual([]); + expect(rowsIn(driver, 'att')).toBe(1); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// [#9974] The UPDATE half — the same seven properties on `beforeUpdate`. +// +// Ruled in on 2026-08-19 (option A) on the recoverability asymmetry: a delete +// leaves a trace of who removed what, an overwrite leaves none — the old value +// is gone on the spot with nothing to restore from — and a forgotten `where` is +// the mistake generated code makes most often. So the verb whose failure is +// LESS recoverable was the one left LESS guarded, and that is what closes here. +// +// ⚠️ This block pins a WIDENED REFUSAL, so its neutrality controls carry more +// weight than the delete block's did: an unscoped `multi: true` update that +// succeeds today starts failing for any object whose guard declares the flag, +// and for NO other object. Both halves of that are asserted. +// ═══════════════════════════════════════════════════════════════════════════ + +const bodyOf = (driver: any, object: string, id: string): unknown => + driver.stores.get(object)?.get(id)?.status; + +describe('[#9974] the whole-operation dispatch on an unscoped predicate update', () => { + it('dispatches ONCE, whole-operation-shaped, before the per-row fan-out', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [ + { id: 'a1', status: 'x', owner: 'u1' }, + { id: 'a2', status: 'y', owner: 'u1' }, + ]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.update('att', { status: 'z' }, { multi: true, context: { userId: 'u1' } } as any); + + // 1 whole-operation dispatch + 2 per-row dispatches, in that order. + expect(seen).toHaveLength(3); + expect(seen[0]).toMatchObject({ id: undefined, where: undefined, multi: true, mode: 'record', index: 0 }); + expect([seen[1]!.id, seen[2]!.id].sort()).toEqual(['a1', 'a2']); + expect(seen[1]!.mode).toBe('per-row'); + // The handler that let it pass did not stop the rewrite — engine policy + // stays with the handler, not the dispatch. (The delete twin's assertion, + // verb-swapped: the engine gained a DISPATCH, not an opinion.) + expect(bodyOf(driver, 'att', 'a1')).toBe('z'); + expect(bodyOf(driver, 'att', 'a2')).toBe('z'); + }); + + it('a refusal from the flagged handler rejects the update BEFORE any driver call', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { + if ((ctx.input as any).id === undefined) { + const err: any = new Error('unscoped update refused by test guard'); + err.code = 'TEST_UNSCOPED_REFUSED'; + err.status = 403; + throw err; + } + }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await expect( + engine.update('att', { status: 'z' }, { multi: true, context: { userId: 'u1' } } as any), + ).rejects.toMatchObject({ code: 'TEST_UNSCOPED_REFUSED', status: 403 }); + // No prior-row read, no updateMany: the refusal cost nothing downstream, + // and — the point of the whole card — the row still holds its old value. + expect(driver.calls).toEqual([]); + expect(bodyOf(driver, 'att', 'a1')).toBe('x'); + }); + + it('the ZERO-MATCH limb: an unscoped update of an EMPTY table still dispatches', async () => { + const { engine, driver } = await boot(); + // No rows at all — the [D1] per-row gate would dispatch nothing, which is + // exactly how the pre-#9974 engine answered "success" to a probe of the + // unscoped shape. + let dispatched = 0; + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { + if ((ctx.input as any).id === undefined) { + dispatched += 1; + const err: any = new Error('unscoped update refused by test guard'); + err.code = 'TEST_UNSCOPED_REFUSED'; + err.status = 403; + throw err; + } + }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await expect( + engine.update('att', { status: 'z' }, { multi: true, context: { userId: 'u1' } } as any), + ).rejects.toMatchObject({ code: 'TEST_UNSCOPED_REFUSED' }); + expect(dispatched).toBe(1); + expect(driver.calls).toEqual([]); + }); + + it('positive control for the zero-match limb: WITHOUT the flag, the same shape dispatches nothing', async () => { + const { engine, driver } = await boot(); + let dispatched = 0; + engine.registerHook( + 'beforeUpdate', + async () => { dispatched += 1; }, + { object: 'att' }, // same handler, same object — flag withheld + ); + + // Resolves: zero rows matched, zero per-row dispatches ([D1]), and no + // whole-operation dispatch without the declaration. This is exactly the + // pre-#9974 wired behaviour, so the zero-match measurement above measures + // the flag rather than the harness. + await expect( + engine.update('att', { status: 'z' }, { multi: true, context: { userId: 'u1' } } as any), + ).resolves.toBeDefined(); + expect(dispatched).toBe(0); + expect(driver.calls).toContain('updateMany:att'); + }); + + it("dispatches on `where: null` — the handler contract's other unscoped spelling", async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.update('att', { status: 'z' }, { multi: true, where: null, context: { userId: 'u1' } } as any); + expect(seen[0]).toMatchObject({ id: undefined, where: null, multi: true, mode: 'record' }); + }); + + it('the whole-operation `scope` is identity-shared with the per-row contexts', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + const perRowSawMarker: unknown[] = []; + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { + const scope = (ctx.dispatch as any)?.scope as Record; + if ((ctx.input as any).id === undefined) scope.marker = 'from-whole-op'; + else perRowSawMarker.push(scope.marker); + }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.update('att', { status: 'z' }, { multi: true, context: { userId: 'u1' } } as any); + expect(perRowSawMarker).toEqual(['from-whole-op']); + }); +}); + +describe('[#9974] scoped updates and undeclared registrations see NO new dispatch', () => { + it('a real `where` predicate gets only the per-row fan-out', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [ + { id: 'a1', status: 'x', owner: 'u1' }, + { id: 'a2', status: 'y', owner: 'u1' }, + ]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.update('att', { owner: 'u2' }, { multi: true, where: { status: 'x' }, context: { userId: 'u1' } } as any); + expect(seen).toHaveLength(1); + expect(seen[0]).toMatchObject({ id: 'a1', mode: 'per-row' }); + expect(driver.stores.get('att')!.get('a2')!.owner).toBe('u1'); + }); + + it('`where: {}` is a REAL match-all query, not an unscoped update', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.update('att', { status: 'z' }, { multi: true, where: {}, context: { userId: 'u1' } } as any); + // Per-row only: the guard's whole-operation branch is not summoned for a + // query that really ran and really matched. An entitled caller may still + // rewrite the table with an EXPLICIT match-all — the refusal is about an + // ABSENT predicate, never about breadth. + expect(seen).toHaveLength(1); + expect(seen[0]).toMatchObject({ id: 'a1', mode: 'per-row' }); + expect(bodyOf(driver, 'att', 'a1')).toBe('z'); + }); + + it('a by-id update is untouched', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await engine.update('att', { status: 'z' }, { where: { id: 'a1' }, context: { userId: 'u1' } } as any); + expect(seen).toHaveLength(1); + expect(seen[0]!.id).toBe('a1'); + expect(seen[0]!.mode).toBe('record'); + expect(bodyOf(driver, 'att', 'a1')).toBe('z'); + }); + + it("an object whose registration does NOT declare the flag keeps exactly today's dispatches — and today's ACCEPT", async () => { + const { engine, driver } = await boot(); + seed(driver, 'task', [ + { id: 't1', status: 'x', owner: 'u1' }, + { id: 't2', status: 'y', owner: 'u1' }, + ]); + const seen: Seen[] = []; + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'task' }, + ); + + await engine.update('task', { status: 'z' }, { multi: true, context: { userId: 'u1' } } as any); + // Per-row dispatches only — no whole-operation call arrived, and the + // unscoped rewrite proceeds as it does today for undeclared objects. This + // is the blast-radius pin for a REFUSAL-widening change: the engine did not + // grow an opinion about unscoped updates in general. + expect(seen).toHaveLength(2); + expect(seen.every((s) => s.id !== undefined && s.mode === 'per-row')).toBe(true); + expect(bodyOf(driver, 'task', 't1')).toBe('z'); + expect(bodyOf(driver, 'task', 't2')).toBe('z'); + }); + + it('a `beforeDelete`-only declaration does not leak the dispatch onto update', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + const seen: Seen[] = []; + // Exactly `sys_attachment`'s shape: the flag on the delete registration + // only. The update below must behave as it did before #9974 — the per-event + // nature of the flag is what lets ONE flag serve a delete-only guard. + engine.registerHook( + 'beforeDelete', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { seen.push(snapshot(ctx)); }, + { object: 'att' }, + ); + + await engine.update('att', { status: 'z' }, { multi: true, context: { userId: 'u1' } } as any); + expect(seen).toHaveLength(1); + expect(seen[0]).toMatchObject({ id: 'a1', mode: 'per-row' }); + expect(bodyOf(driver, 'att', 'a1')).toBe('z'); + }); +}); + +describe('[#9974] the id slot is not a lever on the update whole-operation context', () => { + it('binding `input.id` is refused with HookTargetRebindError, nothing written', async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { + if ((ctx.input as any).id === undefined) (ctx.input as any).id = 'a1'; + }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + await expect( + engine.update('att', { status: 'z' }, { multi: true, context: { userId: 'u1' } } as any), + ).rejects.toMatchObject({ code: HOOK_TARGET_REBIND_ERROR_CODE, path: 'unscoped-multi' }); + expect(driver.calls).toEqual([]); + expect(bodyOf(driver, 'att', 'a1')).toBe('x'); + }); + + it("the rebind refusal names the UPDATE event, not the delete one", async () => { + const { engine, driver } = await boot(); + seed(driver, 'att', [{ id: 'a1', status: 'x', owner: 'u1' }]); + engine.registerHook( + 'beforeUpdate', + async (ctx: HookContext) => { + if ((ctx.input as any).id === undefined) (ctx.input as any).id = 'a1'; + }, + { object: 'att', dispatchUnscopedMultiWrite: true }, + ); + + // One helper serves both verbs, so the event it reports has to be the + // caller's — a hard-coded 'beforeDelete' would misname every update. + await expect( + engine.update('att', { status: 'z' }, { multi: true, context: { userId: 'u1' } } as any), + ).rejects.toMatchObject({ event: 'beforeUpdate' }); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index cb2f501e96..57955560f6 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1329,30 +1329,48 @@ export interface HookEntry { priority: number; packageId?: string; /** - * [#9719] Opt-in: ALSO dispatch this handler ONCE with the whole-operation - * context when a predicate (`multi: true`) delete arrives with no caller - * predicate at all (`options.where` absent or `null`) — BEFORE any matched - * row is resolved. + * [#9719, widened to `beforeUpdate` by #9974] Opt-in: ALSO dispatch this + * handler ONCE with the whole-operation context when a predicate + * (`multi: true`) WRITE arrives with no caller predicate at all + * (`options.where` absent or `null`) — BEFORE any matched row is resolved. * * Why the extra dispatch exists: since #5038/#5574 the predicate path - * dispatches `beforeDelete` per matched row, with `input.id` bound — so a - * guard that refuses the OPERATION SHAPE (the #4757 unscoped-multi-delete - * refusal on `sys_attachment`) never saw the shape it guards: the by-id - * branch of the handler shadowed it on every dispatch, and a zero-match - * unscoped delete dispatched nothing at all. "Nothing to authorize" and - * "nothing was ever queried" are different verdicts, so the shape check - * cannot ride the per-row fan-out; it needs the one dispatch that happens - * regardless of what the predicate matches. + * dispatches `beforeUpdate` / `beforeDelete` per matched row, with + * `input.id` bound — so a guard that refuses the OPERATION SHAPE (the #4757 + * unscoped-multi-delete refusal on `sys_attachment`, the #4630 one on + * `sys_comment`) never saw the shape it guards: the by-id branch of the + * handler shadowed it on every dispatch, and a zero-match unscoped write + * dispatched nothing at all. "Nothing to authorize" and "nothing was ever + * queried" are different verdicts, so the shape check cannot ride the + * per-row fan-out; it needs the one dispatch that happens regardless of what + * the predicate matches. + * + * ## Why it covers both write verbs + * + * #9719 shipped it on `beforeDelete` only, and refused it elsewhere, because + * extending it to `beforeUpdate`'s predicate path changes what the engine + * ACCEPTS on the far hotter update path — a product-behaviour decision, not + * drift. That decision was taken (#9974, maintainer ruling 2026-08-19, + * option A) on the recoverability asymmetry: a delete leaves a trace of who + * removed what, an overwrite leaves none — the old value is gone on the spot + * with nothing to restore from — and a forgotten `where` is the mistake + * generated code makes most often. So the LESS guarded verb was the one + * whose failure is LESS recoverable. One flag now covers both, rather than a + * `dispatchUnscopedMultiUpdate` sibling: the flag is already per-REGISTRATION + * and per-EVENT, so "delete only" (#4757's `sys_attachment` guard, which + * declares no update refusal) is still said exactly — by declaring it on the + * `beforeDelete` registration and not on an update one. * * Deliberately a REGISTRATION declaration, not an engine-wide rule: the * engine stays neutral (no behaviour change for objects whose guards do not - * declare it — generalizing the refusal is a separate product decision), and - * the policy — who is refused, with what error — stays in the ONE handler - * that already declares and tests it. Valid on `beforeDelete` registrations - * only ({@link assertValidUnscopedMultiDeleteFlag}); dispatched by - * {@link ObjectQL.dispatchUnscopedMultiDeleteHooks}. + * declare it — generalizing the refusal to every guarded object is a + * separate product decision), and the policy — who is refused, with what + * error — stays in the ONE handler that already declares and tests it. Valid + * on `beforeUpdate` / `beforeDelete` registrations only + * ({@link assertValidUnscopedMultiWriteFlag}); dispatched by + * {@link ObjectQL.dispatchUnscopedMultiWriteHooks}. */ - dispatchUnscopedMultiDelete?: boolean; + dispatchUnscopedMultiWrite?: boolean; /** * Original metadata-form `Hook` definition this entry was bound from * (when registered via `bindHooksToEngine`). Pure code-paths that call @@ -1580,27 +1598,41 @@ function assertHookScopeNotSelfCancelling( } /** - * [#9719] Registration-time refusal for {@link HookEntry.dispatchUnscopedMultiDelete} - * on any event other than `beforeDelete`. + * [#9719, widened by #9974] The events whose predicate branch actually reads + * {@link HookEntry.dispatchUnscopedMultiWrite}. Declared as data beside the + * assert so the refusal message and the accept set cannot drift apart, and so + * adding a third verb is one edit rather than a rule to restate. + */ +const UNSCOPED_MULTI_WRITE_EVENTS = ['beforeUpdate', 'beforeDelete'] as const; + +/** + * [#9719, widened by #9974] Registration-time refusal for + * {@link HookEntry.dispatchUnscopedMultiWrite} on any event whose dispatch + * never reads it. * - * The flag is consumed by exactly one dispatch site — `ObjectQL.delete()`'s - * predicate branch — so on every other event it would register "successfully" - * and never fire: ADR-0078's silently inert declaration, the same shape the - * two scope asserts above refuse. Statically decidable, so refused at the - * door rather than warned about. (Extending the whole-operation dispatch to - * `beforeUpdate`'s predicate path would be a product-behaviour decision of its - * own, not a widening to make this assert quieter.) + * The flag is consumed by exactly two dispatch sites — the predicate branches + * of `ObjectQL.update()` and `ObjectQL.delete()` — so on every other event it + * would register "successfully" and never fire: ADR-0078's silently inert + * declaration, the same shape the two scope asserts above refuse. Statically + * decidable, so refused at the door rather than warned about. + * + * ⚠️ The accept set widened by RULING, not to make the assert quieter: until + * #9974 this refused `beforeUpdate` too, and its comment named that as "a + * product-behaviour decision of its own". The decision was taken (option A, + * 2026-08-19), so `beforeUpdate` is now a declared reader — and every OTHER + * event is still refused on exactly the original grounds. */ -function assertValidUnscopedMultiDeleteFlag( - dispatchUnscopedMultiDelete: boolean | undefined, +function assertValidUnscopedMultiWriteFlag( + dispatchUnscopedMultiWrite: boolean | undefined, event: string, ): void { - if (!dispatchUnscopedMultiDelete || event === 'beforeDelete') return; + if (!dispatchUnscopedMultiWrite) return; + if ((UNSCOPED_MULTI_WRITE_EVENTS as readonly string[]).includes(event)) return; throw new Error( - `[ObjectQL] Hook '${event}' declares \`dispatchUnscopedMultiDelete\`, which only the ` - + "'beforeDelete' dispatch reads — on this event the flag would register successfully and " - + 'then never fire (ADR-0078: no silently inert declaration). Register the guard on ' - + "'beforeDelete', or drop the flag.", + `[ObjectQL] Hook '${event}' declares \`dispatchUnscopedMultiWrite\`, which only the ` + + `${UNSCOPED_MULTI_WRITE_EVENTS.map((e) => `'${e}'`).join(' / ')} dispatches read — on this ` + + 'event the flag would register successfully and then never fire (ADR-0078: no silently ' + + 'inert declaration). Register the guard on one of those events, or drop the flag.', ); } @@ -2154,11 +2186,12 @@ export class ObjectQL implements IObjectQLEngine { priority?: number; packageId?: string; /** - * [#9719] Opt-in whole-operation dispatch for an UNSCOPED predicate - * delete (`multi: true`, no `where`) — see {@link HookEntry.dispatchUnscopedMultiDelete}. - * Valid on `beforeDelete` registrations only. + * [#9719, widened by #9974] Opt-in whole-operation dispatch for an + * UNSCOPED predicate write (`multi: true`, no `where`) — see + * {@link HookEntry.dispatchUnscopedMultiWrite}. Valid on `beforeUpdate` / + * `beforeDelete` registrations only. */ - dispatchUnscopedMultiDelete?: boolean; + dispatchUnscopedMultiWrite?: boolean; /** Original metadata Hook definition (set by `bindHooksToEngine`). */ meta?: any; /** Stable name from metadata (set by `bindHooksToEngine`). */ @@ -2173,8 +2206,8 @@ export class ObjectQL implements IObjectQLEngine { assertValidHookObject(options?.object, event); // [#6573] Two well-formed faces that cancel out (`'account'` minus `'account'`). assertHookScopeNotSelfCancelling(options?.object, options?.excludeObjects, event); - // [#9719] The unscoped-multi-delete flag on an event whose dispatch never reads it. - assertValidUnscopedMultiDeleteFlag(options?.dispatchUnscopedMultiDelete, event); + // [#9719/#9974] The unscoped-multi-write flag on an event whose dispatch never reads it. + assertValidUnscopedMultiWriteFlag(options?.dispatchUnscopedMultiWrite, event); // [#3195] Guard against enum-vs-dispatch drift: a hook on an event the // engine never triggers would register "successfully" and then silently // never fire. Warn loudly rather than swallow it. Not a hard reject — a @@ -2197,7 +2230,7 @@ export class ObjectQL implements IObjectQLEngine { excludeObjects: options?.excludeObjects, priority: options?.priority ?? 100, packageId: options?.packageId, - dispatchUnscopedMultiDelete: options?.dispatchUnscopedMultiDelete, + dispatchUnscopedMultiWrite: options?.dispatchUnscopedMultiWrite, meta: options?.meta, hookName: options?.hookName, }); @@ -2658,22 +2691,36 @@ export class ObjectQL implements IObjectQLEngine { } /** - * [#9719] The whole-operation `beforeDelete` dispatch for an UNSCOPED - * predicate delete — `multi: true` with no caller predicate at all - * (`options.where` absent or `null`) — delivered ONLY to registrations that - * declared {@link HookEntry.dispatchUnscopedMultiDelete}. + * [#9719, widened to `beforeUpdate` by #9974] The whole-operation + * `beforeUpdate` / `beforeDelete` dispatch for an UNSCOPED predicate write — + * `multi: true` with no caller predicate at all (`options.where` absent or + * `null`) — delivered ONLY to registrations that declared + * {@link HookEntry.dispatchUnscopedMultiWrite} on `event`. * * ## Why this dispatch exists * * The per-row contract (#5038 / #5574) made the operation's SHAPE invisible - * to `beforeDelete` handlers on the predicate path: every per-row context - * arrives with `input.id` bound, and a zero-match predicate dispatches - * nothing at all ([D1]). A guard whose rule is about the shape — #4757's - * "refuse a predicate-less multi-delete outright, regardless of what it - * matches" on `sys_attachment` — therefore could not fire through the wired - * engine: with rows matched its by-id branch shadowed the check, and with - * none matched it never ran. Both limbs need a dispatch that happens BEFORE - * the matched-row read, keyed on the operation's shape alone. + * to `before*` handlers on the predicate path: every per-row context arrives + * with `input.id` bound, and a zero-match predicate dispatches nothing at + * all ([D1]). A guard whose rule is about the shape — #4757's "refuse a + * predicate-less multi-delete outright, regardless of what it matches" on + * `sys_attachment`, and #4630's twin on `sys_comment` — therefore could not + * fire through the wired engine: with rows matched its by-id branch shadowed + * the check, and with none matched it never ran. Both limbs need a dispatch + * that happens BEFORE the matched-row read, keyed on the operation's shape + * alone. + * + * ## Why one helper, both verbs + * + * The update half was measured as a PARTIAL guard rather than a total + * fail-open (#9974): an unscoped `multi: true` update was refused only when + * it happened to sweep a row the caller lacked rights to — the per-row gate + * catching the shape by accident — and resolved silently when the caller was + * entitled to every row, zero-match included. A guard that fires by accident + * reads as enforcement while enforcing nothing, which is why the ruling + * closed it rather than narrowing the declaration. `event` is the ONLY thing + * that differs between the two verbs here, so it is a parameter and not a + * second copy of this method. * * ## What the handler receives * @@ -2690,10 +2737,11 @@ export class ObjectQL implements IObjectQLEngine { * ## Deliberately opt-in * * Undeclared registrations see NO new dispatch: generalizing an unscoped- - * multi-delete refusal to every guarded object is a product decision this - * mechanism does not take (objectstack#9719's ruling commissions the - * `sys_attachment` restoration only). The engine stays neutral; the policy — - * who is refused, with which error — lives in the declaring handler. + * multi-write refusal to every guarded object is a product decision this + * mechanism does not take (#9719 commissions the `sys_attachment` + * restoration, #9974 the `sys_comment` update half — neither commissions an + * engine-wide rule). The engine stays neutral; the policy — who is refused, + * with which error — lives in the declaring handler. * * ## The id slot is not a lever here either * @@ -2703,14 +2751,15 @@ export class ObjectQL implements IObjectQLEngine { * context would retarget nothing — refused rather than ignored, the same * rule as D4 and #6752 (`HookTargetRebindError`, path `'unscoped-multi'`). */ - private async dispatchUnscopedMultiDeleteHooks( + private async dispatchUnscopedMultiWriteHooks( + event: 'beforeUpdate' | 'beforeDelete', object: string, batchCtx: HookContext, ): Promise { - const entries = this.hooks.get('beforeDelete'); + const entries = this.hooks.get(event); if (!entries || entries.length === 0) return; const flagged = entries.filter( - (entry) => entry.dispatchUnscopedMultiDelete && hookMatchesObject(entry, object), + (entry) => entry.dispatchUnscopedMultiWrite && hookMatchesObject(entry, object), ); if (flagged.length === 0) return; // Mirrors `triggerHooks`' opt-out rule: `skipAutomations` suppresses only @@ -2737,7 +2786,7 @@ export class ObjectQL implements IObjectQLEngine { if (observed !== undefined) { throw new HookTargetRebindError({ object, - event: 'beforeDelete', + event, path: 'unscoped-multi', expectedId: undefined, observedId: observed, @@ -9545,6 +9594,37 @@ export class ObjectQL implements IObjectQLEngine { `(the predicate branch was reached without the #2982 seed).`, ); } + // [#9974] The unscoped-multi shape check, BEFORE the matched-row + // read — `delete()`'s twin (#9719), verbatim in condition, slot and + // placement, because the two branches answer the same question about + // the same caller options and a second reading of "unscoped" is how + // the verbs would drift apart. + // + // A `multi: true` update carrying no caller predicate at all + // dispatches ONCE, whole-operation-shaped, to the registrations that + // declared for it — so a shape-of-the-operation refusal (#4630's on + // `sys_comment`) fires regardless of what the predicate would match, + // zero rows included. + // + // ⚠️ This is a BEHAVIOUR CHANGE on the accept set, ruled for + // deliberately (#9974, option A, 2026-08-19), not a restoration: an + // unscoped multi-update by a caller entitled to every matched row + // used to succeed and rewrite the whole table. It now refuses IF the + // object's guard declares the flag — an overwrite leaves no trace + // and no pre-image to restore from, where a delete at least leaves + // one, so the verb whose failure is less recoverable was the one + // left less guarded. Objects with no declaring guard are untouched. + // + // Read from the caller's RAW options — the same slot the handler + // contract reads (`input.options.where`, hook.zod.ts's upper-bound + // rule) — never from the AST, which middleware may have narrowed and + // only ever narrows. `where: {}` is a REAL (match-all) query and + // stays on the per-row authorize path; only an absent or `null` + // predicate is unscoped. + const rawWhere = (hookContext.input.options as { where?: unknown } | undefined)?.where; + if (rawWhere === undefined || rawWhere === null) { + await this.dispatchUnscopedMultiWriteHooks('beforeUpdate', object, hookContext); + } const preOpts = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); readPriorRows = async () => { if (!priorRowsRead) { @@ -11066,9 +11146,10 @@ export class ObjectQL implements IObjectQLEngine { // the AST, which middleware may have narrowed and only ever narrows. // `where: {}` is a REAL (match-all) query and stays on the per-row // authorize path; only an absent or `null` predicate is unscoped. + // `update()`'s predicate branch carries the identical check (#9974). const rawWhere = (hookContext.input.options as { where?: unknown } | undefined)?.where; if (rawWhere === undefined || rawWhere === null) { - await this.dispatchUnscopedMultiDeleteHooks(object, hookContext); + await this.dispatchUnscopedMultiWriteHooks('beforeDelete', object, hookContext); } // [#5038/#5574] Read the doomed rows ONCE, before they are gone — the // only moment their pre-image exists — and serve BOTH phases from it diff --git a/packages/objectql/src/hook-target-rebind-errors.ts b/packages/objectql/src/hook-target-rebind-errors.ts index 9cb5102581..280b6ccac2 100644 --- a/packages/objectql/src/hook-target-rebind-errors.ts +++ b/packages/objectql/src/hook-target-rebind-errors.ts @@ -101,12 +101,14 @@ export type HookTargetRebindPath = /** A per-row `before*` context on a predicate write (D4). */ | 'per-row' /** - * [#9719] The whole-operation `beforeDelete` dispatch an UNSCOPED predicate - * delete delivers to registrations that declared - * `dispatchUnscopedMultiDelete`. Its `input.id` is present-but-`undefined` — - * the exact slot that used to be the batch dispatch's reroute lever — and - * the ladder is resolved before any handler runs, so binding it retargets - * nothing and is refused rather than ignored, same as the other two seams. + * [#9719, both write verbs since #9974] The whole-operation + * `beforeUpdate` / `beforeDelete` dispatch an UNSCOPED predicate write + * delivers to registrations that declared `dispatchUnscopedMultiWrite`. Its + * `input.id` is present-but-`undefined` — the exact slot that used to be the + * batch dispatch's reroute lever — and the ladder is resolved before any + * handler runs, so binding it retargets nothing and is refused rather than + * ignored, same as the other two seams. `event` says which verb; the seam + * and the rule are one. */ | 'unscoped-multi'; @@ -175,11 +177,11 @@ function buildMessage(info: { `'delete()' honoured a rebind until #6752 by re-resolving the new target; that is retired ` + `too, so one rule now covers both.` : path === 'unscoped-multi' - ? ` This is the whole-operation dispatch an UNSCOPED predicate delete delivers to a declared ` + - `shape guard (#9719): its 'id' is present-but-undefined ON PURPOSE — there is no target row ` + - `— and the dispatch ladder was resolved before any handler ran, so binding 'input.id' here ` + - `retargets nothing. It is refused rather than ignored, because a silent no-op is the ` + - `failure this contract exists to abolish.` + ? ` This is the whole-operation dispatch an UNSCOPED predicate write delivers to a declared ` + + `shape guard (#9719, both write verbs since #9974): its 'id' is present-but-undefined ON ` + + `PURPOSE — there is no target row — and the dispatch ladder was resolved before any handler ` + + `ran, so binding 'input.id' here retargets nothing. It is refused rather than ignored, ` + + `because a silent no-op is the failure this contract exists to abolish.` : ` On a predicate write a '${event}' context arrives with 'id' ALREADY bound to its row and the ` + `dispatch decided, so rebinding it retargets nothing (ADR-0058 Addendum II, D4). It is refused ` + `rather than ignored, because a silent no-op is the failure this contract exists to abolish.`; diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts index 7e4fe9547f..5f4ba3daa7 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts @@ -266,8 +266,9 @@ describe('comment access — beforeDelete (author or parent editor)', () => { // construction — a shape the engine's per-row dispatch (#5038/#5574) never // produced on either verb, so it stayed green for a behaviour the wired // engine did the opposite of. It is re-pointed at the REAL engine below — - // see "#4630 through the wired engine", which also PINS the update half's - // still-unreachable state rather than asserting it away. + // see the two "#4630 through the wired engine" blocks. [#9974] The update + // half's block pinned the gap as MEASURED until the dispatch existed; it now + // pins the refusal itself, on both verbs. it('a dangling-thread comment is modifiable only by its author', async () => { const orphan = { id: 'c9', thread_id: 'crm_opportunity:', author_id: 'rep1', body: 'orphan' }; @@ -670,12 +671,21 @@ const wiredComment = (id: string, authorId: string, threadId = 'crm_opportunity: /** ADR-0112 envelope of the #4630 refusal. The first sentence IS the declared * contract (the issue's quoted wording), so it is asserted alongside the code - * and status rather than instead of them. */ -const UNSCOPED_REFUSAL = expect.objectContaining({ + * and status rather than instead of them. + * + * [#9974] Parameterized by verb because the refusal now fires on BOTH, and the + * VERB IN THE MESSAGE is load-bearing: before this card an unscoped multi-update + * that was refused at all came back with the PER-ROW message + * (`Cannot update comment c2: …`), which names a row rather than the shape. A + * matcher that only checked code+status would have passed on that wording, so + * the shape refusal is pinned to say "unscoped". */ +const unscopedRefusal = (verb: 'update' | 'delete') => expect.objectContaining({ code: 'RECORD_NOT_ACCESSIBLE', status: 403, - message: expect.stringContaining('Refusing an unscoped multi-delete of comments'), + message: expect.stringContaining(`Refusing an unscoped multi-${verb} of comments`), }); +const UNSCOPED_REFUSAL = unscopedRefusal('delete'); +const UNSCOPED_UPDATE_REFUSAL = unscopedRefusal('update'); describe('unscoped multi-DELETE (no id, no where) — #4630 through the wired engine (#9798)', () => { it('refuses `{ multi: true }` even when the caller AUTHORED every matched row — and the rows survive', async () => { @@ -779,74 +789,171 @@ describe('unscoped multi-DELETE (no id, no where) — #4630 through the wired en }); }); -describe('unscoped multi-UPDATE (no id, no where) — the still-unreachable half (#9798)', () => { - // ⚠️ THESE PINS DOCUMENT A LIVE FAIL-OPEN, NOT APPROVED BEHAVIOUR. ⚠️ +describe('unscoped multi-UPDATE (no id, no where) — #4630 through the wired engine (#9974)', () => { + // ⚠️ THESE PINS REPLACE THREE `MEASURED GAP` PINS, AS THE CARD REQUIRED. ⚠️ // - // `resolveTargetRows` declares the same refusal for `update`, and the block - // these replace asserted it by direct handler call — green while the wired - // engine did the opposite. The refusal cannot be restored the way DELETE's - // was: `dispatchUnscopedMultiDelete` is valid on `beforeDelete` only, and - // the engine refuses it elsewhere BY DESIGN, because extending the - // whole-operation dispatch to `beforeUpdate`'s predicate path is a - // product-behaviour decision rather than drift (#9719's - // `assertValidUnscopedMultiDeleteFlag`). + // Until #9974 this block documented a LIVE FAIL-OPEN on purpose: the same + // refusal `resolveTargetRows` declares for `delete` was declared for `update` + // and could not fire, because `dispatchUnscopedMultiDelete` was valid on + // `beforeDelete` only and the engine refused it elsewhere BY DESIGN — + // extending the whole-operation dispatch to `beforeUpdate`'s predicate path + // was a product-behaviour decision, not drift. Those pins were annotated to + // go RED when the decision landed, and that is what happened: the maintainer + // ruled option A on 2026-08-19, the flag became `dispatchUnscopedMultiWrite` + // valid on both write verbs, and each MEASURED-GAP assertion below is now the + // REFUSAL it was measuring the absence of — replaced, not relaxed or removed. // - // So the gap is pinned as MEASURED, deliberately: a suite that simply - // dropped the false-green block would leave nothing to notice the fail-open, - // and one that asserted the refusal would be false-green again. When the - // decision lands and the dispatch exists, these pins go RED — that is their - // job, and the fix is to replace them with the refusal assertions, not to - // relax them. Tracked at #9974 (the decision card this half was split into). - it('MEASURED GAP: an unscoped `{ multi: true }` update the caller AUTHORED every row of is not refused — the whole table is rewritten', async () => { - // The delete half's first limb, verb-swapped: the declared refusal is about - // the SHAPE, so it must fire whatever the rows say. It does not — the - // per-row author shortcut licenses each row and no dispatch ever carries - // the unscoped shape. The blast radius is "every row the caller happens to - // be entitled to", which is the issue's measured claim. + // The three limbs, in the order the card tabled them: + // 1. caller authored every row — was "whole table rewritten", now refused; + // 2. empty table (zero match) — was "nothing ran, resolves", now refused; + // 3. a row the caller may not touch is swept — WAS refused, but with the + // PER-ROW message; now refused on the SHAPE, which is a different and + // stronger claim (see limb 3's own note). + // + // ⚠️ Limb 1 is a BEHAVIOUR CHANGE, not a restoration: that call used to + // succeed for an entitled caller. It is what was ruled for — an overwrite + // leaves no trace and no pre-image, so the less recoverable verb must not be + // the less guarded one — and the changeset says so in those terms. + + it('limb 1: an unscoped `{ multi: true }` update is refused even when the caller AUTHORED every row — and the bodies survive', async () => { + // The behaviour change, stated as a test: this exact call resolved before + // #9974 and rewrote both rows. The declared refusal is about the SHAPE, so + // it must fire whatever the rows say — including when every row is the + // caller's own and the per-row gate would have licensed all of them. const { ql, bodies } = await bootWired({ comments: [wiredComment('c1', 'me'), wiredComment('c2', 'me', 'crm_opportunity:opp2')], sharing: { canEdit: async () => false }, }); await expect( ql.update('sys_comment', { body: 'rewritten' }, { multi: true, context: { userId: 'me' } } as any), - ).resolves.toBeDefined(); - expect(bodies()).toEqual(['rewritten', 'rewritten']); + ).rejects.toEqual(UNSCOPED_UPDATE_REFUSAL); + expect(bodies()).toEqual(['body of c1', 'body of c2']); }); - it('MEASURED GAP: an unscoped `{ multi: true }` update of an EMPTY table is not refused either', async () => { - // The zero-match limb: the per-row dispatch is gated on matched rows, so - // nothing runs at all and the caller sees success — the probe that tells an - // attacker the unscoped shape is accepted. + it('refuses an explicitly null `where` the same way', async () => { + const { ql, bodies } = await bootWired({ comments: [wiredComment('c1', 'me')] }); + await expect( + ql.update('sys_comment', { body: 'rewritten' }, { + multi: true, where: null, context: { userId: 'me' }, + } as any), + ).rejects.toEqual(UNSCOPED_UPDATE_REFUSAL); + expect(bodies()).toEqual(['body of c1']); + }); + + it('limb 2: an unscoped `{ multi: true }` update of an EMPTY table is refused too', async () => { + // The zero-match limb: the per-row dispatch is gated on matched rows, so a + // handler-only fix could never fire here — a caller probing against an + // empty table used to see success and ship the unscoped update. const { ql } = await bootWired({ comments: [] }); await expect( ql.update('sys_comment', { body: 'rewritten' }, { multi: true, context: { userId: 'me' } } as any), + ).rejects.toEqual(UNSCOPED_UPDATE_REFUSAL); + }); + + it('positive control: an empty table is not refused per se — a scoped `where: {}` update of it resolves', async () => { + // Proves the empty-table refusal above measures the SHAPE, not emptiness. + const { ql } = await bootWired({ comments: [] }); + await expect( + ql.update('sys_comment', { body: 'rewritten' }, { + multi: true, where: {}, context: { userId: 'me' }, + } as any), ).resolves.toBeDefined(); }); - it('the per-row gate DOES still fire on an unscoped update — it is a partial, row-dependent guard', async () => { - // Why the update half is a smaller hole than delete's was, and why it is - // still a hole: rows the caller may not touch are refused individually - // (with the PER-ROW message, not the unscoped one), so the unscoped update - // is caught only when it happens to sweep a row the caller lacks rights to. + it('limb 3: a swept row the caller may not touch is now refused on the SHAPE, not per-row', async () => { + // The limb that was ALREADY refusing, and the reason it still had to + // change. Before #9974 this answered `Cannot update comment c2: …` — the + // per-row gate catching the unscoped shape by accident, on its way through + // a row it happened to reject. That message names a ROW, so it taught the + // caller "row c2 is protected" when the truth is "this shape is refused"; + // scoping the write to c1 alone would have "fixed" it and left the hole. + // The shape check now runs BEFORE any row is read, so the unscoped message + // arrives whatever the sweep would have found. + const { ql, bodies } = await bootWired({ + comments: [wiredComment('c1', 'me'), wiredComment('c2', 'someone-else')], + sharing: { canEdit: async () => false }, + }); + const err = await ql + .update('sys_comment', { body: 'rewritten' }, { multi: true, context: { userId: 'me' } } as any) + .then(() => null, (e: unknown) => e); + expect(err).toEqual(UNSCOPED_UPDATE_REFUSAL); + // The old per-row wording is GONE from this shape, not merely joined by the + // new one — asserting only the new sentence would pass on a message that + // still led with the row. + expect(String((err as Error).message)).not.toContain('Cannot update comment c2'); + expect(bodies()).toEqual(['body of c1', 'body of c2']); + }); + + it('the per-row gate is a DIFFERENT refusal and still fires through the wire', async () => { + // The delete block's twin: a SCOPED update matching a row the caller may + // not touch keeps answering with the per-row message. The two limbs stay + // distinguishable, which is what makes limb 3's assertion meaningful. const { ql, bodies } = await bootWired({ comments: [wiredComment('c1', 'me'), wiredComment('c2', 'someone-else')], sharing: { canEdit: async () => false }, }); await expect( - ql.update('sys_comment', { body: 'rewritten' }, { multi: true, context: { userId: 'me' } } as any), + ql.update('sys_comment', { body: 'rewritten' }, { + multi: true, + where: { thread_id: 'crm_opportunity:opp1' }, + context: { userId: 'me' }, + } as any), ).rejects.toEqual( expect.objectContaining({ code: 'RECORD_NOT_ACCESSIBLE', status: 403, - message: expect.stringContaining('Cannot update comment c2'), + message: expect.stringContaining('Cannot update comment'), }), ); expect(bodies()).toEqual(['body of c1', 'body of c2']); }); + it('scoped controls still pass: by id, by a real `where`, and by the match-all `where: {}`', async () => { + // Over-firing here would break every legitimate comment edit — the blast + // radius pin for a refusal-widening change. + const byId = await bootWired({ comments: [wiredComment('c1', 'me')] }); + await expect( + byId.ql.update('sys_comment', { body: 'edited' }, { + where: { id: 'c1' }, context: { userId: 'me' }, + } as any), + ).resolves.toBeDefined(); + expect(byId.bodies()).toEqual(['edited']); + + const byWhere = await bootWired({ + comments: [wiredComment('c1', 'me'), wiredComment('c2', 'me', 'crm_opportunity:opp2')], + }); + await expect( + byWhere.ql.update('sys_comment', { body: 'edited' }, { + multi: true, where: { author_id: 'me' }, context: { userId: 'me' }, + } as any), + ).resolves.toBeDefined(); + expect(byWhere.bodies()).toEqual(['edited', 'edited']); + + // `where: {}` is a REAL match-all query (the declared semantics): every + // matched row is authorized per row, and an entitled caller may rewrite the + // table with it — the refusal is about an ABSENT predicate only. + const matchAll = await bootWired({ comments: [wiredComment('c1', 'me')] }); + await expect( + matchAll.ql.update('sys_comment', { body: 'edited' }, { + multi: true, where: {}, context: { userId: 'me' }, + } as any), + ).resolves.toBeDefined(); + expect(matchAll.bodies()).toEqual(['edited']); + }); + + it('system context still bypasses the refusal — engine self-writes and seeds are not the caller', async () => { + const { ql, bodies } = await bootWired({ comments: [wiredComment('c1', 'me')] }); + await expect( + ql.update('sys_comment', { body: 'rewritten' }, { + multi: true, context: { userId: 'x', isSystem: true }, + } as any), + ).resolves.toBeDefined(); + expect(bodies()).toEqual(['rewritten']); + }); + it('control: the scoped update paths are gated exactly as declared', async () => { - // The gap above is about the UNSCOPED shape only — a real predicate still - // reaches the per-row author-or-parent-editor gate, and refuses. + // Unchanged from the pre-#9974 block: a real predicate still reaches the + // per-row author-or-parent-editor gate, and refuses there. const { ql, bodies } = await bootWired({ comments: [wiredComment('c1', 'me'), wiredComment('c2', 'someone-else')], sharing: { canEdit: async () => false }, diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.ts index d992f925f2..114606ec77 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.ts @@ -30,13 +30,10 @@ * matched row to pass, and an update that re-points `thread_id` must * additionally satisfy the insert rule on the NEW thread. A write * carrying NEITHER an id NOR a `where` is refused outright rather than - * authorizing the whole table by resolving zero rows — on DELETE that - * refusal reaches this handler through the `dispatchUnscopedMultiDelete` - * whole-operation dispatch the registration declares (#9719/#9798); on - * UPDATE the engine has no such dispatch yet, so the shape check is - * declared but unreachable through the wire — only the per-row gate - * catches an unscoped update, and only by accident (decision card #9974; - * see the note on `resolveTargetRows`). + * authorizing the whole table by resolving zero rows — on BOTH verbs that + * refusal reaches this handler through the `dispatchUnscopedMultiWrite` + * whole-operation dispatch both registrations declare (#9719/#9798 built + * it for delete; #9974 ruled it onto update). * - {@link installCommentReadVisibility} — the read side: a * `find`/`findOne`/`count`/`aggregate` middleware that intersects the query * with the threads whose parent record the caller can actually read. @@ -77,13 +74,14 @@ export interface CommentAccessEngine { options?: { object?: string; packageId?: string; - /** [#9798] Opt-in whole-operation `beforeDelete` dispatch for an UNSCOPED - * predicate delete (`multi: true`, no `where`) — the engine declaration - * added by #9719. Declared here because this file's registration passes - * it; the mechanism, its `beforeDelete`-only validity and its rebind - * refusal are owned by `HookEntry.dispatchUnscopedMultiDelete` in + /** [#9798, both verbs since #9974] Opt-in whole-operation `beforeUpdate` / + * `beforeDelete` dispatch for an UNSCOPED predicate write + * (`multi: true`, no `where`) — the engine declaration added by #9719. + * Declared here because this file's registrations pass it; the mechanism, + * its `beforeUpdate`/`beforeDelete`-only validity and its rebind refusal + * are owned by `HookEntry.dispatchUnscopedMultiWrite` in * `@objectstack/objectql`, not restated. */ - dispatchUnscopedMultiDelete?: boolean; + dispatchUnscopedMultiWrite?: boolean; }, ): void; /** Onion-model data middleware (runs for find/findOne/count/aggregate AND @@ -371,26 +369,27 @@ export function installCommentAccessHooks( // authorize row-by-row, and "nothing to authorize" must never read as // "allowed" (the engine would hand an unscoped AST to deleteMany). // - // [#9798] Which dispatch can actually deliver this shape differs BY VERB, - // and the difference is load-bearing — the branch reads as verb-neutral - // but is not: - // - `delete`: reachable, and only through the - // `dispatchUnscopedMultiDelete` whole-operation dispatch this module's - // `beforeDelete` registration declares (see below). The per-row - // contract (#5038/#5574) binds `input.id` on every predicate dispatch, - // which routes into the by-id branch above, and a zero-match predicate - // dispatches nothing at all. - // - `update`: NOT reachable through the wired engine today. The engine - // has no whole-operation dispatch on `beforeUpdate`'s predicate path - // and refuses the flag on that event by design, because extending it - // is a product-behaviour decision rather than drift (#9719's - // `assertValidUnscopedMultiDeleteFlag`). Measured: an unscoped - // `multi: true` UPDATE is refused only when it happens to sweep a row - // the caller may not touch — the PER-ROW gate catching it by accident - // — and resolves when the caller is entitled to every row, zero-match - // included. Split out as the decision card #9974; the wired-path pins - // in the test suite measure all three limbs. Do not "fix" it by - // widening this branch: the missing half is a dispatch, not a policy. + // [#9798/#9974] This branch is verb-neutral in what it says AND, since + // #9974, in what reaches it. On BOTH verbs the only dispatch that can + // deliver this shape is the `dispatchUnscopedMultiWrite` whole-operation + // dispatch this module's two `before*` registrations declare (see below): + // the per-row contract (#5038/#5574) binds `input.id` on every predicate + // dispatch, which routes into the by-id branch above, and a zero-match + // predicate dispatches nothing at all. + // + // ⚠️ The update half was NOT always reachable, and the history is the + // reason this comment exists. #9719 built the mechanism on `beforeDelete` + // only and refused the flag elsewhere by design, so until #9974 an + // unscoped `multi: true` UPDATE was refused only when it happened to + // sweep a row the caller may not touch — the PER-ROW gate catching the + // shape by accident, with the per-row message — and resolved silently + // when the caller was entitled to every row, zero-match included. The + // maintainer ruled that closed (option A, 2026-08-19): an overwrite + // leaves no trace and no pre-image, so the verb with the less recoverable + // failure must not be the less guarded one. + // + // ⛔ If this branch ever stops firing on a verb, the fix is the DISPATCH, + // not this policy — do not widen the branch to compensate. forbid(`Refusing an unscoped multi-${verb} of comments — scope the write to the rows you mean`); } const rows = await engine.find('sys_comment', { @@ -472,7 +471,14 @@ export function installCommentAccessHooks( ); } }, - { object: 'sys_comment', packageId: PACKAGE_ID }, + // [#9974] `dispatchUnscopedMultiWrite` is what makes the #4630 unscoped + // refusal in `resolveTargetRows` REACHABLE ON UPDATE — the half #9798 could + // not land, ruled in on 2026-08-19. Without it the predicate path dispatches + // per row with `input.id` bound (so the by-id branch shadows the shape + // check) and a zero-match predicate dispatches nothing, leaving the declared + // refusal to fire only by accident, on the rows rather than on the shape. + // Same declaration the `beforeDelete` registration below carries. + { object: 'sys_comment', packageId: PACKAGE_ID, dispatchUnscopedMultiWrite: true }, ); // ── Delete: author or parent editor ───────────────────────────────── @@ -485,14 +491,14 @@ export function installCommentAccessHooks( if (!rows.length) return; // nothing matched — nothing to authorize await authorizeRows(ctx, rows, 'delete'); }, - // [#9798] `dispatchUnscopedMultiDelete` is what makes the #4630 unscoped + // [#9798] `dispatchUnscopedMultiWrite` is what makes the #4630 unscoped // refusal in `resolveTargetRows` REACHABLE through the wired engine: the // predicate path dispatches per row with `input.id` bound (so the by-id // branch shadows the check), and a zero-match predicate dispatches nothing // at all — the engine's opt-in whole-operation dispatch is the one call // that arrives with no id and the caller's raw `options`, before any row is // resolved. Same declaration `sys_attachment` carries (#9719). - { object: 'sys_comment', packageId: PACKAGE_ID, dispatchUnscopedMultiDelete: true }, + { object: 'sys_comment', packageId: PACKAGE_ID, dispatchUnscopedMultiWrite: true }, ); } diff --git a/packages/services/service-storage/src/attachment-access-hooks.test.ts b/packages/services/service-storage/src/attachment-access-hooks.test.ts index 1147fac45b..1b479e209c 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.test.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.test.ts @@ -214,7 +214,7 @@ describe('attachment access — beforeDelete (uploader or parent editor)', () => // // A real `ObjectQL` + in-memory driver + this module's installer — the exact // path `ql.delete('sys_attachment', …)` takes in production. The unscoped -// refusal reaches the handler through the `dispatchUnscopedMultiDelete` +// refusal reaches the handler through the `dispatchUnscopedMultiWrite` // whole-operation dispatch its registration declares; the per-row gate keeps // firing per matched row; the scoped paths keep resolving. Every refusal here // asserts the rows SURVIVED, because the defect this replaces was precisely a diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index 5256459a70..1ab6188e92 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.ts @@ -32,7 +32,7 @@ import type { * NEITHER an id NOR a `where` is refused outright (#4757) — the engine * would hand `deleteMany` an AST over the whole table, and a gate that * resolved no rows for it would be authorizing exactly that. The refusal - * reaches this handler through the `dispatchUnscopedMultiDelete` + * reaches this handler through the `dispatchUnscopedMultiWrite` * whole-operation dispatch its registration declares (#9719) — the * per-row dispatch alone can never deliver the shape it refuses. * @@ -229,7 +229,7 @@ export function installAttachmentAccessHooks( // (Mirrors #4630's `resolveTargetRows` for sys_comment.) // // [#9719] Reached through the wired engine ONLY via the - // `dispatchUnscopedMultiDelete` whole-operation dispatch declared on + // `dispatchUnscopedMultiWrite` whole-operation dispatch declared on // this registration (see the registration options below): the // per-row contract (#5038/#5574) binds `input.id` on every predicate // dispatch — which routes into the by-id branch above — and a @@ -294,13 +294,20 @@ export function installAttachmentAccessHooks( } } }, - // [#9719] `dispatchUnscopedMultiDelete` is what makes the #4757 branch + // [#9719] `dispatchUnscopedMultiWrite` is what makes the #4757 branch // above REACHABLE through the wired engine: the predicate path dispatches // per row with `input.id` bound (so the by-id branch shadows the check), // and a zero-match predicate dispatches nothing at all — the engine's // opt-in whole-operation dispatch is the one call that arrives with no id // and the caller's raw `options`, before any row is resolved. - { object: 'sys_attachment', packageId: PACKAGE_ID, dispatchUnscopedMultiDelete: true }, + // + // [#9974] The flag was renamed from `dispatchUnscopedMultiDelete` when the + // mechanism was ruled onto `beforeUpdate` as well. This guard stays + // DELETE-ONLY on purpose: #4757 declares an unscoped-multi refusal for the + // delete verb only, and `sys_attachment` registers no `beforeUpdate` guard + // at all — the flag is per-registration, so declaring it here and nowhere + // else says exactly that. Nothing about this object's accept set changed. + { object: 'sys_attachment', packageId: PACKAGE_ID, dispatchUnscopedMultiWrite: true }, ); } diff --git a/packages/services/service-storage/src/attachment-lifecycle.ts b/packages/services/service-storage/src/attachment-lifecycle.ts index 80babec287..83626e6fe1 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.ts @@ -44,13 +44,15 @@ export interface AttachmentLifecycleEngine { object?: string; packageId?: string; /** - * [#9719] Opt-in: the engine ALSO dispatches this handler once with the - * whole-operation context when a `multi: true` delete carries no `where` - * at all — before any row is resolved. The #4757 unscoped-multi-delete - * refusal in `attachment-access-hooks.ts` declares it; nothing else here - * does. `beforeDelete` registrations only. + * [#9719, both write verbs since #9974] Opt-in: the engine ALSO + * dispatches this handler once with the whole-operation context when a + * `multi: true` write carries no `where` at all — before any row is + * resolved. The #4757 unscoped-multi-delete refusal in + * `attachment-access-hooks.ts` declares it on its `beforeDelete` + * registration; nothing else here does, and no attachment guard declares + * it on update. `beforeUpdate` / `beforeDelete` registrations only. */ - dispatchUnscopedMultiDelete?: boolean; + dispatchUnscopedMultiWrite?: boolean; }, ): void; /** Onion-model data middleware (runs for find/findOne/count/aggregate AND From 81c380d51569844c533da70f77eb90d197fee68b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 01:18:28 +0000 Subject: [PATCH 2/2] docs(changeset): grade the unscoped multi-update refusal as a behaviour change A caller entitled to every row loses a call that works today. Stated as a narrowing of the accept set with the call-site fix named, not as "restoring" a guard, so the release notes read honestly for whoever is affected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM --- .changeset/unscoped-multi-update-refusal.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 .changeset/unscoped-multi-update-refusal.md diff --git a/.changeset/unscoped-multi-update-refusal.md b/.changeset/unscoped-multi-update-refusal.md new file mode 100644 index 0000000000..d3e68d0832 --- /dev/null +++ b/.changeset/unscoped-multi-update-refusal.md @@ -0,0 +1,17 @@ +--- +'@objectstack/objectql': minor +'@objectstack/plugin-audit': minor +'@objectstack/service-storage': patch +--- + +**Behaviour change:** an unscoped `multi: true` UPDATE of `sys_comment` is now refused, where it previously succeeded for a caller entitled to every row (#9974). + +This is not the restoration of a guard that used to work — it is a deliberate narrowing of what the engine accepts, ruled by the maintainer on 2026-08-19. If you issue `ql.update('sys_comment', data, { multi: true })` with **no `where` at all**, that call works today and will start failing with `RECORD_NOT_ACCESSIBLE` / 403. **The fix at the call site is to say which rows you mean** — pass a `where`. The explicit match-all `where: {}` is still accepted and still authorizes every matched row individually; only an *absent* or `null` predicate is refused. + +Why the accept set narrowed rather than the declaration: `resolveTargetRows` has declared this refusal for both write verbs since #4630, but on update it could only ever fire by accident — when the sweep happened to touch a row the caller lacked rights to, and then with a per-row message (`Cannot update comment c2: …`) naming a row rather than the shape. A caller who owned every row had the whole table rewritten, and a zero-match probe resolved silently. A guard that fires by accident reads as enforcement while enforcing nothing. The ruling weighed recoverability: a delete leaves a trace of who removed what, an overwrite leaves none — the old value is gone on the spot with nothing to restore from — and a forgotten `where` is the mistake generated code makes most often. + +**Engine (`@objectstack/objectql`).** #9719's opt-in whole-operation dispatch now covers `beforeUpdate`'s predicate path as well as `beforeDelete`'s, and the registration flag is **renamed** `dispatchUnscopedMultiDelete` → **`dispatchUnscopedMultiWrite`** (one flag generalized to both events rather than a second flag; it is per-registration and per-event, so a delete-only guard still says "delete only" by declaring it on `beforeDelete` alone). Declaring it on any other event is still refused at registration time. Binding `input.id` on the whole-operation context is refused on both verbs (`HookTargetRebindError`, path `unscoped-multi`), and the error now names the caller's event. + +**Blast radius.** The dispatch is delivered ONLY to registrations that declare the flag, so `sys_comment` is the only object whose update accept set changes; every other object's unscoped `multi: true` update behaves exactly as before. `sys_attachment` keeps its delete-only declaration and is unaffected on update. A repo-wide structural sweep of 4 663 source files found no in-tree caller — none in `examples/`, none in the dogfood apps, none in `packages/` source — that issues an unscoped `multi: true` update against a declaring object. + +**`@objectstack/service-storage`** is a rename-only follow: its `sys_attachment` guard declares the renamed flag on the same event, with the same behaviour.