Skip to content

Commit 713f83f

Browse files
claude[bot]claude
andauthored
fix(formula): fail closed on a null MEMBER of a resolved membership array in the CEL pushdown (#13496) (#13630)
* fix(formula): fail closed on a null member of a resolved membership array in the CEL pushdown lowerMembership checked only Array.isArray and emitted the list verbatim, so a null MEMBER of a resolved membership variable reached a security $in while a null SCALAR variable took the pinned unresolved-variable fail-closed path. It now refuses the member with the same reason, in every polarity. Refusing rather than stripping: `not in` lowers to $not wrapping $in, and $in: [] matches nothing, so stripping inverts into allow-all under negation. Refusing throws before any $not wrapper is built, so no polarity threading is needed in the lowerer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L * test(formula): widen the local ok() helper so the new cases add no TEST_DEBT errors The module-level ok() pins its second argument to the exact shape of VARS, so the partial contexts this suite builds were 4 new TS2345 on a shrink-only ledger. Same assertion, same throw, widened only where this suite needs it; re-measured @objectstack/formula TEST_DEBT back to the frozen 17. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8c82289 commit 713f83f

3 files changed

Lines changed: 179 additions & 1 deletion

File tree

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
---
2+
"@objectstack/formula": patch
3+
---
4+
5+
fix(formula): fail closed on a null MEMBER of a resolved membership array in the CEL pushdown (#13496)
6+
7+
`compileCelToFilter` already fails closed when a `current_user.*` variable
8+
resolves to `undefined`/`null` — the module's docblock calls it "the no active
9+
org fail-closed path" and it is pinned for the SCALAR case. `lowerMembership`
10+
did not apply the same discipline one level in: it checked only
11+
`Array.isArray(value)` and emitted the list verbatim, so a null MEMBER of a
12+
resolved membership array went straight into a security `$in`. The one shape
13+
that IS a permission predicate was the one shape that did not fail closed.
14+
15+
`lowerMembership` now refuses a `null`/`undefined` member of a **variable-resolved**
16+
membership array with the same `unresolved-variable` reason the scalar path
17+
uses, which the RLS path already turns into the deny sentinel.
18+
19+
Maintainer ruling, 2026-08-31 (quoted unchanged): 「membership 数组中的 null
20+
**成员**触发与 null 标量同款处置——`unresolved-variable` / deny sentinel,⛔ 不
21+
strip、不静默清洗。」
22+
23+
**Why refuse rather than strip.** Stripping the unresolved member is safe in
24+
POSITIVE polarity only. `not in` is a supported, pinned member of the pushdown
25+
subset (`!(x in y)` lowers to `$not` wrapping `$in`), and `$in: []` matches
26+
nothing on every backend — so `$not { $in: [] }` matches the WHOLE table.
27+
Stripping therefore inverts into fail-OPEN exactly where the predicate is a
28+
blocklist, and it silently deletes a blocklist entry in the mixed
29+
`['u1', null]` case. Refusing needs no polarity awareness at all: it throws
30+
before any `$not` wrapper is built. Both polarities are pinned.
31+
32+
**No shipped behaviour changes.** No first-party provider puts a null into a
33+
membership array — `resolve-authz-context.ts` filters non-strings out of
34+
`org_user_ids`, and the kernel spec declares `org_user_ids: z.array(z.string())`
35+
— so the refused shape was never a declared-valid input. This makes the
36+
implementation match the declaration rather than narrowing it. A fully resolved
37+
list, an empty list (`$in: []`, a legitimate declared predicate) and the
38+
authoring-time `isPushdownableCel` shape gate are all unchanged and pinned so.
39+
40+
Out of scope, deliberately: an AUTHORED literal null inside a list
41+
(`record.status in ['lost', null]`) is a declared predicate, not an unresolved
42+
variable, and what such a filter should select is a separate open question. It
43+
is untouched, with a pin recording that.

‎packages/formula/src/cel-to-filter.test.ts‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,3 +216,106 @@ describe('compileCelToFilter — input shapes', () => {
216216
expect(r.ok && r.filter).toEqual({ dept: 'sales' });
217217
});
218218
});
219+
220+
/**
221+
* A null/undefined MEMBER of a resolved membership array fails closed, like the
222+
* null SCALAR variable already pinned above (maintainer ruling, 2026-08-31).
223+
*
224+
* BOTH POLARITIES are pinned, and that is the point of the suite rather than a
225+
* completeness flourish. The alternative repair — stripping the unresolved member
226+
* — is safe in POSITIVE polarity (`$in` over the surviving members never grants
227+
* more than those members grant) and INVERTS under the supported `not in` form:
228+
* `!(x in y)` lowers to `$not` wrapping `$in`, and `$in: []` matching nothing makes
229+
* `$not { $in: [] }` match the whole table. A positive-only suite is green for both
230+
* repairs and therefore pins nothing about the one that was ruled on.
231+
*/
232+
describe('compileCelToFilter — a null MEMBER of a membership array fails closed', () => {
233+
const vars = (org_user_ids: unknown[]) => ({ current_user: { id: 'u_me', org_user_ids } });
234+
/**
235+
* `ok` above pins its second argument to the exact shape of the module-level
236+
* `VARS`, so it cannot take these partial contexts. Same assertion, same throw
237+
* on an unexpected refusal, widened only where this suite needs it.
238+
*/
239+
const filterOf = (src: string, v: Record<string, unknown>) => {
240+
const r = compileCelToFilter(src, { variables: v });
241+
if (!r.ok) throw new Error(`expected ok for "${src}" but got ${r.reason}: ${r.detail}`);
242+
return r.filter;
243+
};
244+
const expectUnresolved = (r: ReturnType<typeof compileCelToFilter>, path = 'current_user.org_user_ids') => {
245+
expect(r.ok).toBe(false);
246+
if (r.ok) return;
247+
expect(r.reason).toBe('unresolved-variable');
248+
expect(r.detail).toContain(path);
249+
expect(r.detail).toContain('unresolved member');
250+
};
251+
252+
// ---- POSITIVE polarity: `x in y` -> $in ---------------------------------
253+
it('positive: null among resolved members → unresolved-variable (no $in emitted)', () => {
254+
expectUnresolved(compileCelToFilter('id in current_user.org_user_ids', { variables: vars(['u_me', null]) }));
255+
});
256+
it('positive: a lone null member → unresolved-variable', () => {
257+
expectUnresolved(compileCelToFilter('id in current_user.org_user_ids', { variables: vars([null]) }));
258+
});
259+
it('positive: an undefined member fails closed too (the scalar path refuses both)', () => {
260+
expectUnresolved(compileCelToFilter('id in current_user.org_user_ids', { variables: vars(['u_me', undefined]) }));
261+
});
262+
263+
// ---- NEGATIVE polarity: `!(x in y)` -> $not wrapping $in ----------------
264+
// This is where stripping inverted into allow-all; refusing must reach the SAME
265+
// result here as in positive polarity, with no polarity threading in the lowerer.
266+
it('negated `not in`: null among resolved members → unresolved-variable (no $not{$in} emitted)', () => {
267+
expectUnresolved(compileCelToFilter('!(id in current_user.org_user_ids)', { variables: vars(['u_me', null]) }));
268+
});
269+
it('negated `not in`: a lone null member → unresolved-variable, NOT $not{$in:[]} (whole table)', () => {
270+
expectUnresolved(compileCelToFilter('!(id in current_user.org_user_ids)', { variables: vars([null]) }));
271+
});
272+
it('negated inside a disjunction: the surviving disjunct does not rescue the compile', () => {
273+
expectUnresolved(
274+
compileCelToFilter("!(id in current_user.org_user_ids) || owner == current_user.id", {
275+
variables: vars([null]),
276+
}),
277+
);
278+
});
279+
it('negated inside a conjunction fails closed as well', () => {
280+
expectUnresolved(
281+
compileCelToFilter("!(id in current_user.org_user_ids) && owner == current_user.id", {
282+
variables: vars(['u_me', null]),
283+
}),
284+
);
285+
});
286+
287+
// ---- the two polarities agree, which is the ruling's "same treatment" ----
288+
it('both polarities and the null SCALAR variable yield the identical reason', () => {
289+
const scalar = compileCelToFilter('record.organization_id == current_user.organization_id', {
290+
variables: { current_user: { organization_id: null } },
291+
});
292+
const positive = compileCelToFilter('id in current_user.org_user_ids', { variables: vars([null]) });
293+
const negated = compileCelToFilter('!(id in current_user.org_user_ids)', { variables: vars([null]) });
294+
const reasons = [scalar, positive, negated].map((r) => (r.ok ? 'ok' : r.reason));
295+
expect(reasons).toEqual(['unresolved-variable', 'unresolved-variable', 'unresolved-variable']);
296+
});
297+
298+
// ---- shapes this guard must NOT move --------------------------------------
299+
it('a fully resolved membership array still compiles, in both polarities', () => {
300+
expect(filterOf('id in current_user.org_user_ids', vars(['u_me', 'u_peer']))).toEqual({
301+
id: { $in: ['u_me', 'u_peer'] },
302+
});
303+
expect(filterOf('!(id in current_user.org_user_ids)', vars(['u_me', 'u_peer']))).toEqual({
304+
$not: { id: { $in: ['u_me', 'u_peer'] } },
305+
});
306+
});
307+
it('an EMPTY membership array still compiles to $in:[] in both polarities (a declared predicate, unchanged here)', () => {
308+
expect(filterOf('id in current_user.org_user_ids', vars([]))).toEqual({ id: { $in: [] } });
309+
expect(filterOf('!(id in current_user.org_user_ids)', vars([]))).toEqual({ $not: { id: { $in: [] } } });
310+
});
311+
it('an AUTHORED literal null in a list is not an unresolved variable — out of this guard scope', () => {
312+
// A declared predicate, the same way `record.x == null` lowers to `$null` rather
313+
// than failing closed. What such a filter SELECTS is a separate open question;
314+
// this pin records only that the compiler still lowers it, unchanged.
315+
expect(ok("record.status in ['lost', null]")).toEqual({ status: { $in: ['lost', null] } });
316+
});
317+
it('isPushdownableCel is untouched: the authoring gate resolves no variables', () => {
318+
expect(isPushdownableCel('id in current_user.org_user_ids').ok).toBe(true);
319+
expect(isPushdownableCel('!(id in current_user.org_user_ids)').ok).toBe(true);
320+
});
321+
});

‎packages/formula/src/cel-to-filter.ts‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,10 @@
3939
* `current_user.org_user_ids` → a pre-resolved membership array for `$in`
4040
* (honours ADR-0055: the runtime pre-resolves the set; the compiler never emits
4141
* a subquery). A variable that resolves to `undefined`/`null` yields
42-
* `unresolved-variable` (the "no active org" fail-closed path).
42+
* `unresolved-variable` (the "no active org" fail-closed path) — and so does a
43+
* null/undefined MEMBER of a resolved membership array, which is the same
44+
* unresolved value one level in. See {@link lowerMembership} for why the member
45+
* is refused rather than dropped.
4346
*/
4447

4548
import type { ASTNode } from '@marcbachmann/cel-js';
@@ -366,6 +369,35 @@ function lowerMembership(elemNode: ASTNode, containerNode: ASTNode, ctx: Ctx): F
366369
if (value !== SHAPE_VALUE && !Array.isArray(value)) {
367370
throw new CompileError('unsupported', `\`in\` requires an array/list on the right`);
368371
}
372+
// A null/undefined MEMBER of a RESOLVED membership variable fails closed, exactly
373+
// as the scalar `resolveValue` path does one level up: the same unresolved value,
374+
// the same `unresolved-variable` reason, the same deny sentinel downstream. Until
375+
// this guard the member was emitted verbatim into a security `$in`, so the one
376+
// shape that IS a permission predicate was the one shape that did not fail closed.
377+
//
378+
// Refused, never dropped. Stripping the member was measured to INVERT under the
379+
// supported `not in` form (`!(x in y)` → `$not` wrapping `$in`): `$in: []` matches
380+
// nothing, so `$not { $in: [] }` matches the WHOLE table. "Matches nothing" is
381+
// fail-closed in POSITIVE polarity only, which makes stripping fail-OPEN precisely
382+
// where the predicate is a blocklist. Refusing here needs no polarity awareness at
383+
// all — it throws before any `$not` wrapper is built, so every enclosing shape
384+
// (`!`, `&&`, `||`) collapses to the single `unresolved-variable` result.
385+
//
386+
// Deliberately NOT this guard's business: an AUTHORED literal null inside a list
387+
// (`record.status in ['lost', null]`). That is a declared predicate rather than an
388+
// unresolved variable — the same distinction `== null` already draws, where a
389+
// literal null lowers to `$null` instead of failing closed — and what such a
390+
// filter should SELECT is a separate open question this compiler does not answer.
391+
if (container.kind === 'var' && Array.isArray(value)) {
392+
const idx = value.findIndex((member) => member === null || member === undefined);
393+
if (idx !== -1) {
394+
throw new CompileError(
395+
'unresolved-variable',
396+
`variable "${container.path.join('.')}" has an unresolved member at index ${idx} ` +
397+
`(${String(value[idx])}); a membership array must resolve every member`,
398+
);
399+
}
400+
}
369401
return { [(elem as { path: string }).path]: { $in: value } } as FilterCondition;
370402
}
371403

0 commit comments

Comments
 (0)