test(plugin-security): feed the VAMA DELETE half a call shape the engine accepts (#6277)

packages/plugins/plugin-security/package.json

@@ -24,6 +24,7 @@
     "@objectstack/spec": "workspace:*"
   },
   "devDependencies": {
+    "@objectstack/metadata-core": "workspace:*",
     "@objectstack/plugin-sharing": "workspace:*",
     "@types/node": "^26.1.2",
     "typescript": "^6.0.3",

packages/plugins/plugin-security/src/vama-write-path-convergence.test.ts

@@ -26,6 +26,13 @@
 // side breaks it, which is the only property worth pinning here: not "explain
 // says X", but "explain and the write it explains say the same thing".
 import { describe, it, expect, vi, beforeEach } from 'vitest';
+import {
+  assertEngineDeleteDispatch,
+  assertEngineUpdateDispatch,
+  resolveEngineDeleteDispatch,
+  ENGINE_DELETE_REJECT_MESSAGE,
+  ENGINE_UPDATE_REJECT_MESSAGE,
+} from '@objectstack/metadata-core';
 import { SharingService, buildSharingMiddleware } from '@objectstack/plugin-sharing';
 import { PermissionSetSchema } from '@objectstack/spec/security';
 import type { PermissionSet } from '@objectstack/spec/security';
@@ -125,20 +132,98 @@ function makeEngine() {
       (tables[object] ??= []).push({ ...data });
       return data;
     },
+    /**
+     * The two WRITE VERBS the middleware chain below terminates in. Both open
+     * with the **producer's own dispatch predicate** (#4550 / #5480) rather
+     * than a hand-mirrored guard, so this double cannot accept a call
+     * `ObjectQL.update` / `ObjectQL.delete` would refuse.
+     *
+     * That line is not decoration here — it is the pin for #6277. This file's
+     * `write('delete', …)` used to hand the chain `options: { id }`, a shape
+     * the real engine REJECTS (`resolveEngineDeleteDispatch` reads
+     * `options.where.id`), so the whole DELETE half was asserting against a
+     * call that could never happen. A boolean terminal could not notice; these
+     * two can, and do, by throwing exactly what a running server throws.
+     */
+    async update(object: string, data: any, options?: any) {
+      const dispatch = assertEngineUpdateDispatch(data, options);
+      const rows = (tables[object] ??= []);
+      const targets =
+        dispatch.kind === 'by-id'
+          ? rows.filter((r) => r.id === dispatch.id)
+          : rows.filter((r) => matches(r, options?.where));
+      for (const r of targets) Object.assign(r, data);
+      // `driver.updateMany` resolves a COUNT; a by-id update resolves the row.
+      return dispatch.kind === 'by-id' ? (targets[0] ?? null) : targets.length;
+    },
+    async delete(object: string, options?: any) {
+      const dispatch = assertEngineDeleteDispatch(options);
+      const rows = (tables[object] ??= []);
+      const targets =
+        dispatch.kind === 'by-id'
+          ? rows.filter((r) => r.id === dispatch.id)
+          : rows.filter((r) => matches(r, options?.where));
+      tables[object] = rows.filter((r) => !targets.includes(r));
+      // `driver.deleteMany` resolves a COUNT; a by-id delete resolves a boolean.
+      return dispatch.kind === 'by-id' ? targets.length > 0 : targets.length;
+    },
   };
 }
 
+/**
+ * The by-id DELETE options this fixture feeds the chain — the **canonical**
+ * shape, and the subject of #6277.
+ *
+ * `ObjectQL.delete` dispatches by-id on a truthy scalar `options.where.id` and
+ * on nothing else, and `SecurityPlugin.extractSingleId` reads the same two
+ * places (`data.id`, then `options.where.id`). The old spelling here,
+ * `options: { id }`, satisfied neither — it is not even a legal delete option
+ * bag, since `id` is outside `{ context, where, multi }` + driver passthrough
+ * and `rejectUnknownEngineOptions` refuses it at the entry point (#4371). So
+ * the security extractor answered `null` and the #1994 row-level pre-image
+ * write gate was skipped wholesale on this half: `write('delete', …)` reached
+ * only plugin-sharing's `canDelete`, one of the two gates the case name claims.
+ * Measured on this branch over all three principals below:
+ *
+ * ```
+ *   options: { id }             extractSingleId -> null              computeRlsFilter: never called
+ *   options: { where: { id } }  extractSingleId -> 'rec_ownerless'   computeRlsFilter('delete'): 1 call
+ * ```
+ *
+ * Note what that measurement does NOT say. The gate is now *reached*; its DENY
+ * branch is still not exercised here, because `computeRlsFilter` resolves
+ * `null` for all three of this file's permission sets — none of them authors an
+ * RLS policy, unlike the real `member_default` whose wildcard `owner_only_writes`
+ * is the co-gate #5492 measures. So this file pins gate REACHABILITY, not the
+ * gate's verdict; the verdict is #5492's subject, and when its paired PR makes
+ * `modifyAllRecords` widen the row write gate, the DELETE case below is the
+ * place in this file where that becomes load-bearing.
+ */
+const byIdDeleteOptions = (recordId: string) => ({ where: { id: recordId } });
+
+/** What a dispatch REJECTION reads like — a fixture defect, never a 403. */
+const ENGINE_DISPATCH_REJECTIONS: readonly string[] = [
+  ENGINE_DELETE_REJECT_MESSAGE,
+  ENGINE_UPDATE_REJECT_MESSAGE,
+];
+
 // ── the stack: real SecurityPlugin + real SharingService, one engine ───────
 
 interface Stack {
   security: any;
   sharing: SharingService;
-  /** Run a by-id write through the REAL middleware chain (security → sharing). */
+  /**
+   * Run a by-id write through the REAL middleware chain (security → sharing)
+   * and, when both gates admit it, through the engine's own write verb — so
+   * `{ ok: true }` means the row was really written, not that a boolean was set.
+   */
   write: (
     operation: 'update' | 'delete',
     recordId: string,
     context: any,
   ) => Promise<{ ok: true } | { ok: false; code?: string; message: string }>;
+  /** The fixture's rows, for asserting that an admitted write actually landed. */
+  rows: (object: string) => any[];
 }
 
 async function makeStack(): Promise<Stack> {
@@ -179,21 +264,35 @@ async function makeStack(): Promise<Stack> {
   return {
     security,
     sharing,
+    rows: (object: string) => (engine._tables[object] ??= []),
     async write(operation, recordId, context) {
       const opCtx: any = {
         object: 'crm_contract',
         operation,
         context: { ...context },
         ...(operation === 'update'
           ? { data: { id: recordId, signed_by: 'x' } }
-          : { options: { id: recordId } }),
+          // [#6277] The canonical by-id delete shape — see `byIdDeleteOptions`.
+          : { options: byIdDeleteOptions(recordId) }),
       };
       let reached = false;
       try {
         await securityMw(opCtx, async () => {
-          await sharingMw(opCtx, async () => { reached = true; });
+          await sharingMw(opCtx, async () => {
+            // The terminal is the ENGINE's own write verb, not a boolean. This
+            // is what keeps the call shape above honest: both verbs open with
+            // the producer's dispatch predicate, so a fixture that drifts back
+            // to a call the engine refuses fails LOUDLY here instead of quietly
+            // collecting a green from gates that never ran (#6277).
+            if (operation === 'delete') await engine.delete(opCtx.object, opCtx.options);
+            else await engine.update(opCtx.object, opCtx.data, opCtx.options);
+            reached = true;
+          });
         });
       } catch (e: any) {
+        // A dispatch rejection is a defect in THIS fixture, never a permission
+        // verdict — it must never be readable as a 403.
+        if (ENGINE_DISPATCH_REJECTIONS.includes(String(e?.message))) throw e;
         return { ok: false, code: e?.code, message: String(e?.message ?? e) };
       }
       return reached ? { ok: true } : { ok: false, message: 'middleware swallowed the write' };
@@ -245,11 +344,54 @@ describe('[#4647] VAMA holder + private OWD + ownerless record — explain vs. t
   });
 
   it('DELETE: same triple, same convergence (canDelete has no share branch to hide behind)', async () => {
+    // [#6277] The precondition the rest of this case rests on, asserted
+    // against the PRODUCER'S predicate rather than assumed: the options bag
+    // `write('delete', …)` builds is one `ObjectQL.delete` dispatches BY ID.
+    // Until this file was fixed it was `options: { id }` — `reject` — and both
+    // the engine and `SecurityPlugin.extractSingleId` refused to see an id in
+    // it, so the #1994 row-level pre-image write gate never ran on this half
+    // and the assertions below were green for want of anything executing.
+    expect(resolveEngineDeleteDispatch(byIdDeleteOptions(OWNERLESS.id))).toEqual({
+      kind: 'by-id',
+      id: OWNERLESS.id,
+    });
+
     const decision = await explainOf(stack, ADMIN_CTX, 'delete');
     const write = await stack.write('delete', OWNERLESS.id, ADMIN_CTX);
     expect(decision.allowed).toBe(true);
     expect(decision.record).toMatchObject({ visible: true, decidedBy: 'vama_bypass' });
     expect(write).toEqual({ ok: true });
+    // …and `ok: true` now means the row is GONE, because the chain terminates
+    // in the engine's own `delete` rather than in a boolean.
+    expect(stack.rows('crm_contract').map((r) => r.id)).toEqual([OWNED_BY_OTHER.id]);
+  });
+
+  // ── the #6277 pin: the shape this fixture used to feed cannot come back ──
+  it('[#6277] the DELETE shape this fixture used to feed is one the engine REJECTS', async () => {
+    // Reverse verification, committed rather than performed once by hand: put
+    // the old spelling back and the producer's predicate — the same one the
+    // fake engine's `delete` opens with — refuses it. `options: { id }` names
+    // neither one row (no `where.id`) nor a bulk intent (no `multi`).
+    //
+    // Worth stating exactly, because a real server refuses that bag TWICE and
+    // this assertion only pins the second refusal: `ObjectQL.delete` runs
+    // `rejectUnknownEngineOptions` FIRST (#4371 — `id` is not among delete's
+    // legal keys `context`/`where`/`multi` + driver passthrough), so a running
+    // server never reaches the dispatch below, and never reaches its middleware
+    // chain either — no gate, security's or sharing's, runs for this bag on the
+    // real write path. The dispatch verdict pinned here is what the call would
+    // get if the key were legal, and it is the one the fake engine enforces.
+    expect(resolveEngineDeleteDispatch({ id: OWNERLESS.id })).toEqual({
+      kind: 'reject',
+      message: ENGINE_DELETE_REJECT_MESSAGE,
+    });
+    await expect(makeEngine().delete('crm_contract', { id: OWNERLESS.id })).rejects.toThrow(
+      ENGINE_DELETE_REJECT_MESSAGE,
+    );
+    // The asymmetry that let it hide for so long: plugin-sharing's
+    // `inferTargetId` DOES accept `options.id`, so `canDelete` kept running and
+    // kept answering — one of the two gates, doing the work of two.
+    expect(await stack.sharing.canDelete('crm_contract', OWNERLESS.id, MEMBER_CTX as any)).toBe(false);
   });
 
   it("the sys_attachment canEdit(parent) gate converges too — it calls the SAME gate", async () => {