diff --git a/.changeset/rls-predicate-over-budget.md b/.changeset/rls-predicate-over-budget.md new file mode 100644 index 0000000000..efc1186c9e --- /dev/null +++ b/.changeset/rls-predicate-over-budget.md @@ -0,0 +1,33 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): 超预算的 RLS 谓词有了自己的规则 id,不再被当成方言写错 (#6778) + +`rowLevelSecurity[].using` / `.check` 里一条**语法完美、可下推**、只是太大的 CEL +谓词(例如 80 项合取,超过 `maxAstNodes` 256),此前报在 +`rls-predicate-unparseable` 名下——那条规则的提示语讲的是 SQL 与 CEL 的方言混淆 +("用 `&&` 别用 `AND`"、"`LIKE` 没有 CEL 拼法")。判决是**对的**,指路是错的: +作者要做的是把谓词改小或拆开,而不是检查自己的方言。 + +新增第三个 id **`rls-predicate-over-budget`**(与既有两个并列导出): + +- 消息点名**具体越界的那个界**和平台取值——`maxAstNodes` (256) / `maxDepth` (32) + / `maxListElements` (64) 各自报各自的,取自 formula 姊妹入口 + `parseCelToAstWithReason` 的 `kind: 'bounds'` 载荷,而不是写死一个;被告知去缩短 + 错误的那根轴,作者就会改错地方。越界谓词按定义很长,引文因此截断到 200 字符。 +- 提示语给的是真正的补救:把长 `||` 链折成 `field in [...]`;把集合预解析成 + `current_user.` 成员键(ADR-0105 D11);把重复子表达式反范式化成本对象上的 + 一个 formula/rollup 字段;以及——**只对顶层 `||`** 可以拆成多条策略(适用策略之 + 间是 OR),顶层 `&&` 这样拆会**放大**访问权限而不是保持它。 + +**没有行为变更。** 判定边界仍然是 `isSupportedRlsExpression` 本身,一个字符没动; +同样的输入照样被拒,只是其中一类被告知了真正的原因。区分只发生在**解释**里: +运行时把越界折叠进 `reason: 'parse-error'` 是有意为之("每个消费者都已经把这个 +reason 路由到自己的拒绝路径"),对只需决定拒不拒的运行时是对的,对职责就是点明该 +改哪里的授时诊断则不然。 + +该规则不读 `cel-pushdown-limits.ts` 的 GA 日期开关,对它保持中立:17.0.0-rc.x 宽限 +窗口内越界谓词仍被放行,本规则一条都不报(实测 0 条);v17 GA 翻转后同一谓词被拒, +落到新 id 上。两个开关位置都有测试钉住,越界与真正的语法错误两侧各自成对钉住, +将来任何把二者重新合并的改动都会变红。 diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index f30bd89309..4c1aa1a773 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -254,6 +254,7 @@ export { validateRlsPredicateEnforceability, RLS_PREDICATE_UNENFORCEABLE, RLS_PREDICATE_UNPARSEABLE, + RLS_PREDICATE_OVER_BUDGET, } from './validate-rls-predicate-enforceability.js'; export type { RlsPredicateFinding, diff --git a/packages/lint/src/validate-rls-predicate-enforceability.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.test.ts index a0886cb973..fe27b311fb 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.test.ts @@ -1,12 +1,13 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. -import { describe, it, expect } from 'vitest'; -import { isSupportedRlsExpression } from '@objectstack/formula'; +import { describe, it, expect, afterEach } from 'vitest'; +import { isSupportedRlsExpression, setCelPushdownLimitsModeForTests } from '@objectstack/formula'; import { validateRlsPredicateEnforceability, RLS_PREDICATE_UNENFORCEABLE, RLS_PREDICATE_UNPARSEABLE, + RLS_PREDICATE_OVER_BUDGET, } from './validate-rls-predicate-enforceability.js'; import { AUTHORING_RULES, runAuthoringRules } from './authoring-rules.js'; @@ -307,3 +308,202 @@ describe('validateRlsPredicateEnforceability — the verdict IS the RLSCompiler\ .toContain(RLS_PREDICATE_UNENFORCEABLE); }); }); + +// ── Over budget is not a dialect mistake (#6778) ───────────────────── +// +// The pushdown compiler collapses a `DEFAULT_LIMITS` overrun into +// `reason: 'parse-error'` deliberately — it is the reason every consumer +// already routes to its deny path. Correct for the runtime, whose only +// decision is deny-or-not; wrong for an authoring diagnostic, whose job is to +// name the edit. Before #6778 an 80-term conjunction — valid, lowerable CEL +// that is merely too big — was reported under `rls-predicate-unparseable`, +// whose hint explains SQL-vs-CEL syntax confusion. +// +// These cases run at BOTH positions of `cel-pushdown-limits.ts`'s dated GA +// switch, because the two positions are where the whole question lives: during +// 17.0.0-rc.x the grace window admits an over-limit predicate and this rule +// must stay silent; at the v17 GA flip the same predicate is refused and must +// be told the truth about why. + +/** Over one `DEFAULT_LIMITS` bound each, and nothing else wrong with them. */ +const OVER_BUDGET = { + maxAstNodes: Array.from({ length: 80 }, (_, i) => `record.f${i} == ${i}`).join(' && '), + maxDepth: '('.repeat(40) + 'record.a == 1' + ')'.repeat(40), + maxListElements: `record.x in [${Array.from({ length: 100 }, (_, i) => i).join(', ')}]`, +} as const; + +/** Genuinely not CEL — the class `rls-predicate-unparseable` was written for. */ +const NOT_CEL = { + 'SQL AND': 'a = current_user.id AND b = 1', + 'a subquery': 'id IN (SELECT id FROM users)', + 'a stray operator': 'record.stage ==', +} as const; + +describe('validateRlsPredicateEnforceability — a bounds overrun is its own id (#6778)', () => { + afterEach(() => { + // A suite must not leak a mode into the next file. + setCelPushdownLimitsModeForTests('rc-grace')(); + }); + + const atGa = (fn: () => T): T => { + const restore = setCelPushdownLimitsModeForTests('fail-closed'); + try { + return fn(); + } finally { + restore(); + } + }; + + // ── The shipped position: nothing changes today ─────────────────── + it.each(Object.entries(OVER_BUDGET))( + 'stays silent on an over-%s predicate during the rc grace window — no behaviour change today', + (_limit, source) => { + // The grace window admits it (it still compiles and WARNs), so + // `isSupportedRlsExpression` is true and the rule never fires. + expect(isSupportedRlsExpression(source)).toBe(true); + expect(validateRlsPredicateEnforceability(policyWith('using', source))).toEqual([]); + }, + ); + + // ── The GA position: the whole point of the card ────────────────── + it('at the GA flip, an over-budget predicate reports over-budget — naming the bound and its value', () => { + const findings = atGa(() => validateRlsPredicateEnforceability(policyWith('using', OVER_BUDGET.maxAstNodes))); + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ + severity: 'error', + rule: RLS_PREDICATE_OVER_BUDGET, + path: 'permissions[0].rowLevelSecurity[0].using', + where: 'permission set "sales_rep" policy "own_leads" on object "lead"', + }); + // The bound and the budget are the two facts "shrink it to fit" needs. + expect(findings[0].message).toMatch(/maxAstNodes/); + expect(findings[0].message).toMatch(/platform limit 256/); + expect(findings[0].message).toMatch(/Exceeded maxAstNodes \(256\)/); + // The verdict is unchanged, so the consequence prose must still be there. + expect(findings[0].message).toMatch(/DROPS the policy at request time/); + expect(findings[0].message).toMatch(/ZERO rows/); + // An over-budget predicate is long by definition — the quote is bounded. + expect(findings[0].message).toContain('...'); + expect(findings[0].message.length).toBeLessThan(OVER_BUDGET.maxAstNodes.length + 1200); + }); + + it('prescribes shrinking, and never sends the author to check their dialect', () => { + const [f] = atGa(() => validateRlsPredicateEnforceability(policyWith('using', OVER_BUDGET.maxAstNodes))); + // The real remedies. + expect(f.hint).toMatch(/field in \[a, b, …\]/); + expect(f.hint).toMatch(/current_user\./); + expect(f.hint).toMatch(/[Dd]enormalise/); + expect(f.hint).toMatch(/hook or action body/); + // Splitting is only sound on a top-level `||`; policies are OR-ed, so + // splitting an `&&` would WIDEN access. Saying so is the point of the hint. + expect(f.hint).toMatch(/never split a top-level `&&`/); + expect(f.hint).toMatch(/WIDEN access/); + // …and explicitly NOT the SQL-vs-CEL prose this class used to get. + expect(f.hint).toMatch(/no syntax or dialect error/); + expect(f.hint).not.toMatch(/canonical CEL \(ADR-0058 D1\)/); + expect(f.hint).not.toMatch(/rather than SQL `AND` \/ `OR`/); + expect(f.hint).not.toMatch(/LIKE/); + }); + + it('names the bound that was actually blown, not a hard-coded one', () => { + // Reading `limit` / `limitValue` off the sister entrance's payload rather + // than assuming `maxAstNodes` is what makes the hint worth reading: an + // author told to shorten the wrong axis edits the wrong thing. + const [depth] = atGa(() => validateRlsPredicateEnforceability(policyWith('using', OVER_BUDGET.maxDepth))); + expect(depth.rule).toBe(RLS_PREDICATE_OVER_BUDGET); + expect(depth.message).toMatch(/maxDepth/); + expect(depth.message).toMatch(/platform limit 32/); + expect(depth.message).not.toMatch(/maxAstNodes/); + + const [list] = atGa(() => validateRlsPredicateEnforceability(policyWith('using', OVER_BUDGET.maxListElements))); + expect(list.rule).toBe(RLS_PREDICATE_OVER_BUDGET); + expect(list.message).toMatch(/maxListElements/); + expect(list.message).toMatch(/platform limit 64/); + expect(list.message).not.toMatch(/maxAstNodes/); + }); + + it('carries the WRITE-path consequence when the over-budget clause is `check`', () => { + const [f] = atGa(() => validateRlsPredicateEnforceability(policyWith('check', OVER_BUDGET.maxAstNodes))); + expect(f).toMatchObject({ + rule: RLS_PREDICATE_OVER_BUDGET, + path: 'permissions[0].rowLevelSecurity[0].check', + }); + expect(f.message).toMatch(/PermissionDeniedError/); + expect(f.message).not.toMatch(/ZERO rows/); + }); + + // ── The discrimination, which IS the card ───────────────────────── + // + // A split that cannot be shown to separate the two classes is decoration. + // Both halves are pinned in one table so a future change that collapses them + // — in either direction — goes red here rather than silently mislabelling + // one class again. + it('discriminates over-budget from not-CEL at the GA position, in both directions', () => { + const expected: Array<[string, string, string]> = [ + ...Object.entries(OVER_BUDGET).map( + ([limit, src]) => [`over ${limit}`, src, RLS_PREDICATE_OVER_BUDGET] as [string, string, string], + ), + ...Object.entries(NOT_CEL).map( + ([label, src]) => [label, src, RLS_PREDICATE_UNPARSEABLE] as [string, string, string], + ), + ]; + const actual = atGa(() => + expected.map(([label, src]) => [label, ids(policyWith('using', src))] as const), + ); + expect(actual).toEqual(expected.map(([label, , rule]) => [label, [rule]])); + }); + + it('a predicate that is BOTH unparseable and huge is unparseable — syntax is judged first', () => { + // 80 SQL `AND` terms: over `maxAstNodes` in size, but the bridge does not + // cover `AND`, so it is not CEL at all. Shortening it would not help; the + // author has to rewrite it, so the syntax id is the useful one. The parse + // never reaches a bounds fault because it throws on `AND` first. + const source = Array.from({ length: 80 }, (_, i) => `f${i} = ${i}`).join(' AND '); + expect(source.length).toBeGreaterThan(OVER_BUDGET.maxAstNodes.length / 2); + expect(atGa(() => ids(policyWith('using', source)))).toEqual([RLS_PREDICATE_UNPARSEABLE]); + }); + + it('leaves the unenforceable class alone — an over-budget check never steals a shape fault', () => { + // Reported at BOTH switch positions: this class does not involve the parse + // bounds at all, so neither position may re-route it. + for (const source of ['size(record.tags) > 0', "record.account.region == 'EU'", 'amount + 1 > 2']) { + expect(ids(policyWith('using', source))).toEqual([RLS_PREDICATE_UNENFORCEABLE]); + expect(atGa(() => ids(policyWith('using', source)))).toEqual([RLS_PREDICATE_UNENFORCEABLE]); + } + }); + + // ── The red/green boundary is untouched ─────────────────────────── + it('refuses exactly what it refused before — only the explanation moved', () => { + // #6778 is explicitly NOT a behaviour change. The rule's verdict is still + // `isSupportedRlsExpression`, so lint-clean must remain that function's own + // answer at BOTH switch positions, over-budget sources included. + const corpus = [ + ...Object.values(OVER_BUDGET), + ...Object.values(NOT_CEL), + 'owner_id == current_user.id', + "status = 'published'", + 'size(record.tags) > 0', + ]; + for (const mode of ['rc-grace', 'fail-closed'] as const) { + const restore = setCelPushdownLimitsModeForTests(mode); + try { + for (const source of corpus) { + const lintIsClean = validateRlsPredicateEnforceability(policyWith('using', source)).length === 0; + expect({ mode, source: source.slice(0, 40), lintIsClean }).toEqual({ + mode, + source: source.slice(0, 40), + lintIsClean: isSupportedRlsExpression(source), + }); + } + } finally { + restore(); + } + } + }); + + it('reaches the author through the real registry, not just a direct call', () => { + const stack = policyWith('using', OVER_BUDGET.maxAstNodes); + expect(atGa(() => runAuthoringRules('validate', { normalized: stack, parsed: stack }).map((f) => f.rule))) + .toEqual([RLS_PREDICATE_OVER_BUDGET]); + }); +}); diff --git a/packages/lint/src/validate-rls-predicate-enforceability.ts b/packages/lint/src/validate-rls-predicate-enforceability.ts index 14cd9a1b66..98675a3ff7 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.ts @@ -72,7 +72,7 @@ * supported, so the gate turns nothing red that works today. That corpus is * pinned in the tests rather than asserted here. * - * ## The two ids, and why not one + * ## The three ids, and why not one * * The consequence is identical, the FIX is not, and allowlists / `--json` * consumers key on the id: @@ -84,6 +84,48 @@ * - {@link RLS_PREDICATE_UNPARSEABLE} — it does not parse as CEL even after * the legacy SQL bridge: SQL `AND` / `OR` / `LIKE`, a subquery, a stray * operator. Fix = write CEL (`&&`, `||`), which is a different edit. + * - {@link RLS_PREDICATE_OVER_BUDGET} — it is syntactically perfect CEL that + * overruns a {@link DEFAULT_LIMITS} parse bound (`maxAstNodes` 256, + * `maxDepth` 32, …). Fix = shrink or split the predicate; there is no + * dialect error to correct. + * + * ## Why the third id (#6778), and why it is not a behaviour change + * + * The pushdown compiler collapses a bounds overrun into `reason: 'parse-error'` + * **on purpose** — `cel-to-filter.ts` says so at the call site: it is "the + * reason every consumer of this compiler already routes to its deny path", and + * a fourth reason would be a new branch none of them have. That is right for + * the RUNTIME, whose only decision is deny-or-not. It is wrong for an AUTHORING + * diagnostic, whose whole job is to name the edit: until #6778 an 80-term + * conjunction — valid, lowerable CEL that is merely too big — was reported + * under `rls-predicate-unparseable`, whose hint explains SQL-vs-CEL syntax + * confusion. The verdict was correct and the sign-post pointed at the wrong + * repair. + * + * So the split is made HERE, in the explanation, and never in the verdict. The + * red/green boundary is still exactly `isSupportedRlsExpression` — untouched — + * and this rule reaches for {@link parseCelToAstWithReason}, the same + * reason-carrying entrance `cel-to-filter.ts` itself parses through, only to + * ask which KIND of refusal the consumer just produced. Identical inputs are + * refused before and after; one class of them is told the truth about why. + * + * It is deliberately mode-agnostic with respect to `cel-pushdown-limits.ts`'s + * dated GA switch, and does not read it. During 17.0.0-rc.x an over-limit + * predicate is admitted by the grace window, so `isSupportedRlsExpression` is + * `true` and this rule never fires at all (measured: zero findings). At the v17 + * GA flip the same predicate is refused and lands here. The one rc-grace case + * that DOES reach this branch — a bounds fault whose unbounded reparse also + * fails, so the grace window has no AST to admit — is a genuine bounds refusal + * and is correctly reported as one. Re-deriving the switch here instead would + * be modelling the consumer, which this file's whole construction refuses. + * + * `overrun.measured` is `null` on this path and that is by the producer's + * design, not an omission: `parseCelToAstWithReason` only measures when the + * caller passes `admitOverLimit`, "because measuring means re-parsing a source + * we have just decided is too big". That option is documented as the grace + * window's alone and as disappearing at GA, so a lint rule must not reach for + * it to decorate a message. The bound and its value — which is what "shrink it + * to fit" needs — are always present. * * Unlike the sharing-rule gate, syntax is reported HERE rather than deferred to * `validateStackExpressions`: that rule does not walk `rowLevelSecurity` at all, @@ -109,12 +151,20 @@ * moment nobody re-runs the linter. */ -import { isPushdownableCel, isSupportedRlsExpression, sqlPredicateToCel } from '@objectstack/formula'; +import { + isPushdownableCel, + isSupportedRlsExpression, + parseCelToAstWithReason, + sqlPredicateToCel, +} from '@objectstack/formula'; +import type { CelBoundsOverrun } from '@objectstack/formula'; /** A predicate outside the pushdown subset — the policy enforces nothing. */ export const RLS_PREDICATE_UNENFORCEABLE = 'rls-predicate-unenforceable'; /** A predicate that does not parse as CEL even after the legacy SQL bridge. */ export const RLS_PREDICATE_UNPARSEABLE = 'rls-predicate-unparseable'; +/** Valid CEL that overruns a platform parse bound (`maxAstNodes`, `maxDepth`, …). */ +export const RLS_PREDICATE_OVER_BUDGET = 'rls-predicate-over-budget'; export type RlsPredicateSeverity = 'error' | 'warning'; @@ -152,6 +202,37 @@ const PUSHDOWN_SUBSET = 'and the string methods `startsWith` / `endsWith` / `contains` — over SINGLE-column field paths ' + '(ADR-0058 D2), compared against a literal or a `current_user.*` value.'; +/** + * The overrun behind a `parse-error`, or `null` when the refusal was a genuine + * syntax fault. + * + * Asked of {@link parseCelToAstWithReason} — the SAME reason-carrying entrance + * `cel-to-filter.ts` parses through — on the SAME bridged source, so `bounds` + * here and the `bounds` that produced the consumer's refusal are one verdict on + * one input. It is graded by error class plus structured `code`, never prose + * (#6223), so a rephrasing upstream cannot silently re-route a diagnostic. + * + * Called only after {@link isSupportedRlsExpression} has already returned + * `false` AND the reason is `parse-error`: it never widens or narrows what is + * reported, it only decides which of two explanations a refusal gets. + */ +function boundsOverrunOf(bridged: string): CelBoundsOverrun | null { + // No `admitOverLimit`: that option is the rc-grace window's alone (and goes + // away with it), and buying `measured` costs an unbounded re-parse of a + // source already known to be too big. + const parsed = parseCelToAstWithReason(bridged); + return !parsed.ok && parsed.kind === 'bounds' ? parsed.overrun : null; +} + +/** + * An over-budget predicate is by definition long, so the diagnostic quotes a + * bounded prefix rather than the whole source — the same 200-char courtesy + * `cel-to-filter.ts`'s grace WARN extends for the same reason. + */ +function quote(source: string): string { + return source.length > 200 ? `${source.slice(0, 197)}...` : source; +} + /** What the runtime does with a predicate it cannot compile, per clause. */ function consequence(clause: 'using' | 'check'): string { const dropped = @@ -198,9 +279,14 @@ export function validateRlsPredicateEnforceability(stack: unknown): RlsPredicate // ── The explanation. Re-derived only to tell the author WHICH fix they // need; the red/green boundary above never consults it. (Both agree by // construction — pinned in both directions in this rule's tests.) - const why = isPushdownableCel(sqlPredicateToCel(source)); + const bridged = sqlPredicateToCel(source); + const why = isPushdownableCel(bridged); const detail = why.ok ? '' : why.detail; const parseError = !why.ok && why.reason === 'parse-error'; + // A bounds overrun and a syntax fault both arrive as `parse-error` (the + // runtime deliberately collapses them — see this file's docblock), so + // the two are separated here, and only here. + const overrun = parseError ? boundsOverrunOf(bridged) : null; const psName = str(ps.name) || String(psIndex); const policyName = str(policy.name) || String(pIndex); @@ -209,6 +295,39 @@ export function validateRlsPredicateEnforceability(stack: unknown): RlsPredicate `permission set "${psName}" policy "${policyName}"` + (object ? ` on object "${object}"` : ''); const path = `permissions[${psIndex}].rowLevelSecurity[${pIndex}].${clause}`; + if (overrun) { + // `limit` is null only for a bounds fault this package cannot NAME + // (unreachable on cel-js 8.0.0). Degrade honestly rather than guess a + // key, which would send the author to shorten the wrong axis. + const bound = overrun.limit ?? 'an unnamed platform CEL bound'; + const budget = overrun.limitValue !== null ? ` (platform limit ${overrun.limitValue})` : ''; + const measured = overrun.measured !== null ? `, this predicate measures ${overrun.measured}` : ''; + findings.push({ + severity: 'error', + rule: RLS_PREDICATE_OVER_BUDGET, + where, + path, + message: + `RLS ${clause} \`${quote(source)}\` is syntactically valid, lowerable CEL but overruns the ` + + `platform parse bound ${bound}${budget}${measured} (${overrun.summary}), ` + + consequence(clause), + hint: + `There is no syntax or dialect error to correct here — the predicate is well-formed CEL and ` + + `is simply too large for ${bound}${budget}, so the fix is to make it smaller or to move the ` + + `work off the predicate. (1) Collapse a long \`field == a || field == b || …\` chain into a ` + + `single \`field in [a, b, …]\`, which is far fewer AST nodes (\`maxListElements\` is 64, so a ` + + `very large set needs option 2). (2) Pre-resolve the set into a membership key the runtime ` + + `exposes and test \`field in current_user.\` (ADR-0105 D11) — one comparison whatever ` + + `the set size. (3) Denormalise a repeated sub-expression onto this object as a ` + + `formula/rollup field and test that single column. (4) Split a TOP-LEVEL \`||\` across ` + + `several \`rowLevelSecurity\` policies: applicable policies are OR-ed, so that is ` + + `equivalent — but never split a top-level \`&&\` this way, which would WIDEN access rather ` + + `than preserve it. Logic genuinely this large is not a row filter: move it to a hook or ` + + `action body (\`ScriptBody { language: 'js' }\`, the L2 sandboxed surface).`, + }); + continue; + } + if (parseError) { findings.push({ severity: 'error',