Merge origin/main into claude/issue-4784-hook-condition-previous-scope

Author: claude

.changeset/attachment-unscoped-multi-delete.md

@@ -0,0 +1,37 @@
+---
+"@objectstack/service-storage": patch
+---
+
+fix(service-storage): an UNSCOPED multi-delete of `sys_attachment` is refused instead of authorized (#4757)
+
+`installAttachmentAccessHooks`'s `beforeDelete` gate resolved the rows a delete
+matches in two ways — by `input.id`, or by `input.options.where` — and then
+short-circuited with `if (!rows.length) return`. A delete carrying **neither**
+an id **nor** a `where` took neither branch, so `rows` stayed empty and the gate
+returned *allow*. That is not "nothing matched": nothing was ever queried.
+
+The engine reads the same call as a bulk delete over everything — with no
+single id it seeds the delete AST as `{ object }` and hands that to
+`driver.deleteMany` — so `ql.delete('sys_attachment', { multi: true })` emptied
+the whole attachment table with the record-level gate having authorized exactly
+zero rows. Neither layer underneath catches it: plugin-sharing composes no
+row-scoping predicate for an object with no owner field (`sys_attachment`'s
+provenance column is `uploaded_by`), and plugin-security only refuses callers
+whose grants lack the delete bit on `sys_attachment` — an app shipping the
+domain grant the attachments panel requires passes RBAC and lands here.
+
+The gate now fails **closed** on that shape: no id and no `where` is refused
+with 403 `ATTACHMENT_DELETE_DENIED` ("Refusing an unscoped multi-delete of
+attachments — scope the delete to the rows you mean"), the posture #4630 gave
+`sys_comment` in `resolveTargetRows`. "Nothing to authorize" and "nothing was
+ever queried" are different verdicts, and reading the second as the first is
+fail-open.
+
+Scoped deletes are unchanged: an id-bound delete, a `where`-bound multi-delete,
+and even `where: {}` (which matches every row but is a real query) still resolve
+their rows and authorize each one uploader-or-parent-editor as before — a delete
+that legitimately matches no row still passes. Only the predicate-less call is
+newly refused. If you were relying on `ql.delete('sys_attachment', { multi:
+true })` to clear the table, pass a predicate (`{ multi: true, where: {} }`
+authorizes row-by-row) or perform the sweep under a system context, which
+bypasses the gate as before.

packages/services/service-storage/src/attachment-access-hooks.test.ts

@@ -198,4 +198,73 @@ describe('attachment access — beforeDelete (uploader or parent editor)', () =>
     await expect(beforeDelete(deleteCtx({ id: 'a1' }, { isSystem: true, userId: 'x' }))).resolves.toBeUndefined();
     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 } }],
+    ];
+
+    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,
+        });
+      });
+    }
+
+    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();
+    });
+
+    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();
+    });
+
+    // 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('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' });
+    });
+  });
 });

packages/services/service-storage/src/attachment-access-hooks.ts

@@ -25,7 +25,10 @@ import type {
  *  - beforeDelete: the caller must be the uploader OR hold edit on the
  *    parent record (sharing service's `canEdit`; public-model parents are
  *    editable by design). Fail-closed 403 `ATTACHMENT_DELETE_DENIED`; a
- *    multi-delete requires EVERY matched row to pass.
+ *    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.
  *
  * System-context operations (engine self-writes, seeds, lifecycle sweeps)
  * bypass both gates, as do context-less programmatic calls on bare kernels
@@ -151,9 +154,22 @@ export function installAttachmentAccessHooks(
           const row = await engine.findOne('sys_attachment', { where: { id }, context: { ...SYSTEM_CTX } });
           if (row) rows.push(row);
         }
-      } else if (ctx?.input?.options?.where) {
+      } else {
+        const where = ctx?.input?.options?.where;
+        if (where === undefined || where === null) {
+          // #4757 — no id AND no predicate: the engine hands `deleteMany` an
+          // AST of `{ object }`, i.e. the WHOLE table. Falling through here
+          // would authorize that by resolving zero rows, so refuse instead.
+          // "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.)
+          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)',
+          );
+        }
         rows = await engine.find('sys_attachment', {
-          where: ctx.input.options.where,
+          where,
           limit: MULTI_DELETE_AUTH_LIMIT + 1,
           context: { ...SYSTEM_CTX },
         });
@@ -164,7 +180,9 @@ export function installAttachmentAccessHooks(
           );
         }
       }
-      if (!rows.length) return; // nothing matched — nothing to authorize
+      // Reached only after a real resolve: the query ran and matched no row
+      // (or the ids name no live row), so there is genuinely nothing to gate.
+      if (!rows.length) return;
 
       const sharing = getSharing();
       const callerCtx = callerContext(ctx);