fix(metadata-protocol,rest): the id list is the only thing deleteMany can select on (#3897)

`deleteManyData` built `{ where: { id: { $in: ids } } }` and then spread the
caller-supplied `options` OVER it, and the REST route splatted the whole request
body into the protocol request. So `{"ids":["a"],"options":{"multi":true,"where":{}}}`
reached `engine.delete` as an unscoped bulk delete — widening "delete these
records" into "delete everything this caller is allowed to delete" (measured on
a stock CRM dev deployment: one id in, all 8 rows gone). The same spread also
accepted `context`, i.e. a forged principal where the route is reachable
without auth.

The engine options are now built from the validated id list alone — caller
`options` is a `BatchOptions` bag that carries nothing `engine.delete` consumes,
so merging it could only ever smuggle in engine keys. Ids must be scalars, so an
operator object cannot reach `where.id` either. The REST route parses the body
against `DeleteManyDataRequestSchema` one hop earlier; Zod strips unknown keys,
so `options.where`, top-level `where` and a body `context` never survive the
ingress.

The endpoint also works for the first time: `multi` was never set, so a
well-formed `{"ids":[…]}` only ever hit the engine's
'Delete requires an ID or options.multi=true' throw. Deletes now run per id by
primary key, which also honours `deleteBehavior` (the bulk branch skips
`cascadeDeleteRelations`) and makes the declared `BatchUpdateResponse` — per-id
`results`, `atomic`, `continueOnError` — actually deliverable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TzLE9cw4gZKNyPN2ZP4iTt

Author: claude

.changeset/delete-many-id-predicate.md

@@ -0,0 +1,63 @@
+---
+"@objectstack/metadata-protocol": minor
+"@objectstack/rest": minor
+---
+
+fix(metadata-protocol,rest): the id list is the only thing deleteMany can select on (#3897)
+
+`deleteManyData` built the predicate its endpoint is named after and then spread
+the caller's `options` **over** it:
+
+```js
+return this.engine.delete(request.object, {
+    where: { id: { $in: request.ids } },
+    ...request.options          // ← lands after `where`, so it can replace it
+});
+```
+
+`request.options` is caller-supplied — `POST /data/:object/deleteMany` splatted
+the whole request body into the protocol request (`{ object, ...req.body }`) —
+so one body key rewrote the operation:
+
+```json
+{"ids":["a"], "options":{"multi":true,"where":{}}}
+```
+
+reached `engine.delete` as an unscoped bulk delete. The engine's write
+middleware still composes RLS/sharing predicates onto the AST, so the blast
+radius is not automatically the whole table: it is **everything the caller is
+allowed to delete**. For an ordinary user with delete permission that is the
+difference between the 3 records they asked for and every record they can see;
+measured on a stock CRM dev deployment, that payload against one id removed all
+8 rows in the object and returned the raw driver count (`8`). The same spread
+also accepted `context`, i.e. a forged principal wherever the route is reachable
+without auth.
+
+**The id set is now authoritative, structurally.** The engine options are built
+from the validated id list and nothing else — caller `options` is a
+`BatchOptions` bag (`atomic` / `returnRecords` / `continueOnError` /
+`validateOnly`) that carries nothing `engine.delete` consumes, so merging it
+could only ever smuggle in engine keys. Ids must be scalars, so an operator
+object (`{"ids":[{"$ne":null}]}`) cannot reach `where.id` either; a malformed
+list is a `400 VALIDATION_FAILED` instead of a wider delete. The REST route
+parses the body against `DeleteManyDataRequestSchema` first, one hop earlier —
+Zod object schemas strip unknown keys, so `options.where`, top-level `where` and
+a body `context` no longer survive the ingress at all.
+
+**The endpoint also works now.** `deleteManyData` never set `multi`, so a
+correctly-formed `{"ids":[…]}` hit the engine's
+`'Delete requires an ID or options.multi=true'` throw — only the requests that
+triggered the override above ever completed. Deletes now go one id at a time by
+primary key, the same shape `batchData`'s `delete` case uses, which closes two
+gaps behind that: the bulk branch skips `cascadeDeleteRelations`, so
+`deleteBehavior` (`cascade` / `set_null` / `restrict`) was not honoured for the
+rows it removed; and the declared `BatchUpdateResponse` contract (per-record
+`results`, `atomic`, `continueOnError`) was unimplementable from a bulk row
+count. Both are delivered rather than declared.
+
+**Behaviour change.** The endpoint returns a `BatchUpdateResponse`
+(`{ success, operation, total, succeeded, failed, results }`) where it
+previously returned the driver's raw delete count — on the paths where it
+returned anything at all. The caller's execution context is threaded to every
+delete, so RLS/FLS now run under the caller here as they do on the single-record
+route.

content/docs/api/data-api.mdx

@@ -126,7 +126,16 @@ Batch update multiple records.
 
 Batch delete records by ID list.
 
-**Body**: `{ ids: ["1", "2", "3"] }`
+**Body**: `{ "ids": ["1", "2", "3"], "options": { "atomic": true } }` — `options` is
+the same `BatchOptions` bag `/batch` takes. The body is validated against the
+contract and unknown keys are dropped: the id list is the *only* thing that
+selects rows, so no body key can widen the delete into a filter.
+
+**Response**: `BatchUpdateResponse` — one `results` entry per id. Records are
+deleted one at a time by primary key, so each honours `deleteBehavior`
+(`cascade` / `set_null` / `restrict`) on relations pointing at it. `atomic`
+(default) stops the run at the first failure; `atomic: false` with
+`continueOnError: true` processes the remaining ids and reports the failures.
 
 ---
 

packages/metadata-protocol/src/protocol.delete-many.test.ts

@@ -0,0 +1,159 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+//
+// [#3897] `deleteManyData` used to spread the caller-supplied `options` OVER
+// the id predicate it had just built, so a request body could replace `where`
+// outright and widen "delete these ids" into "delete everything this caller is
+// allowed to delete". The same spread could also smuggle in `context` (a forged
+// principal) and `multi`. These tests pin the three properties that make that
+// impossible now: the id set is authoritative, caller `options` never reaches
+// the engine, and the happy path actually deletes (it used to throw
+// `'Delete requires an ID or options.multi=true'` because `multi` was never set).
+
+import { describe, it, expect, vi } from 'vitest';
+import { ObjectStackProtocolImplementation } from './protocol.js';
+
+const SCHEMA = { name: 'invoice', fields: { title: { name: 'title', type: 'text' } } };
+
+function makeProtocol(deleteImpl?: (object: string, options: any) => Promise<any>) {
+  const del = vi.fn(deleteImpl ?? (async () => ({ deleted: true })));
+  const engine = {
+    registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) },
+    delete: del,
+  };
+  return { p: new ObjectStackProtocolImplementation(engine as any), del };
+}
+
+describe('deleteManyData — the id set is the contract (#3897)', () => {
+  it('deletes exactly the supplied ids and reports a BatchUpdateResponse', async () => {
+    const { p, del } = makeProtocol();
+    const res: any = await p.deleteManyData({ object: 'invoice', ids: ['a', 'b', 'c'] } as any);
+
+    expect(del).toHaveBeenCalledTimes(3);
+    expect(del.mock.calls.map((c) => c[1].where)).toEqual([
+      { id: 'a' }, { id: 'b' }, { id: 'c' },
+    ]);
+    expect(res).toMatchObject({
+      success: true, operation: 'delete', total: 3, succeeded: 3, failed: 0,
+    });
+    expect(res.results).toEqual([
+      { id: 'a', success: true }, { id: 'b', success: true }, { id: 'c', success: true },
+    ]);
+  });
+
+  it('the happy path no longer trips the engine\'s "ID or options.multi=true" guard', async () => {
+    // Stand in for engine.delete's real dispatch: a scalar `where.id` is a
+    // by-id delete; anything else needs `options.multi` or it throws. The
+    // pre-#3897 implementation produced `{ id: { $in: [...] } }` with no
+    // `multi`, so a well-formed request could only ever hit the throw.
+    const { p } = makeProtocol(async (_object, options) => {
+      const id = options?.where?.id;
+      const scalar = typeof id === 'string' || typeof id === 'number';
+      if (!scalar && !options?.multi) throw new Error('Delete requires an ID or options.multi=true');
+      return { id };
+    });
+
+    const res: any = await p.deleteManyData({ object: 'invoice', ids: ['a'] } as any);
+    expect(res.success).toBe(true);
+    expect(res.failed).toBe(0);
+  });
+
+  it('a body-supplied options.where cannot replace the id predicate', async () => {
+    const { p, del } = makeProtocol();
+    // The exploit payload from the issue, verbatim.
+    await p.deleteManyData({
+      object: 'invoice',
+      ids: ['a'],
+      options: { multi: true, where: {} },
+    } as any);
+
+    expect(del).toHaveBeenCalledTimes(1);
+    const opts = del.mock.calls[0][1];
+    expect(opts.where).toEqual({ id: 'a' });
+    expect(opts.multi).toBeUndefined();
+  });
+
+  it('a body-supplied options.context cannot forge the caller principal', async () => {
+    const { p, del } = makeProtocol();
+    await p.deleteManyData({
+      object: 'invoice',
+      ids: ['a'],
+      options: { context: { userId: 'root', roles: ['admin'] } },
+    } as any);
+
+    expect(del.mock.calls[0][1].context).toBeUndefined();
+  });
+
+  it('threads the resolved execution context to every engine delete', async () => {
+    const { p, del } = makeProtocol();
+    const ctx = { userId: 'u1' };
+    await p.deleteManyData({ object: 'invoice', ids: ['a', 'b'], context: ctx } as any);
+
+    expect(del.mock.calls[0][1].context).toBe(ctx);
+    expect(del.mock.calls[1][1].context).toBe(ctx);
+  });
+
+  it('rejects non-scalar ids instead of letting an operator object reach where.id', async () => {
+    const { p, del } = makeProtocol();
+    await expect(
+      p.deleteManyData({ object: 'invoice', ids: [{ $ne: null }] } as any),
+    ).rejects.toMatchObject({ code: 'VALIDATION_FAILED', status: 400 });
+    expect(del).not.toHaveBeenCalled();
+  });
+
+  it('rejects a missing / non-array ids rather than deleting unscoped', async () => {
+    const { p, del } = makeProtocol();
+    await expect(
+      p.deleteManyData({ object: 'invoice', options: { multi: true } } as any),
+    ).rejects.toMatchObject({ code: 'VALIDATION_FAILED', status: 400 });
+    expect(del).not.toHaveBeenCalled();
+  });
+
+  it('an empty id list deletes nothing', async () => {
+    const { p, del } = makeProtocol();
+    const res: any = await p.deleteManyData({ object: 'invoice', ids: [] } as any);
+    expect(del).not.toHaveBeenCalled();
+    expect(res).toMatchObject({ success: true, total: 0, succeeded: 0, failed: 0, results: [] });
+  });
+});
+
+describe('deleteManyData — partial-failure semantics (#3897)', () => {
+  function failOn(badId: string) {
+    return makeProtocol(async (_object, options) => {
+      if (options.where.id === badId) throw new Error('RLS: not visible');
+      return { id: options.where.id };
+    });
+  }
+
+  it('stops at the first failure by default and reports it per row', async () => {
+    const { p, del } = failOn('b');
+    const res: any = await p.deleteManyData({ object: 'invoice', ids: ['a', 'b', 'c'] } as any);
+
+    expect(del).toHaveBeenCalledTimes(2);
+    expect(res).toMatchObject({ success: false, total: 3, succeeded: 1, failed: 1 });
+    expect(res.results[1]).toEqual({ id: 'b', success: false, error: 'RLS: not visible' });
+  });
+
+  it('continueOnError keeps going and still marks the batch unsuccessful', async () => {
+    const { p, del } = failOn('b');
+    const res: any = await p.deleteManyData({
+      object: 'invoice',
+      ids: ['a', 'b', 'c'],
+      options: { atomic: false, continueOnError: true },
+    } as any);
+
+    expect(del).toHaveBeenCalledTimes(3);
+    expect(res).toMatchObject({ success: false, total: 3, succeeded: 2, failed: 1 });
+  });
+
+  it('atomic aborts the remaining ids on the first failure', async () => {
+    const { p, del } = failOn('b');
+    const res: any = await p.deleteManyData({
+      object: 'invoice',
+      ids: ['a', 'b', 'c'],
+      options: { atomic: true, continueOnError: true },
+    } as any);
+
+    expect(del).toHaveBeenCalledTimes(2);
+    expect(res.succeeded).toBe(1);
+  });
+});

packages/metadata-protocol/src/protocol.ts

@@ -3805,13 +3805,92 @@ export class ObjectStackProtocolImplementation implements
         throw new Error('triggerAutomation requires plugin-automation service. Install and register a plugin that provides the "automation" service.');
     }
 
-    async deleteManyData(request: DeleteManyDataRequest): Promise<any> {
-        this.assertObjectRegistered(request.object); // [#3770]
-        // This expects deleting by IDs.
-        return this.engine.delete(request.object, {
-            where: { id: { $in: request.ids } },
-            ...request.options
-        });
+    /**
+     * Bulk delete by id — the `POST /data/:object/deleteMany` ingress.
+     *
+     * [#3897] This used to build `{ where: { id: { $in: ids } } }` and then
+     * spread `...request.options` OVER it. `options` is caller-supplied (the
+     * REST route splats the whole request body into the protocol request), so
+     * a body key replaced the predicate the endpoint is named after:
+     *
+     *   {"ids":["a"],"options":{"multi":true,"where":{}}}
+     *
+     * reached `engine.delete` as an unscoped bulk delete — widening "delete
+     * these 3 records" into "delete everything this caller is allowed to
+     * delete" (RLS/sharing middleware still composes onto the AST, so the blast
+     * radius is the caller's visible set, not the whole table). The same spread
+     * could smuggle in `context`, i.e. a forged principal on any deployment
+     * where the route is reachable without auth.
+     *
+     * The fix is structural rather than a re-ordered spread: caller `options`
+     * is a `BatchOptions` bag (`atomic` / `returnRecords` /
+     * `continueOnError` / `validateOnly`) and carries NOTHING `engine.delete`
+     * consumes, so it is never merged into the engine options. The engine call
+     * is built here from the validated id list alone, and each id is deleted by
+     * scalar primary key — the same shape `batchData`'s `delete` case uses.
+     *
+     * Deleting per id (instead of one `$in` bulk delete) also fixes the second
+     * half of #3897 and two silent gaps behind it:
+     *   - the endpoint's happy path never worked at all — `deleteManyData` never
+     *     set `multi`, so a well-formed `{"ids":[…]}` hit engine.ts's
+     *     `'Delete requires an ID or options.multi=true'` throw, and ONLY the
+     *     requests that triggered the override above got through;
+     *   - the bulk branch skips `cascadeDeleteRelations`, so `deleteBehavior`
+     *     (`cascade` / `set_null` / `restrict`) was not honoured for the rows it
+     *     removed;
+     *   - the declared {@link BatchUpdateResponse} contract (per-record results,
+     *     `atomic` / `continueOnError`) was unimplementable from a bulk row
+     *     count. It is now actually delivered.
+     */
+    async deleteManyData(request: DeleteManyDataRequest & { context?: any }): Promise<BatchUpdateResponse> {
+        const { object, options, context } = request;
+        this.assertObjectRegistered(object); // [#3770]
+
+        // Fail CLOSED on anything that is not a list of scalar ids. A non-scalar
+        // entry (`{"ids":[{"$ne":null}]}`) must never reach `where.id` as an
+        // operator object — that is the same "predicate widening" this endpoint
+        // was just hardened against, one layer down.
+        const isScalarId = (v: unknown) =>
+            (typeof v === 'string' && v.length > 0) || typeof v === 'number' || typeof v === 'bigint';
+        const ids = request.ids as unknown;
+        if (!Array.isArray(ids) || ids.some((id) => !isScalarId(id))) {
+            const err: any = new Error(
+                `deleteMany on '${object}' requires 'ids' to be an array of record ids`,
+            );
+            err.code = 'VALIDATION_FAILED';
+            err.status = 400;
+            throw err;
+        }
+
+        const results: Array<{ id?: string; success: boolean; error?: string }> = [];
+        let succeeded = 0;
+        let failed = 0;
+        const ctxOpt = context !== undefined ? { context } : {};
+
+        for (const id of ids) {
+            try {
+                await this.engine.delete(object, { where: { id }, ...ctxOpt } as any);
+                results.push({ id: String(id), success: true });
+                succeeded++;
+            } catch (err: any) {
+                results.push({ id: String(id), success: false, error: err?.message });
+                failed++;
+                // Same stop semantics as `batchData`: `atomic` aborts the rest on
+                // the first failure, and without `continueOnError` a failure ends
+                // the run rather than silently ploughing on.
+                if (options?.atomic) break;
+                if (!options?.continueOnError) break;
+            }
+        }
+
+        return {
+            success: failed === 0,
+            operation: 'delete',
+            total: ids.length,
+            succeeded,
+            failed,
+            results,
+        } as BatchUpdateResponse;
     }
 
     /**