From 89243a92f2decfcb8b2b626f689281e722b603c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 22:45:31 +0000 Subject: [PATCH 1/3] fix(objectql,service-storage): restore the #4757 unscoped multi-delete refusal through the wired engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The predicate path's per-row dispatch (#5038/#5574) binds input.id on every beforeDelete dispatch, so sys_attachment's declared #4757 refusal of a predicate-less multi: true delete always took the handler's by-id branch — and a zero-match predicate dispatched nothing at all. The refusal could not fire through ObjectQL.delete on exactly the shape it refuses. Engine: a new opt-in registration declaration, dispatchUnscopedMultiDelete (beforeDelete only, refused elsewhere), makes ObjectQL.delete's predicate branch dispatch the whole-operation context ONCE — before the matched-row read, zero-match included — to registrations that declared for it. Binding input.id on that context is refused (HookTargetRebindError, path 'unscoped-multi'), mirroring D4/#6752. Undeclared objects see no new dispatch. service-storage: the sys_attachment access-hook registration declares the flag, making the handler's own #4757 branch reachable again with its declared envelope (ATTACHMENT_DELETE_DENIED, 403). The unit block that pinned the whole-operation shape by direct handler call — green while the wired engine did the opposite — is re-pointed at a real ObjectQL engine. Fixes #9719 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM --- ...ine-unscoped-multi-delete-dispatch.test.ts | 381 ++++++++++++++++++ packages/objectql/src/engine.ts | 161 ++++++++ .../objectql/src/hook-target-rebind-errors.ts | 23 +- .../src/attachment-access-hooks.test.ts | 281 ++++++++++--- .../src/attachment-access-hooks.ts | 21 +- .../src/attachment-lifecycle.ts | 13 +- 6 files changed, 815 insertions(+), 65 deletions(-) create mode 100644 packages/objectql/src/engine-unscoped-multi-delete-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 new file mode 100644 index 0000000000..e6a96ad852 --- /dev/null +++ b/packages/objectql/src/engine-unscoped-multi-delete-dispatch.test.ts @@ -0,0 +1,381 @@ +// 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); + engine.registry.registerObject(taskObject); + return { engine, driver }; +} + +/** Seed rows straight into the driver store — no insert hooks involved. */ +function seed(driver: any, object: string, rows: Array>) { + for (const row of rows) 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.ts b/packages/objectql/src/engine.ts index 782d989967..25736849bf 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1328,6 +1328,31 @@ export interface HookEntry { excludeObjects?: string | string[]; 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. + * + * 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. + * + * 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}. + */ + dispatchUnscopedMultiDelete?: boolean; /** * Original metadata-form `Hook` definition this entry was bound from * (when registered via `bindHooksToEngine`). Pure code-paths that call @@ -1554,6 +1579,31 @@ function assertHookScopeNotSelfCancelling( ); } +/** + * [#9719] Registration-time refusal for {@link HookEntry.dispatchUnscopedMultiDelete} + * on any event other than `beforeDelete`. + * + * 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.) + */ +function assertValidUnscopedMultiDeleteFlag( + dispatchUnscopedMultiDelete: boolean | undefined, + event: string, +): void { + if (!dispatchUnscopedMultiDelete || event === 'beforeDelete') 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.", + ); +} + /** Function registry entry — see `registerFunction`. */ export interface FunctionEntry { handler: HookHandler; @@ -2103,6 +2153,12 @@ export class ObjectQL implements IObjectQLEngine { excludeObjects?: string | string[]; 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. + */ + dispatchUnscopedMultiDelete?: boolean; /** Original metadata Hook definition (set by `bindHooksToEngine`). */ meta?: any; /** Stable name from metadata (set by `bindHooksToEngine`). */ @@ -2117,6 +2173,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); // [#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 @@ -2139,6 +2197,7 @@ export class ObjectQL implements IObjectQLEngine { excludeObjects: options?.excludeObjects, priority: options?.priority ?? 100, packageId: options?.packageId, + dispatchUnscopedMultiDelete: options?.dispatchUnscopedMultiDelete, meta: options?.meta, hookName: options?.hookName, }); @@ -2598,6 +2657,94 @@ 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}. + * + * ## 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. + * + * ## What the handler receives + * + * The whole-operation context: `input.id` undefined, `input.options` the + * caller's raw options — the contractual upper-bound read `hook.zod.ts` + * blesses for `before*` handlers (`input.options.where` / `.multi`) — and + * `session` built by the engine, so the handler's own bypass rules (system + * context, no identity envelope) apply unchanged. It is a DERIVED context, + * not the batch-level one (`hook.zod.ts`: no handler is dispatched on that), + * carrying `dispatch: { mode: 'record', index: 0 }` — this call stands for + * the caller's whole write, not for one row of it — with the batch `scope` + * object kept identity-shared so a stash still reaches the after phase. + * + * ## 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. + * + * ## The id slot is not a lever here either + * + * The old batch dispatch's `input.id` was present-but-`undefined`, and + * binding it rerouted the write onto the by-id branch. The ladder is now + * resolved before any handler runs, so a handler binding `input.id` on this + * context would retarget nothing — refused rather than ignored, the same + * rule as D4 and #6752 (`HookTargetRebindError`, path `'unscoped-multi'`). + */ + private async dispatchUnscopedMultiDeleteHooks( + object: string, + batchCtx: HookContext, + ): Promise { + const entries = this.hooks.get('beforeDelete'); + if (!entries || entries.length === 0) return; + const flagged = entries.filter( + (entry) => entry.dispatchUnscopedMultiDelete && hookMatchesObject(entry, object), + ); + if (flagged.length === 0) return; + // Mirrors `triggerHooks`' opt-out rule: `skipAutomations` suppresses only + // metadata-bound entries. No metadata binding can set the flag today, so + // this is parity with the dispatch loop rather than a reachable branch — + // kept so the two rules cannot drift if that ever changes. + const skipAutomations = + (batchCtx.session as { skipAutomations?: boolean } | undefined)?.skipAutomations === true; + const wholeOpCtx = { + ...batchCtx, + dispatch: { + mode: 'record', + index: 0, + scope: (batchCtx.dispatch as { scope: Record }).scope, + }, + } as unknown as HookContext; + for (const entry of flagged) { + if (skipAutomations && entry.meta) continue; + await entry.handler(wholeOpCtx); + } + // `input` is identity-shared with the batch context, so the observation + // covers everything the handlers touched. + const observed = (wholeOpCtx.input as { id?: unknown }).id; + if (observed !== undefined) { + throw new HookTargetRebindError({ + object, + event: 'beforeDelete', + path: 'unscoped-multi', + expectedId: undefined, + observedId: observed, + }); + } + } + /** * [#5038, one ceiling for both phases since #5574] Ceiling on the matched-row * set a predicate write fires per-row hooks over. @@ -10786,6 +10933,20 @@ export class ObjectQL implements IObjectQLEngine { `(the predicate branch was reached without the #2982 seed).`, ); } + // [#9719] The unscoped-multi shape check, BEFORE the matched-row read: + // a `multi: true` delete 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 (#4757) fires regardless of + // what the predicate would match, zero rows included. 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.dispatchUnscopedMultiDeleteHooks(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 // (D7). Gated on this object actually having delete-side hooks, so a diff --git a/packages/objectql/src/hook-target-rebind-errors.ts b/packages/objectql/src/hook-target-rebind-errors.ts index 6f75090d78..9cb5102581 100644 --- a/packages/objectql/src/hook-target-rebind-errors.ts +++ b/packages/objectql/src/hook-target-rebind-errors.ts @@ -99,7 +99,16 @@ export type HookTargetRebindPath = /** A by-id `update()` / `delete()` whose `before*` handler moved or cleared the id. */ | 'by-id' /** A per-row `before*` context on a predicate write (D4). */ - | 'per-row'; + | '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. + */ + | 'unscoped-multi'; export class HookTargetRebindError extends Error { override readonly name = 'HookTargetRebindError'; @@ -165,9 +174,15 @@ function buildMessage(info: { `validation rules — so a by-id target is immutable once a handler runs, on BOTH verbs. ` + `'delete()' honoured a rebind until #6752 by re-resolving the new target; that is retired ` + `too, so one rule now covers both.` - : ` 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.`; + : 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.` + : ` 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.`; const routes = ` To write a DIFFERENT row, call 'ctx.api' / 'ctx.ql' for that row explicitly. To write MANY rows, ` + 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 ce8d3509ce..08c1e47296 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.test.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.test.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; import { installAttachmentAccessHooks, type AttachmentSharingLike } from './attachment-access-hooks.js'; import type { AttachmentLifecycleEngine } from './attachment-lifecycle.js'; @@ -199,73 +200,237 @@ describe('attachment access — beforeDelete (uploader or parent editor)', () => await expect(beforeDelete(deleteCtx({ id: 'missing' }, { userId: 'x' }))).resolves.toBeUndefined(); }); - // #4757 — no id AND no `where` is not "nothing matched", it is "nothing was - // ever queried": the engine seeds `{ object }` as the delete AST and hands - // it to `driver.deleteMany`, which empties the table. The gate must refuse - // rather than fall through the empty-`rows` short-circuit. - describe('unscoped multi-delete (no id, no where) — #4757', () => { - const unscopedShapes: Array<[string, any]> = [ - ['options.multi with no where', { options: { multi: true } }], - ['no id and no options at all', {}], - ['an explicitly null where', { options: { multi: true, where: null } }], - ['an explicitly undefined where', { options: { multi: true, where: undefined } }], - ]; + // The #4757 unscoped-multi-delete block that used to sit here called the + // handler DIRECTLY with a whole-operation context of this file's own + // construction — a shape the engine's per-row dispatch (#5038/#5574) never + // produced, so the block stayed green for a behaviour the wired engine did + // the opposite of (#9719's measurement: an uploader-owned table was wiped by + // `{ multi: true }` while these cases passed). It is re-pointed at the REAL + // engine below — see "#4757 through the wired engine". +}); + +// ───────────────────────────────────────────────────────────────────────── +// #4757 through the WIRED engine (#9719) +// +// 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` +// 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 +// green suite over a wiped table. +// +// NOTE `@objectstack/objectql` is deliberately un-aliased in this package's +// vitest config (see `KNOWN_UNALIASED_TEST_IMPORTS`): these pins run against +// objectql's BUILT dist, so a stale build tests yesterday's engine — rebuild +// `@objectstack/objectql` before trusting a verdict from this block. +// ───────────────────────────────────────────────────────────────────────── - for (const [label, input] of unscopedShapes) { - it(`refuses ${label} (403 ATTACHMENT_DELETE_DENIED)`, async () => { - const { beforeDelete } = install({ attachments: [row], sharing: { canEdit: async () => true } }); - await expect(beforeDelete(deleteCtx(input, { userId: 'uploader' }))).rejects.toMatchObject({ - code: 'ATTACHMENT_DELETE_DENIED', - status: 403, - }); - }); +const ATT_FIELDS = { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + file_id: { name: 'file_id', label: 'File', type: 'text' as const }, + parent_object: { name: 'parent_object', label: 'Parent Object', type: 'text' as const }, + parent_id: { name: 'parent_id', label: 'Parent Id', type: 'text' as const }, + uploaded_by: { name: 'uploaded_by', label: 'Uploaded By', type: 'text' as const }, +}; +const sysAttachmentObject = { name: 'sys_attachment', label: 'Attachment', fields: ATT_FIELDS }; +const attSecretObject = { + name: 'att_secret', + label: 'Secret', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + }, +}; + +/** In-memory driver whose WHERE matcher REFUSES combinators/operator values by + * throwing — the conforming shape `check-where-matcher-conformance.mjs` asks + * of a double (silently wrong answers are the defect class, not incompleteness). */ +function makeWiredDriver() { + 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(`wired stub driver: unsupported combinator ${k}`); + if (v !== null && typeof v === 'object') throw new Error(`wired 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, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, async syncSchema() {}, + async find(o: string, ast: any) { + return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(o: string, ast: any) { + 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) { 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) { + 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; +} - it('refuses even the uploader of every matched row — the AST is unscoped, not row-scoped', async () => { - // The uploader shortcut is per RESOLVED row; with nothing resolved there - // is no row whose ownership could license emptying the table. - const canEdit = vi.fn(async () => true); - const { beforeDelete } = install({ - attachments: [{ ...row, uploaded_by: 'uploader' }], - sharing: { canEdit }, - }); - await expect( - beforeDelete(deleteCtx({ options: { multi: true } }, { userId: 'uploader' })), - ).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED' }); - expect(canEdit).not.toHaveBeenCalled(); - }); +async function bootWired(opts: { + attachments?: Array>; + sharing?: AttachmentSharingLike | null; +} = {}) { + const ql = new ObjectQL(); + const driver = makeWiredDriver(); + ql.registerDriver(driver, true); + await ql.init(); + ql.registry.registerObject(sysAttachmentObject as any); + ql.registry.registerObject(attSecretObject as any); + // `engine as any` mirrors the production wiring in storage-service-plugin.ts. + installAttachmentAccessHooks(ql as any, () => opts.sharing ?? null, silentLogger()); + for (const row of opts.attachments ?? []) { + driver.stores.get('sys_attachment') ?? driver.stores.set('sys_attachment', new Map()); + driver.stores.get('sys_attachment')!.set(String(row.id), { ...row }); + } + const remaining = () => driver.stores.get('sys_attachment')?.size ?? 0; + return { ql, driver, remaining }; +} - it('still bypasses for system context and context-less calls', async () => { - const { beforeDelete } = install({ attachments: [row], sharing: { canEdit: async () => false } }); - await expect( - beforeDelete(deleteCtx({ options: { multi: true } }, { isSystem: true, userId: 'x' })), - ).resolves.toBeUndefined(); - await expect(beforeDelete(deleteCtx({ options: { multi: true } }, {}))).resolves.toBeUndefined(); +const wiredRow = (id: string, uploadedBy: string, parentId = 'r1') => ({ + id, file_id: `f_${id}`, parent_object: 'att_secret', parent_id: parentId, uploaded_by: uploadedBy, +}); + +const UNSCOPED_REFUSAL = expect.objectContaining({ + code: 'ATTACHMENT_DELETE_DENIED', + status: 403, + // The first sentence IS the declared contract (#4757's quoted refusal). + message: expect.stringContaining('Refusing an unscoped multi-delete of attachments'), +}); + +describe('unscoped multi-delete (no id, no where) — #4757 through the wired engine (#9719)', () => { + it('refuses `{ multi: true }` even when the caller is the uploader of EVERY matched row — and the rows survive', async () => { + // #9719's measured gap verbatim: before the fix this call RESOLVED and + // wiped both rows; the per-row uploader shortcut licensed each row and no + // dispatch ever carried the unscoped shape. + const { ql, remaining } = await bootWired({ + attachments: [wiredRow('a1', 'uploader'), wiredRow('a2', 'uploader', 'r2')], }); + await expect( + ql.delete('sys_attachment', { multi: true, context: { userId: 'uploader' } } as any), + ).rejects.toEqual(UNSCOPED_REFUSAL); + expect(remaining()).toBe(2); + }); - // The scoped paths must be untouched by the fix: an id-bound delete and a - // `where`-bound one still authorize row-by-row and still ALLOW when they - // pass. `where: {}` matches every row but is a real query — every matched - // row is authorized, so it stays on the authorize path, not the refuse one. - it('leaves the legitimate scoped paths alone', async () => { - const { beforeDelete } = install({ attachments: [row], sharing: { canEdit: async () => true } }); - await expect(beforeDelete(deleteCtx({ id: 'a1' }, { userId: 'stranger' }))).resolves.toBeUndefined(); - await expect( - beforeDelete( - deleteCtx({ options: { where: { parent_object: 'att_secret' }, multi: true } }, { userId: 'stranger' }), - ), - ).resolves.toBeUndefined(); - await expect( - beforeDelete(deleteCtx({ options: { where: {}, multi: true } }, { userId: 'stranger' })), - ).resolves.toBeUndefined(); + it('refuses an explicitly null `where` the same way', async () => { + const { ql, remaining } = await bootWired({ attachments: [wiredRow('a1', 'uploader')] }); + await expect( + ql.delete('sys_attachment', { multi: true, where: null, context: { userId: 'uploader' } } as any), + ).rejects.toEqual(UNSCOPED_REFUSAL); + expect(remaining()).toBe(1); + }); + + it('refuses on an EMPTY table — "nothing was ever queried" is not "nothing to authorize"', async () => { + // The zero-match limb (#9719): the per-row dispatch is gated on matched + // rows, so a handler-only fix can never fire here — a caller probing + // against an empty table would see success and ship the unscoped delete. + const { ql } = await bootWired({ attachments: [] }); + await expect( + ql.delete('sys_attachment', { multi: true, context: { userId: 'uploader' } } as any), + ).rejects.toEqual(UNSCOPED_REFUSAL); + }); + + it('positive control: an empty table is not refused per se — a scoped `where: {}` delete of it resolves', async () => { + // Proves the empty-table refusal above measures the SHAPE, not the + // emptiness: the same table, queried for real, deletes zero rows quietly. + const { ql } = await bootWired({ attachments: [] }); + await expect( + ql.delete('sys_attachment', { multi: true, where: {}, context: { userId: 'uploader' } } as any), + ).resolves.toBeDefined(); + }); + + it('the per-row gate is a DIFFERENT refusal and still fires through the wire', async () => { + // #9719's other measured row: a scoped delete matching a row the caller is + // not entitled to still answers with the PER-ROW message, not #4757's. + const { ql, remaining } = await bootWired({ + attachments: [wiredRow('a1', 'member'), wiredRow('a2', 'someone-else', 'r2')], + sharing: { canEdit: async () => false }, }); + await expect( + ql.delete('sys_attachment', { + multi: true, + where: { parent_object: 'att_secret' }, + context: { userId: 'member' }, + } as any), + ).rejects.toEqual( + expect.objectContaining({ + code: 'ATTACHMENT_DELETE_DENIED', + status: 403, + message: expect.stringContaining('Cannot delete attachment'), + }), + ); + expect(remaining()).toBe(2); + }); - it('an empty `where` still authorizes every matched row (one failing row denies)', async () => { - const { beforeDelete } = install({ attachments: [row], sharing: { canEdit: async () => false } }); - await expect( - beforeDelete(deleteCtx({ options: { where: {}, multi: true } }, { userId: 'stranger' })), - ).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED' }); + 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 attachment delete. + const byId = await bootWired({ attachments: [wiredRow('a1', 'uploader')] }); + await expect( + byId.ql.delete('sys_attachment', { where: { id: 'a1' }, context: { userId: 'uploader' } } as any), + ).resolves.toBeDefined(); + expect(byId.remaining()).toBe(0); + + const byWhere = await bootWired({ + attachments: [wiredRow('a1', 'uploader'), wiredRow('a2', 'uploader', 'r2')], }); + await expect( + byWhere.ql.delete('sys_attachment', { + multi: true, + where: { uploaded_by: 'uploader' }, + context: { userId: 'uploader' }, + } as any), + ).resolves.toBeDefined(); + expect(byWhere.remaining()).toBe(0); + + // `where: {}` is a REAL match-all query (the declared semantics): every + // matched row is authorized per row, and an entitled caller may empty the + // table with it — the refusal is about an ABSENT predicate only. + const matchAll = await bootWired({ attachments: [wiredRow('a1', 'uploader')] }); + await expect( + matchAll.ql.delete('sys_attachment', { multi: true, where: {}, context: { userId: 'uploader' } } as any), + ).resolves.toBeDefined(); + expect(matchAll.remaining()).toBe(0); + }); + + it('still bypasses for system context and for context-less programmatic calls', async () => { + const system = await bootWired({ attachments: [wiredRow('a1', 'someone')] }); + await expect( + system.ql.delete('sys_attachment', { multi: true, context: { isSystem: true } } as any), + ).resolves.toBeDefined(); + expect(system.remaining()).toBe(0); + + const bare = await bootWired({ attachments: [wiredRow('a1', 'someone')] }); + await expect( + bare.ql.delete('sys_attachment', { multi: true } as any), + ).resolves.toBeDefined(); + expect(bare.remaining()).toBe(0); }); }); diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index 8e57f0d885..d753eaf021 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.ts @@ -31,7 +31,10 @@ import type { * multi-delete requires EVERY matched row to pass, and one carrying * 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. + * resolved no rows for it would be authorizing exactly that. The refusal + * reaches this handler through the `dispatchUnscopedMultiDelete` + * whole-operation dispatch its registration declares (#9719) — the + * per-row dispatch alone can never deliver the shape it refuses. * * System-context operations (engine self-writes, seeds, lifecycle sweeps) * bypass both gates, as do context-less programmatic calls on bare kernels @@ -214,6 +217,14 @@ export function installAttachmentAccessHooks( // "Nothing to authorize" and "nothing was ever queried" are not the // same verdict; reading the second as the first is fail-open. // (Mirrors #4630's `resolveTargetRows` for sys_comment.) + // + // [#9719] Reached through the wired engine ONLY via the + // `dispatchUnscopedMultiDelete` 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 + // zero-match predicate dispatches nothing at all, so without that + // declaration this refusal cannot fire, whatever this file says. forbid( 'ATTACHMENT_DELETE_DENIED', 'Refusing an unscoped multi-delete of attachments — scope the delete to the rows you mean (an id or a where predicate)', @@ -273,7 +284,13 @@ export function installAttachmentAccessHooks( } } }, - { object: 'sys_attachment', packageId: PACKAGE_ID }, + // [#9719] `dispatchUnscopedMultiDelete` 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 }, ); } diff --git a/packages/services/service-storage/src/attachment-lifecycle.ts b/packages/services/service-storage/src/attachment-lifecycle.ts index 90be7c144b..80babec287 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.ts @@ -40,7 +40,18 @@ export interface AttachmentLifecycleEngine { registerHook( event: string, handler: (ctx: any) => void | Promise, - options?: { object?: string; packageId?: string }, + options?: { + 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. + */ + dispatchUnscopedMultiDelete?: boolean; + }, ): void; /** Onion-model data middleware (runs for find/findOne/count/aggregate AND * writes) — the only seam that filters `count()` (→ list `total`) From 46cb6e3f7dc9053209ecd4df6c1c20ce44496767 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 23:09:57 +0000 Subject: [PATCH 2/3] chore: changeset for the #4757 unscoped multi-delete restoration Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM --- .changeset/unscoped-multi-delete-refusal.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 .changeset/unscoped-multi-delete-refusal.md diff --git a/.changeset/unscoped-multi-delete-refusal.md b/.changeset/unscoped-multi-delete-refusal.md new file mode 100644 index 0000000000..2e7806dd62 --- /dev/null +++ b/.changeset/unscoped-multi-delete-refusal.md @@ -0,0 +1,10 @@ +--- +'@objectstack/objectql': minor +'@objectstack/service-storage': patch +--- + +Restore the #4757 unscoped multi-delete refusal on `sys_attachment` through the wired engine (#9719). + +`ObjectQL.registerHook` gains an opt-in `dispatchUnscopedMultiDelete` declaration (valid on `beforeDelete` registrations only — anything else is refused at registration): when a `multi: true` delete arrives with no `where` at all (absent or `null`), the engine's predicate path dispatches the whole-operation context ONCE to declaring registrations — before any matched row is resolved, zero-match included — so a guard about the operation's shape can refuse it. Binding `input.id` on that context is refused (`HookTargetRebindError`, path `'unscoped-multi'`). Undeclared registrations, scoped deletes (including the match-all `where: {}`), and by-id deletes see no new dispatch. + +The `sys_attachment` access guard declares the flag, so its documented refusal of a predicate-less multi-delete fires again with its declared envelope (`ATTACHMENT_DELETE_DENIED`, HTTP 403): since the per-row dispatch contract (#5038/#5574) that branch was unreachable, and a predicate-less `multi: true` delete quietly removed every row the caller happened to be entitled to. System-context and context-less programmatic deletes bypass the guard exactly as before. From f906baf2b80394ed5c91c6990ea4c8adb702d320 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 23:39:36 +0000 Subject: [PATCH 3/3] test: keep the new suites out of the frozen TEST_DEBT ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check:type-check-debt re-measured @objectstack/objectql at +3 over its frozen 355: two 1-arg registerObject calls and one unused loop variable, all in the new dispatch test. Fixed at the source (registerObject gets its packageId; the seed loop no longer declares an unused row) — the ledger number is back to exactly 355. The service-storage harness carried the same two idioms latently (its test layer is not ledger-measured today) — fixed the same way. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM --- .../src/engine-unscoped-multi-delete-dispatch.test.ts | 6 +++--- .../service-storage/src/attachment-access-hooks.test.ts | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/objectql/src/engine-unscoped-multi-delete-dispatch.test.ts b/packages/objectql/src/engine-unscoped-multi-delete-dispatch.test.ts index e6a96ad852..fa748edac8 100644 --- a/packages/objectql/src/engine-unscoped-multi-delete-dispatch.test.ts +++ b/packages/objectql/src/engine-unscoped-multi-delete-dispatch.test.ts @@ -113,14 +113,14 @@ async function boot() { const driver = makeStubDriver(); engine.registerDriver(driver, true); await engine.init(); - engine.registry.registerObject(attObject); - engine.registry.registerObject(taskObject); + 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>) { - for (const row of rows) driver.stores.get(object) ?? driver.stores.set(object, new Map()); + 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 }); } 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 08c1e47296..e9922f1eff 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.test.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.test.ts @@ -302,12 +302,12 @@ async function bootWired(opts: { const driver = makeWiredDriver(); ql.registerDriver(driver, true); await ql.init(); - ql.registry.registerObject(sysAttachmentObject as any); - ql.registry.registerObject(attSecretObject as any); + ql.registry.registerObject(sysAttachmentObject as any, 'app:test'); + ql.registry.registerObject(attSecretObject as any, 'app:test'); // `engine as any` mirrors the production wiring in storage-service-plugin.ts. installAttachmentAccessHooks(ql as any, () => opts.sharing ?? null, silentLogger()); + if (!driver.stores.get('sys_attachment')) driver.stores.set('sys_attachment', new Map()); for (const row of opts.attachments ?? []) { - driver.stores.get('sys_attachment') ?? driver.stores.set('sys_attachment', new Map()); driver.stores.get('sys_attachment')!.set(String(row.id), { ...row }); } const remaining = () => driver.stores.get('sys_attachment')?.size ?? 0;