From bef5f7bc6333176300f10da5d84afa33b8357ddd Mon Sep 17 00:00:00 2001 From: os-warren Date: Wed, 26 Aug 2026 13:41:27 +0000 Subject: [PATCH 1/3] fix(objectql): run the pre-delete reference check under the system identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-delete reference check issued a `find` against every referencing object using the CALLING OPERATOR's identity. A caller with full delete rights on the target but no read grant on any referencing object got a blanket 403 — regardless of whether a reference existed, and with the referencing table EMPTY. It silently made "delete permission" mean "delete + read on every referencing table", a coupling invisible in the permission UI. The probe now runs `sudo()`-shaped (`{ ...context, isSystem: true }`), so the caller's transaction, tenant scope and userId survive the elevation. Nothing else about the delete path changes identity: the `set_null` UPDATE, the `cascade` DELETE and the target's own delete still run as the caller. The refusal discloses the dependent COUNT only when the caller's own identity would have produced the same rows — otherwise the elevated probe would turn `DELETE_RESTRICTED` into a cardinality oracle over a table the caller may not read. The referenced OBJECT is named either way. Maintainer ruling 2026-08-26 (option A) with its four binding constraints. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o --- ...ne-reference-check-system-identity.test.ts | 222 +++++++++ packages/objectql/src/engine.ts | 280 +++++++++++- ...-reference-cleanup-system-identity.test.ts | 421 ++++++++++++++++++ packages/spec/src/system/operation-message.ts | 40 ++ 4 files changed, 958 insertions(+), 5 deletions(-) create mode 100644 packages/objectql/src/engine-reference-check-system-identity.test.ts create mode 100644 packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts diff --git a/packages/objectql/src/engine-reference-check-system-identity.test.ts b/packages/objectql/src/engine-reference-check-system-identity.test.ts new file mode 100644 index 0000000000..2a01cb615e --- /dev/null +++ b/packages/objectql/src/engine-reference-check-system-identity.test.ts @@ -0,0 +1,222 @@ +// 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>>(); + 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, 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; }, + async find(o: string, ast: any) { return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); }, + 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) { + 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) { + 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) { + 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[]) { 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); + 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); + 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'); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index d45aff81c2..1ef2d439b1 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -10930,6 +10930,173 @@ export class ObjectQL implements IObjectQLEngine { return current.filter((v) => String(v) !== String(id)); } + /** + * [#12166] The identity the pre-delete reference CHECK runs under — system, + * unconditionally (maintainer ruling 2026-08-26, option A). + * + * `sudo()`-shaped, never a bare `{ isSystem: true }`. The full reasoning sits + * at the one call site (`cascadeDeleteRelations`'s dependents probe); the + * short form is that the spread carries three things the bare spelling would + * drop — the caller's transaction handle, the TENANT scope, and `userId`, + * which is the audit ledger's triggered-by half beside `isSystem`'s + * executed-as half. + * + * A helper rather than an inline spread because the disclosure re-probe below + * has to be able to say "the caller's OWN context" by contrast, and because a + * second inline spelling is how one of the two would later gain a key the + * other lacks. + */ + private static referenceCheckContext(context?: ExecutionContext): ExecutionContext { + return { ...(context ?? {}), isSystem: true } as ExecutionContext; + } + + /** + * [#12166, ruling constraint 3] File the reference-check elevation, BOTH + * halves: `triggeredBy` = the operator who asked for the delete, + * `executedAs: 'system'` = the identity the check actually ran under. + * + * ## Why a record exists at all + * + * The elevation is the whole of what this card changed, and an elevation + * nobody can see afterwards is the thing a security reviewer has no way to + * audit. The ruling names the Salesforce/Dataverse ledger shape for exactly + * this reason: a platform-executed integrity action is attributable to the + * human who triggered it, not laundered into "system did something". + * + * ## Why the LOGGER and not a `sys_audit_log` row + * + * Stated plainly, because a narrow audit edge has to be visible to be honest + * (`read-audit.ts` takes the same posture about its own narrowing): + * + * The elevated operation is a READ, and `plugin-audit`'s read writer declares + * — and pins — that a system-elevated read produces NO row, deliberately: + * those are the platform reading for its own bookkeeping, and recording them + * would bury the human views that ledger exists to make findable. Minting a + * row here would either contradict that declared boundary or require the + * engine to reach into a plugin it does not depend on. The engine's logger is + * the channel it owns, and it is where the reporting deployment read the + * defect in the first place — the issue's evidence is a server log line. + * + * So: this is a LOG record, at `info`, and the honest limit of it is that a + * deployment which does not retain engine logs retains no trace. A durable + * `sys_audit_log` row for the elevation is a follow-up that belongs to + * plugin-audit, which owns that row shape. + * + * `debug` was not an option: a record that is off by default in production is + * not an audit record. + * + * ⛔ The record names the referenced OBJECT and the relation FIELD — declared + * metadata — and never a row id, a field value, or a count. Constraint 2 + * governs what the CALLER is told; this is the server-side ledger and could + * defensibly say more, but saying more here is how a count reaches a support + * transcript, and the two surfaces are kept on one rule so neither has to be + * re-audited alone. + */ + private recordReferenceCheckElevation( + object: string, + id: string | number, + childName: string, + fieldName: string, + context?: ExecutionContext, + ): void { + // The triggered-by half. `userId` survives the `sudo()`-shaped elevation, + // which is what makes the two halves recordable at all; `actor` is the + // service-principal channel (ADR-0014 D2) that keeps a non-user- + // authenticated caller attributable instead of anonymous. + const triggeredBy = + (context?.userId != null && String(context.userId)) || + (typeof (context as any)?.actor === 'string' && (context as any).actor.trim()) || + // Not "unknown": a context with neither channel IS the platform acting + // for itself (boot provisioning, seed replay, an inner cascade), and + // naming that honestly is more useful than a null. + (context?.isSystem === true ? 'system' : 'anonymous'); + this.logger.info( + `[reference-cleanup] referential integrity check on '${childName}' executed as SYSTEM ` + + `for delete of ${object}/${String(id)} triggered by ${triggeredBy}`, + { + triggeredBy, + executedAs: 'system', + object, + recordId: String(id), + referencedObject: childName, + relationField: fieldName, + }, + ); + } + + /** + * [#12166, ruling constraint 2] May the refusal disclose HOW MANY rows + * reference this record? + * + * The elevation above is what makes this question exist. Before it, the + * dependents probe ran as the caller, so every number the `DELETE_RESTRICTED` + * envelope reported (`dependentCount`, the localized message's `count`, the + * developerMessage) was derived from rows the caller could read by + * definition. After it, the probe sees rows the caller may have no grant on + * at all — and shipping that count back would hand a caller with NO read + * permission on `childName` an exact, repeatable cardinality oracle over it: + * delete-probe each record of `object`, read the count off the 409, and + * reconstruct the hidden table's reference histogram without ever being + * allowed to read a row. + * + * The ruling closes that: the error names the referenced OBJECT and nothing + * about rows — "one notch more conservative than Salesforce". It does NOT ask + * for the count to be dropped for everyone, and dropping it would be its own + * regression (the count is what makes the refusal actionable, and it predates + * this card for callers who could always compute it). So the count is + * disclosed on exactly one condition: **the caller's own identity would have + * produced the same rows.** + * + * The comparison is on ROW IDENTITY, not on length. Two different sets can + * have the same size, and RLS narrowing is precisely the case that produces + * one — a caller who may read `childName` at the object level but whose + * row-level filter hides two of three referencing rows must not be told + * "three" either. Comparing id sets makes the object-level denial and the + * row-level narrowing the same answer, which is what they are. + * + * Costs one extra query, and only on the refusal path — the delete is being + * aborted anyway, so this is not on any successful delete's critical path. + * + * Fails CLOSED, on the DISCLOSURE axis: any throw at all (a genuine + * permission denial, an outage, a driver error) answers "not disclosable". + * That direction is deliberate and is the opposite of #8895's stance for the + * integrity probe itself — an integrity guard that cannot run must not + * silently pass, but a DISCLOSURE that cannot be justified must not silently + * happen. The two guards face opposite ways because a wrong answer costs + * opposite things: there, a permitted delete that should have been refused; + * here, a leak. Nothing about the refusal itself depends on this — an + * unanswerable probe still refuses the delete, just without the number. + */ + private async dependentCountIsDisclosable( + childName: string, + probeWhere: Record, + probedIds: readonly string[], + fieldName: string, + multiValued: boolean, + id: string | number, + context?: ExecutionContext, + ): Promise { + // The caller IS the system (seed replay, migration, an internal cascade + // recursion). Nothing was elevated PAST them, so there is nothing to + // withhold — and withholding here would strip the count from the engine's + // own internal paths for no gain. + if (context?.isSystem === true) return true; + try { + let own = await this.find(childName, { where: probeWhere, context } as any); + // The SAME narrowing the elevated probe applied (#9362), or the two sets + // would be compared through different definitions of "references this + // record" and a multi-value relation would read as non-disclosable + // always. + if (multiValued && own) { + own = own.filter((row: any) => ObjectQL.storedReferenceIncludes(row?.[fieldName], id)); + } + const ownIds = new Set((own ?? []).map((r: any) => String(r?.id))); + return ownIds.size === probedIds.length && probedIds.every((rid) => ownIds.has(rid)); + } catch { + return false; + } + } + /** * Apply referential delete behavior for relations pointing AT this record, * before it is removed. For every registered object with a `master_detail` @@ -10981,6 +11148,12 @@ export class ObjectQL implements IObjectQLEngine { // raised reaches the caller with its envelope intact, exactly as #8895's // probe failure does. const objects: ServiceObject[] = this._registry.getAllObjects(); + // [#12166, ruling constraint 3] Referenced objects this call has already + // filed an elevation record for. One record per referenced OBJECT, not per + // relation: two lookup fields on the same child pointing at the same parent + // are one "the platform read object X as system" fact, and filing it twice + // would make the ledger's row count a function of the child's field layout. + const elevationRecorded = new Set(); for (const child of objects) { const childName = (child as any)?.name as string | undefined; const fields = (child as any)?.fields as Record | undefined; @@ -11099,11 +11272,73 @@ export class ObjectQL implements IObjectQLEngine { behavior = 'restrict'; } + // [#12166] The probe filter, hoisted: the DISCLOSURE re-probe below has + // to ask the identical question under the caller's own identity, and a + // second call to `referenceProbeFilter` would be a second spelling of + // "which rows reference this record" that could drift from this one. + const probeWhere = this.referenceProbeFilter(fieldName, fdef, id); + + // [#12166, ruling constraint 3] File the elevation BEFORE it happens, + // and file BOTH halves — triggered-by = the deleting operator, + // executed-as = system (the Salesforce/Dataverse ledger shape). + // + // Before the probe rather than after, because the fact being recorded + // is that the platform read this object under an identity the caller + // does not hold. That is true whether the probe then finds rows, finds + // none, refuses the delete, or fails — and a record written only on the + // success path would be missing exactly the runs an auditor goes + // looking for. + if (!elevationRecorded.has(childName)) { + elevationRecorded.add(childName); + this.recordReferenceCheckElevation(object, id, childName, fieldName, context); + } + let dependents: any[]; try { dependents = await this.find( childName, - { where: this.referenceProbeFilter(fieldName, fdef, id), context } as any, + // [#12166] (maintainer ruling 2026-08-26, option A) SYSTEM identity, + // unconditionally. This probe is the platform's own referential- + // integrity read, not a query the caller asked for, and running it + // as the caller made "delete permission" silently mean "delete + + // read on EVERY referencing table": a caller with full delete rights + // on `object` but no read grant on `childName` got a blanket 403 + // from the security middleware — whether or not a reference existed, + // and with `childName` EMPTY. Measured on a real deployment across + // 17 role×object pairs, with the A/B control that granting read-only + // on the referencing object (touching NOTHING about delete rights) + // turned the identical delete into a 200. + // + // Referential-integrity actions are engine responsibility executed + // under system identity on every mainstream platform — the RDBMS FK + // baseline, Salesforce (lookup clearing / cascade delete documented + // as bypassing sharing), Dataverse, ServiceNow, Odoo. Caller + // identity here was the outlier. + // + // The elevation is `sudo()`-SHAPED (`{ ...context, isSystem: true }`), + // never a bare `{ isSystem: true }` — the same posture + // `recomputeSummaries` holds one axis over. Three reasons, and each + // one is a defect if dropped: + // - the open transaction handle, `tenantId` and `timezone` must + // survive, or this probe leaves the caller's transaction and + // stops being TENANT-scoped — a bare system context would widen + // the probe across the tenant wall, which is the opposite of + // what this card relaxes; + // - `userId` survives, and that IS the audit ledger's + // "triggered-by" half (ruling constraint 3): the record carries + // triggered-by = the deleting operator and executed-as = system. + // `read-audit.ts` states the same property of `sudo()`; + // - it is a NARROW elevation: nothing else about the delete path + // changes identity (ruling constraint 1). The `set_null` UPDATE + // and the `cascade` DELETE below still run as the caller, byte + // for byte, so this relaxes the reference CHECK and not the + // caller's own authority over the dependent rows. + // + // [Constraint 4] If the spec later declares per-relationship + // on-delete behaviour, BEHAVIOUR follows the declaration; the + // identity of this probe stays system, unconditionally. Do not make + // this line conditional on `behavior`. + { where: probeWhere, context: ObjectQL.referenceCheckContext(context) } as any, ); } catch (error) { // [#8895] Discriminate by error TYPE — this probe IS the referential @@ -11155,6 +11390,16 @@ export class ObjectQL implements IObjectQLEngine { } if (!dependents || dependents.length === 0) continue; + // [#12166] The elevated probe's row IDENTITY, captured here — after the + // multi-value narrowing and BEFORE the `requiredSetNull && multiValued` + // narrowing below reduces `dependents` to the emptied subset. This is + // the set the disclosure decision compares against, and the stage + // matters: the emptied subset is computed from these rows' own stored + // values, so a caller who can see exactly these rows can derive the + // emptied count too, while a caller who cannot see them can derive + // neither. Comparing at the later stage would leak the difference. + const probedIds: readonly string[] = dependents.map((r: any) => String(r?.id)); + // [#9688] The deferred half of the required escalation, decided per // ROW now that the rows are read and exactly narrowed — every row // here genuinely holds `id`, so its remainder is its set minus that @@ -11214,29 +11459,54 @@ export class ObjectQL implements IObjectQLEngine { const required = fdef.deleteBehavior !== 'restrict' && fdef.required === true; const msgCtx = this.validationMessageContext(object, context); const parent = objects.find((o) => (o as any)?.name === object); + // [#12166, ruling constraint 2] Whether this refusal may carry the + // row COUNT — see `dependentCountIsDisclosable` for why the elevation + // above is what makes the question exist, and why the answer is "only + // if the caller's own identity would have produced the same rows". + // The OBJECT is named either way: that half is what the ruling + // requires the caller to learn. + const discloseCount = await this.dependentCountIsDisclosable( + childName, probeWhere, probedIds, fieldName, multiValued, id, context, + ); const err: any = new Error( renderOperationMessage( { - messageKey: required ? 'delete_restricted_required' : 'delete_restricted', + messageKey: discloseCount + ? (required ? 'delete_restricted_required' : 'delete_restricted') + : (required ? 'delete_restricted_required_opaque' : 'delete_restricted_opaque'), params: { object: this.objectDisplayLabel(object, (parent as any)?.label, msgCtx), dependentObject: this.objectDisplayLabel(childName, (child as any)?.label, msgCtx), field: resolveFieldLabel(fieldName, fdef, { ...msgCtx, objectName: childName }), - count: dependents.length, + ...(discloseCount ? { count: dependents.length } : {}), }, }, { locale: msgCtx.locale, translate: msgCtx.translate }, ), ); + // The DEVELOPER half is subject to the same rule, and the reason is + // measured rather than assumed: for `DELETE_RESTRICTED` (unlike the + // #7414 permission denial) this sentence RIDES THE ENVELOPE — REST's + // `mapDataError` ships it — so a count withheld from `message` and + // left here would not be withheld at all. err.developerMessage = - `Cannot delete ${object} (${id}): ${dependents.length} dependent ${childName} record(s) reference it via ${fieldName}` + + `Cannot delete ${object} (${id}): ` + + `${discloseCount ? `${dependents.length} dependent ${childName} record(s) reference it` : `dependent ${childName} record(s) reference it`} via ${fieldName}` + `${required ? ` (${fieldName} is required, so it cannot be cleared)` : ''}. ` + `Delete or reassign them first, or set deleteBehavior:'cascade' on ${childName}.${fieldName}.`; err.code = 'DELETE_RESTRICTED'; err.status = 409; err.object = object; + // Constraint 2's REQUIRED half — the referenced object is named + // unconditionally, because "which table is blocking me" is the one + // thing the reporting deployment's admins could not self-diagnose. err.dependentObject = childName; - err.dependentCount = dependents.length; + // …and its withheld half. Absent rather than zeroed: `0` would be a + // false statement about the rows (there is at least one, or this + // branch would not have been reached), and REST's envelope builder + // already omits the key for a non-number, so an absent count reaches + // the client as an absent key rather than as a lie. + if (discloseCount) err.dependentCount = dependents.length; throw err; } diff --git a/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts b/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts new file mode 100644 index 0000000000..2bb969c592 --- /dev/null +++ b/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts @@ -0,0 +1,421 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #12166 — the delete-time reference check runs under the SYSTEM identity. + * Maintainer ruling 2026-08-26, option A, with four binding constraints. + * + * ## What was broken, and why this suite is HERE and not in objectql + * + * 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 — whether or not a + * reference existed, and with the referencing table EMPTY. Measured on a real + * deployment (`@objectstack/*@17.2.0`) across 17 role×object pairs; its A/B + * control was that granting read-only on the referencing object, touching + * NOTHING about delete rights, turned the identical operation into a 200. + * + * The refusal is produced by plugin-security's CRUD middleware, and the fix is + * in `objectql`'s engine. Neither package alone can ask the question: an + * objectql-only suite would have to double the very middleware whose verdict is + * the subject, and a plugin-security-only suite has no delete path to run. So + * this suite drives the REAL `SecurityPlugin` middleware over a REAL `ObjectQL` + * engine, and the 403 it starts from is the deployment's own, not a stand-in. + * (`@objectstack/objectql` is aliased to the producer's SOURCE by this + * package's `vitest.config.ts`, so these verdicts are about the tree in the + * checkout rather than about the last build.) + * + * ## The contract pinned here is the TERMINAL STATE, not the call + * + * Deliberately NOT "the probe was issued with `isSystem`" — that pin goes green + * the moment someone wraps the call differently while the caller still 403s. + * What is pinned is the report's own A/B control: + * + * empty referencing table + no read grant on it + full delete rights + * on the target ⇒ the delete SUCCEEDS. + * + * …and its converse, because a relaxation must not become a hole: a caller + * without delete rights on the target is still refused. Both directions, or the + * first one alone would also be satisfied by deleting the permission check. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { PermissionSet } from '@objectstack/spec/security'; +import { ObjectQL } from '@objectstack/objectql'; +import { SecurityPlugin } from './security-plugin.js'; + +// --------------------------------------------------------------------------- +// The three-part fixture from the report: object A, referenced by B's lookup, +// and a role with full delete on A and nothing at all on B. +// --------------------------------------------------------------------------- + +/** A — the object being deleted. */ +const PRODUCT = { + name: 'os_ehr_product', + label: 'Product', + sharingModel: 'private', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', type: 'text' as const }, + organization_id: { name: 'organization_id', type: 'text' as const }, + owner_id: { name: 'owner_id', type: 'text' as const }, + }, +}; + +/** + * B — the referencing object from the report's server log. Its lookup is + * OPTIONAL, so the resolved behaviour is `set_null`: this is the shape whose + * probe used to 403 while the table was empty. + */ +const ANDON = { + name: 'os_ehr_andon_record', + label: 'Andon Record', + sharingModel: 'private', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + organization_id: { name: 'organization_id', type: 'text' as const }, + owner_id: { name: 'owner_id', type: 'text' as const }, + product: { name: 'product', type: 'lookup' as const, reference: 'os_ehr_product' }, + }, +}; + +/** + * C — a REQUIRED lookup, so a row here escalates `set_null` → `restrict` and + * the delete is refused with `DELETE_RESTRICTED`. That is the branch ruling + * constraint 2 governs: it is the only path on which the elevated probe's + * findings reach the caller at all. + */ +const BATCH = { + name: 'os_ehr_batch', + label: 'Batch', + sharingModel: 'private', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true }, + organization_id: { name: 'organization_id', type: 'text' as const }, + owner_id: { name: 'owner_id', type: 'text' as const }, + product: { name: 'product', type: 'lookup' as const, reference: 'os_ehr_product', required: true }, + }, +}; + +/** + * The reporting deployment's role: everything on A, NOTHING on B or C — not a + * narrowed read, no entry at all. `modifyAllRecords` because the report's role + * held full delete rights; the card is about object-level read on the + * REFERENCING table, not about row scoping on the target. + */ +const LINE_LEAD: PermissionSet = { + name: 'ehr_line_lead', + label: 'Line Lead', + objects: { + os_ehr_product: { + allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, + modifyAllRecords: true, viewAllRecords: true, + }, + }, +} as unknown as PermissionSet; + +/** + * The A/B control's OTHER arm, and the disclosure control for constraint 2: + * identical to `LINE_LEAD` plus read on C. Nothing about delete rights differs. + */ +const SIGHTED_LEAD: PermissionSet = { + name: 'ehr_sighted_lead', + label: 'Sighted Lead', + objects: { + os_ehr_product: { + allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, + modifyAllRecords: true, viewAllRecords: true, + }, + os_ehr_batch: { allowRead: true, viewAllRecords: true }, + }, +} as unknown as PermissionSet; + +/** + * The converse arm: may READ everything, may not DELETE the target. This is the + * role that proves the relaxation did not become a hole — if the fix had + * loosened the delete gate itself rather than the reference check, this one + * would start succeeding. + */ +const READER: PermissionSet = { + name: 'ehr_reader', + label: 'Reader', + objects: { + os_ehr_product: { allowRead: true, viewAllRecords: true }, + os_ehr_andon_record: { allowRead: true, viewAllRecords: true }, + os_ehr_batch: { allowRead: true, viewAllRecords: true }, + }, +} as unknown as PermissionSet; + +function makeStubDriver() { + const stores = new Map>>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + let nextId = 0; + // `$contains` / `$or` are answered because `referenceProbeFilter` spells a + // `multiple: true` probe that way (#9362); a double that ignored them would + // report "no dependents" and turn a refusal into a silent success — the + // fail-OPEN direction #8895 ruled out for this guard. + const matchOne = (stored: unknown, spec: unknown): boolean => { + if (spec !== null && typeof spec === 'object' && !Array.isArray(spec)) { + const [op, cmp] = Object.entries(spec as Record)[0] ?? []; + if (op === '$contains') { + const values = Array.isArray(stored) ? stored : [stored]; + return values.some((v) => v != null && typeof v !== 'object' && String(v) === String(cmp)); + } + if (op === '$eq') return (stored ?? null) === ((cmp as any) ?? null); + return false; + } + return (stored ?? null) === ((spec as any) ?? null); + }; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$or') { if (!(v as any[]).some((sub) => matches(row, sub))) return false; continue; } + if (k === '$and') { if (!(v as any[]).every((sub) => matches(row, sub))) return false; continue; } + if (k.startsWith('$')) continue; + if (!matchOne(row[k], v)) 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; }, + async find(o: string, ast: any) { return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); }, + 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) { + 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) { + 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) { + 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[]) { 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 }; +} + +interface LogRecord { msg: string; meta?: Record } + +async function boot(sets: PermissionSet[] = [LINE_LEAD]) { + const info: LogRecord[] = []; + const engineLogger = { + info: vi.fn((msg: string, meta?: Record) => { info.push({ msg, meta }); }), + warn: vi.fn(), error: vi.fn(), debug: vi.fn(), + }; + const engine = new ObjectQL({ logger: engineLogger }); + const { driver, stores } = makeStubDriver(); + engine.registerDriver(driver, true); + await engine.init(); + for (const o of [PRODUCT, ANDON, BATCH]) engine.registry.registerObject(o as any); + + const schemas: Record = { + os_ehr_product: PRODUCT, os_ehr_andon_record: ANDON, os_ehr_batch: BATCH, + }; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { get: async (n: string) => schemas[n], list: async () => sets }, + }; + const ctx: any = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: vi.fn(), + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ defaultPermissionSets: sets, fallbackPermissionSet: sets[0]!.name }); + await plugin.init(ctx); + await plugin.start(ctx); + + const SYSTEM = { context: { isSystem: true } } as any; + return { + engine, + stores, + engineInfo: info, + /** Seed rows past the middleware — fixture setup is not the subject. */ + seed: (object: string, row: Record) => engine.insert(object, row, SYSTEM), + caller: (userId = 'u_lead') => ({ userId, tenantId: 'org-1', positions: [], permissions: [] }), + deleteAs: (object: string, id: string, context: any) => + engine.delete(object, { where: { id }, context } as any).then(() => null, (e: any) => e), + }; +} + +// --------------------------------------------------------------------------- +// The contract: the report's A/B control, both directions. +// --------------------------------------------------------------------------- + +describe('#12166 — pre-delete reference check runs as SYSTEM (ruling A)', () => { + it('THE CONTRACT: empty referencing table + no read grant on it + full delete rights ⇒ the delete SUCCEEDS', async () => { + const h = await boot(); + const p = await h.seed('os_ehr_product', { name: 'Widget' }); + + // `os_ehr_andon_record` is EMPTY and the caller has no grant on it at all. + expect(h.stores.get('os_ehr_andon_record')?.size ?? 0).toBe(0); + + const err = await h.deleteAs('os_ehr_product', p.id, h.caller()); + + // Before the fix this was: + // PERMISSION_DENIED / 403, developerMessage + // "[Security] Access denied: operation 'find' on object + // 'os_ehr_andon_record' is not permitted for positions []" + // — the report's log line, reproduced verbatim by this fixture. + expect(err).toBe(null); + expect(h.stores.get('os_ehr_product')?.has(p.id)).toBe(false); + }); + + it('THE CONVERSE: a caller without delete rights on the TARGET is still refused', async () => { + // The relaxation must not become a hole. If the fix had loosened the delete + // gate rather than the reference check, this goes green and nobody notices: + // the case above cannot tell the two apart on its own. + const h = await boot([READER]); + const p = await h.seed('os_ehr_product', { name: 'Widget' }); + + const err = await h.deleteAs('os_ehr_product', p.id, h.caller('u_reader')); + + expect(err).not.toBe(null); + expect(err.code).toBe('PERMISSION_DENIED'); + expect(err.statusCode ?? err.status).toBe(403); + // …and refused about the TARGET, not about a referencing table. + expect(err.details?.object).toBe('os_ehr_product'); + expect(err.details?.operation).toBe('delete'); + expect(h.stores.get('os_ehr_product')?.has(p.id)).toBe(true); + }); + + it('the elevation is the CHECK only — a non-empty referencing table still needs the caller\'s own write authority (constraint 1)', async () => { + // Ruling constraint 1: "Nothing else about the delete path changes + // identity." The `set_null` UPDATE below still runs as the caller, so a + // caller with no grant on the referencing object is refused here — the + // reference CHECK was relaxed, the caller's authority over the dependent + // rows was not. Pinned so the boundary is visible rather than discovered: + // a later edit that elevated the cleanup WRITES too would turn this green. + const h = await boot(); + const p = await h.seed('os_ehr_product', { name: 'Widget' }); + await h.seed('os_ehr_andon_record', { product: p.id }); + + const err = await h.deleteAs('os_ehr_product', p.id, h.caller()); + + expect(err).not.toBe(null); + expect(err.code).toBe('PERMISSION_DENIED'); + // The refusal now names the WRITE it could not perform, not the read the + // check used to fail on. + expect(err.details?.object).toBe('os_ehr_andon_record'); + expect(err.details?.operation).toBe('update'); + }); +}); + +describe('#12166 constraint 2 — the refusal names the OBJECT and leaks nothing about rows', () => { + it('a caller who cannot read the referencing object is told WHICH object blocks them, and no count', async () => { + const h = await boot(); + const p = await h.seed('os_ehr_product', { name: 'Widget' }); + await h.seed('os_ehr_batch', { product: p.id }); + await h.seed('os_ehr_batch', { product: p.id }); + + const err = await h.deleteAs('os_ehr_product', p.id, h.caller()); + + expect(err).not.toBe(null); + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.status).toBe(409); + // Constraint 2's REQUIRED half — the blocking object is named. This is the + // fact the reporting deployment's admins had no way to obtain. + expect(err.dependentObject).toBe('os_ehr_batch'); + expect(err.message).toContain('Batch'); + // …and its WITHHELD half. The probe saw two rows under an identity this + // caller does not hold; saying "2" would hand them an exact cardinality + // oracle over a table they may not read. + expect(err.dependentCount).toBeUndefined(); + expect(err.message).not.toMatch(/\d/); + expect(err.developerMessage).not.toMatch(/\d dependent/); + // Nothing about the REFERENCING object's rows, in either sentence. + // + // Scoped to `os_ehr_batch`'s row ids deliberately. The target's own id DOES + // appear in `developerMessage` ("Cannot delete os_ehr_product (r_2)") and is + // not a disclosure at all: the caller passed it in. Constraint 2 is about + // the rows the ELEVATION let the engine see, which are these. + for (const rowId of h.stores.get('os_ehr_batch')!.keys()) { + expect(err.message).not.toContain(rowId); + expect(err.developerMessage).not.toContain(rowId); + } + // The user-facing sentence renders LABELS only — no API name, no id. + expect(err.message).not.toContain(p.id); + expect(err.message).not.toContain('os_ehr_batch'); + }); + + it('CONTROL: a caller who CAN read the referencing object still gets the count', async () => { + // Without this arm the case above is also satisfied by deleting the count + // for everyone — which is a regression, not the ruling: the count predates + // this card and is what makes the refusal actionable for a caller who could + // always have computed it. The suppression is CONDITIONAL, and this is the + // condition. + const h = await boot([SIGHTED_LEAD]); + const p = await h.seed('os_ehr_product', { name: 'Widget' }); + await h.seed('os_ehr_batch', { product: p.id }); + await h.seed('os_ehr_batch', { product: p.id }); + + const err = await h.deleteAs('os_ehr_product', p.id, h.caller('u_sighted')); + + expect(err.code).toBe('DELETE_RESTRICTED'); + expect(err.dependentObject).toBe('os_ehr_batch'); + expect(err.dependentCount).toBe(2); + expect(err.message).toContain('2'); + }); +}); + +describe('#12166 constraint 3 — the ledger records BOTH halves', () => { + it('triggered-by = the deleting operator, executed-as = system', async () => { + const h = await boot(); + const p = await h.seed('os_ehr_product', { name: 'Widget' }); + + await h.deleteAs('os_ehr_product', p.id, h.caller('u_lead')); + + const filed = h.engineInfo.filter((r) => r.msg.includes('[reference-cleanup]')); + expect(filed.length).toBeGreaterThan(0); + const andon = filed.find((r) => r.meta?.referencedObject === 'os_ehr_andon_record'); + expect(andon).toBeDefined(); + expect(andon!.meta).toMatchObject({ + triggeredBy: 'u_lead', // the operator who asked for the delete + executedAs: 'system', // the identity the check actually ran under + object: 'os_ehr_product', + recordId: p.id, + referencedObject: 'os_ehr_andon_record', + relationField: 'product', + }); + // ⛔ The ledger names declared metadata only — never a row id, a value or a + // count. Constraint 2 governs what the CALLER is told; the same rule is + // held here so a support transcript cannot become the leak. + expect(Object.keys(andon!.meta!)).not.toContain('dependentCount'); + }); + + it('files the elevation even when the check then REFUSES the delete', async () => { + // Filed before the probe, not after it: a record written only on the + // success path would be missing exactly the runs an auditor looks for. + const h = await boot(); + const p = await h.seed('os_ehr_product', { name: 'Widget' }); + await h.seed('os_ehr_batch', { product: p.id }); + + const err = await h.deleteAs('os_ehr_product', p.id, h.caller('u_lead')); + expect(err.code).toBe('DELETE_RESTRICTED'); + + expect(h.engineInfo.some( + (r) => r.msg.includes('[reference-cleanup]') && r.meta?.referencedObject === 'os_ehr_batch', + )).toBe(true); + }); +}); diff --git a/packages/spec/src/system/operation-message.ts b/packages/spec/src/system/operation-message.ts index 553545afc1..5d085612c7 100644 --- a/packages/spec/src/system/operation-message.ts +++ b/packages/spec/src/system/operation-message.ts @@ -108,6 +108,30 @@ export function operationMessageTranslationKey(messageKey: string): string { * rule the field catalog states: `DELETE_RESTRICTED` stays one member of the * ADR-0112 vocabulary that clients match on. * + * [#12166] Each has an `_opaque` twin — the SAME sentence with `{{count}}` + * removed, and nothing else changed. Four templates, still one wire code. + * + * They exist because the delete-time reference check now runs under the SYSTEM + * identity (maintainer ruling 2026-08-26): the engine's dependents probe sees + * rows the caller may hold no read grant on, so the count in the counted + * variants can be a number the caller could not have obtained. Shipping it + * would turn the refusal into a cardinality oracle over a table the caller + * cannot read. The ruling's second constraint — the error names the referenced + * OBJECT, never record contents, "one notch more conservative than Salesforce" + * — is what these serve. + * + * ⛔ They are NOT a general "shorter" variant, and the choice between the pairs + * is not cosmetic. `ObjectQL.dependentCountIsDisclosable` owns it: the counted + * form is used only when the caller's own identity would have produced the same + * rows. Rendering a counted variant unconditionally re-opens the leak; making + * the opaque one the default withholds an actionable number from every caller + * who could always compute it. Both halves matter. + * + * The object and the relation field stay named in the opaque forms. They are + * DECLARED METADATA, not rows, and they are the whole of what makes the refusal + * self-diagnosable — the reporting deployment's admins could see a delete + * button that always 403'd and had no way to learn which table was blocking it. + * * Placeholders: `{{object}}` and `{{dependentObject}}` are LABELS in the * caller's locale (the API names live on `developerMessage` and on the * structured `object` / `dependentObject` fields), `{{field}}` is the @@ -155,6 +179,10 @@ export const BUILTIN_OPERATION_MESSAGES: Record> 'This {{object}} is still referenced by {{count}} {{dependentObject}} record(s) through “{{field}}”. Delete or reassign them first.', delete_restricted_required: 'This {{object}} is still referenced by {{count}} {{dependentObject}} record(s) through “{{field}}”, which is required and cannot be cleared. Delete or reassign them first.', + delete_restricted_opaque: + 'This {{object}} is still referenced by {{dependentObject}} record(s) through “{{field}}”. Delete or reassign them first.', + delete_restricted_required_opaque: + 'This {{object}} is still referenced by {{dependentObject}} record(s) through “{{field}}”, which is required and cannot be cleared. Delete or reassign them first.', }, 'zh-CN': { permission_denied: '您没有执行此操作的权限,如需访问请联系管理员。', @@ -164,6 +192,10 @@ export const BUILTIN_OPERATION_MESSAGES: Record> '该{{object}}正被 {{count}} 条{{dependentObject}}记录通过「{{field}}」引用,请先删除或改派这些记录。', delete_restricted_required: '该{{object}}正被 {{count}} 条{{dependentObject}}记录通过「{{field}}」引用,且该字段为必填、无法清空,请先删除或改派这些记录。', + delete_restricted_opaque: + '该{{object}}正被{{dependentObject}}记录通过「{{field}}」引用,请先删除或改派这些记录。', + delete_restricted_required_opaque: + '该{{object}}正被{{dependentObject}}记录通过「{{field}}」引用,且该字段为必填、无法清空,请先删除或改派这些记录。', }, 'ja-JP': { permission_denied: 'この操作を実行する権限がありません。アクセスが必要な場合は管理者にお問い合わせください。', @@ -175,6 +207,10 @@ export const BUILTIN_OPERATION_MESSAGES: Record> 'この{{object}}は {{count}} 件の{{dependentObject}}レコードから「{{field}}」で参照されています。先にそれらを削除するか、参照先を変更してください。', delete_restricted_required: 'この{{object}}は {{count}} 件の{{dependentObject}}レコードから「{{field}}」で参照されています。この項目は必須のため空にできません。先にそれらを削除するか、参照先を変更してください。', + delete_restricted_opaque: + 'この{{object}}は{{dependentObject}}レコードから「{{field}}」で参照されています。先にそれらを削除するか、参照先を変更してください。', + delete_restricted_required_opaque: + 'この{{object}}は{{dependentObject}}レコードから「{{field}}」で参照されています。この項目は必須のため空にできません。先にそれらを削除するか、参照先を変更してください。', }, 'es-ES': { permission_denied: @@ -187,6 +223,10 @@ export const BUILTIN_OPERATION_MESSAGES: Record> '{{count}} registro(s) de {{dependentObject}} todavía hacen referencia a este {{object}} mediante «{{field}}». Elimínelos o reasígnelos primero.', delete_restricted_required: '{{count}} registro(s) de {{dependentObject}} todavía hacen referencia a este {{object}} mediante «{{field}}», un campo obligatorio que no puede vaciarse. Elimínelos o reasígnelos primero.', + delete_restricted_opaque: + 'Todavía hay registros de {{dependentObject}} que hacen referencia a este {{object}} mediante «{{field}}». Elimínelos o reasígnelos primero.', + delete_restricted_required_opaque: + 'Todavía hay registros de {{dependentObject}} que hacen referencia a este {{object}} mediante «{{field}}», un campo obligatorio que no puede vaciarse. Elimínelos o reasígnelos primero.', }, }; From 31645534b2ca101e1354c4a47ea14ab51471ec21 Mon Sep 17 00:00:00 2001 From: os-warren Date: Wed, 26 Aug 2026 15:15:16 +0000 Subject: [PATCH 2/3] fix(objectql,spec): withhold the dependent count from a caller the elevation carried past MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows the gate union derived over the real changeset: - `check:objectql-double-limit` — both new `find` doubles now apply the caller's `limit` bound by presence, so neither is looser than the engine. - `check:durability-read-invention` — the disclosure probe's catch no longer answers silently. It still withholds rather than rethrowing (propagating a DISCLOSURE probe's failure would turn a correct 409 into a 500), but it now says so, which is the rule's own second remedy. - `check:query-options-erasure` — the new `find` call carries the declared `EngineQueryOptions` type instead of `as any`, so the engine.ts ratchet does not grow. Adds the changeset (minor on both packages — a permission-behaviour change should not arrive as a patch bump). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o --- .../delete-reference-check-system-identity.md | 78 +++++++++++++++++++ ...ne-reference-check-system-identity.test.ts | 8 +- packages/objectql/src/engine.ts | 34 +++++++- ...-reference-cleanup-system-identity.test.ts | 8 +- 4 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 .changeset/delete-reference-check-system-identity.md diff --git a/.changeset/delete-reference-check-system-identity.md b/.changeset/delete-reference-check-system-identity.md new file mode 100644 index 0000000000..949437a6f4 --- /dev/null +++ b/.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. diff --git a/packages/objectql/src/engine-reference-check-system-identity.test.ts b/packages/objectql/src/engine-reference-check-system-identity.test.ts index 2a01cb615e..08d39180a5 100644 --- a/packages/objectql/src/engine-reference-check-system-identity.test.ts +++ b/packages/objectql/src/engine-reference-check-system-identity.test.ts @@ -72,7 +72,13 @@ function makeStubDriver() { const driver: any = { name: 'memory', version: '0.0.0', supports: {}, async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, - async find(o: string, ast: any) { return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); }, + // 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) { nextId += 1; diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 1ef2d439b1..6415d61ce3 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -11082,7 +11082,13 @@ export class ObjectQL implements IObjectQLEngine { // own internal paths for no gain. if (context?.isSystem === true) return true; try { - let own = await this.find(childName, { where: probeWhere, context } as any); + // Typed through the DECLARED options type rather than cast: `where` is + // `Record` on `EngineQueryOptionsSchema` and `context` + // rides `BaseEngineOptions`, so nothing here needs erasing + // (`check:query-options-erasure` — the grandfathered `as any` on the + // elevated probe above is a pre-existing site this card does not sweep, + // tracked in #4918). + let own: any[] = await this.find(childName, { where: probeWhere, context }); // The SAME narrowing the elevated probe applied (#9362), or the two sets // would be compared through different definitions of "references this // record" and a multi-value relation would read as non-disclosable @@ -11092,7 +11098,31 @@ export class ObjectQL implements IObjectQLEngine { } const ownIds = new Set((own ?? []).map((r: any) => String(r?.id))); return ownIds.size === probedIds.length && probedIds.every((rid) => ownIds.has(rid)); - } catch { + } catch (error) { + // Says something rather than rethrowing, and the asymmetry is the point + // (`check:durability-read-invention` accepts either — "say something, or + // ask the error's type"). + // + // Rethrowing is WRONG here specifically. The integrity answer is already + // in hand: the elevated probe succeeded, the dependents are known, and + // this delete is being refused either way. Propagating a failure of the + // DISCLOSURE probe would convert a correct `409 DELETE_RESTRICTED` into a + // `500` — a worse answer for the caller, produced by a query that exists + // only to decide how much detail the refusal carries. + // + // Nor is the invented value a durability hazard of the class that rule + // guards: no declared set is being read, and `false` withholds. The + // failure mode of getting this wrong is a slightly less informative + // refusal, never a permitted delete and never an over-disclosure. + // + // The one thing that WOULD be wrong is doing it silently — an operator + // seeing count-free refusals has no way to tell "withheld on purpose" + // from "the disclosure probe has been failing for a week". + this.logger.warn( + `[reference-cleanup] could not determine whether the caller may be told how many '${childName}' ` + + `record(s) reference this one; withholding the count from the refusal (the refusal itself stands)`, + { object: childName, error: (error as Error)?.message ?? String(error) }, + ); return false; } } diff --git a/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts b/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts index 2bb969c592..12c477454b 100644 --- a/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts +++ b/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts @@ -183,7 +183,13 @@ function makeStubDriver() { const driver: any = { name: 'memory', version: '0.0.0', supports: {}, async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, - async find(o: string, ast: any) { return Array.from(storeFor(o).values()).filter((r) => matches(r, ast?.where)); }, + // 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) { nextId += 1; From 16d9a430569523a82afb363130a7d1a7aa230c9a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 04:33:53 +0000 Subject: [PATCH 3/3] test(objectql,plugin-security): give the #12166 pin suites the declared registerObject arity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Type Check · debt ledger` went red on this branch: `check:type-check-debt --re-measure` measured `@objectstack/objectql` at 356 against a recorded 354 (+2) and `@objectstack/plugin-security` at 12 against a recorded 11 (+1). All three are the same diagnostic in the two suites this PR added: engine-reference-check-system-identity.test.ts(125,61): error TS2554: Expected 2-5 arguments, but got 1. engine-reference-check-system-identity.test.ts(176,61): error TS2554: Expected 2-5 arguments, but got 1. delete-reference-cleanup-system-identity.test.ts(233,60): error TS2554: Expected 2-5 arguments, but got 1. `ObjectRegistry.registerObject(schema, packageId, namespace?, ownership?, priority?)` declares `packageId` as required; the three fixture registrations passed the schema alone. The remedy the gate names is to fix the errors -- a shrink-only ledger is raised by the maintainer, never by an author -- so the calls now pass the owning package id, `'test'`, the spelling 30 other suites in this workspace already use. Type-level only: no source file, contract face or assertion is touched, and the three call sites are the whole diff. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MnijPVVDakqK2J335JoJtq --- .../src/engine-reference-check-system-identity.test.ts | 4 ++-- .../src/delete-reference-cleanup-system-identity.test.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/objectql/src/engine-reference-check-system-identity.test.ts b/packages/objectql/src/engine-reference-check-system-identity.test.ts index 08d39180a5..46a48a26b0 100644 --- a/packages/objectql/src/engine-reference-check-system-identity.test.ts +++ b/packages/objectql/src/engine-reference-check-system-identity.test.ts @@ -122,7 +122,7 @@ describe('#12166 — the reference check is elevated, sudo()-shaped', () => { const { driver } = makeStubDriver(); engine.registerDriver(driver, true); await engine.init(); - for (const o of [acct, note, task]) engine.registry.registerObject(o as any); + 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 }); @@ -173,7 +173,7 @@ describe('#12166 constraint 1 — nothing ELSE on the delete path changes identi const { driver } = makeStubDriver(); engine.registerDriver(driver, true); await engine.init(); - for (const o of [acct, note, task]) engine.registry.registerObject(o as any); + 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 }); diff --git a/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts b/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts index 12c477454b..0ecc0291ed 100644 --- a/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts +++ b/packages/plugins/plugin-security/src/delete-reference-cleanup-system-identity.test.ts @@ -230,7 +230,7 @@ async function boot(sets: PermissionSet[] = [LINE_LEAD]) { const { driver, stores } = makeStubDriver(); engine.registerDriver(driver, true); await engine.init(); - for (const o of [PRODUCT, ANDON, BATCH]) engine.registry.registerObject(o as any); + for (const o of [PRODUCT, ANDON, BATCH]) engine.registry.registerObject(o as any, 'test'); const schemas: Record = { os_ehr_product: PRODUCT, os_ehr_andon_record: ANDON, os_ehr_batch: BATCH,