Skip to content

Commit 7001918

Browse files
fix(plugin-security): security/explain answers enforcement's refusal for a row-level policy comparing two fields of no shared comparison class (#20598)
Fixes #20431 Clause-②: no ## What was wrong A row-level policy can compare two fields that share no comparison class, for example a text field against a number field. Enforcement refuses every request such a policy scopes. The find answers `INVALID_FILTER` / 400. A by-id update or delete fails closed at its row-level gate (403), because that gate's pre-image read is the same refused read. The record-grained explanation judged the same predicate in-process without the object's declared columns. It compared the two raw values and reported a record verdict: `visible: true` for one ordering of a pair, and `visible: false` (rls `excluded`) for the other. Both answers covered a request that enforcement refuses. ## What changed The landing point is `packages/plugins/plugin-security/src/explain-engine.ts`, as the dispatch expected; the other files are the pin file and the changeset. There are no changes to `security-plugin.ts`, `packages/formula`, `packages/spec`, the REST layer, or enforcement. - The record matcher (`matchesFilterCondition`) now receives the object's declared columns (`options.fields`), as the RLS write check does. They are read from `ql.getSchema(object)`: the schema the engine already reads for the OWD, and the ObjectQL registry that the find's driver compiles against. A schema that cannot be read hands over no columns, and the matcher judges values only, as before. - With the columns, the matcher refuses the comparison. Explain answers with that refusal: the explanation fails with `INVALID_FILTER` / 400 (the matcher's code and status, the envelope the find answers with), and no record verdict is reported. The message names the policy and both fields with their declared types. The matcher's own error rides as `cause`. - Naming the fields discloses nothing new. The report explain gives the same caller for the same object already publishes that predicate (`readFilter`, or the `rls` layer's `rowFilter`). ## Why a refusal and not a fail-closed report: dispatch assumption A3 did not hold A3 said to reuse PR #20030's shape (layer `not_evaluated`, `record.visible: false`) for "enforcement refuses this read". Measurement on `main` says otherwise: - Explain already answers the matcher's other `INVALID_FILTER` refusals as a refusal. - `rls-stored-list-ordering-fails-closed.test.ts` (landed in `de091b50`, PR #20310) pins it: "explain read 400 = find 400; explain update 400, the by-id update 403". One of its cells is a field-to-field comparison against a list-holding field. - PR #20030's shape covers a dependency call that fails, not a predicate the matcher refuses. My first commit used the report shape. The full `plugin-security` suite then turned 2 cells of that landed pin red, because its field-to-field cell is now caught first by the comparison-class rule. Keeping the report shape would have added the second refusal dialect the dispatch forbids. So this PR follows the ruling's intent: "the read is refused … both orderings answer the same refusal as find". ## Measurement: before and after (better-sqlite3, the same stack as the pins) | policy class | find | by-id update / delete | explain read / update / delete, before | after | |---|---|---|---|---| | text vs number | 400 `INVALID_FILTER` | 403 `PERMISSION_DENIED` | `visible: true`, `decidedBy: 'rls'`, rls `admitted` | refused, 400 `INVALID_FILTER` | | number vs text (the other ordering) | 400 `INVALID_FILTER` | 403 `PERMISSION_DENIED` | `visible: false`, `decidedBy: 'rls'`, rls `excluded` | refused, 400 `INVALID_FILTER` | | text vs text (control) | the row | admitted | `visible: true`, `decidedBy: 'rls'` | unchanged | ## Tests New file: `packages/plugins/plugin-security/src/explain-cross-class-refusal.test.ts`. It uses the real `SecurityPlugin`, `ObjectQL` and SQL drivers (better-sqlite3 and sqlite-wasm; PostgreSQL when `OS_TEST_POSTGRES_URL` is set), on PR #20427's harness. Every refused cell asserts both halves with their envelope `code` and `status`: explain's answer, and the caller's real request. - Five cells: text vs number, text vs image, text vs formula, text vs json, and number vs text. Each checks read, update and delete. Explain answers `{ code: 'INVALID_FILTER', status: 400 }` and its message names the policy and both fields. Find answers `INVALID_FILTER` / 400, update and delete answer `PERMISSION_DENIED` / 403, and nothing is stored. - Both orderings of one pair get `{ find: INVALID, explain: INVALID }`. - Control, same class: find returns only the matching row. Explain reports `visible: true` / `admitted` for it and `visible: false` / `excluded` for the other row. The update is admitted and matches explain. Pre-fix run: `main`'s `explain-engine.ts` restored from the base blob `92716c91`, under a trap whose restore is proven by the HEAD blob and an empty `git diff HEAD`. Result: `Tests 12 failed | 2 passed | 7 skipped (21)`. The 2 passes are the controls. **Ablation:** only the declared-columns argument was removed, through `scripts/ablation-replace.mjs`. The anchor hit 1 → 0 and the blob went `a46456db` → `5a314958`. Result: `Tests 12 failed | 2 passed | 7 skipped (21)`. Every refused cell on both drivers failed: ```text AssertionError: expected 'answered' not to be 'answered' // Object.is equality AssertionError: expected { find: { …(2) }, explain: 'admitted' } to deeply equal { find: { …(2) }, explain: { …(2) } } ``` Restore: `ok restored: blob == HEAD (a46456d) and git diff HEAD is empty`. All figures below were measured at `5e48f52c`, the head after merging `origin/main` `c876a742`: - `pnpm --filter @objectstack/plugin-security exec vitest run --maxWorkers=2`: `Test Files 144 passed (144)`, `Tests 3066 passed | 23 skipped (3089)`. - `pnpm --filter @objectstack/plugin-security typecheck`: exit 0, with the test layer OK. `tsc -p tsconfig.test.json --listFiles` counts the new file once. - Gates: `node scripts/pm/dispatch-gates.mjs --commands` derived 64 commands, and all 64 ran with exit 0. Three first answered exit 3 `PREREQUISITE NOT MET` (`check:dual-build-cjs-loads`, `check:i18n`, `check:type-check-debt`). I rebuilt with `turbo run build --filter='./packages/*' --filter='./packages/*/*'` (71/71 tasks) and re-ran them; all three answered exit 0. `dispatch-gates --ran`: `64 derived, 64 run, 0 NOT-MEASURED, 0 UNRUN`. - Lint, narrowed: `eslint --no-inline-config --format json` over the two touched `.ts` files gives 2 files, 0 errors, 0 warnings. `eslint --print-config` shows no `parserOptions.project` / `projectService`. Linting is not type-aware, so this diff cannot move any untouched file's verdict. ## Acceptance notes - **The REST door answers 500 for this refusal.** The explain route's catch maps only `PERMISSION_DENIED` → 403 and `OBJECT_NOT_FOUND` → 404; every other throw becomes `500 EXPLAIN_FAILED`. I measured it through the real handler (`security-explain-envelope.test.ts` harness): a service refusal carrying `INVALID_FILTER` / 400 comes back as `{ status: 500, error: { code: 'EXPLAIN_FAILED', message: … } }`. The refusal's message survives. PR #20310's refusals were already answered this way. It lives in `packages/rest/src/rest-server.ts`, outside this card's surface, so it is reported, not fixed here. - **The object-level answer is unchanged.** An explanation without a `recordId` runs no record matcher. For a read under such a policy, it still reports `allowed: true` and rls `narrows`, where the find answers 400. This PR does not change that; it is reported separately. - **Missing record, not measured.** When the record does not exist, the matcher never runs, so explain keeps its missing-record answer (`visible: false`, no `decidedBy`) for a policy the find would refuse. - **Duplicated attribution.** The policy-name attribution (`refusedPolicyNamesOf`) copies the RLS write check's attribution in `security-plugin.ts`. That file is held by #20555, so one shared helper is left to whoever next touches both files. --- _Generated by [Claude Code](https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3a89d45 commit 7001918

3 files changed

Lines changed: 396 additions & 6 deletions

File tree

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
---
4+
5+
fix(plugin-security): `security/explain` answers with enforcement's refusal for a row-level policy that compares two fields of no shared comparison class, instead of a record verdict (#20431)
6+
7+
Clause-②: no
8+
9+
A row-level policy can compare two fields that share no comparison class: text against a number, or any field against a file field, a formula field, or a field that holds a list or an object. The platform defines no answer for such a comparison. The SQL driver refuses to compile it, so every find the policy scopes answers `INVALID_FILTER` / 400. A by-id update or delete fails closed at its row-level gate, because that gate's pre-image read is the same refused read.
10+
11+
The explain engine's record attribution judged the same predicate in-process, without the object's declared columns. So it compared the two raw values, and it reported `record.visible` as `true` or `false` depending on how those values happened to compare. For one ordering of a pair, it reported the record visible where enforcement refuses the read.
12+
13+
The record matcher now receives the object's declared columns, as the RLS write check already does, and it refuses the comparison the way the driver does. A record-grained explanation (`recordId`) under such a policy is now refused with the matcher's envelope: `INVALID_FILTER` / 400, the same envelope the find answers with. No record verdict is reported. The message names the policy and both fields with their declared types. This is the answer explain already gives to the matcher's other `INVALID_FILTER` refusals, including a field-to-field comparison against a field that holds a list. Both orderings of one pair now get this one answer.
14+
15+
Unchanged:
16+
17+
- Enforcement admits and refuses exactly what it did before.
18+
- A comparison between two fields of one class keeps its record verdict.
19+
- A schema that cannot be read hands over no columns, so the matcher judges values only, as before.
20+
- An object-level explanation (no `recordId`), which runs no record matcher.
Lines changed: 263 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,263 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#20431] `security.explain` answers what enforcement answers for a row-level
5+
* policy that compares two fields of no shared comparison class: the request
6+
* is REFUSED, with the find's own envelope (`INVALID_FILTER` / 400), and no
7+
* record verdict is reported.
8+
*
9+
* `record.status != record.amount` lowers to `{ status: { $ne: { $field:
10+
* 'amount' } } }`. driver-sql refuses to compile a column-to-column comparison
11+
* across comparison classes, so the caller's find answers `INVALID_FILTER` /
12+
* 400, and a by-id update or delete fails closed at its row-level gate (403),
13+
* whose pre-image re-read is that same refused read (#20355's table). The
14+
* explain engine's record matcher was handed no declared columns, so it
15+
* compared the two raw values. Measured on this stack before the fix:
16+
*
17+
* | policy | find | by-id update / delete | explain read / update / delete, `record` |
18+
* |---|---|---|---|
19+
* | `record.status != record.amount` | 400 | 403 | `visible: true`, `decidedBy: 'rls'`, rls `admitted` |
20+
* | `record.amount > record.status` | 400 | 403 | `visible: false`, `decidedBy: 'rls'`, rls `excluded` |
21+
* | `record.status != record.title` (control) | the row | admitted | `visible: true`, `decidedBy: 'rls'` |
22+
*
23+
* The matcher is now handed the object's declared columns (the write check's
24+
* `options.fields`, #20355), refuses the comparison, and explain answers with
25+
* that refusal: the envelope the find answers with, the message naming the
26+
* policy and both columns. That is the answer explain already gives the
27+
* matcher's other `INVALID_FILTER` refusals, a `{ $field }` comparison against
28+
* a list-holding column among them (`rls-stored-list-ordering-fails-closed.test.ts`).
29+
*
30+
* Every refused cell asserts both halves: explain's answer, and the real
31+
* request through the real `SecurityPlugin`, `ObjectQL` and SQL driver. A
32+
* future fork between the two turns a cell here red. The control, a same-class
33+
* comparison, keeps its row verdict on both sides.
34+
*
35+
* PostgreSQL runs when `OS_TEST_POSTGRES_URL` names a server (CI does not run
36+
* this package against a live server, so there it is skipped).
37+
*/
38+
39+
import { describe, it, expect, afterEach, vi } from 'vitest';
40+
import { ObjectQL } from '@objectstack/objectql';
41+
import { SqlDriver } from '@objectstack/driver-sql';
42+
import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm';
43+
import { PermissionSetSchema } from '@objectstack/spec/security';
44+
import type { ExplainDecision } from '@objectstack/spec/security';
45+
import { SecurityPlugin } from './security-plugin.js';
46+
import { defaultPermissionSets } from './objects/default-permission-sets.js';
47+
48+
const SYS_CTX = { isSystem: true, userId: 'usr_system' };
49+
const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!;
50+
const PG_URL = process.env.OS_TEST_POSTGRES_URL;
51+
const POLICY = 'deal_guard';
52+
53+
type Driver = { disconnect?: () => Promise<void> };
54+
const DRIVERS: Array<[name: string, make: () => Driver, available: boolean]> = [
55+
['driver-sql (better-sqlite3)', () =>
56+
new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }), true],
57+
['driver-sqlite-wasm', () => new SqliteWasmDriver({ filename: ':memory:' }), true],
58+
['driver-sql (PostgreSQL)', () => new SqlDriver({ client: 'pg', connection: PG_URL! }), !!PG_URL],
59+
];
60+
61+
const booted: Array<{ engine: ObjectQL; driver: Driver; table: string }> = [];
62+
afterEach(async () => {
63+
while (booted.length) {
64+
const { engine, driver, table } = booted.pop()!;
65+
// A live server outlives the run: drop what this file created.
66+
const knex = (driver as { knex?: { schema: { dropTableIfExists: (t: string) => Promise<unknown> } } }).knex;
67+
if (PG_URL && knex && driver instanceof SqlDriver) {
68+
try { await knex.schema.dropTableIfExists(table); } catch { /* noop */ }
69+
}
70+
try { await engine.destroy(); } catch { /* noop */ }
71+
}
72+
});
73+
74+
type ExplainOp = 'read' | 'update' | 'delete';
75+
76+
let seq = 0;
77+
async function boot(makeDriver: () => Driver, predicate: string) {
78+
const OBJ = `qa_deal_20431_${process.pid}_${++seq}`;
79+
const driver = makeDriver();
80+
const engine = new ObjectQL();
81+
engine.registerDriver(driver as never, true);
82+
await engine.init();
83+
engine.registerApp({
84+
id: `com.objectstack.qa.explain-cross-class-20431-${seq}`,
85+
name: 'Explain: RLS cross-class field comparison',
86+
version: '1.0.0',
87+
type: 'plugin',
88+
scope: 'system',
89+
objects: [
90+
{
91+
name: OBJ,
92+
label: 'Deal',
93+
sharingModel: 'public_read_write',
94+
fields: {
95+
id: { name: 'id', type: 'text', primaryKey: true },
96+
status: { name: 'status', type: 'text' },
97+
title: { name: 'title', type: 'text' },
98+
amount: { name: 'amount', type: 'number' },
99+
photo: { name: 'photo', type: 'image' },
100+
meta: { name: 'meta', type: 'json' },
101+
is_open: {
102+
name: 'is_open',
103+
type: 'formula',
104+
expression: { dialect: 'cel', source: "record.status != 'closed'" },
105+
returnType: 'boolean',
106+
},
107+
},
108+
},
109+
],
110+
} as never);
111+
await engine.syncSchemas();
112+
booted.push({ engine, driver, table: OBJ });
113+
114+
const set = PermissionSetSchema.parse({
115+
name: 'qa_deal_guard',
116+
objects: { [OBJ]: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } },
117+
rowLevelSecurity: [{ name: POLICY, object: OBJ, operation: 'all', using: predicate }],
118+
});
119+
const services: Record<string, unknown> = {
120+
manifest: { register: vi.fn() },
121+
objectql: engine,
122+
metadata: {
123+
get: async (_type: string, name: string) => engine.getSchema(name) ?? null,
124+
list: async () => [MEMBER_DEFAULT, set],
125+
},
126+
};
127+
const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() };
128+
const ctx = {
129+
logger,
130+
registerService: vi.fn(),
131+
getService: (name: string) => {
132+
if (!(name in services)) throw new Error(`service not registered: ${name}`);
133+
return services[name];
134+
},
135+
};
136+
const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' });
137+
await plugin.init(ctx as never);
138+
await plugin.start(ctx as never);
139+
vi.spyOn((engine as unknown as { logger: { warn: () => void } }).logger, 'warn').mockImplementation(() => undefined);
140+
141+
const caller = { userId: 'usr_member', positions: ['qa_pos'], permissions: [set.name], posture: 'MEMBER' };
142+
/** The service method `POST /api/v1/security/explain` calls, for the caller's own access. */
143+
const explain = (operation: ExplainOp, recordId: string): Promise<ExplainDecision> =>
144+
plugin.explainAccessForCaller({ object: OBJ, operation, recordId }, caller);
145+
/** The caller's own request for the one record, through the real middleware and driver. */
146+
const request = (operation: ExplainOp, recordId: string): Promise<unknown> =>
147+
operation === 'read'
148+
? engine.find(OBJ, { where: { id: recordId }, context: caller } as never)
149+
: operation === 'update'
150+
? engine.update(OBJ, { title: 'y' }, { where: { id: recordId }, context: caller } as never)
151+
: engine.delete(OBJ, { where: { id: recordId }, context: caller } as never);
152+
const stored = async () =>
153+
((await engine.find(OBJ, { context: SYS_CTX } as never)) as Array<Record<string, unknown>>)
154+
.map((r) => ({ id: r.id, status: r.status, title: r.title, amount: r.amount }))
155+
.sort((a, b) => String(a.id).localeCompare(String(b.id)));
156+
return { OBJ, engine, caller, explain, request, stored };
157+
}
158+
159+
type Envelope = { code: string; status: number };
160+
const DENIED: Envelope = { code: 'PERMISSION_DENIED', status: 403 };
161+
const INVALID: Envelope = { code: 'INVALID_FILTER', status: 400 };
162+
163+
const envelopeOf = (e: unknown): Envelope => {
164+
const x = e as { code?: string; status?: number; statusCode?: number };
165+
return { code: String(x?.code), status: Number(x?.statusCode ?? x?.status) };
166+
};
167+
const outcome = (p: Promise<unknown>): Promise<'admitted' | Envelope> =>
168+
p.then(() => 'admitted' as const, (e: unknown) => envelopeOf(e));
169+
/** A refusal's envelope and message, or `'answered'` for an explanation that came back. */
170+
const refusalOf = (p: Promise<unknown>): Promise<'answered' | (Envelope & { message: string })> =>
171+
p.then(
172+
() => 'answered' as const,
173+
(e: unknown) => ({ ...envelopeOf(e), message: String((e as Error)?.message) }),
174+
);
175+
176+
/** What enforcement answers the caller's own request with, per operation. */
177+
const ENFORCED: Record<ExplainOp, Envelope> = { read: INVALID, update: DENIED, delete: DENIED };
178+
179+
const ROW = { id: 'r1', status: 'open', title: 'x', amount: 5 };
180+
181+
const rlsRecordOf = (d: ExplainDecision) => d.layers.find((l) => l.layer === 'rls')?.record;
182+
183+
/**
184+
* Explain's answer is the find's refusal: the same envelope, no decision and so
185+
* no record verdict, and a message that names the policy and both columns.
186+
*/
187+
async function expectExplainRefuses(p: Promise<unknown>, columns: [string, string]): Promise<void> {
188+
const r = await refusalOf(p);
189+
expect(r).not.toBe('answered');
190+
if (r === 'answered') return;
191+
expect({ code: r.code, status: r.status }).toEqual(INVALID);
192+
expect(r.message).toContain(`'${POLICY}'`);
193+
for (const column of columns) expect(r.message).toContain(`"${column}"`);
194+
}
195+
196+
interface Case {
197+
id: string;
198+
predicate: string;
199+
/** The two columns of the refused comparison. */
200+
columns: [string, string];
201+
}
202+
203+
/** X1 and X5 are the two orderings of one pair; X2–X4 are the no-class columns. */
204+
const REFUSED: Case[] = [
205+
{ id: 'X1 text vs number', predicate: 'record.status != record.amount', columns: ['status', 'amount'] },
206+
{ id: 'X2 text vs image', predicate: 'record.status != record.photo', columns: ['status', 'photo'] },
207+
{ id: 'X3 text vs formula', predicate: 'record.status != record.is_open', columns: ['status', 'is_open'] },
208+
{ id: 'X4 text vs json', predicate: 'record.status != record.meta', columns: ['status', 'meta'] },
209+
{ id: 'X5 number vs text', predicate: 'record.amount > record.status', columns: ['amount', 'status'] },
210+
];
211+
212+
for (const [driverName, makeDriver, available] of DRIVERS) {
213+
describe.skipIf(!available)(`[#20431] ${driverName}: explain reports the refusal enforcement gives a cross-class field comparison`, () => {
214+
for (const c of REFUSED) {
215+
it(`${c.id} \`${c.predicate}\` — read, update and delete: enforcement refuses, explain answers INVALID_FILTER / 400 and no row verdict`, async () => {
216+
const w = await boot(makeDriver, c.predicate);
217+
await w.engine.insert(w.OBJ, [ROW], { context: SYS_CTX } as never);
218+
const before = await w.stored();
219+
220+
for (const op of ['read', 'update', 'delete'] as const) {
221+
expect(await outcome(w.request(op, 'r1')), `${op}: enforcement`).toEqual(ENFORCED[op]);
222+
await expectExplainRefuses(w.explain(op, 'r1'), c.columns);
223+
}
224+
expect(await w.stored()).toEqual(before);
225+
});
226+
}
227+
228+
it('the two orderings of one pair (`status != amount`, `amount > status`) get one answer from explain, as from the find', async () => {
229+
const answers: unknown[] = [];
230+
for (const predicate of ['record.status != record.amount', 'record.amount > record.status']) {
231+
const w = await boot(makeDriver, predicate);
232+
await w.engine.insert(w.OBJ, [ROW], { context: SYS_CTX } as never);
233+
answers.push({
234+
find: await outcome(w.request('read', 'r1')),
235+
explain: await outcome(w.explain('read', 'r1')),
236+
});
237+
}
238+
expect(answers[0]).toEqual({ find: INVALID, explain: INVALID });
239+
expect(answers[1]).toEqual(answers[0]);
240+
});
241+
242+
it('the control `record.status != record.title` (text vs text) keeps its row verdict on both sides', async () => {
243+
const w = await boot(makeDriver, 'record.status != record.title');
244+
await w.engine.insert(w.OBJ, [ROW, { ...ROW, id: 'r2', title: 'open' }], { context: SYS_CTX } as never);
245+
246+
const rows = (await w.engine.find(w.OBJ, { context: w.caller } as never)) as Array<{ id: string }>;
247+
expect(rows.map((x) => x.id)).toEqual(['r1']);
248+
249+
const admitted = await w.explain('read', 'r1');
250+
expect(admitted.record).toEqual({ recordId: 'r1', visible: true, decidedBy: 'rls' });
251+
expect(rlsRecordOf(admitted)).toMatchObject({ outcome: 'admitted', matchesRecord: true });
252+
253+
const excluded = await w.explain('read', 'r2');
254+
expect(excluded.record).toEqual({ recordId: 'r2', visible: false, decidedBy: 'rls' });
255+
expect(rlsRecordOf(excluded)).toMatchObject({ outcome: 'excluded', matchesRecord: false });
256+
257+
expect(await outcome(w.request('update', 'r1'))).toBe('admitted');
258+
expect((await w.explain('update', 'r1')).record).toEqual({ recordId: 'r1', visible: true, decidedBy: 'rls' });
259+
expect(await outcome(w.request('update', 'r2'))).toEqual(DENIED);
260+
expect((await w.explain('update', 'r2')).record).toEqual({ recordId: 'r2', visible: false, decidedBy: 'rls' });
261+
});
262+
});
263+
}

0 commit comments

Comments
 (0)