diff --git a/.changeset/cel-pushdown-membership-null-member-fail-closed.md b/.changeset/cel-pushdown-membership-null-member-fail-closed.md new file mode 100644 index 0000000000..1e56d5dcf9 --- /dev/null +++ b/.changeset/cel-pushdown-membership-null-member-fail-closed.md @@ -0,0 +1,43 @@ +--- +"@objectstack/formula": patch +--- + +fix(formula): fail closed on a null MEMBER of a resolved membership array in the CEL pushdown (#13496) + +`compileCelToFilter` already fails closed when a `current_user.*` variable +resolves to `undefined`/`null` — the module's docblock calls it "the no active +org fail-closed path" and it is pinned for the SCALAR case. `lowerMembership` +did not apply the same discipline one level in: it checked only +`Array.isArray(value)` and emitted the list verbatim, so a null MEMBER of a +resolved membership array went straight into a security `$in`. The one shape +that IS a permission predicate was the one shape that did not fail closed. + +`lowerMembership` now refuses a `null`/`undefined` member of a **variable-resolved** +membership array with the same `unresolved-variable` reason the scalar path +uses, which the RLS path already turns into the deny sentinel. + +Maintainer ruling, 2026-08-31 (quoted unchanged): 「membership 数组中的 null +**成员**触发与 null 标量同款处置——`unresolved-variable` / deny sentinel,⛔ 不 +strip、不静默清洗。」 + +**Why refuse rather than strip.** Stripping the unresolved member is safe in +POSITIVE polarity only. `not in` is a supported, pinned member of the pushdown +subset (`!(x in y)` lowers to `$not` wrapping `$in`), and `$in: []` matches +nothing on every backend — so `$not { $in: [] }` matches the WHOLE table. +Stripping therefore inverts into fail-OPEN exactly where the predicate is a +blocklist, and it silently deletes a blocklist entry in the mixed +`['u1', null]` case. Refusing needs no polarity awareness at all: it throws +before any `$not` wrapper is built. Both polarities are pinned. + +**No shipped behaviour changes.** No first-party provider puts a null into a +membership array — `resolve-authz-context.ts` filters non-strings out of +`org_user_ids`, and the kernel spec declares `org_user_ids: z.array(z.string())` +— so the refused shape was never a declared-valid input. This makes the +implementation match the declaration rather than narrowing it. A fully resolved +list, an empty list (`$in: []`, a legitimate declared predicate) and the +authoring-time `isPushdownableCel` shape gate are all unchanged and pinned so. + +Out of scope, deliberately: an AUTHORED literal null inside a list +(`record.status in ['lost', null]`) is a declared predicate, not an unresolved +variable, and what such a filter should select is a separate open question. It +is untouched, with a pin recording that. diff --git a/packages/formula/src/cel-to-filter.test.ts b/packages/formula/src/cel-to-filter.test.ts index 7b775604d1..22369884dd 100644 --- a/packages/formula/src/cel-to-filter.test.ts +++ b/packages/formula/src/cel-to-filter.test.ts @@ -216,3 +216,106 @@ describe('compileCelToFilter — input shapes', () => { expect(r.ok && r.filter).toEqual({ dept: 'sales' }); }); }); + +/** + * A null/undefined MEMBER of a resolved membership array fails closed, like the + * null SCALAR variable already pinned above (maintainer ruling, 2026-08-31). + * + * BOTH POLARITIES are pinned, and that is the point of the suite rather than a + * completeness flourish. The alternative repair — stripping the unresolved member + * — is safe in POSITIVE polarity (`$in` over the surviving members never grants + * more than those members grant) and INVERTS under the supported `not in` form: + * `!(x in y)` lowers to `$not` wrapping `$in`, and `$in: []` matching nothing makes + * `$not { $in: [] }` match the whole table. A positive-only suite is green for both + * repairs and therefore pins nothing about the one that was ruled on. + */ +describe('compileCelToFilter — a null MEMBER of a membership array fails closed', () => { + const vars = (org_user_ids: unknown[]) => ({ current_user: { id: 'u_me', org_user_ids } }); + /** + * `ok` above pins its second argument to the exact shape of the module-level + * `VARS`, so it cannot take these partial contexts. Same assertion, same throw + * on an unexpected refusal, widened only where this suite needs it. + */ + const filterOf = (src: string, v: Record) => { + const r = compileCelToFilter(src, { variables: v }); + if (!r.ok) throw new Error(`expected ok for "${src}" but got ${r.reason}: ${r.detail}`); + return r.filter; + }; + const expectUnresolved = (r: ReturnType, path = 'current_user.org_user_ids') => { + expect(r.ok).toBe(false); + if (r.ok) return; + expect(r.reason).toBe('unresolved-variable'); + expect(r.detail).toContain(path); + expect(r.detail).toContain('unresolved member'); + }; + + // ---- POSITIVE polarity: `x in y` -> $in --------------------------------- + it('positive: null among resolved members → unresolved-variable (no $in emitted)', () => { + expectUnresolved(compileCelToFilter('id in current_user.org_user_ids', { variables: vars(['u_me', null]) })); + }); + it('positive: a lone null member → unresolved-variable', () => { + expectUnresolved(compileCelToFilter('id in current_user.org_user_ids', { variables: vars([null]) })); + }); + it('positive: an undefined member fails closed too (the scalar path refuses both)', () => { + expectUnresolved(compileCelToFilter('id in current_user.org_user_ids', { variables: vars(['u_me', undefined]) })); + }); + + // ---- NEGATIVE polarity: `!(x in y)` -> $not wrapping $in ---------------- + // This is where stripping inverted into allow-all; refusing must reach the SAME + // result here as in positive polarity, with no polarity threading in the lowerer. + it('negated `not in`: null among resolved members → unresolved-variable (no $not{$in} emitted)', () => { + expectUnresolved(compileCelToFilter('!(id in current_user.org_user_ids)', { variables: vars(['u_me', null]) })); + }); + it('negated `not in`: a lone null member → unresolved-variable, NOT $not{$in:[]} (whole table)', () => { + expectUnresolved(compileCelToFilter('!(id in current_user.org_user_ids)', { variables: vars([null]) })); + }); + it('negated inside a disjunction: the surviving disjunct does not rescue the compile', () => { + expectUnresolved( + compileCelToFilter("!(id in current_user.org_user_ids) || owner == current_user.id", { + variables: vars([null]), + }), + ); + }); + it('negated inside a conjunction fails closed as well', () => { + expectUnresolved( + compileCelToFilter("!(id in current_user.org_user_ids) && owner == current_user.id", { + variables: vars(['u_me', null]), + }), + ); + }); + + // ---- the two polarities agree, which is the ruling's "same treatment" ---- + it('both polarities and the null SCALAR variable yield the identical reason', () => { + const scalar = compileCelToFilter('record.organization_id == current_user.organization_id', { + variables: { current_user: { organization_id: null } }, + }); + const positive = compileCelToFilter('id in current_user.org_user_ids', { variables: vars([null]) }); + const negated = compileCelToFilter('!(id in current_user.org_user_ids)', { variables: vars([null]) }); + const reasons = [scalar, positive, negated].map((r) => (r.ok ? 'ok' : r.reason)); + expect(reasons).toEqual(['unresolved-variable', 'unresolved-variable', 'unresolved-variable']); + }); + + // ---- shapes this guard must NOT move -------------------------------------- + it('a fully resolved membership array still compiles, in both polarities', () => { + expect(filterOf('id in current_user.org_user_ids', vars(['u_me', 'u_peer']))).toEqual({ + id: { $in: ['u_me', 'u_peer'] }, + }); + expect(filterOf('!(id in current_user.org_user_ids)', vars(['u_me', 'u_peer']))).toEqual({ + $not: { id: { $in: ['u_me', 'u_peer'] } }, + }); + }); + it('an EMPTY membership array still compiles to $in:[] in both polarities (a declared predicate, unchanged here)', () => { + expect(filterOf('id in current_user.org_user_ids', vars([]))).toEqual({ id: { $in: [] } }); + expect(filterOf('!(id in current_user.org_user_ids)', vars([]))).toEqual({ $not: { id: { $in: [] } } }); + }); + it('an AUTHORED literal null in a list is not an unresolved variable — out of this guard scope', () => { + // A declared predicate, the same way `record.x == null` lowers to `$null` rather + // than failing closed. What such a filter SELECTS is a separate open question; + // this pin records only that the compiler still lowers it, unchanged. + expect(ok("record.status in ['lost', null]")).toEqual({ status: { $in: ['lost', null] } }); + }); + it('isPushdownableCel is untouched: the authoring gate resolves no variables', () => { + expect(isPushdownableCel('id in current_user.org_user_ids').ok).toBe(true); + expect(isPushdownableCel('!(id in current_user.org_user_ids)').ok).toBe(true); + }); +}); diff --git a/packages/formula/src/cel-to-filter.ts b/packages/formula/src/cel-to-filter.ts index 44d77cbeb4..b87311f1d2 100644 --- a/packages/formula/src/cel-to-filter.ts +++ b/packages/formula/src/cel-to-filter.ts @@ -39,7 +39,10 @@ * `current_user.org_user_ids` → a pre-resolved membership array for `$in` * (honours ADR-0055: the runtime pre-resolves the set; the compiler never emits * a subquery). A variable that resolves to `undefined`/`null` yields - * `unresolved-variable` (the "no active org" fail-closed path). + * `unresolved-variable` (the "no active org" fail-closed path) — and so does a + * null/undefined MEMBER of a resolved membership array, which is the same + * unresolved value one level in. See {@link lowerMembership} for why the member + * is refused rather than dropped. */ import type { ASTNode } from '@marcbachmann/cel-js'; @@ -366,6 +369,35 @@ function lowerMembership(elemNode: ASTNode, containerNode: ASTNode, ctx: Ctx): F if (value !== SHAPE_VALUE && !Array.isArray(value)) { throw new CompileError('unsupported', `\`in\` requires an array/list on the right`); } + // A null/undefined MEMBER of a RESOLVED membership variable fails closed, exactly + // as the scalar `resolveValue` path does one level up: the same unresolved value, + // the same `unresolved-variable` reason, the same deny sentinel downstream. Until + // this guard the member was emitted verbatim into a security `$in`, so the one + // shape that IS a permission predicate was the one shape that did not fail closed. + // + // Refused, never dropped. Stripping the member was measured to INVERT under the + // supported `not in` form (`!(x in y)` → `$not` wrapping `$in`): `$in: []` matches + // nothing, so `$not { $in: [] }` matches the WHOLE table. "Matches nothing" is + // fail-closed in POSITIVE polarity only, which makes stripping fail-OPEN precisely + // where the predicate is a blocklist. Refusing here needs no polarity awareness at + // all — it throws before any `$not` wrapper is built, so every enclosing shape + // (`!`, `&&`, `||`) collapses to the single `unresolved-variable` result. + // + // Deliberately NOT this guard's business: an AUTHORED literal null inside a list + // (`record.status in ['lost', null]`). That is a declared predicate rather than an + // unresolved variable — the same distinction `== null` already draws, where a + // literal null lowers to `$null` instead of failing closed — and what such a + // filter should SELECT is a separate open question this compiler does not answer. + if (container.kind === 'var' && Array.isArray(value)) { + const idx = value.findIndex((member) => member === null || member === undefined); + if (idx !== -1) { + throw new CompileError( + 'unresolved-variable', + `variable "${container.path.join('.')}" has an unresolved member at index ${idx} ` + + `(${String(value[idx])}); a membership array must resolve every member`, + ); + } + } return { [(elem as { path: string }).path]: { $in: value } } as FilterCondition; }