harden owner_id guard + bulk-write seam from adversarial review (#3004, #2982)

Adversarial code review of the fix surfaced several edge holes; close them:

engine.ts (#2982 seam):
- delete() gains update()'s scalar-id guard, so an id-list bulk delete
  ({ id: { $in: [...] } }, multi) routes to the scoped deleteMany instead of a
  garbage single-id driver.delete — closing the same #2982 hole on delete.
- both multi branches now consume the seeded opCtx.ast and fail CLOSED (throw)
  if it is absent, instead of rebuilding an unscoped { object, where } fallback
  that would silently drop every composed row filter (AGENTS.md PD #12).

plugin-security step 3.5 (owner_id guard):
- own-property (hasOwnProperty) membership test — a polluted prototype can no
  longer spoof an ownership write.
- owner_id must be a non-empty SCALAR; a non-scalar (array/object) is denied,
  not String()-coerced (which let owner_id:[self] pass and corrupt the anchor).
- owner_id:undefined on update is now denied (mongo $set would persist null =
  an ungated disown).
- array-shaped update change-sets fail CLOSED instead of silently bypassing.
- the no-op-echo pre-image is read under the CALLER's context, not isSystem:
  threads the open transaction (no spurious in-tx denial) and closes the
  owner-enumeration oracle a system read opened.
- update short-circuits the field-set lookup when the change-set omits owner_id.

plugin-sharing composeAnd: preserve sibling top-level keys of a
{ $and:[...], k:v } filter (was dropping k) — prevents a caller's AND-ed
predicate from silently widening a bulk write.

spec: allowTransfer describe text corrected — it is enforced NOW via the
insert/update owner_id door, not only the pending M2 transfer op.

Follow-ups filed for out-of-scope findings: public-form owner_id forge (#3022)
and cascade set_null vs the disown guard on sys_user deletion (#3023).

Author: claude

.changeset/owner-id-anchor-and-bulk-write-scoping.md

@@ -1,6 +1,7 @@
 ---
 '@objectstack/plugin-security': patch
 '@objectstack/objectql': patch
+'@objectstack/plugin-sharing': patch
 ---
 
 fix(security): guard the `owner_id` ownership anchor and scope bulk writes to owner-visible rows (#3004, #2982)
@@ -20,11 +21,18 @@ row-level scoping keys off to decide who may update/delete a record.
   invisible to their creator), and a supplied foreign owner is denied; on update
   a supplied `owner_id` is a transfer/disown and is denied — the unchanged no-op
   echo of a form save is tolerated via a pre-image compare, and a bulk
-  change-set carrying `owner_id` fails closed. Both require the transfer grant
+  change-set carrying `owner_id` fails closed. A non-scalar `owner_id`
+  (array/object) is rejected outright rather than string-coerced, and the
+  change-set membership test uses own-property semantics so a polluted
+  prototype cannot spoof an ownership write. Both require the transfer grant
   (`allowTransfer`, or `modifyAllRecords` which implies it) to proceed. System
-  context (`ctx.isSystem`) stays fully exempt (import / OAuth provisioning / cron
-  snapshots / seed claims), and under delegation both principals must hold the
-  grant (ADR-0090 D10 intersection).
+  context (`ctx.isSystem`) stays fully exempt (OAuth provisioning / cron
+  snapshots / seed claims / migrations), and under delegation both principals
+  must hold the grant (ADR-0090 D10 intersection). Note a REST **import** runs
+  under the importer's own context (not `isSystem`), so a non-privileged user
+  importing a CSV whose `owner_id` column names other users is correctly denied
+  unless they hold the transfer grant — administrators (who carry
+  `modifyAllRecords`) are unaffected.
 
 - **#2982 — bulk writes skipped owner scoping on OWD-`private` objects.** A
   `update({ multi: true })` / bulk delete rebuilt the driver AST from
@@ -34,7 +42,17 @@ row-level scoping keys off to decide who may update/delete a record.
   peers'. The engine now seeds `opCtx.ast` from the caller's predicate BEFORE the
   chain (the same seam reads use) and hands the middleware-composed AST to
   `driver.updateMany` / `driver.deleteMany`, so bulk writes are constrained to the
-  rows the caller may edit — matching single-id write behavior.
+  rows the caller may edit — matching single-id write behavior. `delete` now
+  applies the same scalar-`id` guard `update` already had, so an id-list bulk
+  delete (`where: { id: { $in: […] } }, multi: true`) is owner-scoped too, and
+  both multi branches fail CLOSED (throw) rather than silently rebuilding an
+  unscoped predicate if the row-scoping AST is ever absent.
+
+  Consequences of routing bulk writes through the AST: the anti-oracle
+  predicate guard now also applies to bulk `update`/`delete` (a bulk write
+  filtering on an FLS-unreadable field is rejected, as reads already are), and a
+  principal-less (no-`userId`, non-system) bulk write on an owner-scoped object
+  now correctly affects zero rows instead of all of them.
 
 Proven end-to-end on the real showcase app
 (`packages/dogfood/test/owner-anchor-and-bulk-writes.dogfood.test.ts`) and pinned

packages/objectql/src/engine.test.ts

@@ -661,6 +661,41 @@ describe('ObjectQL Engine', () => {
             expect(mockDriver.update).toHaveBeenCalledTimes(1);
             expect((mockDriver as any).updateMany).not.toHaveBeenCalled();
         });
+
+        it('seeds opCtx.ast for an id-LIST bulk delete ({id:{$in}}) so it routes to a scoped deleteMany (#2982 parity)', async () => {
+            // Regression: delete() lacked update()'s scalar-id guard, so an
+            // operator-object where.id was mistaken for a single id — skipping
+            // the seed and routing to driver.delete with a garbage id, never
+            // reaching the owner-scoped deleteMany path.
+            engine.registerMiddleware(async (opCtx: any, next: () => Promise<void>) => {
+                if (opCtx.operation === 'delete' && opCtx.ast) {
+                    opCtx.ast.where = { $and: [opCtx.ast.where, { owner_id: 'u1' }] };
+                }
+                await next();
+            });
+
+            await engine.delete('task', { where: { id: { $in: ['a', 'b'] } }, multi: true } as any);
+
+            expect((mockDriver as any).deleteMany).toHaveBeenCalledTimes(1);
+            expect(mockDriver.delete).not.toHaveBeenCalled();
+            const [, ast] = (mockDriver as any).deleteMany.mock.calls[0];
+            expect(ast.where).toEqual({ $and: [{ id: { $in: ['a', 'b'] } }, { owner_id: 'u1' }] });
+        });
+
+        it('fails CLOSED (throws) if a hook clears the target id so the multi branch runs without a seeded ast', async () => {
+            // The only way to reach the multi branch with no seeded ast is a
+            // beforeUpdate hook clearing input.id after a truthy-id seed skip.
+            // The old `?? { object, where }` fallback would have silently
+            // rebuilt an UNSCOPED predicate; we now refuse it.
+            engine.registerHook('beforeUpdate', async (ctx: any) => {
+                ctx.input.id = undefined; // force the multi branch, no seeded ast
+            });
+
+            await expect(
+                engine.update('task', { id: 't1', status: 'done' }, { multi: true } as any),
+            ).rejects.toThrow(/row-scoping AST was not seeded/);
+            expect((mockDriver as any).updateMany).not.toHaveBeenCalled();
+        });
     });
 
     describe('Expand Related Records', () => {

packages/objectql/src/engine.ts

@@ -2375,9 +2375,12 @@ export class ObjectQL implements IDataEngine {
      // editable-rows filter) never reached the driver, and a bulk write hit
      // every matching row regardless of ownership. Seed the AST with the
      // caller's predicate BEFORE the chain runs — the same seam the read path
-     // uses — and let the multi branch consume the composed result.
+     // uses — and let the multi branch consume the composed result. Keyed on
+     // the SAME falsy-`id` test the multi branch dispatches on (below), so the
+     // seed and the branch never disagree. `where` is included only when
+     // supplied, mirroring the read path's AST shape.
      if (!id) {
-       opCtx.ast = { object, where: options?.where } as QueryAST;
+       opCtx.ast = { object, ...(options?.where !== undefined ? { where: options.where } : {}) } as QueryAST;
      }
 
      // [#2948] Snapshot the keys the CALLER supplied, BEFORE any middleware /
@@ -2453,10 +2456,20 @@ export class ObjectQL implements IDataEngine {
                if (!opCtx.context?.isSystem) {
                    hookContext.input.data = stripReadonlyFields(updateSchema as any, hookContext.input.data as Record<string, unknown>, suppliedKeys, this.logger) as any;
                }
-               // [#2982] Use the middleware-composed AST (seeded above from the
-               // caller's `where`), so injected row-scoping actually binds the
-               // driver operation instead of being rebuilt-away here.
-               const ast: QueryAST = (opCtx.ast as QueryAST) ?? { object, where: options.where };
+               // [#2982] Consume the middleware-composed AST seeded above, so
+               // the injected row-scoping (RLS write filter, sharing's
+               // editable-rows filter) actually binds the driver operation. Fail
+               // CLOSED if it is somehow absent — rebuilding `{ object, where }`
+               // here would silently drop every composed filter and reopen the
+               // unscoped-bulk-write hole this fix closes (AGENTS.md PD #12: no
+               // lenient fallback that tolerates the broken invariant).
+               const ast = opCtx.ast;
+               if (!ast) {
+                   throw new Error(
+                     `[Security] Refusing bulk update on '${object}': row-scoping AST was not seeded ` +
+                       `(a hook cleared the target id after the security filter was composed).`,
+                   );
+               }
                result = await driver.updateMany(object, ast, hookContext.input.data as Record<string, unknown>, hookContext.input.options as any);
            } else {
                throw new Error('Update requires an ID or options.multi=true');
@@ -2617,10 +2630,19 @@ export class ObjectQL implements IDataEngine {
     this.assertWriteAllowed(object, 'delete');
     const driver = this.getDriver(object);
 
-    // Extract ID logic similar to update
+    // Extract ID logic mirroring update(): only a SCALAR `where.id` means
+    // "delete one row by primary key". An operator object ({ $in: [...] }, …)
+    // is a multi-row predicate — treating it as an id would bind the object
+    // literally (driver.delete(object, {$in:[…]})) and both skip the #2982 AST
+    // seeding below AND bypass the by-id RLS pre-image check. Leave `id`
+    // undefined so the call routes to deleteMany with the scoped AST.
     let id: any = undefined;
     if (options?.where && typeof options.where === 'object' && 'id' in options.where) {
-        id = (options.where as Record<string, unknown>).id;
+        const whereId = (options.where as Record<string, unknown>).id;
+        const t = typeof whereId;
+        if (whereId !== null && (t === 'string' || t === 'number' || t === 'bigint')) {
+            id = whereId;
+        }
     }
 
     const opCtx: OperationContext = {
@@ -2634,9 +2656,10 @@ export class ObjectQL implements IDataEngine {
     // `driver.deleteMany` with an AST that used to be rebuilt from
     // `options.where` after the middleware chain, discarding any row-scoping a
     // middleware composed onto `opCtx.ast`. Seed the caller's predicate before
-    // the chain; the multi branch consumes the composed result.
-    if (id == null) {
-      opCtx.ast = { object, where: options?.where } as QueryAST;
+    // the chain, keyed on the SAME falsy-`id` test the multi branch dispatches
+    // on; the multi branch consumes the composed result.
+    if (!id) {
+      opCtx.ast = { object, ...(options?.where !== undefined ? { where: options.where } : {}) } as QueryAST;
     }
 
     await this.executeWithMiddleware(opCtx, async () => {
@@ -2669,9 +2692,17 @@ export class ObjectQL implements IDataEngine {
               await this.cascadeDeleteRelations(object, hookContext.input.id as string | number, opCtx.context);
               result = await driver.delete(object, hookContext.input.id as string, hookContext.input.options as any);
           } else if (options?.multi && driver.deleteMany) {
-               // [#2982] Use the middleware-composed AST (seeded above) so
-               // injected row-scoping binds the bulk delete.
-               const ast: QueryAST = (opCtx.ast as QueryAST) ?? { object, where: options.where };
+               // [#2982] Consume the middleware-composed AST seeded above so the
+               // injected row-scoping binds the bulk delete. Fail CLOSED if it
+               // is absent rather than rebuilding an unscoped `{ object, where }`
+               // (AGENTS.md PD #12).
+               const ast = opCtx.ast;
+               if (!ast) {
+                   throw new Error(
+                     `[Security] Refusing bulk delete on '${object}': row-scoping AST was not seeded ` +
+                       `(a hook cleared the target id after the security filter was composed).`,
+                   );
+               }
                result = await driver.deleteMany(object, ast, hookContext.input.options as any);
           } else {
                throw new Error('Delete requires an ID or options.multi=true');

packages/plugins/plugin-security/src/security-plugin.test.ts

@@ -322,6 +322,53 @@ describe('SecurityPlugin', () => {
       };
       await harness.run(opCtx); // must not throw, no pre-image read needed
     });
+
+    it('update disowning via owner_id:undefined is denied (mongo $set null hazard)', async () => {
+      const harness = await boot([memberSet], () => ({ id: 't1', owner_id: 'u1' }));
+      const opCtx: any = {
+        object: 'task', operation: 'update', data: { id: 't1', owner_id: undefined },
+        context: memberCtx(),
+      };
+      await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/);
+    });
+
+    it('insert with a NON-SCALAR owner_id (array) is denied, not coerced to self', async () => {
+      const harness = await boot([memberSet]);
+      const opCtx: any = {
+        object: 'task', operation: 'insert', data: { name: 'A', owner_id: ['u1'] },
+        context: memberCtx(),
+      };
+      // String(['u1']) === 'u1' would have passed as self; scalar-only rejects it.
+      await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/);
+    });
+
+    it('update echo with a NON-SCALAR owner_id equal-by-coercion is denied', async () => {
+      const harness = await boot([memberSet], () => ({ id: 't1', owner_id: 'u1' }));
+      const opCtx: any = {
+        object: 'task', operation: 'update', data: { id: 't1', owner_id: ['u1'] },
+        context: memberCtx(),
+      };
+      await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/);
+    });
+
+    it('array-shaped update change-set carrying owner_id fails closed (no silent bypass)', async () => {
+      const harness = await boot([memberSet]);
+      const opCtx: any = {
+        object: 'task', operation: 'update', data: [{ id: 't1', owner_id: 'attacker' }],
+        context: memberCtx(),
+      };
+      await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/);
+    });
+
+    it('a prototype-chain owner_id (not own property) does not trip the guard', async () => {
+      const harness = await boot([memberSet]);
+      const proto = { owner_id: 'attacker' };
+      const data: any = Object.create(proto);
+      data.id = 't1';
+      data.name = 'renamed'; // only own props; owner_id is inherited
+      const opCtx: any = { object: 'task', operation: 'update', data, context: memberCtx() };
+      await harness.run(opCtx); // must not throw — owner_id is not an own property
+    });
   });
 
   it('without org-scoping plugin — strips tenant_isolation RLS so find applies no tenant where', async () => {