Skip to content

Commit de8cd58

Browse files
fix(plugin-security)!: a field the caller may not read is judged as a cross-field comparand exactly as it is as a filter key (#20932) (#20954)
Fixes #20932 Clause-②: no (narrowing) ## What this changes The security layer's field predicate guard refuses a query that filters, sorts, groups or aggregates by a field the caller's field-level permissions hide (`403 PERMISSION_DENIED`), because which rows answer would disclose the value the field mask withholds. It collected the fields a condition names from the condition's **keys** only, so a hidden field named only as a **cross-field comparand** was never judged by that rule. `collectConditionFields` now also collects the field every cross-field comparand inside a field constraint names, into the same set, and the existing field rule judges it. One collection, one rule, no second guard. A hidden field as a comparand now answers the same refusal (code, status and body) as the same hidden field written as a key. Dispatched by the `domain:services` seat (seat post #6021), session `session_01XY5uCwTjZj7884yYtyur4H`; claim comment 5919655256, triage direction 5919556782. ## Per position (disclosure-safe: classes, no request shapes) | Comparand position the filter grammar admits | Door | Before (class) | After | |---|---|---|---| | The whole comparand of a scalar comparison (each of `$eq` `$ne` `$gt` `$gte` `$lt` `$lte`) | data read | served: the comparison was answered | `403 PERMISSION_DENIED`, body equal to the key form's | | Under a logical group (`$and`, `$or`, `$not`, and nested groups) | data read | served | same refusal as the key form | | The whole-day offset wrapper: its base reference | data read | served | same refusal as the key form | | The whole-day offset wrapper: its offset column | data read | served | same refusal as the key form | | The query condition of a grouped query (plain, and under `$or`) | aggregate path | served | same refusal as the key form | | A per-aggregation filter | aggregate path | served | same refusal as the key form | | `having` | guard (unit pin) | not collected | collected, same refusal as the key form at the guard; not pinned through a route (see Acceptance notes) | | A member of a list operator, an endpoint of a range operator | none | not a comparand position: the grammar refuses a reference there | unchanged, not pinned | A comparand naming a field the caller may read is served as before (the control, in every position above). ## Mechanism readings - **D1, the collector and its callers.** `collectConditionFields` is called by itself (logical groups) and by `collectQueryFields` (`where`, `having`, each aggregation's `filter`). `collectQueryFields` is called only by `assertReadableQueryFields`, which has one call site: the security plugin's engine middleware, for every verb that carries a predicate (the read, `findOne`, `count` and `aggregate` on the caller's query; bulk `update` / `delete` on the caller's own condition). The package index re-exports all three; no in-repo consumer outside tests. So the aggregate path is not a separate caller: it gets the widened collection through the same call. - **D2, the comparand reader.** `FieldReferenceSchema` from `@objectstack/spec/data`, the grammar's exported declaration of a cross-field reference (including the whole-day offset's own nested reference). No function that yields the referenced field is exported: the filter module's and the driver readers' reference predicates are module-private. So each node of a field constraint is asked of the schema itself (`safeParse`), the same reading `objectql`'s having filter already takes. The walk carries no operator list and no position list: it visits every node of the constraint, so a position the grammar admits is covered without being named. `packages/spec/src`, `packages/objectql/src` and `service-analytics` are untouched. - **D3, positions.** The grammar admits a reference as the whole comparand of the six scalar comparisons, with or without the whole-day offset (whose offset may itself be a reference), under any logical nesting, in each clause that carries a condition. It refuses one as a list member or a range endpoint. Every admitted position is pinned. - **D4, the refusal.** The key form is the reference: the route pins read it first, per hidden field and per path, and require the comparand form's status and whole body to equal it; the unit pins require equal `code`, `status`, message and details. - **D6, `Clause-②`.** Widening measured absent: no export added or removed (the package index and manifest are untouched, the new helper is module-private), and the collected set only grows, so nothing refused before is served now. Narrowing measured present: on the pre-fix source, all 15 route refusal pins answered 200 where the fix answers 403. Line 2 read by `scripts/pm/clause2-line.mjs` (`readClause2Line`): declared, value `no`, arm `narrowing`. The changeset is `minor` with the BREAKING banner and its ADR-0087 marker. ## Pins, red/green and ablation Commits: the pins (`9e602fe84`, red on that commit by design), the fix (`3b34899fc`), the changeset wording (`215bc699b`, HEAD). Pushed together only after the fix was committed. - `packages/plugins/plugin-security/src/predicate-guard.test.ts`: the collection and the verdict, per position, with the key form as the reference and a readable-comparand control. - `packages/rest/src/data-field-comparand-permission.test.ts`: the real `SecurityPlugin` on a real `ObjectQL` over a real `SqlDriver`, through the data query route, on the data read and the aggregate path, each refusal compared with the key form's, each position with its readable control. All readings below were taken on HEAD `215bc699b`. - **Red/green on the committed tree** (`predicate-guard.ts` restored from the pins commit, pins at HEAD, package rebuilt and the built output proven to carry no comparand collection): unit 23 failed / 12 passed (35), route 15 failed / 15 passed (30). Every refusal pin red, every control green. Green leg (restored from HEAD by absolute path, blob equal to the HEAD blob, `git diff HEAD` 0 bytes, rebuilt, fix proven present in the built output): unit 35/35, route 30/30. - **Ablation of the widened collection** (the collector's call replaced by a no-op reference so the package still compiles; the mutation proven on disk by anchor count 1 to 0 and blob change; rebuilt; built output proven without the call): predicted unit 23 failed / 12 passed and route 15 failed / 15 passed; observed exactly that. Restored: blob equal to the HEAD blob, `git diff HEAD` empty, rebuilt, the call present in the built output again. A first attempt that deleted the call outright did not build (the helper became unused), so the suite would have read a stale build; it was discarded and is not counted. ## Tests and gates - `pnpm --filter @objectstack/plugin-security test`: 149 files passed; 3252 passed, 23 skipped. Exit 0. - `pnpm --filter @objectstack/plugin-security typecheck`: exit 0 (source, scripts, and the test layer under `tsconfig.test.json`, 0 debt entries). - `pnpm --filter @objectstack/rest test`: 244 files passed; 4895 passed, 114 skipped. Exit 0. - `pnpm --filter @objectstack/rest typecheck`: exit 0; the new route pin is in the program. - Gates: `node scripts/pm/dispatch-gates.mjs --commands` at `215bc699b` derives 64 commands; those plus the 4 roster families named at dispatch (`check-changeset-fixed`, `check:authz-resolver`, `check:error-code-casing`, `check:filter-alias-parity`) were run one at a time, each exit code captured before any pipe: 68 of 68 exit 0. A full build (72 tasks) preceded the gates that read built output. Reconciliation: `✓ dispatch-gates --ran: 64 derived famil(ies) accounted for — 64 run, 0 NOT-MEASURED`. - Lint, narrowed and proven: the population is the three changed TypeScript files (eslint's own config applies to each; it ignores the changeset); `eslint --no-inline-config --format json` reports 3 files, 0 errors, 0 warnings; this repo's config enables no type-aware linting, so the diff cannot move the verdict on any untouched file. The repo-wide `pnpm lint` is CI's. - The client envelope-caller census needs no row: the pins call no censused client method. ## Acceptance notes - **`having`**: a comparand there is collected and pinned at the guard (unit); it is NOT MEASURED through a route. - **`findOne`, `count`, bulk `update` / `delete`**: covered by construction (the guard's single call site serves every verb that carries a predicate) and by the collector pins; not pinned per verb in the committed suite. - **Analytics**: inherits the fix through the engine path; not touched and not pinned here. Its own gate is #20917's (PR #20931). - **A comparand inside a nested-relation condition** is collected by the same walk and judged against the queried object's own field permissions (the refusing direction). NOT MEASURED. - **Drivers**: the route pin runs on SQLite only; the guard runs before any driver. - **`main` moved**: `origin/main` is at `013f97df9`, 8 commits past this branch's base `1571aedce` (read just before this PR opened). None touches this claim's surface, so the branch is not merged (per the dispatch order); this PR's CI on the merge ref reads the merged tree. `dispatch-gates` flagged three of its derivation inputs as changed on `main` (two ADR-anchor files for other packages and the doc-authoring prose-id baseline); the derived family list is unchanged. - **CI**: not awaited; the CI-only lanes `dispatch-gates` names (the test shards, temporal conformance, dogfood, build core, the workspace type-check lanes, the artifact-roster and wide-population families) are CI's. --- _Generated by [Claude Code](https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c6b3a01 commit de8cd58

4 files changed

Lines changed: 379 additions & 0 deletions

File tree

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
'@objectstack/plugin-security': minor
3+
---
4+
5+
fix(plugin-security)!: a field the caller may not read is refused as a cross-field comparand exactly as it is refused as a filter key (#20932)
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) a refusal of a QUERY at the security layer's field predicate guard: a query that names a field the caller's field-level permissions hide as a cross-field comparand now answers the 403 PERMISSION_DENIED the same field already answers as a filter key. No authorable key, spelling, export or stored metadata shape moves: FieldReferenceSchema, every filter shape and every object definition parse as before, and @objectstack/plugin-security exports nothing new and nothing less (collectConditionFields, collectQueryFields and assertReadableQueryFields keep their signatures). There is nothing for objectstack migrate meta to rewrite, since what changes is which caller may run a query, not what any metadata says. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id covers a permission verdict and this diff adds none (not registered / already-registered); and the change is runtime behaviour, not a declaration (not runtime-interface-only / type-surface-only). -->
10+
11+
**BREAKING for queries that compare a column against a field the caller may not read.**
12+
13+
**What changed.** The security layer refuses a query that filters, sorts, groups or aggregates by a field the caller's field-level permissions hide, with `403 PERMISSION_DENIED`: which rows answer would disclose the value that the field mask withholds from the result. A cross-field comparand (`FieldReferenceSchema`, "compare against another column of the same row") reads the field it names in the same way, and it is now collected into the same set and judged by the same rule. A hidden field named as a comparand, in any position the filter grammar admits for one, in `where`, `having` or a per-aggregation `filter`, answers the same `403 PERMISSION_DENIED`, in the same words, as the same field written as a filter key. This covers `engine.find`, `findOne`, `count`, `aggregate` and the bulk `update` / `delete` predicate, and every route that reaches them. Before, such a comparison was answered.
14+
15+
**What is not affected.** A comparand naming a field the caller may read answers as before. A system context, and a caller with no permission sets, are unaffected. Row-level policies may still compare against fields the caller cannot read, because they are applied after the guard. A comparand the filter grammar refuses is still refused; when it names a hidden field, that refusal may now be the `403` rather than `400 INVALID_FILTER`, as it already was for a hidden filter key.
16+
17+
**If a query stopped answering for some users,** it compares against a field those users may not read. Grant that field's read permission to the users who need it, or compare against a field they can read.

‎packages/plugins/plugin-security/src/predicate-guard.test.ts‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,3 +102,99 @@ describe('assertReadableQueryFields', () => {
102102
expect(() => assertReadableQueryFields({ where: { salary: 1 } }, {}, 'employee')).not.toThrow();
103103
});
104104
});
105+
106+
/**
107+
* A cross-field comparand names a field exactly as a condition key does: the
108+
* comparison reads that field's value, so it is collected by the same walk and
109+
* judged by the same rule. The positions below are every one the filter
110+
* grammar admits for a comparand (`FieldReferenceSchema`, `data/filter.zod.ts`):
111+
* the whole comparand of the six scalar comparisons, the whole-day offset
112+
* wrapper (its base and its offset column), under any logical nesting, in each
113+
* clause that carries a condition.
114+
*/
115+
describe('a cross-field comparand is collected and judged like a condition key', () => {
116+
const SEALED = { sealed_n: { readable: false, editable: false } };
117+
const ref = (field: string, extra: Record<string, unknown> = {}) => ({ $field: field, ...extra });
118+
119+
it.each(['$eq', '$ne', '$gt', '$gte', '$lt', '$lte'])(
120+
'collects the field a %s comparand names',
121+
(op) => {
122+
expect([...collectConditionFields({ seen_a: { [op]: ref('seen_b') } })].sort()).toEqual(['seen_a', 'seen_b']);
123+
},
124+
);
125+
126+
it('collects a comparand under every logical nesting', () => {
127+
const fields = collectConditionFields({
128+
$and: [
129+
{ $or: [{ seen_a: 1 }, { seen_b: { $gt: ref('seen_c') } }] },
130+
{ $not: { seen_d: { $lte: ref('seen_e') } } },
131+
],
132+
});
133+
expect([...fields].sort()).toEqual(['seen_a', 'seen_b', 'seen_c', 'seen_d', 'seen_e']);
134+
});
135+
136+
it('collects both fields a whole-day offset comparand names: its base and its offset column', () => {
137+
expect([...collectConditionFields({ seen_day: { $lte: ref('base_day', { addDays: 3 }) } })].sort())
138+
.toEqual(['base_day', 'seen_day']);
139+
expect([...collectConditionFields({ seen_day: { $lte: ref('base_day', { addDays: ref('offset_n') }) } })].sort())
140+
.toEqual(['base_day', 'offset_n', 'seen_day']);
141+
});
142+
143+
it('gates a dotted comparand on its first segment, as it gates a dotted key', () => {
144+
expect([...collectConditionFields({ seen_a: { $gt: ref('link.seen_b') } })].sort()).toEqual(['link', 'seen_a']);
145+
});
146+
147+
it('collects comparands in where, having and a per-aggregation filter', () => {
148+
const fields = collectQueryFields({
149+
where: { seen_a: { $gt: ref('in_where') } },
150+
having: { total: { $lt: ref('in_having') } },
151+
aggregations: [{ function: 'count', alias: 'n', filter: { seen_b: { $ne: ref('in_agg_filter') } } }],
152+
});
153+
expect(['in_where', 'in_having', 'in_agg_filter'].filter((f) => !fields.has(f))).toEqual([]);
154+
});
155+
156+
/** The refusal a query gets, or `undefined` when it is admitted. */
157+
function refusalOf(ast: Record<string, unknown>): { code?: unknown; statusCode?: unknown; message?: unknown; details?: unknown } | undefined {
158+
try {
159+
assertReadableQueryFields(ast, SEALED, 'probe_object');
160+
} catch (e) {
161+
return e as { code?: unknown; statusCode?: unknown; message?: unknown; details?: unknown };
162+
}
163+
return undefined;
164+
}
165+
166+
/** The hidden field named as a KEY — the reference answer every comparand position is held to. */
167+
const KEY_FORM = refusalOf({ where: { sealed_n: { $gt: 1 } } });
168+
169+
it('the key form is refused 403 PERMISSION_DENIED (the reference)', () => {
170+
expect(isPermissionDeniedError(KEY_FORM)).toBe(true);
171+
expect({ code: KEY_FORM?.code, status: KEY_FORM?.statusCode }).toEqual({ code: 'PERMISSION_DENIED', status: 403 });
172+
});
173+
174+
const POSITIONS: Array<[string, Record<string, unknown>]> = [
175+
...['$eq', '$ne', '$gt', '$gte', '$lt', '$lte'].map(
176+
(op): [string, Record<string, unknown>] => [`the whole comparand of ${op}`, { where: { seen_a: { [op]: ref('sealed_n') } } }],
177+
),
178+
['a comparand under $and', { where: { $and: [{ seen_b: 'x' }, { seen_a: { $gt: ref('sealed_n') } }] } }],
179+
['a comparand under $or', { where: { $or: [{ seen_b: 'x' }, { seen_a: { $gt: ref('sealed_n') } }] } }],
180+
['a comparand under $not', { where: { $not: { seen_a: { $gt: ref('sealed_n') } } } }],
181+
['the base of a whole-day offset comparand', { where: { seen_day: { $lte: ref('sealed_n', { addDays: 1 }) } } }],
182+
['the offset column of a whole-day offset comparand', { where: { seen_day: { $lte: ref('base_day', { addDays: ref('sealed_n') }) } } }],
183+
['a comparand in having', { having: { total: { $gt: ref('sealed_n') } } }],
184+
['a comparand in a per-aggregation filter', { aggregations: [{ function: 'count', alias: 'n', filter: { seen_a: { $gt: ref('sealed_n') } } }] }],
185+
];
186+
187+
it.each(POSITIONS)('a hidden field as %s answers the key form\'s refusal', (_position, ast) => {
188+
const refusal = refusalOf(ast);
189+
expect(isPermissionDeniedError(refusal)).toBe(true);
190+
expect({ code: refusal?.code, status: refusal?.statusCode, message: refusal?.message, details: refusal?.details })
191+
.toEqual({ code: KEY_FORM?.code, status: KEY_FORM?.statusCode, message: KEY_FORM?.message, details: KEY_FORM?.details });
192+
});
193+
194+
it('CONTROL a readable comparand in the same positions is admitted', () => {
195+
for (const [, ast] of POSITIONS) {
196+
const readable = JSON.parse(JSON.stringify(ast).replaceAll('sealed_n', 'seen_c')) as Record<string, unknown>;
197+
expect(refusalOf(readable)).toBeUndefined();
198+
}
199+
});
200+
});

‎packages/plugins/plugin-security/src/predicate-guard.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
* the caller cannot read (e.g. `owner_id`), and must not be rejected.
2424
*/
2525

26+
import { FieldReferenceSchema } from '@objectstack/spec/data';
2627
import { PermissionDeniedError } from './errors.js';
2728

2829
interface FieldPermissionLike {
@@ -36,6 +37,12 @@ const LOGICAL_KEYS = new Set(['$and', '$or', '$not']);
3637
* Collect every field name referenced by a FilterCondition. Dotted paths
3738
* and nested-relation conditions gate on their FIRST segment / top-level
3839
* relation field — local field permissions govern local traversal.
40+
*
41+
* A field is referenced by a condition KEY and equally by a cross-field
42+
* COMPARAND (`FieldReferenceSchema`, `{ $field }` — "compare against another
43+
* column of the same row"): the comparison reads the named field's value, so
44+
* row presence discloses it exactly as a filter on the field itself would.
45+
* Both are collected here, into the one set the one field rule judges.
3946
*/
4047
export function collectConditionFields(condition: unknown, out: Set<string> = new Set()): Set<string> {
4148
if (!condition || typeof condition !== 'object' || Array.isArray(condition)) return out;
@@ -46,10 +53,40 @@ export function collectConditionFields(condition: unknown, out: Set<string> = ne
4653
continue;
4754
}
4855
out.add(key.split('.')[0]);
56+
collectComparandFields(value, out);
4957
}
5058
return out;
5159
}
5260

61+
/**
62+
* Collect the field every cross-field comparand inside one field constraint
63+
* names, gated on its first segment like a key.
64+
*
65+
* What a comparand reference IS is the filter grammar's answer, not this
66+
* guard's: each node is asked of `FieldReferenceSchema` itself, the spec's
67+
* declaration of the reference (its `$field` and the whole-day `addDays`
68+
* offset's own nested reference) — the reading `objectql`'s aggregation
69+
* filter also takes. The walk carries no operator list and no position list:
70+
* it visits every node of the constraint (operator maps, list members, a
71+
* reference's own offset), so a position the grammar admits is covered
72+
* without being named here, and one it later admits is covered the day it
73+
* does.
74+
*
75+
* A node the schema refuses is not collected: every executor refuses the
76+
* same malformed reference (`INVALID_FILTER`) instead of reading a field
77+
* through it, so there is no value for it to disclose.
78+
*/
79+
function collectComparandFields(operand: unknown, out: Set<string>): void {
80+
if (!operand || typeof operand !== 'object') return;
81+
if (Array.isArray(operand)) {
82+
for (const member of operand) collectComparandFields(member, out);
83+
return;
84+
}
85+
const reference = FieldReferenceSchema.safeParse(operand);
86+
if (reference.success) out.add(reference.data.$field.split('.')[0]);
87+
for (const member of Object.values(operand as Record<string, unknown>)) collectComparandFields(member, out);
88+
}
89+
5390
/**
5491
* Collect every field referenced by the query's row-shaping clauses:
5592
* where / orderBy / groupBy / having / aggregations (field + FILTER).

0 commit comments

Comments
 (0)