diff --git a/.changeset/explain-recognises-fail-closed-rls-denial.md b/.changeset/explain-recognises-fail-closed-rls-denial.md new file mode 100644 index 0000000000..dacbabce8a --- /dev/null +++ b/.changeset/explain-recognises-fail-closed-rls-denial.md @@ -0,0 +1,49 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(plugin-security): `explain` now reports a fail-closed RLS denial as `denies` with `allowed: false` (#13639) + +**A wrong answer is corrected — read this if you consume `explain`.** For one +class of request, `explain` previously answered `decision.allowed: true` about a +request that is guaranteed to return zero rows. It now answers `false`. + +**The class.** When applicable RLS policies exist but none can be compiled +against the current execution context — typically a required `current_user.*` +variable resolving to nothing, e.g. a caller with no active organization — the +compiler fails **closed** and composes plugin-security's `RLS_DENY_FILTER` +(`{ id: '__rls_deny__:…' }`), a predicate no record can satisfy. Enforcement +was always correct: the caller saw zero rows. + +`explain`, however, recognised only its own `__deny_all__` sentinel, so it +reported that composition with layer verdict **`narrows`** and +`decision.allowed: **true**`. That is the diagnostic tool giving an +affirmatively wrong answer to the operator asking why a user sees nothing — +every available signal pointing away from the cause. + +**What changed.** Deny recognition is now value-agnostic and routed through one +named predicate, so both the object-level `rls` verdict and the record-grained +layer attribution recognise either sentinel: + +- the `rls` layer verdict flips `narrows` → `denies`, with the matching detail; +- `decision.allowed` flips `true` → `false` for this class; +- the record-grained `tenant_isolation` and `rls` layers report the fail-closed + prose instead of "record does not match" prose. + +⭐ **Deliberately NOT changed — no payload moves.** `readFilter` (and a layer's +`rowFilter`) keeps reporting the predicate that was **actually composed**: a +deployment that receives `{ id: '__rls_deny__:…' }` today keeps receiving it +byte-for-byte. The documented `__deny_all__` collapse still fires for +`__deny_all__` alone, and the two sentinels are **not** merged. Rewriting the +published payload, and unifying the sentinel vocabulary, are recorded on #13639 +as separate deployment-facing decisions. + +**Record-level correctness did not move**, only its prose: the record-grained +`outcome`, `matchesRecord` and rule `effect` were already right, because the +sentinel excludes every real record on its own. + +**If you assert on `explain` output**, expectations that encoded the old answer +for a fail-closed RLS denial — verdict `narrows`, or `allowed: true` — now fail, +and they were asserting the defect. Enforcement behaviour is unchanged in every +respect; `explain` is a diagnostic surface and no enforcement path reads its +verdict. diff --git a/packages/plugins/plugin-security/src/explain-engine.test.ts b/packages/plugins/plugin-security/src/explain-engine.test.ts index 98c2f576d0..d87944bc1f 100644 --- a/packages/plugins/plugin-security/src/explain-engine.test.ts +++ b/packages/plugins/plugin-security/src/explain-engine.test.ts @@ -6,6 +6,7 @@ import { resolveUserAuthzGrants, resetPlatformAdminEmailMemo } from '@objectstac import { PermissionSetSchema } from '@objectstack/spec/security'; import { PermissionEvaluator } from './permission-evaluator'; import { explainAccess, buildContextForUser, type ExplainEngineDeps } from './explain-engine'; +import { RLS_DENY_FILTER } from './rls-compiler'; import { assertEngineFindOnePredicate, type EngineFindOneQueryInput } from '@objectstack/metadata-core'; // [#13176] `ExplainDecision.layers` is `ExplainLayer[]` — the z.INPUT shape @@ -28,6 +29,21 @@ const ADMIN = PermissionSetSchema.parse({ const PRIVATE_SCHEMA = { name: 'leave_request', sharingModel: 'private' }; +// The ONE engine double this file declares: a `ql` whose `findOne` resolves the +// `onBehalfOf` delegator, so D10 resolves instead of failing closed as +// 'missing' (which is a different report entirely). Shared by every test that +// needs a resolvable delegator — a second inline copy would be a second double +// for `check:engine-double-contract` to ratchet, and reusing the one the file +// already pins is cheaper than growing that ledger. +const DELEGATOR_QL = { + getSchema: () => PRIVATE_SCHEMA, + findOne: async (object: string, query?: EngineFindOneQueryInput) => { + assertEngineFindOnePredicate(object, query); + return { id: 'u_boss' }; + }, + find: async () => [], +}; + function makeDeps(overrides: Partial & { sets?: any[]; schema?: any; rls?: any } = {}): ExplainEngineDeps { const evaluator = new PermissionEvaluator(); return { @@ -346,13 +362,7 @@ describe('explainAccess — fls reports partial masking (#9127)', () => { it('keeps the D10 delegator suffix when only partial masks apply', async () => { const d = await explainAccess( makeDeps({ - // A resolvable delegator — otherwise D10 fails closed as 'missing' and - // `delegatorSets` stays null, which is a different report entirely. - ql: { - getSchema: () => PRIVATE_SCHEMA, - findOne: async (object: string, query?: EngineFindOneQueryInput) => { assertEngineFindOnePredicate(object, query); return ({ id: 'u_boss' }); }, - find: async () => [], - }, + ql: DELEGATOR_QL, // resolvable delegator — otherwise D10 fails closed getPartialMaskRules: async () => ({ phone: 'phone' }), }), { @@ -1259,3 +1269,191 @@ describe('buildContextForUser bypasses the #11971 grants cache (ruled bypass lis } }); }); + + +// ═══════════════════════════════════════════════════════════════════════════ +// [#13639] A fail-closed RLS denial is reported as a DENIAL. +// +// Before this suite's repair, `explain` recognised only its own `__deny_all__` +// sentinel, so the "no active organization" path — which composes +// plugin-security's `RLS_DENY_FILTER` (`__rls_deny__:…`) and is guaranteed to +// return zero rows — was reported with verdict `narrows` and +// `decision.allowed: TRUE`. Not an imprecise label: an affirmatively wrong +// answer, handed to the operator asking why a user sees nothing. +// +// ⛔ The repair is RECOGNITION ONLY. The sentinels are not merged and the +// published `readFilter` payload is NOT rewritten — options B and C on #13639 +// are the maintainer's, and the boundary between A and B is PINNED below, not +// left to a comment. +// ═══════════════════════════════════════════════════════════════════════════ +describe('explainAccess — fail-closed RLS denial (#13639)', () => { + // [ADR-0123 D2] Producers SPREAD the frozen constant rather than returning + // it, which is why identity is decided on the sentinel's VALUE. Every + // fixture here spreads it too, so a recognition that compared REFERENCES + // would pass nothing. + const rlsDeny = (): Record => ({ ...RLS_DENY_FILTER }); + const readOf = async (deps: ExplainEngineDeps) => + explainAccess(deps, { object: 'leave_request', operation: 'read', context: CTX }); + + // `DELEGATOR_QL` resolves the delegator, so BOTH computeRlsFilter calls happen + // and `readFilter` is a real two-part composite — the only shape in which the + // payload collapse at §9 is observable at all. + const composedOf = async (delegatorFilter: Record) => + explainAccess( + makeDeps({ + ql: DELEGATOR_QL, + computeRlsFilter: async (_s: any, _o: string, _op: string, ctx: any) => + ctx?.userId === 'u_boss' ? delegatorFilter : { owner_id: 'u1' }, + }), + { object: 'leave_request', operation: 'read', context: { ...CTX, onBehalfOf: { userId: 'u_boss' } } }, + ); + + // ── the object-level `rls` verdict ────────────────────────────────────── + + it('reports the rls layer as `denies`, not `narrows`', async () => { + const rls = (await readOf(makeDeps({ rls: rlsDeny() }))).layers.find((l) => l.layer === 'rls')!; + expect(rls.verdict).toBe('denies'); + expect(rls.detail).toContain('DENY ALL'); + expect(rls.detail).not.toContain('narrows the row set'); + }); + + // ── ⭐ the assertion that matters most ────────────────────────────────── + + it('⭐ answers decision.allowed = FALSE for a request that cannot return a row', async () => { + expect((await readOf(makeDeps({ rls: rlsDeny() }))).allowed).toBe(false); + }); + + it('decides identity on the sentinel VALUE, not on reference to the frozen constant', async () => { + const spread = rlsDeny(); + expect(spread).not.toBe(RLS_DENY_FILTER); // control: the fixture really is a copy + expect(spread).toEqual({ id: RLS_DENY_FILTER.id }); + expect((await readOf(makeDeps({ rls: spread }))).allowed).toBe(false); + }); + + // ── ⭐ THE A/B BOUNDARY — the payload is reported AS COMPOSED ─────────── + + it('⭐ does NOT rewrite readFilter: the composed __rls_deny__ predicate is published unchanged', async () => { + const d = await readOf(makeDeps({ rls: rlsDeny() })); + expect(d.readFilter).toEqual({ id: RLS_DENY_FILTER.id }); + // ⛔ Option B — rewriting the payload for every deployment whose RLS fails + // closed — is explicitly NOT what happened here. + expect(d.readFilter).not.toEqual({ id: '__deny_all__' }); + }); + + it('⭐ leaves a two-part composite INTACT when the DELEGATOR half denies — no collapse', async () => { + const d = await composedOf(rlsDeny()); + expect(d.allowed).toBe(false); + expect(d.layers.find((l) => l.layer === 'rls')!.verdict).toBe('denies'); + expect(d.readFilter).toEqual({ $and: [{ owner_id: 'u1' }, { id: RLS_DENY_FILTER.id }] }); + }); + + // ── `__deny_all__` is unchanged in EVERY respect, collapse included ───── + + it('leaves the __deny_all__ verdict and decision exactly as they were', async () => { + const d = await readOf(makeDeps({ rls: { id: '__deny_all__' } })); + expect(d.allowed).toBe(false); + const rls = d.layers.find((l) => l.layer === 'rls')!; + expect(rls.verdict).toBe('denies'); + expect(rls.detail).toBe('Row-level security composes to DENY ALL for this principal.'); + expect(d.readFilter).toEqual({ id: '__deny_all__' }); + }); + + it('⭐ still COLLAPSES a composite to { id: "__deny_all__" } for THAT sentinel', async () => { + const d = await composedOf({ id: '__deny_all__' }); + expect(d.allowed).toBe(false); + // The documented payload rewrite survives untouched for the sentinel the + // published contract names. + expect(d.readFilter).toEqual({ id: '__deny_all__' }); + }); + + // ── the negative ─────────────────────────────────────────────────────── + + it('a policy that genuinely NARROWS still narrows, and stays allowed', async () => { + const d = await readOf(makeDeps({ rls: { owner_id: 'u1' } })); + expect(d.allowed).toBe(true); + const rls = d.layers.find((l) => l.layer === 'rls')!; + expect(rls.verdict).toBe('narrows'); + expect(d.readFilter).toEqual({ owner_id: 'u1' }); + }); + + it('an id predicate that is NOT a sentinel is a narrowing, not a denial', async () => { + const d = await readOf(makeDeps({ rls: { id: 'r1' } })); + expect(d.allowed).toBe(true); + expect(d.layers.find((l) => l.layer === 'rls')!.verdict).toBe('narrows'); + }); + + it('no RLS policy at all is still not_applicable and allowed', async () => { + const d = await readOf(makeDeps({ rls: null })); + expect(d.allowed).toBe(true); + expect(d.layers.find((l) => l.layer === 'rls')!.verdict).toBe('not_applicable'); + }); + + // ── record-grained layers: the CORRECTNESS must not move, the prose does ─ + // + // Slice 1 measured that `outcome` and `matchesRecord` are ALREADY right at + // both `isDenyAll` call sites today, because `matchesFilterCondition` excludes + // every real record against either sentinel. So the two groups below are + // asserted for opposite reasons: the correctness assertions are CONTROLS + // (green with and without the repair), the `detail` assertions are the ones + // the repair moves. + describe('record-grained attribution (the isDenyAll call sites)', () => { + const REC_CTX = { userId: 'u1', tenantId: 'org1', positions: ['sales_rep', 'everyone'], permissions: [] }; + const recOf = async (layered: { layer0: any; layer1: any }) => + explainAccess( + { + ...makeDeps({ rls: null }), + computeLayeredRlsFilter: async () => layered, + fetchRecord: async () => ({ id: 'r1', organization_id: 'org1', owner_id: 'u1' }), + sharingReadFilter: async () => null, + listRecordShares: async () => [], + canEditRecord: async () => false, + } as ExplainEngineDeps, + { object: 'leave_request', operation: 'read', context: REC_CTX, recordId: 'r1' }, + ); + + it('layer 0: outcome/matchesRecord/effect do NOT move — only the prose does', async () => { + const l0 = (await recOf({ layer0: rlsDeny(), layer1: null })).layers.find((l) => l.layer === 'tenant_isolation')!; + // CONTROL — already correct before the repair, must stay correct after. + expect(l0.verdict).toBe('denies'); + expect(l0.record!.outcome).toBe('excluded'); + expect(l0.record!.matchesRecord).toBe(false); + expect(l0.record!.rules?.[0]?.effect).toBe('excludes'); + expect(l0.record!.rowFilter).toEqual({ id: RLS_DENY_FILTER.id }); // payload untouched here too + // THE PROSE — this is what the repair moves. + expect(l0.record!.detail).toBe( + 'No active organization on the context — the tenant wall denies all rows (fail closed).', + ); + expect(l0.record!.detail).not.toContain('does not match'); + }); + + it('layer 1: outcome/matchesRecord/effect do NOT move — only the prose does', async () => { + const l1 = (await recOf({ layer0: { organization_id: 'org1' }, layer1: rlsDeny() })).layers.find((l) => l.layer === 'rls')!; + // CONTROL — already correct before the repair. + expect(l1.record!.outcome).toBe('excluded'); + expect(l1.record!.matchesRecord).toBe(false); + expect(l1.record!.rules?.[0]?.effect).toBe('excludes'); + expect(l1.record!.rowFilter).toEqual({ id: RLS_DENY_FILTER.id }); + // THE PROSE. + expect(l1.record!.detail).toBe('Business RLS composes to DENY ALL for this principal.'); + expect(l1.record!.detail).not.toContain('does not satisfy'); + }); + + it('a record the layers genuinely admit is still admitted (negative)', async () => { + const d = await recOf({ layer0: { organization_id: 'org1' }, layer1: { owner_id: 'u1' } }); + const l0 = d.layers.find((l) => l.layer === 'tenant_isolation')!; + expect(l0.record!.outcome).toBe('admitted'); + expect(l0.record!.matchesRecord).toBe(true); + expect(d.layers.find((l) => l.layer === 'rls')!.record!.outcome).toBe('admitted'); + }); + + it('the __deny_all__ prose at both call sites is unchanged (control)', async () => { + const d = await recOf({ layer0: { id: '__deny_all__' }, layer1: { id: '__deny_all__' } }); + expect(d.layers.find((l) => l.layer === 'tenant_isolation')!.record!.detail).toBe( + 'No active organization on the context — the tenant wall denies all rows (fail closed).', + ); + expect(d.layers.find((l) => l.layer === 'rls')!.record!.detail).toBe( + 'Business RLS composes to DENY ALL for this principal.', + ); + }); + }); +}); diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index 6442421ead..e9ea653f2b 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -40,6 +40,7 @@ import type { } from '@objectstack/spec/security'; import type { PermissionEvaluator } from './permission-evaluator.js'; import { superuserBypassBitForOperation } from './permission-evaluator.js'; +import { RLS_DENY_FILTER } from './rls-compiler.js'; import { unresolvedPostureExplainDetail, type UnresolvedPostureCause, @@ -118,9 +119,61 @@ function derivePosture(context: any): AuthzPosture { }); } -/** True iff a composed filter is the zero-rows deny sentinel. */ +/** + * The `explain`/sharing zero-rows deny sentinel — the one named in the PUBLISHED + * `readFilter` contract (`ExplainDecisionSchema.readFilter`, + * `ExplainRecordAttributionSchema.rowFilter`), produced by plugin-sharing's + * `buildReadFilter`/`buildWriteFilter` and by this engine's own + * `computeRlsFilter` catch fallback in §9 below. + */ +const DENY_ALL_SENTINEL_ID = '__deny_all__'; + +/** + * True iff `filter` is THAT sentinel specifically — the narrow question, kept + * apart from {@link isDenyAll} because §9's payload collapse is keyed to this + * one value and must NOT follow the widened recognition (see below). + */ +function isDenyAllSentinel(filter: unknown): boolean { + return !!filter && typeof filter === 'object' && (filter as { id?: unknown }).id === DENY_ALL_SENTINEL_ID; +} + +/** + * True iff `filter` is plugin-security's fail-closed RLS sentinel + * (`RLS_DENY_FILTER`) — an `id` equality against a UUID-shaped string no record + * can carry, so the SQL layer returns zero rows without raising. + * + * Identity is decided on the sentinel's own VALUE, not on its shape — the same + * rule `isTenantWallDenial` states (ADR-0123 D2), and for the same reason: + * producers SPREAD the frozen constant (`{ ...RLS_DENY_FILTER }`), so a + * reference check would answer `false` for every real denial. + */ +function isRlsDenySentinel(filter: unknown): boolean { + return !!filter && typeof filter === 'object' && (filter as { id?: unknown }).id === RLS_DENY_FILTER.id; +} + +/** + * [#13639] True iff a composed filter denies EVERY row — whichever of the two + * sentinels the platform composed. ⭐ This is THE question the engine asks, and + * every deny-recognition site routes through it. + * + * It used to recognise `__deny_all__` alone, and the object-level `rls` verdict + * in §9 compared the same literal a second time, INLINE. So a fail-closed RLS + * denial — the "no active organization" path, which composes `RLS_DENY_FILTER` + * and is guaranteed to return zero rows — was reported as verdict `narrows` + * with `decision.allowed: TRUE`. That is not an imprecise label; it is an + * affirmatively wrong answer about a request that cannot return a row, handed + * to the operator debugging exactly "why does this user see nothing?". + * + * ⛔ Recognition is deliberately all this widening does. The two sentinels are + * NOT merged (`__deny_all__` is in the published spec schema and docs; + * `__rls_deny__` is pinned as a bound SQL parameter by service-analytics and + * dispatched on by value by `isTenantWallDenial`), and §9's payload + * replacement is NOT extended to `__rls_deny__` — a deployment that receives + * `{ id: '__rls_deny__:…' }` in `readFilter` today keeps receiving it. Both of + * those are deployment-facing changes recorded on #13639 as the maintainer's. + */ function isDenyAll(filter: unknown): boolean { - return !!filter && typeof filter === 'object' && (filter as any).id === '__deny_all__'; + return isDenyAllSentinel(filter) || isRlsDenySentinel(filter); } /** Explain-operation → engine-operation (the middleware's vocabulary). */ @@ -1294,7 +1347,7 @@ export async function explainAccess(deps: ExplainEngineDeps, input: ExplainInput try { agentFilter = await deps.computeRlsFilter(sets, object, dataOp, context); } catch { - agentFilter = { id: '__deny_all__' }; + agentFilter = { id: DENY_ALL_SENTINEL_ID }; } // [ADR-0090 D10] AND the delegator's read filter into the composite — the // delegated principal sees only rows BOTH principals may see. @@ -1303,14 +1356,19 @@ export async function explainAccess(deps: ExplainEngineDeps, input: ExplainInput try { delegatorFilter = await deps.computeRlsFilter(delegatorSets, object, dataOp, delegatorContextForRls); } catch { - delegatorFilter = { id: '__deny_all__' }; + delegatorFilter = { id: DENY_ALL_SENTINEL_ID }; } } const filterParts = [agentFilter, delegatorFilter].filter(Boolean) as Record[]; let readFilter: Record | null | undefined = filterParts.length === 0 ? undefined : filterParts.length === 1 ? filterParts[0] : { $and: filterParts }; - const denyAll = filterParts.some((f) => (f as any).id === '__deny_all__'); - if (denyAll) readFilter = { id: '__deny_all__' }; + const denyAll = filterParts.some(isDenyAll); + // [#13639] The verdict above is value-agnostic; this payload collapse is NOT. + // It rewrites the published `readFilter` into the documented `__deny_all__` + // shape, so it stays keyed to the sentinel that shape already names. An + // `__rls_deny__` denial is now reported as `denies` while `readFilter` keeps + // reporting the predicate that was ACTUALLY composed. + if (filterParts.some(isDenyAllSentinel)) readFilter = { id: DENY_ALL_SENTINEL_ID }; layers.push({ layer: 'rls', verdict: denyAll ? 'denies' : readFilter ? 'narrows' : 'not_applicable',