From cedbce308657df7934b4611a1ba1b1c6b519639b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 02:35:01 +0000 Subject: [PATCH] test(plugin-security): feed the VAMA DELETE half a call shape the engine accepts (#6277) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `vama-write-path-convergence.test.ts` built its by-id delete as `options: { id }` — a bag `ObjectQL.delete` refuses twice over: `id` is outside delete's legal option keys (#4371), and the dispatch predicate reads `options.where.id`, so the call is `reject` (#4550). Consequently `SecurityPlugin.extractSingleId` returned `null` and the #1994 row-level pre-image write gate never ran on this half; only plugin-sharing's `canDelete` did, because its `inferTargetId` does accept `options.id`. Measured over all three principals: options: { id } extractSingleId -> null computeRlsFilter: 0 calls options: { where: { id } } extractSingleId -> id computeRlsFilter('delete'): 1 call The fixture now feeds the canonical shape, and the shape is pinned rather than merely re-spelled: the middleware chain terminates in the fake engine's own write verbs, which open with `assertEngineDeleteDispatch` / `assertEngineUpdateDispatch` (both slices of check:engine-double-contract now report this file as pinned). A drift back to the old spelling fails loudly at the terminal instead of buying a green from gates that never ran. No assertion changed verdict: `computeRlsFilter` resolves `null` for all three of this file's permission sets, none of which authors an RLS policy. The gate is reached, not yet exercised to a verdict — that is #5492's subject, noted in the fixture. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JwwiU9bjhwy2SWj13ho8uv --- packages/plugins/plugin-security/package.json | 1 + .../src/vama-write-path-convergence.test.ts | 148 +++++++++++++++++- pnpm-lock.yaml | 3 + 3 files changed, 149 insertions(+), 3 deletions(-) diff --git a/packages/plugins/plugin-security/package.json b/packages/plugins/plugin-security/package.json index 0b65390b21..1ff10b679c 100644 --- a/packages/plugins/plugin-security/package.json +++ b/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", diff --git a/packages/plugins/plugin-security/src/vama-write-path-convergence.test.ts b/packages/plugins/plugin-security/src/vama-write-path-convergence.test.ts index 40a7003df6..0429c18f8b 100644 --- a/packages/plugins/plugin-security/src/vama-write-path-convergence.test.ts +++ b/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 { @@ -179,6 +264,7 @@ async function makeStack(): Promise { return { security, sharing, + rows: (object: string) => (engine._tables[object] ??= []), async write(operation, recordId, context) { const opCtx: any = { object: 'crm_contract', @@ -186,14 +272,27 @@ async function makeStack(): Promise { 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 () => { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index cac5c54c86..3637989226 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1658,6 +1658,9 @@ importers: specifier: workspace:* version: link:../../spec devDependencies: + '@objectstack/metadata-core': + specifier: workspace:* + version: link:../../metadata-core '@objectstack/plugin-sharing': specifier: workspace:* version: link:../plugin-sharing