diff --git a/.changeset/attachment-unscoped-multi-delete.md b/.changeset/attachment-unscoped-multi-delete.md new file mode 100644 index 0000000000..45bf88febd --- /dev/null +++ b/.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. 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 02b8fc2ec2..dab3567a52 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.test.ts +++ b/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' }); + }); + }); }); diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index ba3dc489de..537363d8c7 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.ts +++ b/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);