fix(objectql,spec): run the pre-delete reference check under the system identity

.changeset/delete-reference-check-system-identity.md

@@ -0,0 +1,78 @@
+---
+"@objectstack/objectql": minor
+"@objectstack/spec": minor
+---
+
+fix(objectql,spec): run the pre-delete reference check under the system identity (#12166)
+
+**Grade: `minor`, not `patch` — argued, because a permission-behaviour change
+should not arrive as a bug-fix bump.** Configurations that returned `403` now
+return `200`. Nothing gets more restrictive and no API changes shape, so this
+is not `major`; but "records a role could never delete are now deletable" is a
+security-surface accept-set change an upgrader must be able to see in a
+release-notes heading, and a `patch` line is exactly where it would not be
+looked for. The spec half ships `minor` alongside because the message catalog
+gains two keys.
+
+Deleting a record runs the platform's pre-delete reference check, which issues
+a `find` against every referencing object. That probe ran as the **calling
+operator**, so a caller with full delete rights on the target but no read grant
+on any referencing object got a blanket `403 PERMISSION_DENIED` — regardless of
+whether a reference actually existed. An **empty** referencing table 403'd too.
+The reporting deployment (`@objectstack/*@17.2.0`) measured 17 role×object
+pairs where the UI shows a delete button that always fails, with the A/B
+control that granting read-only on the referencing object — touching *nothing*
+about delete rights — turned the identical operation into a `200`.
+
+It silently made "delete permission" mean "delete **plus read on every
+referencing table**", a coupling invisible in the permission UI and impossible
+for an administrator to self-diagnose: the refusal said only "You do not have
+permission to perform this action."
+
+Referential-integrity actions are engine responsibility executed under system
+identity on every mainstream platform — the RDBMS FK baseline, Salesforce
+(lookup clearing and cascade delete documented as bypassing sharing),
+Dataverse, ServiceNow, Odoo. Caller identity here was the outlier. Maintainer
+ruling 2026-08-26, option A.
+
+**What changed.** The dependents probe now runs `sudo()`-shaped —
+`{ ...context, isSystem: true }`, following the in-repo precedent in
+`packages/objectql/src/integrity/dangling-reference-audit.ts`. The spread is
+load-bearing: the caller's open transaction handle, **tenant scope** and
+`userId` all survive, so the probe does not leave the caller's transaction and
+does not read across the tenant wall.
+
+**What did NOT change.** Only the reference *check* switches identity. The
+caller's own delete authorisation on the target is untouched; the `set_null`
+`UPDATE`, the `cascade` `DELETE` and the target's own delete all still run as
+the caller. A caller without delete rights on the target is refused exactly as
+before — pinned in both directions, because a relaxation must not become a
+hole.
+
+**Refusal copy.** Because the probe now sees rows the caller may hold no read
+grant on, `DELETE_RESTRICTED` discloses the dependent **count** only when the
+caller's own identity would have produced the same rows (compared on row
+identity, so row-level narrowing counts too). Otherwise the count is withheld
+and the refusal renders one of two new catalog keys,
+`delete_restricted_opaque` / `delete_restricted_required_opaque` — the same
+sentences minus `{{count}}`, in all four bundled locales. Without that, the
+refusal would be an exact, repeatable cardinality oracle over a table the
+caller may not read. The referenced **object** and the relation field are named
+either way: those are declared metadata, and they are the whole of what makes
+the refusal self-diagnosable. `dependentCount` is **absent** rather than `0` in
+the withheld case — `0` would be a false statement about the rows.
+
+**Audit.** The elevation is filed with both halves, the Salesforce/Dataverse
+ledger shape: `triggeredBy` = the deleting operator, `executedAs: 'system'`,
+plus the referenced object and relation field — never a row id, value or count.
+It is filed *before* the probe, so a refused or failed check is recorded too.
+Declared limit: this is an engine **log** record, not a `sys_audit_log` row —
+the elevated operation is a read, and `plugin-audit`'s read writer declares and
+pins that a system-elevated read produces no row. A durable row belongs to the
+plugin that owns that shape.
+
+**Upgrade note.** If a deployment was relying on the `403` as a de-facto delete
+gate, that gate is gone. Under the industry baseline such usage is itself
+non-standard and should be expressed as an explicit `deleteBehavior: 'restrict'`
+on the relationship rather than as a read-permission side effect. No such
+reliance was measured; the ruling records this as a known confidence gap.

packages/objectql/src/engine-reference-check-system-identity.test.ts

@@ -0,0 +1,228 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #12166 — the SHAPE of the pre-delete reference check's identity, at the
+ * engine face. Maintainer ruling 2026-08-26, option A.
+ *
+ * The end-to-end contract — "empty referencing table + no read grant on it +
+ * full delete rights on the target ⇒ the delete succeeds", and its converse —
+ * is pinned against the REAL security middleware in
+ * `packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts`,
+ * because only that package has the gate whose 403 was the defect. This file
+ * pins what that one structurally cannot see: WHICH context object each
+ * operation of the delete path carries.
+ *
+ * Two facts, and they are the pair — either alone is satisfiable by a wrong
+ * implementation:
+ *
+ * 1. the reference CHECK is elevated, and elevated `sudo()`-SHAPED — a bare
+ * `{ isSystem: true }` would also pass a test that only asked "is
+ * isSystem set?", while silently dropping the caller's TENANT scope and
+ * leaving the probe reading across the tenant wall;
+ * 2. nothing else does (ruling constraint 1) — the `set_null` UPDATE and the
+ * `cascade` DELETE still run as the caller.
+ */
+
+import { describe, it, expect, beforeEach, vi } from 'vitest';
+import { ObjectQL } from './engine.js';
+
+const acct = {
+ name: 'acct',
+ label: 'Account',
+ fields: {
+ id: { name: 'id', type: 'text' as const, primaryKey: true },
+ name: { name: 'name', type: 'text' as const },
+ },
+};
+/** Optional lookup → resolved behaviour `set_null`: the cleanup WRITE path. */
+const note = {
+ name: 'note',
+ label: 'Note',
+ fields: {
+ id: { name: 'id', type: 'text' as const, primaryKey: true },
+ account: { name: 'account', type: 'lookup' as const, reference: 'acct' },
+ },
+};
+/** Explicit cascade → the recursive DELETE path. */
+const task = {
+ name: 'task',
+ label: 'Task',
+ fields: {
+ id: { name: 'id', type: 'text' as const, primaryKey: true },
+ account: { name: 'account', type: 'lookup' as const, reference: 'acct', deleteBehavior: 'cascade' },
+ },
+};
+
+function makeStubDriver() {
+ const stores = new Map<string, Map<string, Record<string, unknown>>>();
+ const storeFor = (o: string) => {
+ let s = stores.get(o);
+ if (!s) { s = new Map(); stores.set(o, s); }
+ return s;
+ };
+ let nextId = 0;
+ const matches = (row: Record<string, unknown>, where: any): boolean => {
+ if (!where || typeof where !== 'object') return true;
+ for (const [k, v] of Object.entries(where)) {
+ if (k.startsWith('$')) continue;
+ if ((row[k] ?? null) !== ((v as any) ?? null)) return false;
+ }
+ return true;
+ };
+ const driver: any = {
+ name: 'memory', version: '0.0.0', supports: {},
+ async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
+ // The caller's bound is applied AFTER the filter and BY PRESENCE — a
+ // double looser than the engine on `limit` would let a probe that
+ // relies on a bound read as unbounded here (`check:objectql-double-limit`).
+ async find(o: string, ast: any) {
+ const rows = Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where));
+ return typeof ast?.limit === 'number' ? rows.slice(0, ast.limit) : rows;
+ },
+ async findOne(o: string, ast: any) { for (const r of storeFor(o).values()) if (matches(r, ast?.where)) return r; return null; },
+ async create(o: string, data: Record<string, unknown>) {
+ nextId += 1;
+ const id = (data.id as string) ?? `r_${nextId}`;
+ const row = { ...data, id }; storeFor(o).set(id, row); return row;
+ },
+ async update(o: string, id: string, data: Record<string, unknown>) {
+ const s = storeFor(o); const cur = s.get(id);
+ if (!cur) throw new Error(`nf ${o}/${id}`);
+ const up = { ...cur, ...data, id }; s.set(id, up); return up;
+ },
+ async upsert(o: string, data: Record<string, unknown>) {
+ const id = data.id as string | undefined;
+ return id && storeFor(o).has(id) ? this.update(o, id, data) : this.create(o, data);
+ },
+ async delete(o: string, id: string) { return storeFor(o).delete(id); },
+ async count(o: string, ast: any) { return (await this.find(o, ast)).length; },
+ async bulkCreate(o: string, rows: Record<string, unknown>[]) { return Promise.all(rows.map((r) => this.create(o, r))); },
+ async bulkUpdate() { return []; }, async bulkDelete() {},
+ async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; },
+ async commit() {}, async rollback() {},
+ };
+ return { driver, stores };
+}
+
+/** The caller: a real principal, with a tenant and a timezone to lose. */
+const CALLER = () => ({
+ userId: 'u_operator',
+ tenantId: 'org-77',
+ timezone: 'Asia/Shanghai',
+ positions: ['p_line'],
+ permissions: [],
+} as any);
+
+describe('#12166 — the reference check is elevated, sudo()-shaped', () => {
+ let engine: ObjectQL;
+ let seen: Array<{ operation: string; object: string; context: any }>;
+
+ beforeEach(async () => {
+ engine = new ObjectQL({ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() } });
+ const { driver } = makeStubDriver();
+ engine.registerDriver(driver, true);
+ await engine.init();
+ for (const o of [acct, note, task]) engine.registry.registerObject(o as any, 'test');
+ seen = [];
+ engine.registerMiddleware(async (ctx: any, next: any) => {
+ seen.push({ operation: ctx.operation, object: ctx.object, context: ctx.context });
+ await next();
+ });
+ });
+
+ const probeOf = (object: string) =>
+ seen.find((s) => s.operation === 'find' && s.object === object)?.context;
+
+ it('the dependents probe carries isSystem — AND keeps the caller\'s tenant, user and timezone', async () => {
+ const a = await engine.insert('acct', { name: 'Acme' }, { context: { isSystem: true } } as any);
+ await engine.delete('acct', { where: { id: a.id }, context: CALLER() } as any);
+
+ const probe = probeOf('note');
+ expect(probe).toBeDefined();
+
+ // The elevation…
+ expect(probe.isSystem).toBe(true);
+ // …and the three things a BARE `{ isSystem: true }` would have dropped.
+ // `tenantId` is the load-bearing one: without it this probe reads across
+ // the tenant wall, which is a WIDER change than the card authorises —
+ // and a test asserting only `isSystem` would not notice.
+ expect(probe.tenantId).toBe('org-77');
+ expect(probe.userId).toBe('u_operator');
+ expect(probe.timezone).toBe('Asia/Shanghai');
+ });
+
+ it('the caller\'s own context object is not mutated — the elevation is a derivative', async () => {
+ const caller = CALLER();
+ const a = await engine.insert('acct', { name: 'Acme' }, { context: { isSystem: true } } as any);
+ await engine.delete('acct', { where: { id: a.id }, context: caller } as any);
+
+ // A `context.isSystem = true` assignment instead of a spread would
+ // elevate the CALLER for the rest of the request — every later write in
+ // the same transaction included. That is the silent version of this
+ // card's defect with the sign flipped.
+ expect(caller.isSystem).toBeUndefined();
+ });
+});
+
+describe('#12166 constraint 1 — nothing ELSE on the delete path changes identity', () => {
+ let engine: ObjectQL;
+ let seen: Array<{ operation: string; object: string; context: any }>;
+
+ beforeEach(async () => {
+ engine = new ObjectQL({ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() } });
+ const { driver } = makeStubDriver();
+ engine.registerDriver(driver, true);
+ await engine.init();
+ for (const o of [acct, note, task]) engine.registry.registerObject(o as any, 'test');
+ seen = [];
+ engine.registerMiddleware(async (ctx: any, next: any) => {
+ seen.push({ operation: ctx.operation, object: ctx.object, context: ctx.context });
+ await next();
+ });
+ });
+
+ it('the set_null cleanup WRITE still runs as the caller', async () => {
+ const a = await engine.insert('acct', { name: 'Acme' }, { context: { isSystem: true } } as any);
+ await engine.insert('note', { account: a.id }, { context: { isSystem: true } } as any);
+ seen = [];
+
+ await engine.delete('acct', { where: { id: a.id }, context: CALLER() } as any);
+
+ const write = seen.find((s) => s.operation === 'update' && s.object === 'note');
+ expect(write).toBeDefined();
+ // NOT elevated. The caller's authority over the dependent rows is
+ // untouched by this card — only the CHECK was relaxed.
+ expect(write!.context.isSystem).toBeFalsy();
+ expect(write!.context.userId).toBe('u_operator');
+ // The #3023 integrity marker still rides it, unchanged.
+ expect(write!.context.__referentialFieldClear).toBe(true);
+ });
+
+ it('the cascade DELETE of a child still runs as the caller', async () => {
+ const a = await engine.insert('acct', { name: 'Acme' }, { context: { isSystem: true } } as any);
+ await engine.insert('task', { account: a.id }, { context: { isSystem: true } } as any);
+ seen = [];
+
+ await engine.delete('acct', { where: { id: a.id }, context: CALLER() } as any);
+
+ const childDelete = seen.find((s) => s.operation === 'delete' && s.object === 'task');
+ expect(childDelete).toBeDefined();
+ expect(childDelete!.context.isSystem).toBeFalsy();
+ expect(childDelete!.context.userId).toBe('u_operator');
+ });
+
+ it('the target\'s OWN delete still runs as the caller', async () => {
+ const a = await engine.insert('acct', { name: 'Acme' }, { context: { isSystem: true } } as any);
+ seen = [];
+
+ await engine.delete('acct', { where: { id: a.id }, context: CALLER() } as any);
+
+ const own = seen.find((s) => s.operation === 'delete' && s.object === 'acct');
+ expect(own).toBeDefined();
+ // The whole point of the ruling's first constraint: the caller's own
+ // delete authorisation is exactly as it was. If this ever reads
+ // `isSystem`, the card has become a privilege escalation.
+ expect(own!.context.isSystem).toBeFalsy();
+ expect(own!.context.userId).toBe('u_operator');
+ });
+});