diff --git a/.changeset/22161-lint-slice-4-one-line.md b/.changeset/22161-lint-slice-4-one-line.md new file mode 100644 index 00000000000..a46595e25d5 --- /dev/null +++ b/.changeset/22161-lint-slice-4-one-line.md @@ -0,0 +1,16 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): the RLS-predicate and sharing-rule findings print one verdict line, and `os explain ` carries their reasoning + +Clause-②: no + +- **Shorter verdicts.** Each finding of these 9 rule ids now prints a `message` of one verdict sentence. Every finding the rules' own test suites, the CLI's RLS admission suite and the runtime publish gate's suite fire is at most 195 characters; before, the longest of each ran from 492 to 2,056 characters. The ids: + - RLS policies (`permissions[].rowLevelSecurity[].using` / `.check`): `rls-predicate-unparseable`, `rls-predicate-unenforceable`, `rls-predicate-unknown-user-variable`, `rls-predicate-unknown-field`, `rls-predicate-over-budget`; + - sharing rules (`sharingRules[]`): `sharing-rule-unlowerable-condition`, `sharing-rule-object-not-shareable`, `sharing-rule-object-controlled-by-parent`, `sharing-rule-runtime-variable-condition`. + + A finding no longer echoes the predicate or the condition: its `path` names the clause, and the predicate is author text of any length (the old message quoted up to 200 characters of it, or all of it). Each verdict names what is wrong and closes on what it costs: an RLS policy the runtime drops ends "so the policy is dropped and grants no access" (a `using`) or "… and admits no write" (a `check`); a field compared with a field that holds a list or an object, or across comparison classes, quotes the first such comparison, names the column or the two classes, counts the rest, and ends on the `INVALID_FILTER / 400` refusal; a sharing condition that does not lower ends "so the rule is never seeded and grants nothing"; an anchor object a grant cannot widen names its `sharingModel` and the `SHARING_NOT_ENABLED` refusal. A quoted refusal — the compiler's, the platform's shared filter check's, or the engine's (`RLS using (CODE / STATUS): …`) — is quoted up to its own suggested rewrite, or to its first sentence. The `fix` (the CLI's `fix:` line, the runtime issue's `hint`), every rule id, severity and `path`, and what each rule accepts or refuses are unchanged. A tool that matched the old message text should match on `rule` and `path` instead. +- **`os explain ` takes these 9 ids**, for example `os explain rls-predicate-unenforceable`. It prints the reasoning the verdicts no longer carry: what `RLSCompiler` does with a policy it cannot compile (the deny sentinel and its ZERO rows, the OR with other policies, the INSERT check a `using` doubles as, the `check` set), the one WARN line each drop leaves, which `current_user` keys the kernel resolves and with which type, why a column that holds a list or an object — or two columns of different comparison classes — cannot be compared, what the engine's own filter admission refuses and what that costs analytics, and, for sharing rules, why a condition is compiled once at boot, what an unlowerable or refused condition leaves behind, and why a grant on a public or `controlled_by_parent` object is refused. The paragraphs the RLS and sharing rules share are one text, printed under every id they explain. The `rule:` line under each of these findings now ends with `` — `os explain ` for … ``. The no-argument listing and its `--json` `rules` array list the 9 ids, and so does the unknown-id error's `Rules with an explanation:` line. `RULE_EXPLANATIONS` in `@objectstack/lint` gains the 9 entries. +- **Where the new text prints.** On the CLI, all 9 ids: `os validate`, `os build` (and `os compile`, which `os dev` runs on every compile), `os lint`, `os verify` and `os init`'s scaffold check print the new `message` on the text face, and `os validate --json` and `os build --json` carry it in their `errors` and author-time `issues`. At the runtime publish gate (Studio, REST `/meta`, MCP), a `permission` write carries the five `rls-predicate-*` ids, all errors: the 422 issue's `message` and the refusal log line under `OS_ALLOW_UNLINTED_METADATA_WRITES` change. Each issue's `hint` is unchanged. +- **Never at the runtime gate:** the four `sharing-rule-*` ids. The rule is registered for the CLI doors only (no write type dispatches it at the publish gate), so they speak only on the CLI doors above. diff --git a/packages/cli/test/rls-policy-authoring-admission.test.ts b/packages/cli/test/rls-policy-authoring-admission.test.ts index c723c3ae345..5a6c202a66e 100644 --- a/packages/cli/test/rls-policy-authoring-admission.test.ts +++ b/packages/cli/test/rls-policy-authoring-admission.test.ts @@ -208,8 +208,7 @@ describe('RLS read-scope admission at authoring time — fifteen classes, two do if (row.engineCode) { // The engine's own verdict, quoted with its code — never withheld at // authoring time, where the text is the author's own. - expect(cli[0].message).toContain(`(${row.engineCode} / 400): `); - expect(cli[0].message).toContain('lowers, but the engine refuses to run the lowered filter on "deal"'); + expect(cli[0].message.startsWith(`RLS using (${row.engineCode} / 400): `)).toBe(true); } }); } @@ -276,7 +275,7 @@ describe('a field compared with a json / multiple field is refused at both doors expect(cli.map((f) => ({ severity: f.severity, rule: f.rule, path: f.path }))).toEqual([ { severity: 'error', rule: UNENFORCEABLE, path: `permissions[0].rowLevelSecurity[0].${row.clause}` }, ]); - expect(cli[0].message).toContain('lowers, but compares a field with a field that holds a list or an object'); + expect(cli[0].message).toContain('holds a list or an object, so '); expect(saved.accepted).toBe(false); expect({ code: saved.code, status: saved.status }).toEqual({ code: 'INVALID_METADATA', status: 422 }); @@ -333,7 +332,7 @@ describe('a field compared with a field of another comparison class is refused a expect(cli.map((f) => ({ severity: f.severity, rule: f.rule, path: f.path }))).toEqual([ { severity: 'error', rule: UNENFORCEABLE, path: `permissions[0].rowLevelSecurity[0].${row.clause}` }, ]); - expect(cli[0].message).toContain('lowers, but compares two fields that share no comparison class'); + expect(cli[0].message).toContain('which no comparison class spans ('); expect(saved.accepted).toBe(false); expect({ code: saved.code, status: saved.status }).toEqual({ code: 'INVALID_METADATA', status: 422 }); diff --git a/packages/lint/src/rule-explanations.ts b/packages/lint/src/rule-explanations.ts index 378d31296ef..fe81aa69631 100644 --- a/packages/lint/src/rule-explanations.ts +++ b/packages/lint/src/rule-explanations.ts @@ -1044,6 +1044,282 @@ const ACTION_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION: RuleExplanation = { ], }; +// ── Row-level security and sharing rules (the two enforceability rules) ───── +// Two security surfaces are compiled to a row filter rather than interpreted: +// an RLS policy's `using` / `check` (`validate-rls-predicate-enforceability.ts`) +// and a declared sharing rule's `condition` +// (`validate-sharing-rule-enforceability.ts`). Both rules ask the runtime's own +// compiler whether a predicate lowers, and both judge a lowered field-to-field +// comparison by the declared field map, so the reasoning they share is written +// ONCE below and every entry it explains references it. The rule files share +// their verdict clauses the same way. + +/** How the runtime treats an RLS predicate it cannot compile — every id that drops a policy. */ +const RLS_UNCOMPILABLE_DROP = + '`RLSCompiler` compiles every RLS predicate to a row filter at request time and DROPS a policy whose ' + + 'predicate it cannot compile. One WARN line — "has an uncompilable predicate … and was DROPPED (no ' + + 'enforcement)" — is the only signal, and nothing else reports it at authoring time. The runtime fails ' + + 'CLOSED, which is why such a policy is survivable; it reads as an authorization and behaves as a refusal.'; + +/** + * What a dropped `using` does. The INSERT half: the ADR-0058 D4 write check + * takes its set from `writeCheckPolicies`, so when no applicable policy for the + * insert declares a `check`, every applicable policy's `using` is compiled as + * its check. Measured through the real `SecurityPlugin` on an `insert` and an + * `all` policy, for all three kinds of drop (an unlowerable shape, an + * unresolved `current_user.*`, an undeclared column): with nothing else + * compiling in that set, every single-record insert is refused (403); with + * another applicable policy's `using` compiling, that one alone decides; with a + * declared `check` beside it, the declared check alone decides. + */ +const RLS_DROPPED_USING = + 'A dropped `using`: when it is the only applicable policy for that object and operation, `compileFilter` ' + + 'returns the `RLS_DENY_FILTER` sentinel instead, which is AND-ed onto the where clause, so every select / ' + + 'update / delete on the object matches ZERO rows and the object disappears for every holder of the ' + + 'permission set. When other policies also apply, this one just vanishes from the OR and grants none of the ' + + 'access it appears to. On an `insert` or `all` policy the same `using` is also the single-record INSERT check ' + + 'whenever no applicable policy for the insert declares a `check` (ADR-0058 D4): when nothing else in that set ' + + 'compiles, every single-record insert it governs fails with `PermissionDeniedError`; when another policy\'s ' + + '`using` compiles, that one alone decides the insert.'; + +/** + * What a dropped `check` does. The write check OR-combines the declared checks + * of all the applicable policies for the operation, so a dropped `check` is a + * blanket refusal only when no other declared `check` in that set compiles + * (measured: beside a compiling declared `check`, that one alone decided; + * beside a USING-only sibling, every single-record insert was refused, because + * a USING-only sibling takes no part once any policy declares a `check`). + */ +const RLS_DROPPED_CHECK = + 'A dropped `check`: on the ADR-0058 D4 write path it leaves the post-image `check` as the `RLS_DENY_FILTER` ' + + 'sentinel, which no record can satisfy, so every single-record insert and by-id update the policy governs ' + + 'fails with `PermissionDeniedError`: the policy reads as a write rule and behaves as a blanket refusal. That ' + + 'holds when no other applicable policy for the operation declares a `check` that compiles; when one does, ' + + 'that `check` alone decides and this one contributes nothing.'; + +/** A comparison with a column that holds a list or an object — both rules' list-holding arm. */ +const LIST_HOLDING_COMPARISON = + 'A column that holds a list or an object — one declared with a structured JSON type such as `json` or ' + + '`address`, or a multi-value field such as `multiselect`, `tags` or a lookup flagged `multiple: true` — is ' + + 'not one comparable value, on either side of a field-to-field comparison, so the platform refuses the ' + + 'comparison instead of evaluating it. The compiler accepts it, because it knows the predicate\'s text and not ' + + 'the object\'s field types; the rule judges it by the declared type, as driver-sql does.'; + +/** A comparison across comparison classes — both rules' cross-class arm. */ +const CROSS_CLASS_COMPARISON = + 'Two columns are compared only within one comparison class — the class decides how their stored values ' + + 'order and equal, and across classes SQL and the in-memory evaluator answer differently — and a file field ' + + 'or a formula field has no class at all: no row filter can compare a file field with another column, and a ' + + 'formula field has no stored column a row filter can read. So the platform defines no comparison between ' + + 'such columns. The classification is the spec\'s (`crossFieldComparisonVerdict`), the one driver-sql applies.'; + +const RLS_PREDICATE_UNPARSEABLE_EXPLANATION: RuleExplanation = { + rule: 'rls-predicate-unparseable', + covers: 'why a predicate that does not parse is dropped', + paragraphs: [ + 'An RLS predicate is CEL (ADR-0058 D1). Before parsing, a legacy SQL bridge rewrites the historic subset ' + + 'it covers — a bare `=` to `==` and `IN` to `in`, never inside a quoted literal — and everything else must ' + + 'already be CEL: SQL `AND` / `OR` / `LIKE`, a subquery or a stray operator does not parse.', + RLS_UNCOMPILABLE_DROP, + RLS_DROPPED_USING, + RLS_DROPPED_CHECK, + 'A predicate that is flawless CEL but too large for a platform parse bound is `rls-predicate-over-budget` ' + + 'instead: the runtime refuses both the same way, and the fix differs.', + ], +}; + +const RLS_PREDICATE_OVER_BUDGET_EXPLANATION: RuleExplanation = { + rule: 'rls-predicate-over-budget', + covers: 'why an oversized predicate is dropped', + paragraphs: [ + 'The compiler parses a predicate under the platform\'s CEL bounds (`maxAstNodes` 256, `maxDepth` 32, ' + + '`maxListElements` 64 and the rest) and refuses one that overruns a bound exactly as it refuses a syntax ' + + 'error — on purpose, because the runtime\'s only decision is whether to drop the policy. This rule tells ' + + 'the two apart because the fix differs: there is no syntax or dialect error to correct, so the predicate ' + + 'must get smaller. The finding names the bound and its value; its `path` locates the predicate.', + RLS_UNCOMPILABLE_DROP, + RLS_DROPPED_USING, + RLS_DROPPED_CHECK, + ], +}; + +const RLS_PREDICATE_UNENFORCEABLE_EXPLANATION: RuleExplanation = { + rule: 'rls-predicate-unenforceable', + covers: 'what a predicate the runtime cannot enforce does', + paragraphs: [ + 'An RLS predicate is compiled to a row filter (ADR-0056 D4), so only the pushdown subset lowers. A ' + + 'function call (`size(…)`, `has(…)`), arithmetic, a ternary, a related-record path, a list literal under ' + + '`==` / `!=` or the bare `current_user` root used as a value is outside it, and the compiler refuses it. ' + + 'The finding quotes the verdict half of the compiler\'s own refusal.', + RLS_UNCOMPILABLE_DROP, + // The two key lists are held equal to the rule's per-key probe types by the rule's test. + 'A predicate can pass that shape check and still be refused for the TYPE of the value a `current_user` ' + + 'reference holds on every request. `ExecutionContext` declares, and the kernel resolves, the membership ' + + 'sets `accessible_org_ids`, `org_user_ids` and `positions` as LISTS and `email`, `id` and ' + + '`organization_id` as one value each, and `current_user` alone is the whole caller context object. A ' + + 'list under `==` / `!=` or handed to a string method, one value on the right of `in`, or the whole ' + + 'context object where one value or one set belongs is refused on EVERY request, so `RLSCompiler` drops ' + + 'the policy on every request. The ' + + 'shape check passes, so the "uncompilable predicate" WARN is never logged; the only signal is a ' + + 'per-request "DENY (fail closed)" WARN, emitted only when nothing else applicable compiles. A refusal that ' + + 'depends on WHICH caller asks — a constant comparison such as `current_user.email == \'ops@acme.com\'` — is ' + + 'not reported: no single probe can stand for every request.', + 'The same per-request drop, with the same signal, follows a lowered comparand the platform\'s shared filter ' + + 'check refuses — a `null` list member or a `null` ordering bound, which no two backends agree on: ' + + '`RLSCompiler` runs that check on every compiled policy filter before any backend sees it.', + RLS_DROPPED_USING, + RLS_DROPPED_CHECK, + LIST_HOLDING_COMPARISON, + CROSS_CLASS_COMPARISON, + 'Such a comparison is not dropped; it is refused where it runs. On a `using`, every read the policy scopes ' + + 'is refused on the SQL drivers (`INVALID_FILTER` / 400: driver-sql refuses the comparison by the columns\' ' + + 'declared types), and every by-id update or delete it scopes fails closed (`PERMISSION_DENIED` / 403). On ' + + 'a `check` — and on the `using` of an `insert`, `update` or `all` policy whenever no applicable policy for ' + + 'that operation declares a `check` (ADR-0058 D4), where the `using` is the write check — the in-process ' + + 'write check refuses the comparison too (`INVALID_FILTER` / 400) and stores nothing: against a list-holding ' + + 'column every single-record insert and by-id update whose record holds a list or an object there, across ' + + 'two classes every insert or update it judges.', + 'Last, the `using` of a `select` or `all` policy — the object\'s row-level READ SCOPE — is judged by the ' + + 'engine\'s own filter admission, which reads what the object\'s fields ARE: a text operator aimed at a ' + + 'number field, a date field compared with a value its storage cannot read, a filter on a formula field or ' + + 'a `{placeholder}` string (judged with no caller, so it never resolves) is refused there. The finding ' + + 'quotes the engine\'s verdict with its code and status. The analytics face hands the read scope to this ' + + 'same admission before it runs a query, so every analytics query over the object that this policy scopes ' + + 'is refused. Each clause earns one finding: the engine judges only a clause every earlier check left clean.', + ], +}; + +// This text was rewritten once, and why it was wrong is worth keeping: it said +// the field half had TWO directions — fail-closed in the leading position +// `extractTargetField` recognises, fail-OPEN everywhere else — and credited the +// WRITE leg's fail-closed to that same safety net. The runtime now judges +// column existence on the COMPILED predicate, inside `RLSCompiler.compileFilter`, +// which the read layer and the ADR-0058 D4 write gate both pass through, so a +// miss fails CLOSED everywhere, on both clauses; and `computeWriteCheckFilter` +// never had an `extractTargetField` net — which is why a negated `check` miss +// PERMITTED the write until the compiler-side guard landed. +const RLS_PREDICATE_UNKNOWN_FIELD_EXPLANATION: RuleExplanation = { + rule: 'rls-predicate-unknown-field', + covers: 'what a policy naming a missing column does', + paragraphs: [ + 'The predicate lowers, but a column it names is not declared on the policy\'s object — most often what a ' + + 'column RENAME leaves behind. `RLSCompiler` judges column existence on the COMPILED predicate, so the ' + + 'position and the polarity it is written in make no difference: a leading `field ==`, a negation ' + + '(`field != x`, `!(field == x)`, `!(field in [...])`) and any arm after the first all lower to the same ' + + 'tree and all DROP the policy at request time, with one WARN line as the only signal.', + RLS_DROPPED_USING, + 'The object then disappears not because its readers were denied, but because the narrowing they were ' + + 'granted names a column that is not there.', + RLS_DROPPED_CHECK, + 'On a runtime older than that compiled-predicate guard a `check` miss failed OPEN rather than closed: the ' + + 'write path had no field-existence check at all, so a negated miss was satisfied VACUOUSLY by the ' + + 'post-image and PERMITTED exactly the writes the policy was written to refuse, on every driver. Fix the ' + + 'name rather than relying on either behaviour.', + ], +}; + +const RLS_PREDICATE_UNKNOWN_USER_VARIABLE_EXPLANATION: RuleExplanation = { + rule: 'rls-predicate-unknown-user-variable', + covers: 'which current_user values a predicate can read', + paragraphs: [ + // Held equal to `RESERVED_RLS_MEMBERSHIP_KEYS` by the rule's test. + 'The kernel-resolved `current_user` keys are exactly accessible_org_ids, email, id, org_user_ids, ' + + 'organization_id, positions. The only other keys that can EVER appear are §7.3.1 membership sets, which ' + + 'the runtime stages as ARRAYS and which are therefore usable only as `field in current_user.KEY`. A key ' + + 'outside that list in a scalar position — compared with `==` / `!=` / `<` / `>` or handed to a string ' + + 'method — can therefore never be supplied by any request.', + 'The compiler answers `unresolved-variable` for it in EVERY position — including under `!` and in a ' + + 'trailing `||` arm — so `RLSCompiler` DROPS the policy at request time, and one WARN line is the only ' + + 'signal.', + RLS_DROPPED_USING, + 'The object then disappears not because its readers were denied, but because the narrowing they were ' + + 'granted resolves to nothing.', + RLS_DROPPED_CHECK, + 'An unknown key in a membership position (`field in current_user.KEY`) is never reported: there it is ' + + 'indistinguishable from a correct §7.3.1 reference an app stages.', + ], +}; + +/** What happens to a declared sharing rule whose `condition` does not lower — the two condition ids. */ +const SHARING_RULE_SKIPPED = + 'A declared sharing rule\'s `condition` is compiled ONCE, at boot: `bootstrapDeclaredSharingRules` lowers it ' + + 'to a static `criteria_json` filter with no variables bound. A condition that does not lower is not ' + + 'degraded, partially applied or deferred — the rule is SKIPPED: it is never written to `sys_sharing_rule`, ' + + 'no `sys_record_share` grant is ever materialised, and the only signal is one WARN line in the boot log. An ' + + 'unlowerable condition is never seeded as a permissive match-all (ADR-0049), so the rule is declared and ' + + 'grants nothing.'; + +/** Why a grant on the rule's anchor object can be refused outright — the two anchor ids. */ +const SHARING_INERT_GRANT = + '`SharingRuleService.reconcile` hands each matched record to `SharingService.grant`, whose ' + + '`assertNotInertGrant` pre-flight (ADR-0111 D7) refuses, with SHARING_NOT_ENABLED, a grant no access gate ' + + 'would ever consult. The refusal fails the rule\'s boot backfill and no `sys_record_share` row is ever ' + + 'written. The boot WARN it leaves appears only for a rule whose criteria matched at least one record, so a ' + + 'rule that matches nothing is just as dead and says nothing — which is why the anchor is judged here, on ' + + 'the declaration.'; + +const SHARING_RULE_UNLOWERABLE_CONDITION_EXPLANATION: RuleExplanation = { + rule: 'sharing-rule-unlowerable-condition', + covers: 'why a condition the runtime cannot evaluate grants nothing', + paragraphs: [ + 'A `condition` outside the pushdown subset — a function call such as `has(…)` (correct in an object ' + + 'validation, which is interpreted, and wrong here, where the condition is compiled), arithmetic, a ' + + 'ternary or a related-record path — does not lower. The finding quotes the compiler\'s own refusal.', + SHARING_RULE_SKIPPED, + LIST_HOLDING_COMPARISON, + CROSS_CLASS_COMPARISON, + 'Such a comparison lowers, so the rule IS seeded into `sys_sharing_rule`, but every criteria query it runs ' + + 'is refused on the SQL drivers (`INVALID_FILTER` / 400: driver-sql refuses the comparison by the ' + + 'columns\' declared types), and `SharingRuleService` reads a refused query as matching no record. No ' + + '`sys_record_share` grant is ever materialised, at boot or on any later write, and the only signal is a ' + + 'WARN line in the server log. The rule is declared and grants nothing.', + 'A condition that does not parse is not reported here: the expression rule gates the same field with a ' + + 'message written about syntax.', + ], +}; + +const SHARING_RULE_RUNTIME_VARIABLE_CONDITION_EXPLANATION: RuleExplanation = { + rule: 'sharing-rule-runtime-variable-condition', + covers: 'why a sharing condition cannot read current_user', + paragraphs: [ + 'A criteria sharing rule is MATERIALISED: the seeder compiles one static `criteria_json` per rule and the ' + + 'evaluator writes `sys_record_share` rows from it, so there is no "current user" at compile time and the ' + + 'compiler refuses a `current_user.*` read.', + SHARING_RULE_SKIPPED, + 'The fix is a different mechanism, not a different spelling, and which one depends on the anchor object\'s ' + + '`sharingModel`; the `fix:` line names both.', + ], +}; + +const SHARING_RULE_OBJECT_NOT_SHAREABLE_EXPLANATION: RuleExplanation = { + rule: 'sharing-rule-object-not-shareable', + covers: 'why a rule on a public object grants nothing', + paragraphs: [ + 'Sharing only ever WIDENS an organization-wide default (OWD) baseline, and on the widest baseline there is ' + + 'nothing left to widen. An object\'s effective sharing model is `public` when it declares ' + + '`sharingModel: \'public_read_write\'`, or when it declares none and is a system object (`isSystem: true` ' + + 'or a `sys_` name), which ADR-0090 D1 resolves to public. A custom object with no `sharingModel` resolves ' + + 'to private and is not reported.', + SHARING_INERT_GRANT, + 'Here the refusal reads "\'OBJECT\' is not under record-sharing enforcement". Measured: `buildReadFilter` ' + + 'returns `null` for such an object — NO record-level filter at all — so every principal already reads ' + + 'every row, and the rule advertises a restriction that does not exist.', + ], +}; + +const SHARING_RULE_OBJECT_CONTROLLED_BY_PARENT_EXPLANATION: RuleExplanation = { + rule: 'sharing-rule-object-controlled-by-parent', + covers: 'why a rule on a detail object grants nothing', + paragraphs: [ + 'A master-detail DETAIL (`sharingModel: \'controlled_by_parent\'`) has no record-level access of its own: ' + + 'its visibility is DERIVED from its master (ADR-0055), so it holds no shares to widen.', + SHARING_INERT_GRANT, + 'Here the refusal reads "\'OBJECT\' is controlled by its parent (master-detail); share the master record ' + + 'instead", and the recipients the rule names get whatever the MASTER grants them — which may be nothing. ' + + 'The grant is declared and does not exist.', + ], +}; + /** * Every rule explanation this package ships, keyed by rule id. `os explain * ` reads this table and nothing else. @@ -1100,6 +1376,15 @@ export const RULE_EXPLANATIONS: Readonly> = Obje [HOOK_API_UPDATE_READONLY_FIELD_EXPLANATION.rule]: HOOK_API_UPDATE_READONLY_FIELD_EXPLANATION, [HOOK_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION.rule]: HOOK_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION, [ACTION_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION.rule]: ACTION_API_UPDATE_READONLY_WHEN_FIELD_EXPLANATION, + [RLS_PREDICATE_UNPARSEABLE_EXPLANATION.rule]: RLS_PREDICATE_UNPARSEABLE_EXPLANATION, + [RLS_PREDICATE_OVER_BUDGET_EXPLANATION.rule]: RLS_PREDICATE_OVER_BUDGET_EXPLANATION, + [RLS_PREDICATE_UNENFORCEABLE_EXPLANATION.rule]: RLS_PREDICATE_UNENFORCEABLE_EXPLANATION, + [RLS_PREDICATE_UNKNOWN_FIELD_EXPLANATION.rule]: RLS_PREDICATE_UNKNOWN_FIELD_EXPLANATION, + [RLS_PREDICATE_UNKNOWN_USER_VARIABLE_EXPLANATION.rule]: RLS_PREDICATE_UNKNOWN_USER_VARIABLE_EXPLANATION, + [SHARING_RULE_UNLOWERABLE_CONDITION_EXPLANATION.rule]: SHARING_RULE_UNLOWERABLE_CONDITION_EXPLANATION, + [SHARING_RULE_RUNTIME_VARIABLE_CONDITION_EXPLANATION.rule]: SHARING_RULE_RUNTIME_VARIABLE_CONDITION_EXPLANATION, + [SHARING_RULE_OBJECT_NOT_SHAREABLE_EXPLANATION.rule]: SHARING_RULE_OBJECT_NOT_SHAREABLE_EXPLANATION, + [SHARING_RULE_OBJECT_CONTROLLED_BY_PARENT_EXPLANATION.rule]: SHARING_RULE_OBJECT_CONTROLLED_BY_PARENT_EXPLANATION, }); /** The explanation for `rule`, or `undefined` when the rule has none. Exact id match. */ diff --git a/packages/lint/src/validate-rls-predicate-enforceability.cross-class-field.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.cross-class-field.test.ts index 8d80f1a3f70..4d1c477f34e 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.cross-class-field.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.cross-class-field.test.ts @@ -24,12 +24,28 @@ import { describe, expect, it } from 'vitest'; import type { EngineFilterJudgement, EngineFilterJudgementOptions } from '@objectstack/spec/contracts'; import { - validateRlsPredicateEnforceability, + validateRlsPredicateEnforceability as validateRlsPredicateEnforceabilityUnrecorded, RLS_PREDICATE_UNENFORCEABLE, RLS_PREDICATE_UNKNOWN_FIELD, } from './validate-rls-predicate-enforceability.js'; import { runAuthoringRules } from './authoring-rules.js'; import { runRuntimeAuthoringRules } from './runtime-gate.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding is one verdict sentence; the reasoning it used to +// carry is the id's `os explain` entry. Every call below records what it +// fired, and the last case in this file holds each recorded verdict to one +// line of at most 200 characters. Run the whole file: that case reads what the +// cases above fired. +const fired: Array<{ rule: string; message: string }> = []; +const validateRlsPredicateEnforceability: typeof validateRlsPredicateEnforceabilityUnrecorded = (...args) => { + const findings = validateRlsPredicateEnforceabilityUnrecorded(...args); + fired.push(...findings); + return findings; +}; + +/** The `os explain` text of `rule`, one string. */ +const explanationOf = (rule: string): string => explainRule(rule)?.paragraphs.join('\n') ?? ''; const deal = { name: 'deal', @@ -90,40 +106,26 @@ const OPERATORS: ReadonlyArray<{ op: string; spell: (a: string, b: string) => st ]; /** - * The three measured cells, each against `status` (text). `declared` is what - * the finding says about the column at fault: for a cross-class pair BOTH - * columns and their classes, for a column with no class that column alone. + * The three measured cells, each against `status` (text). `side` is how the + * finding names the cell's column: its class, or why it has none — the two + * sides in the written order. */ const CELLS = [ - { - cell: 'text vs number', - field: 'amount', - declared: (statusFirst: boolean) => - statusFirst - ? "`status` is declared `type: 'text'`, compared as text and `amount` is declared `type: 'number'`, compared as a number" - : "`amount` is declared `type: 'number'`, compared as a number and `status` is declared `type: 'text'`, compared as text", - }, - { - cell: 'text vs a single image', - field: 'photo', - declared: () => "`photo` is declared `type: 'image'`, a file field, which no row filter can compare with another column", - }, - { - cell: 'text vs a formula field', - field: 'is_open', - declared: () => "`is_open` is declared `type: 'formula'`, a formula field, which has no stored column a row filter can read", - }, + { cell: 'text vs number', field: 'amount', side: 'a number' }, + { cell: 'text vs a single image', field: 'photo', side: 'a file field' }, + { cell: 'text vs a formula field', field: 'is_open', side: 'a formula field' }, ] as const; -const CLASS_SENTENCE = - 'Two columns are compared only within one comparison class — the class decides how their stored values order ' + - 'and equal, and across classes SQL and the in-memory evaluator answer differently — and a file field or a ' + - 'formula field has no class at all, so the platform defines no comparison between these columns'; +/** What the refusal costs, per clause — the verdict's closing clause. */ +const REFUSED = { + using: 'so the SQL drivers refuse every read it scopes (INVALID_FILTER / 400)', + check: 'so the write check refuses the writes it judges (INVALID_FILTER / 400)', +} as const; describe('validateRlsPredicateEnforceability — a field compared with a field of another class is REFUSED (#20347)', () => { for (const { clause, operation } of CLAUSES) { for (const { op, spell, quoted } of OPERATORS) { - for (const { cell, field, declared } of CELLS) { + for (const { cell, field, side } of CELLS) { for (const order of ['text first', 'text second'] as const) { const statusFirst = order === 'text first'; const [left, right] = statusFirst ? ['status', field] : [field, 'status']; @@ -135,9 +137,10 @@ describe('validateRlsPredicateEnforceability — a field compared with a field o ]); const [finding] = findings; expect(finding.where).toBe('permission set "sales" policy "p" on object "deal"'); - expect(finding.message).toContain( - `RLS ${clause} \`${predicate}\` lowers, but compares two fields that share no comparison class: ` + - `\`record.${left} ${quoted} record.${right}\`, where ${declared(statusFirst)}. ${CLASS_SENTENCE}: `, + const why = statusFirst ? `text vs ${side}` : `${side} vs text`; + expect(finding.message).toBe( + `RLS ${clause} compares \`record.${left} ${quoted} record.${right}\`, which no comparison class spans ` + + `(${why}), ${REFUSED[clause]}`, ); expect(finding.hint).toMatch(/^Compare a field only with a field of the same comparison class: /); }); @@ -150,18 +153,25 @@ describe('validateRlsPredicateEnforceability — a field compared with a field o expect(CLAUSES.length * OPERATORS.length * CELLS.length * 2).toBe(336); }); - it('states the consequence of its own clause — the refused read and the write check for `using`, the write for `check`', () => { + it('states the consequence of its own clause — the read for `using`, the write for `check`', () => { const using = validateRlsPredicateEnforceability(stackWith({ operation: 'select', using: 'record.status != record.amount' }))[0]; - expect(using.message).toContain( - 'every read this policy scopes is refused on the SQL drivers (`INVALID_FILTER` / 400: driver-sql refuses the ' + - "comparison by the two columns' declared types), and every by-id update or delete it scopes fails closed " + - '(`PERMISSION_DENIED` / 403).', - ); - expect(using.message).toContain('the same `using` is also the write check whenever no applicable policy'); + expect(using.message.endsWith(REFUSED.using)).toBe(true); const check = validateRlsPredicateEnforceability(stackWith({ operation: 'insert', check: 'record.status != record.amount' }))[0]; - expect(check.message).toContain('every insert or update it judges is refused and nothing is stored'); - expect(check.message).not.toContain('every read this policy scopes'); + expect(check.message.endsWith(REFUSED.check)).toBe(true); + expect(check.message).not.toContain('every read it scopes'); + }); + + it('`os explain` carries the class rule and both clauses\' measured consequence the verdict no longer states', () => { + const text = explanationOf(RLS_PREDICATE_UNENFORCEABLE); + expect(text).toContain( + 'Two columns are compared only within one comparison class — the class decides how their stored values ' + + 'order and equal, and across classes SQL and the in-memory evaluator answer differently', + ); + expect(text).toContain('no row filter can compare a file field with another column'); + expect(text).toContain('a formula field has no stored column a row filter can read'); + expect(text).toContain('every by-id update or delete it scopes fails closed (`PERMISSION_DENIED` / 403)'); + expect(text).toContain('across two classes every insert or update it judges'); }); it('the prescription names every class with the declared types it holds, read from the spec', () => { @@ -214,15 +224,13 @@ describe('validateRlsPredicateEnforceability — every class against every other it('a column compared with itself is judged by its own class: a number is comparable, a file field is not', () => { expect(validateRlsPredicateEnforceability(stackWith({ operation: 'select', using: 'record.amount == record.amount' }))).toEqual([]); const [finding] = validateRlsPredicateEnforceability(stackWith({ operation: 'select', using: 'record.photo == record.photo' })); - expect(finding.message).toContain( - "`record.photo == record.photo`, where `photo` is declared `type: 'image'`, a file field, which no row filter can compare with another column.", - ); + expect(finding.message).toContain('`record.photo == record.photo`, which no comparison class spans (a file field vs a file field),'); }); it('a registry-injected column is judged by the definition the registry provisions: `created_at` is a datetime', () => { const [finding] = validateRlsPredicateEnforceability(stackWith({ operation: 'select', using: 'record.status != record.created_at' })); expect(finding.rule).toBe(RLS_PREDICATE_UNENFORCEABLE); - expect(finding.message).toContain("`created_at` is declared `type: 'datetime'`, compared as a datetime"); + expect(finding.message).toContain('`record.status != record.created_at`, which no comparison class spans (text vs a datetime),'); expect(validateRlsPredicateEnforceability(stackWith({ operation: 'select', using: 'record.signed_at > record.created_at' }))).toEqual([]); }); }); @@ -248,16 +256,13 @@ describe('validateRlsPredicateEnforceability — same-class comparisons stay CLE }); describe('validateRlsPredicateEnforceability — the arm is the graph\'s, and reports once (#20347)', () => { - it('names every offending comparison of one clause in ONE finding', () => { + it('reports every offending comparison of one clause in ONE finding: the first quoted, the rest counted', () => { const findings = validateRlsPredicateEnforceability( stackWith({ operation: 'select', using: 'record.status != record.amount || record.close_date < record.signed_at' }), ); expect(findings).toHaveLength(1); expect(findings[0].message).toContain( - "class: `record.status != record.amount`, where `status` is declared `type: 'text'`, compared as text and " + - "`amount` is declared `type: 'number'`, compared as a number; `record.close_date < record.signed_at`, where " + - "`close_date` is declared `type: 'date'`, compared as a date and `signed_at` is declared `type: 'datetime'`, " + - 'compared as a datetime. ', + 'compares `record.status != record.amount`, which no comparison class spans (text vs a number) (and 1 more), ', ); }); @@ -265,8 +270,8 @@ describe('validateRlsPredicateEnforceability — the arm is the graph\'s, and re for (const predicate of ['record.status != record.tags', 'record.photo != record.tags', 'record.is_open == record.tags']) { const findings = validateRlsPredicateEnforceability(stackWith({ operation: 'select', using: predicate })); expect(findings, predicate).toHaveLength(1); - expect(findings[0].message, predicate).toContain('compares a field with a field that holds a list or an object'); - expect(findings[0].message, predicate).not.toContain('share no comparison class'); + expect(findings[0].message, predicate).toContain('`tags` holds a list or an object'); + expect(findings[0].message, predicate).not.toContain('which no comparison class spans'); } }); @@ -275,8 +280,8 @@ describe('validateRlsPredicateEnforceability — the arm is the graph\'s, and re stackWith({ operation: 'select', using: 'record.status != record.tags && record.status != record.amount' }), ); expect(findings.map((f) => f.rule)).toEqual([RLS_PREDICATE_UNENFORCEABLE, RLS_PREDICATE_UNENFORCEABLE]); - expect(findings[0].message).toContain('holds a list or an object: `record.status != record.tags`'); - expect(findings[1].message).toContain('share no comparison class: `record.status != record.amount`'); + expect(findings[0].message).toContain('compares `record.status != record.tags`, where `tags` holds a list or an object'); + expect(findings[1].message).toContain('compares `record.status != record.amount`, which no comparison class spans'); }); it('judges nothing the graph cannot answer: an object outside the stack, a field map it cannot read, `id`', () => { @@ -341,3 +346,17 @@ describe('validateRlsPredicateEnforceability — the arm is the graph\'s, and re expect(refused[0].message).toBe(cli[0].message); }); }); + +describe('[#22161] one-line verdicts', () => { + it('every verdict the cases above fired is one line of at most 200 characters', () => { + // The coverage control first: the cases above fired the cross-class and + // the list-holding verdicts, so the shape assertion cannot pass over an + // empty record. + expect(fired.some((f) => f.message.includes('which no comparison class spans'))).toBe(true); + expect(fired.some((f) => f.message.includes('holds a list or an object'))).toBe(true); + for (const f of fired) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); +}); diff --git a/packages/lint/src/validate-rls-predicate-enforceability.engine-judge.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.engine-judge.test.ts index 6e728dc1c5d..eedab8fecea 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.engine-judge.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.engine-judge.test.ts @@ -16,9 +16,25 @@ import { describe, expect, it } from 'vitest'; import type { EngineFilterJudgement, EngineFilterJudgementOptions } from '@objectstack/spec/contracts'; -import { validateRlsPredicateEnforceability, RLS_PREDICATE_UNENFORCEABLE } from './validate-rls-predicate-enforceability.js'; +import { + validateRlsPredicateEnforceability as validateRlsPredicateEnforceabilityUnrecorded, + RLS_PREDICATE_UNENFORCEABLE, +} from './validate-rls-predicate-enforceability.js'; import { AUTHORING_RULES, runAuthoringRules } from './authoring-rules.js'; import { runRuntimeAuthoringRules, runtimeAuthoringRulesFor } from './runtime-gate.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding is one verdict sentence; the reasoning it used to +// carry is the id's `os explain` entry. Every call below records what it +// fired, and the last case in this file holds each recorded verdict to one +// line of at most 200 characters. Run the whole file: that case reads what the +// cases above fired. +const fired: Array<{ rule: string; message: string }> = []; +const validateRlsPredicateEnforceability: typeof validateRlsPredicateEnforceabilityUnrecorded = (...args) => { + const findings = validateRlsPredicateEnforceabilityUnrecorded(...args); + fired.push(...findings); + return findings; +}; interface JudgeCall { object: string; @@ -78,7 +94,7 @@ describe('validateRlsPredicateEnforceability — the engine judge is an INPUT (# ]); }); - it('turns a refusal into ONE rls-predicate-unenforceable finding that quotes the engine verbatim', () => { + it('turns a refusal into ONE rls-predicate-unenforceable finding that quotes the engine\'s verdict', () => { const { judgeFilter } = recordingJudge(REFUSAL); const findings = validateRlsPredicateEnforceability(stackWith({ using: "amount.startsWith('5')" }), { judgeFilter, @@ -91,8 +107,24 @@ describe('validateRlsPredicateEnforceability — the engine judge is an INPUT (# where: 'permission set "sales" policy "p" on object "deal"', }); const refusal = REFUSAL as Extract; - expect(findings[0].message).toContain(`(${refusal.code} / ${refusal.status}): ${refusal.message}`); - expect(findings[0].message).toMatch(/^RLS using `amount\.startsWith\('5'\)` lowers, but the engine refuses/); + expect(findings[0].message).toBe(`RLS using (${refusal.code} / ${refusal.status}): ${refusal.message}`); + }); + + it('[#22161] quotes the verdict half of the engine\'s leading sentence, never its reasoning or remedy', () => { + const { judgeFilter } = recordingJudge({ + ok: false, + code: 'INVALID_FIELD', + status: 400, + message: + "find('deal') filters on 'amount', a virtual field — no driver materialises it. The predicate was not applied. " + + "Denormalise the value onto 'deal'.", + }); + const [finding] = validateRlsPredicateEnforceability(stackWith({ using: "amount.startsWith('5')" }), { judgeFilter }); + expect(finding.message).toBe("RLS using (INVALID_FIELD / 400): find('deal') filters on 'amount', a virtual field"); + // What the refusal costs is the `os explain` entry's. + expect(explainRule(RLS_PREDICATE_UNENFORCEABLE)?.paragraphs.join('\n')).toContain( + 'every analytics query over the object that this policy scopes is refused', + ); }); it('prescribes by the engine CODE: a placeholder is sent to `current_user`, a field to denormalisation', () => { @@ -212,3 +244,15 @@ describe('the judge reaches the rule through BOTH run signatures (#20158)', () = expect(result.errors.filter((f) => f.rule.startsWith('rls-predicate-'))).toEqual([]); }); }); + +describe('[#22161] one-line verdicts', () => { + it('every verdict the cases above fired is one line of at most 200 characters', () => { + // The coverage control first: the cases above fired the engine verdict, so + // the shape assertion cannot pass over an empty record. + expect(fired.some((f) => f.message.startsWith('RLS using (INVALID_FILTER / 400): '))).toBe(true); + for (const f of fired) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); +}); diff --git a/packages/lint/src/validate-rls-predicate-enforceability.list-holding-field.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.list-holding-field.test.ts index c495ca4fd3f..1d7165418bf 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.list-holding-field.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.list-holding-field.test.ts @@ -23,12 +23,28 @@ import { describe, expect, it } from 'vitest'; import type { EngineFilterJudgement, EngineFilterJudgementOptions } from '@objectstack/spec/contracts'; import { - validateRlsPredicateEnforceability, + validateRlsPredicateEnforceability as validateRlsPredicateEnforceabilityUnrecorded, RLS_PREDICATE_UNENFORCEABLE, RLS_PREDICATE_UNKNOWN_FIELD, } from './validate-rls-predicate-enforceability.js'; import { runAuthoringRules } from './authoring-rules.js'; import { runRuntimeAuthoringRules } from './runtime-gate.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding is one verdict sentence; the reasoning it used to +// carry is the id's `os explain` entry. Every call below records what it +// fired, and the last case in this file holds each recorded verdict to one +// line of at most 200 characters. Run the whole file: that case reads what the +// cases above fired. +const fired: Array<{ rule: string; message: string }> = []; +const validateRlsPredicateEnforceability: typeof validateRlsPredicateEnforceabilityUnrecorded = (...args) => { + const findings = validateRlsPredicateEnforceabilityUnrecorded(...args); + fired.push(...findings); + return findings; +}; + +/** The `os explain` text of `rule`, one string. */ +const explanationOf = (rule: string): string => explainRule(rule)?.paragraphs.join('\n') ?? ''; const deal = { name: 'deal', @@ -78,18 +94,20 @@ const OPERATORS: ReadonlyArray<{ op: string; spell: (a: string, b: string) => st ]; const COLUMNS = [ - { column: 'a json field', field: 'tags', declared: "`tags` is declared `type: 'json'`" }, - { column: 'a multiple lookup', field: 'reviewers', declared: "`reviewers` is declared `type: 'lookup'`, `multiple: true`" }, + { column: 'a json field', field: 'tags' }, + { column: 'a multiple lookup', field: 'reviewers' }, ] as const; -const CLASS_SENTENCE = - 'A column that holds a list or an object is not one comparable value, on either side of a field-to-field ' + - 'comparison, so the platform refuses the comparison instead of evaluating it'; +/** What the refusal costs, per clause — the verdict's closing clause. */ +const REFUSED = { + using: 'so the SQL drivers refuse every read it scopes (INVALID_FILTER / 400)', + check: 'so the write check refuses the writes it judges (INVALID_FILTER / 400)', +} as const; describe('validateRlsPredicateEnforceability — a field compared with a json / multiple field is REFUSED (#19886)', () => { for (const { clause, operation } of CLAUSES) { for (const { op, spell, quoted } of OPERATORS) { - for (const { column, field, declared } of COLUMNS) { + for (const { column, field } of COLUMNS) { for (const order of ['scalar first', 'list first'] as const) { const [left, right] = order === 'scalar first' ? ['status', field] : [field, 'status']; const predicate = spell(left, right); @@ -100,9 +118,9 @@ describe('validateRlsPredicateEnforceability — a field compared with a json / ]); const [finding] = findings; expect(finding.where).toBe('permission set "sales" policy "p" on object "deal"'); - expect(finding.message).toContain( - `RLS ${clause} \`${predicate}\` lowers, but compares a field with a field that holds a list or an ` + - `object: \`record.${left} ${quoted} record.${right}\`, where ${declared}. ${CLASS_SENTENCE}: `, + expect(finding.message).toBe( + `RLS ${clause} compares \`record.${left} ${quoted} record.${right}\`, where \`${field}\` holds a list ` + + `or an object, ${REFUSED[clause]}`, ); expect(finding.hint).toMatch(/^A field compared with a `json` or `multiple` field has no row-filter form/); }); @@ -115,21 +133,27 @@ describe('validateRlsPredicateEnforceability — a field compared with a json / expect(CLAUSES.length * OPERATORS.length * COLUMNS.length * 2).toBe(224); }); - it('states the consequence of its own clause — the read and the fail-closed write for `using`, the write for `check`', () => { + it('states the consequence of its own clause — the read for `using`, the write for `check`', () => { const using = validateRlsPredicateEnforceability(stackWith({ operation: 'select', using: 'record.status != record.tags' }))[0]; - expect(using.message).toContain( - 'every read this policy scopes is refused on the SQL drivers (`INVALID_FILTER` / 400: driver-sql refuses a ' + - 'cross-field comparison against such a column by its declared type), and every by-id update or delete it ' + - 'scopes fails closed (`PERMISSION_DENIED` / 403).', - ); - expect(using.message).toContain('the same `using` is also the write check whenever no applicable policy'); + expect(using.message.endsWith(REFUSED.using)).toBe(true); const check = validateRlsPredicateEnforceability(stackWith({ operation: 'insert', check: 'record.status != record.tags' }))[0]; - expect(check.message).toContain( - 'every single-record insert and by-id update whose record holds a list or an object in that column is ' + - 'refused (`INVALID_FILTER` / 400) and stores nothing', + expect(check.message.endsWith(REFUSED.check)).toBe(true); + expect(check.message).not.toContain('every read it scopes'); + }); + + it('`os explain` carries the class and both clauses\' measured consequence the verdict no longer states', () => { + const text = explanationOf(RLS_PREDICATE_UNENFORCEABLE); + expect(text).toContain( + 'is not one comparable value, on either side of a field-to-field comparison, so the platform refuses the ' + + 'comparison instead of evaluating it', + ); + expect(text).toContain('every by-id update or delete it scopes fails closed (`PERMISSION_DENIED` / 403)'); + expect(text).toContain('where the `using` is the write check'); + expect(text).toContain( + 'against a list-holding column every single-record insert and by-id update whose record holds a list or an ' + + 'object there', ); - expect(check.message).not.toContain('every read this policy scopes'); }); }); @@ -155,14 +179,14 @@ describe('validateRlsPredicateEnforceability — every declared list-or-object c ['multiple file', { type: 'file', multiple: true }, "`type: 'file'`, `multiple: true`"], ['multiple image', { type: 'image', multiple: true }, "`type: 'image'`, `multiple: true`"], ]; - for (const [label, def, declared] of LIST_HOLDING) { - it(`${label}: refused, naming the declaration`, () => { + for (const [label, def] of LIST_HOLDING) { + it(`${label}: refused, naming the column`, () => { const objects = [{ ...deal, fields: { ...deal.fields, subject: { label: 'Subject', ...def } } }, account]; const findings = validateRlsPredicateEnforceability( stackWith({ operation: 'select', using: 'record.status != record.subject' }, objects), ); expect(findings.map((f) => f.rule)).toEqual([RLS_PREDICATE_UNENFORCEABLE]); - expect(findings[0].message).toContain(`\`record.status != record.subject\`, where \`subject\` is declared ${declared}.`); + expect(findings[0].message).toContain('`record.status != record.subject`, where `subject` holds a list or an object,'); }); } }); @@ -208,21 +232,22 @@ describe('validateRlsPredicateEnforceability — the one-value spellings stay CL ); expect(findings.map((f) => f.rule)).toEqual([RLS_PREDICATE_UNENFORCEABLE]); expect(findings[0].message).not.toContain('holds a list or an object'); - expect(findings[0].message).toContain('share no comparison class'); + expect(findings[0].message).toContain('which no comparison class spans (text vs a file field)'); }); }); describe('validateRlsPredicateEnforceability — the arm is the graph\'s, and reports once (#19886)', () => { - it('names every offending comparison of one clause in ONE finding', () => { + it('reports every offending comparison of one clause in ONE finding: the first quoted, the rest counted', () => { const findings = validateRlsPredicateEnforceability( stackWith({ operation: 'select', using: 'record.status != record.tags || record.reviewers == record.tags' }), ); expect(findings).toHaveLength(1); expect(findings[0].message).toContain( - "object: `record.status != record.tags`, where `tags` is declared `type: 'json'`; " + - "`record.reviewers == record.tags`, where `reviewers` is declared `type: 'lookup'`, `multiple: true` and " + - "`tags` is declared `type: 'json'`. ", + 'compares `record.status != record.tags`, where `tags` holds a list or an object (and 1 more), ', ); + // Both columns of one comparison are named when both hold a list. + const both = validateRlsPredicateEnforceability(stackWith({ operation: 'select', using: 'record.reviewers == record.tags' })); + expect(both[0].message).toContain('where `reviewers` and `tags` hold a list or an object, '); }); it('judges nothing the graph cannot answer: an object outside the stack, a field map it cannot read', () => { @@ -279,3 +304,17 @@ describe('validateRlsPredicateEnforceability — the arm is the graph\'s, and re expect(refused[0].message).toBe(cli[0].message); }); }); + +describe('[#22161] one-line verdicts', () => { + it('every verdict the cases above fired is one line of at most 200 characters', () => { + // The coverage control first: the cases above fired the list-holding and + // the cross-class verdicts, so the shape assertion cannot pass over an + // empty record. + expect(fired.some((f) => f.message.includes('holds a list or an object'))).toBe(true); + expect(fired.some((f) => f.message.includes('which no comparison class spans'))).toBe(true); + for (const f of fired) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); +}); diff --git a/packages/lint/src/validate-rls-predicate-enforceability.test.ts b/packages/lint/src/validate-rls-predicate-enforceability.test.ts index da61febda2b..54772b062a6 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.test.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.test.ts @@ -6,7 +6,7 @@ import { compileCelToFilter, isSupportedRlsExpression, setCelPushdownLimitsModeF import { RESERVED_RLS_MEMBERSHIP_KEYS } from '@objectstack/spec/contracts'; import { - validateRlsPredicateEnforceability, + validateRlsPredicateEnforceability as validateRlsPredicateEnforceabilityUnrecorded, RLS_PREDICATE_UNENFORCEABLE, RLS_PREDICATE_UNPARSEABLE, RLS_PREDICATE_OVER_BUDGET, @@ -14,6 +14,30 @@ import { RLS_PREDICATE_UNKNOWN_USER_VARIABLE, } from './validate-rls-predicate-enforceability.js'; import { AUTHORING_RULES, runAuthoringRules } from './authoring-rules.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding of the five ids is one verdict sentence; the reasoning +// it used to carry is the id's `os explain` entry. Every call below records +// what it fired, and the last cases in this file hold each recorded verdict to +// one line of at most 200 characters — so the pin covers every firing variant +// this suite exercises, not a chosen few. Run the whole file: those cases read +// what the cases above fired. +const RLS_RULE_IDS: readonly string[] = [ + RLS_PREDICATE_UNENFORCEABLE, + RLS_PREDICATE_UNPARSEABLE, + RLS_PREDICATE_OVER_BUDGET, + RLS_PREDICATE_UNKNOWN_FIELD, + RLS_PREDICATE_UNKNOWN_USER_VARIABLE, +]; +const fired: Array<{ rule: string; message: string }> = []; +const validateRlsPredicateEnforceability: typeof validateRlsPredicateEnforceabilityUnrecorded = (...args) => { + const findings = validateRlsPredicateEnforceabilityUnrecorded(...args); + fired.push(...findings); + return findings; +}; + +/** The `os explain` text of `rule`, one string. */ +const explanationOf = (rule: string): string => explainRule(rule)?.paragraphs.join('\n') ?? ''; const ids = (stack: unknown) => validateRlsPredicateEnforceability(stack).map((f) => f.rule); @@ -56,10 +80,12 @@ describe('validateRlsPredicateEnforceability — predicates the runtime can only path: 'permissions[0].rowLevelSecurity[0].using', where: 'permission set "sales_rep" policy "own_leads" on object "lead"', }); - // The message must say what the runtime DOES, not merely "unsupported". - expect(findings[0].message).toMatch(/DROPS the policy at request time/); - expect(findings[0].message).toMatch(/RLS_DENY_FILTER/); - expect(findings[0].message).toMatch(/ZERO rows/); + // The verdict says what the runtime DOES, not merely "unsupported"… + expect(findings[0].message).toMatch(/^RLS using is not lowerable \(.+\), so the policy is dropped and grants no access$/); + // …and `os explain` says how: the drop, the deny sentinel, the zero rows. + expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch(/DROPS a policy/); + expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch(/RLS_DENY_FILTER/); + expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch(/ZERO rows/); // …and prescribe the fix that works on THIS surface. expect(findings[0].hint).toMatch(/field != null/); expect(findings[0].hint).toMatch(/INTERPRETED/); @@ -93,9 +119,10 @@ describe('validateRlsPredicateEnforceability — predicates the runtime can only path: 'permissions[0].rowLevelSecurity[0].check', }); // ADR-0058 D4: the post-image check becomes the deny sentinel → every write denied. - expect(findings[0].message).toMatch(/PermissionDeniedError/); - expect(findings[0].message).toMatch(/blanket refusal/); - expect(findings[0].message).not.toMatch(/ZERO rows/); + expect(findings[0].message).toMatch(/so the policy is dropped and admits no write$/); + expect(findings[0].message).not.toMatch(/grants no access/); + expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch(/PermissionDeniedError/); + expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch(/blanket refusal/); }); it('reports `using` and `check` on one policy separately', () => { @@ -383,12 +410,13 @@ describe('validateRlsPredicateEnforceability — a bounds overrun is its own id 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); + // The verdict is unchanged, so the consequence must still be there. + expect(findings[0].message).toMatch(/so the policy is dropped and grants no access$/); + expect(explanationOf(RLS_PREDICATE_OVER_BUDGET)).toMatch(/ZERO rows/); + // An over-budget predicate is long by definition — the finding does not + // echo it; its `path` locates it. + expect(findings[0].message).not.toContain('record.f0 == 0'); + expect(findings[0].message.length).toBeLessThanOrEqual(200); }); it('prescribes shrinking, and never sends the author to check their dialect', () => { @@ -432,8 +460,8 @@ describe('validateRlsPredicateEnforceability — a bounds overrun is its own id rule: RLS_PREDICATE_OVER_BUDGET, path: 'permissions[0].rowLevelSecurity[0].check', }); - expect(f.message).toMatch(/PermissionDeniedError/); - expect(f.message).not.toMatch(/ZERO rows/); + expect(f.message).toMatch(/so the policy is dropped and admits no write$/); + expect(f.message).not.toMatch(/grants no access/); }); // ── The discrimination, which IS the card ───────────────────────── @@ -630,13 +658,15 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not // polarity are normalised away before the check runs. An author told "one // of these directions is fail-OPEN" would now harden against a hole that // no longer exists, and would not fix the name. - expect(f.message).not.toMatch(/fail-OPEN/); - expect(f.message).toMatch(/judges column existence on the COMPILED predicate/); - expect(f.message).toMatch(/the position and the polarity you wrote it in make no difference/); + // [#22161] The account of WHY is `os explain`'s now; it is pinned there. + const why = explanationOf(RLS_PREDICATE_UNKNOWN_FIELD); + expect(why).not.toMatch(/fail-OPEN/); + expect(why).toMatch(/judges column existence on the COMPILED predicate/); + expect(why).toMatch(/the position and the polarity it is written in make no difference/); // …and every shape the old text split across two directions is named in // the one direction, so an author recognises their own predicate in it. - expect(f.message).toMatch(/`field != x`/); - expect(f.message).toMatch(/any arm after the first/); + expect(why).toMatch(/`field != x`/); + expect(why).toMatch(/any arm after the first/); // ⛔ …and NOT by citing a tracker id. This string reaches authors, // operators and generated surfaces, none of whom can resolve `#NNNN` // (`check:doc-authoring`); the id lives in the adjacent `//` comment, which @@ -644,19 +674,21 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not // author does not re-add it and learn this from CI instead. expect(f.message).not.toMatch(/#\d{3,}/); expect(f.hint).not.toMatch(/#\d{3,}/); + expect(why).not.toMatch(/#\d{3,}/); // the cost, which is the same cost the variable half carries - expect(f.message).toMatch(/DROP the policy at request time/); - expect(f.message).toMatch(/RLS_DENY_FILTER/); - expect(f.message).toMatch(/ZERO rows/); - expect(f.message).toMatch(/DISAPPEARS for every holder of this permission set/); + expect(f.message).toMatch(/The policy is dropped and grants no access\.$/); + expect(why).toMatch(/DROP the policy at request time/); + expect(why).toMatch(/RLS_DENY_FILTER/); + expect(why).toMatch(/ZERO rows/); + expect(why).toMatch(/disappears for every holder of the permission set/); // ⛔ …and NOT the three claims the rewritten text retired. The // cross-tenant sentence went with them: it was there to bound a leak - // reading that the message no longer makes, and a denial needs no such + // reading that the text no longer makes, and a denial needs no such // disclaimer. Overstating the old defect was the risk; restating a bound // on a defect the text does not describe is just noise. - expect(f.message).not.toMatch(/leaves the policy KEPT/); - expect(f.message).not.toMatch(/DEFEATED/); - expect(f.message).not.toMatch(/driver-sql is NOT MEASURED/); + expect(why).not.toMatch(/leaves the policy KEPT/); + expect(why).not.toMatch(/DEFEATED/); + expect(why).not.toMatch(/driver-sql is NOT MEASURED/); // …and the miss itself, with the platform's own "did you mean". expect(f.message).toMatch(/"is_private_nope" is not a field on object "crm_opportunity"/); expect(f.message).toMatch(/Did you mean "is_private"\?/); @@ -669,13 +701,14 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not const [f] = validateRlsPredicateEnforceability(siteWith('using', 'owner_id == current_user.nope')); expect(f.rule).toBe(RLS_PREDICATE_UNKNOWN_USER_VARIABLE); expect(f.message).toMatch(/reads `current_user\.nope`, which nothing pre-resolves/); - expect(f.message).toMatch(/scalar position, so no request can ever supply it/); + expect(f.message).toMatch(/in a scalar position no request can ever fill, so the policy is dropped/); // The variable half really is fail-closed in EVERY position — the compiler // refuses it under `!` and in a trailing `||` arm alike — so unlike the // field half it may say so without qualification. - expect(f.message).toMatch(/unresolved-variable` in EVERY position/); - expect(f.message).toMatch(/DISAPPEARS for every holder of this permission set/); - expect(f.message).not.toMatch(/fail-OPEN/); + const why = explanationOf(RLS_PREDICATE_UNKNOWN_USER_VARIABLE); + expect(why).toMatch(/unresolved-variable` for it in EVERY position/); + expect(why).toMatch(/disappears for every holder of the permission set/); + expect(why).not.toMatch(/fail-OPEN/); expect(f.hint).toMatch(/IRlsMembershipResolver/); expect(f.hint).toMatch(/never compared with `==`/); }); @@ -684,17 +717,19 @@ describe('validateRlsPredicateEnforceability — the messages name the COST, not const [f] = validateRlsPredicateEnforceability(siteWith('check', 'nope_field == 1')); expect(f.rule).toBe(RLS_PREDICATE_UNKNOWN_FIELD); expect(f.path).toBe('permissions[0].rowLevelSecurity[0].check'); - expect(f.message).toMatch(/PermissionDeniedError/); + expect(f.message).toMatch(/The policy is dropped and admits no write\.$/); // ⚠️ The write leg says the SAME thing the read leg does — one direction — // and it is the leg whose old text was not merely stale but misattributed: // it credited a fail-closed to the `extractTargetField` safety net, and // `computeWriteCheckFilter` never had one. The vacuous-permit sentence - // survives as an explicit statement about an OLDER runtime, so an operator - // reading this against a deployment that predates the guard is not told the - // wrong thing. - expect(f.message).toMatch(/On a runtime older than that guard this clause failed OPEN/); - expect(f.message).toMatch(/PERMITTED exactly the writes the policy was written to refuse/); - expect(f.message).not.toMatch(/select \/ update \/ delete matches ZERO/); + // survives in `os explain` as an explicit statement about an OLDER runtime, + // so an operator reading it against a deployment that predates the guard + // is not told the wrong thing. + const why = explanationOf(RLS_PREDICATE_UNKNOWN_FIELD); + expect(why).toMatch(/PermissionDeniedError/); + expect(why).toMatch(/On a runtime older than that compiled-predicate guard a `check` miss failed OPEN/); + expect(why).toMatch(/PERMITTED exactly the writes the policy was written to refuse/); + expect(f.message).not.toMatch(/ZERO/); }); }); @@ -895,23 +930,37 @@ describe('validateRlsPredicateEnforceability — the refusals the SHAPE check ca it('a TYPE fault names the reference, what it holds, and a DROP with the clause’s own consequence', () => { const [using] = validateRlsPredicateEnforceability(siteWith('using', TYPE_FAULTS[0][0])); - expect(using.message).toContain('`current_user.org_user_ids` is a membership set'); - expect(using.message).toMatch(/DROPS the policy on EVERY request/); - expect(using.message).toMatch(/RLS_DENY_FILTER/); + expect(using.message).toBe( + 'RLS using: `current_user.org_user_ids` is a membership set, a list on every request, used where one value ' + + 'belongs, so the policy is dropped and grants no access', + ); const [check] = validateRlsPredicateEnforceability(siteWith('check', 'record.assigned_to_id in current_user.id')); - expect(check.message).toContain('`current_user.id` holds ONE value'); - expect(check.message).toMatch(/PermissionDeniedError/); + expect(check.message).toBe( + 'RLS check: `current_user.id` holds ONE value on every request, used where a set belongs, so the policy is ' + + 'dropped and admits no write', + ); + + const [root] = validateRlsPredicateEnforceability(siteWith('using', 'record.assigned_to_id in current_user')); + expect(root.message).toContain('`current_user` alone is the whole caller context object'); + + const why = explanationOf(RLS_PREDICATE_UNENFORCEABLE); + expect(why).toMatch(/drops the policy on every request/); + expect(why).toMatch(/per-request "DENY \(fail closed\)" WARN/); + expect(why).toMatch(/RLS_DENY_FILTER/); + expect(why).toMatch(/PermissionDeniedError/); }); it('a NULL comparand is reported as a DROP with the clause’s own consequence, as the runtime drops it', () => { const [using] = validateRlsPredicateEnforceability(siteWith('using', NULL_COMPARANDS[2][0])); - expect(using.message).toMatch(/DROPS the policy on EVERY request/); - expect(using.message).toMatch(/RLS_DENY_FILTER/); + expect(using.message).toMatch(/^RLS using lowers to a refused comparand \(Operator "\$in" on field "status" /); + expect(using.message).toMatch(/so the policy is dropped and grants no access$/); const [check] = validateRlsPredicateEnforceability(siteWith('check', NULL_COMPARANDS[2][0])); - expect(check.message).toMatch(/DROPS the policy on EVERY request/); - expect(check.message).toMatch(/PermissionDeniedError/); + expect(check.message).toMatch(/so the policy is dropped and admits no write$/); + expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toMatch( + /runs that check on every compiled policy filter before any backend sees it/, + ); }); it('rewrites EVERY site the fault lands in, in one finding', () => { @@ -1120,3 +1169,50 @@ describe('validateRlsPredicateEnforceability — the field set is read off the C ]); }); }); + +describe('[#22161] one-line verdicts — the five ids', () => { + it('every verdict the cases above fired is one line of at most 200 characters', () => { + // The coverage control first: each id fired at least once, so the shape + // assertion below cannot pass over an empty record. + expect([...new Set(fired.map((f) => f.rule))].sort()).toEqual([...RLS_RULE_IDS].sort()); + for (const f of fired) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); + + // What each verdict stopped saying, which `os explain RULE_ID` now prints. + const MOVED: Record = { + [RLS_PREDICATE_UNENFORCEABLE]: ['pushdown subset', 'One WARN line', 'ZERO rows', 'single-record INSERT check', 'PERMISSION_DENIED', 'analytics query'], + [RLS_PREDICATE_UNPARSEABLE]: ['legacy SQL bridge', '`IN` to `in`', 'ZERO rows', 'PermissionDeniedError', 'rls-predicate-over-budget'], + [RLS_PREDICATE_OVER_BUDGET]: ['maxAstNodes', 'no syntax or dialect error', 'ZERO rows', 'PermissionDeniedError'], + [RLS_PREDICATE_UNKNOWN_FIELD]: ['RENAME', 'COMPILED predicate', 'ZERO rows', 'failed OPEN'], + [RLS_PREDICATE_UNKNOWN_USER_VARIABLE]: ['§7.3.1 membership sets', 'ARRAYS', 'EVERY position', 'never reported'], + }; + + it('covers exactly the five ids', () => { + expect(Object.keys(MOVED).sort()).toEqual([...RLS_RULE_IDS].sort()); + }); + + it.each([...RLS_RULE_IDS])('`os explain %s` carries what its verdict no longer says', (rule) => { + const explanation = explainRule(rule); + expect(explanation, `no \`os explain ${rule}\` entry`).toBeDefined(); + const text = explanation!.paragraphs.join('\n'); + for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); + }); + + it('the explanations name exactly the kernel-resolved `current_user` keys, each with its runtime type', () => { + // The explanation module imports nothing, so the keys are written out + // there; this holds them to the contract and to this file's type table. + expect(explanationOf(RLS_PREDICATE_UNKNOWN_USER_VARIABLE)).toContain( + `The kernel-resolved \`current_user\` keys are exactly ${[...RESERVED_RLS_MEMBERSHIP_KEYS].sort().join(', ')}.`, + ); + const spell = (keys: string[]) => + keys.sort().map((k) => `\`${k}\``).reduce((acc, k, i, all) => (i === 0 ? k : `${acc}${i === all.length - 1 ? ' and ' : ', '}${k}`), ''); + const ofType = (type: 'scalar' | 'array') => + Object.entries(KERNEL_KEY_RUNTIME_TYPE).filter(([, t]) => t === type).map(([k]) => k); + expect(explanationOf(RLS_PREDICATE_UNENFORCEABLE)).toContain( + `the membership sets ${spell(ofType('array'))} as LISTS and ${spell(ofType('scalar'))} as one value each`, + ); + }); +}); diff --git a/packages/lint/src/validate-rls-predicate-enforceability.ts b/packages/lint/src/validate-rls-predicate-enforceability.ts index bd1c7348ac9..36a07968cfa 100644 --- a/packages/lint/src/validate-rls-predicate-enforceability.ts +++ b/packages/lint/src/validate-rls-predicate-enforceability.ts @@ -197,8 +197,11 @@ * defect earns one finding. * * The finding keeps the id {@link RLS_PREDICATE_UNENFORCEABLE} and quotes the - * engine's message verbatim, with its code and status: at authoring time the - * text is the author's own, so nothing is withheld. + * engine's verdict with its code and status: at authoring time the text is the + * author's own, so nothing is withheld. [#22161] It quotes the verdict half of + * the engine's leading sentence ({@link refusalVerdict}), as it does for the + * shared filter check; the engine's reasoning and remedy are the `os explain` + * entry's and the hint's. * * ## A field compared with a field that holds a list or an object (#19886) * @@ -355,78 +358,43 @@ function boundsOverrunOf(bridged: string): CelBoundsOverrun | null { 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; -} +/* ──────────────────────────────────────────────────────────────────────────── + * [#22161] The verdict clauses. Every finding of the five ids prints one + * verdict sentence; what a dropped policy, a refused comparison or an engine + * refusal does at request time — and the measurements behind it — is each id's + * `os explain` entry (`rule-explanations.ts`, the "Row-level security and + * sharing rules" block). The finding does not echo the predicate: its `path` + * names the clause, and the predicate is author text of any length. + * ──────────────────────────────────────────────────────────────────────────── */ -/** - * The INSERT half of a dropped `using`, appended to every `using` consequence - * below. The ADR-0058 D4 write check takes its set from `writeCheckPolicies`: - * when no applicable policy for the insert declares a `check`, every applicable - * policy's `using` is compiled as its check, so the same drop reaches the - * single-record insert too. Measured through the real `SecurityPlugin` on an - * `insert` and an `all` policy, for all three kinds of drop (an unlowerable shape, an unresolved - * `current_user.*`, an undeclared column): with nothing else compiling in that - * set, every single-record insert is refused (403); with another applicable - * policy's `using` compiling, that one alone decides; with a declared `check` - * beside it, the declared check alone decides and this `using` takes no part. - */ -const USING_INSERT_CONSEQUENCE = - ' On an `insert` or `all` policy the same `using` is also the single-record INSERT check ' + - 'whenever no applicable policy for the insert declares a `check` (ADR-0058 D4): when nothing ' + - 'else in that set compiles, every single-record insert it governs fails with ' + - "`PermissionDeniedError`; when another policy's `using` compiles, that one alone decides the insert."; +/** What a dropped policy no longer does, per clause. */ +const DROPPED_OUTCOME: Readonly> = { + using: 'grants no access', + check: 'admits no write', +}; -/** - * Qualifies every `check` consequence below. The write check OR-combines the - * declared checks of all the applicable policies for the operation, so a - * dropped `check` is a blanket refusal only when no other declared `check` in - * that set compiles (measured: beside a compiling declared `check`, that one - * alone decided; beside a USING-only sibling, every single-record insert was - * refused, because a USING-only sibling takes no part once any policy declares - * a `check`). - */ -const CHECK_SET_QUALIFIER = - ' That holds when no other applicable policy for the operation declares a `check` that compiles; ' + - 'when one does, that `check` alone decides and this one contributes nothing.'; +/** The clause that closes every verdict whose policy the runtime drops. */ +function droppedClause(clause: 'using' | 'check'): string { + return `so the policy is dropped and ${DROPPED_OUTCOME[clause]}`; +} -/** How an UNCOMPILABLE predicate is dropped: the runtime's own WARN names the predicate. */ -const DROPPED_UNCOMPILABLE = - 'so `RLSCompiler` DROPS the policy at request time (one WARN line — "has an uncompilable predicate ' + - '… and was DROPPED (no enforcement)" — is the only signal, and nothing reports it at authoring time). '; +/** The same outcome as a sentence, after a verdict that ends one of its own. */ +function droppedSentence(clause: 'using' | 'check'): string { + return `The policy is dropped and ${DROPPED_OUTCOME[clause]}.`; +} /** - * How a predicate the compiler refuses PER REQUEST is dropped. The shape check - * `compileFilter` consults passes (the refused value exists only per request), - * so it never logs the "uncompilable predicate" line: it collects the policy as - * that request's denial and WARNs only when that denial is what the request gets. + * The verdict half of a quoted refusal: the text up to the first ` — `, where + * the producer's own elaboration or suggested rewrite begins. That half is the + * `fix:` line's and the `os explain` entry's job, and keeping it would carry + * the verdict past one line. A refusal with no ` — ` is quoted whole. + * + * Exported for `validate-sharing-rule-enforceability.ts`, which quotes the + * same compiler. */ -const DROPPED_EVERY_REQUEST = - 'so `RLSCompiler` DROPS the policy on EVERY request (a request that resolves no value drops it too). The ' + - 'shape check passes, so the "uncompilable predicate" WARN is never logged; the only signal is a per-request ' + - '"DENY (fail closed)" WARN, emitted only when nothing else applicable compiles, and nothing reports it at ' + - 'authoring time. '; - -/** What the runtime does with a predicate it cannot compile, per clause. */ -function consequence(clause: 'using' | 'check', dropped: string = DROPPED_UNCOMPILABLE): string { - return clause === 'using' - ? dropped + - 'When it is the only applicable policy for that object and operation, `compileFilter` returns the ' + - '`RLS_DENY_FILTER` sentinel instead, which is AND-ed onto the where clause: every select / update / ' + - 'delete on the object matches ZERO rows. When other policies also apply, this one just vanishes ' + - 'from the OR and grants none of the access it appears to.' + - USING_INSERT_CONSEQUENCE - : dropped + - 'On the ADR-0058 D4 write path that leaves the post-image `check` as the `RLS_DENY_FILTER` ' + - 'sentinel, which no record can satisfy: every single-record insert and by-id update the policy ' + - 'governs fails with `PermissionDeniedError`. The policy reads as a write rule and behaves as a ' + - 'blanket refusal.' + - CHECK_SET_QUALIFIER; +export function refusalVerdict(detail: string): string { + const cut = detail.indexOf(' — '); + return cut === -1 ? detail : detail.slice(0, cut); } /* ──────────────────────────────────────────────────────────────────────────── @@ -899,15 +867,16 @@ function typeFaultRewrite(fault: TypeFault, site: LoweredSite): string { return `rewrite the comparison on \`${f}\` so \`${v}\` sits where its value fits`; } -const HOLDING_SENTENCE: Readonly string>> = { - list: (v) => - `\`${v}\` is a membership set: \`ExecutionContext\` declares it, and the kernel resolves it, as a LIST on ` + - 'every request, so no request can ever put one value in this position.', - 'one value': (v) => - `\`${v}\` holds ONE value: \`ExecutionContext\` declares it, and the kernel resolves it, as a scalar on ` + - 'every request, so no request can ever put a list in this position.', +/** + * What the reference holds and where it was used — the type-fault verdict. + * That `ExecutionContext` declares, and the kernel resolves, each kernel key + * with this type on every request is the `os explain` entry's. + */ +const HOLDING_VERDICT: Readonly string>> = { + list: (v) => `\`${v}\` is a membership set, a list on every request, used where one value belongs`, + 'one value': (v) => `\`${v}\` holds ONE value on every request, used where a set belongs`, 'context object': () => - '`current_user` alone is the whole caller context object on every request, never one value and never one set.', + '`current_user` alone is the whole caller context object, used where one value or one set belongs', }; /** The one-sentence rule each holding breaks, leading its hint. */ @@ -998,19 +967,6 @@ function firstSentence(message: string): string { return cut === -1 ? message : message.slice(0, cut); } -/** - * How a face-refused comparand is dropped at request time. [#20212] - * `RLSCompiler.compileFilter` runs the same two faces on every compiled policy - * filter, for `using` and `check` alike, so the policy joins the per-request - * denial route before any backend or the in-process `check` evaluator sees the - * comparand: the same drop, WARN and per-clause consequence as a TYPE fault. - */ -const DROPPED_FACE_REFUSED = - 'so `RLSCompiler` DROPS the policy on EVERY request: it runs that same check on every compiled policy ' + - 'filter, before any backend sees it. The shape check passes, so the "uncompilable predicate" WARN is never ' + - 'logged; the only signal is a per-request "DENY (fail closed)" WARN, emitted only when nothing else ' + - 'applicable compiles. '; - /** * The lowered field-to-field operators, back to the CEL operator an author * writes. They are the six `cel-to-filter.ts` emits with a `{ $field }` @@ -1024,24 +980,21 @@ const FIELD_COMPARISON_SYMBOL: ReadonlyMap = new Map([ ]); /** - * The declaration that makes a field hold a list or an object, spelled as the - * author wrote it, or `null` when the field holds one value (see this file's - * header for the two spec classes). + * Is the field declared to hold a list or an object (see this file's header + * for the two spec classes)? */ -function listHoldingDeclaration(meta: GraphField | undefined): string | null { +function holdsListOrObject(meta: GraphField | undefined): boolean { const type = meta?.type; - if (!type) return null; - if (STRUCTURED_JSON_TYPES.has(type)) return `\`type: '${type}'\``; - if (!isMultiValueField({ type, multiple: meta.multiple === true })) return null; - return meta.multiple === true ? `\`type: '${type}'\`, \`multiple: true\`` : `\`type: '${type}'\``; + if (!type) return false; + return STRUCTURED_JSON_TYPES.has(type) || isMultiValueField({ type, multiple: meta.multiple === true }); } /** One lowered comparison between two columns, at least one of which holds a list or an object. */ export interface ListHoldingComparison { /** The comparison as the author wrote it, back in CEL. */ written: string; - /** Each list-holding column, with what it is declared as. */ - columns: string[]; + /** The column(s) of the comparison declared to hold a list or an object. */ + holders: string[]; } /** @@ -1066,39 +1019,49 @@ export function listHoldingComparisons( typeof (operand as Record).$field === 'string'); for (const site of sites) { const referenced = (site.operand as { $field: string }).$field; - const columns: string[] = []; + const holders: string[] = []; for (const name of new Set([site.field, referenced])) { const verdict = resolveFieldPath(graph, object, name); - const held = verdict?.kind === 'ok' ? listHoldingDeclaration(verdict.meta) : null; - if (held) columns.push(`\`${name}\` is declared ${held}`); + if (verdict?.kind === 'ok' && holdsListOrObject(verdict.meta)) holders.push(name); } - if (columns.length === 0) continue; + if (holders.length === 0) continue; const written = `record.${site.field} ${FIELD_COMPARISON_SYMBOL.get(site.op)} record.${referenced}`; - found.set(written, { written, columns }); + found.set(written, { written, holders }); } return [...found.values()]; } +/** The comparisons after the first, which a verdict counts rather than quotes. */ +function moreComparisons(count: number): string { + return count > 1 ? ` (and ${count - 1} more)` : ''; +} + /** - * What a comparison against a list-holding column does at request time, per - * clause. Measured through the real plugin-security on driver-sql, every - * clause and operation: see this file's header. + * [#22161] The list-holding verdict, after its subject: the first comparison, + * the column(s) in it that hold a list or an object, and a count of the rest. + * Why such a column is not one comparable value, and what the refusal costs, + * is the `os explain` entry's. Shared with the sharing-rule arm. */ -function listHoldingConsequence(clause: 'using' | 'check'): string { - const write = - 'every single-record insert and by-id update whose record holds a list or an object in that column is ' + - 'refused (`INVALID_FILTER` / 400) and stores nothing, because the write check compares one value with ' + - 'one value and will not guess what a list means'; - return clause === 'using' - ? 'every read this policy scopes is refused on the SQL drivers (`INVALID_FILTER` / 400: driver-sql ' + - 'refuses a cross-field comparison against such a column by its declared type), and every by-id ' + - 'update or delete it scopes fails closed (`PERMISSION_DENIED` / 403). On an `insert`, `update` or ' + - '`all` policy the same `using` is also the write check whenever no applicable policy for that ' + - `operation declares a \`check\` (ADR-0058 D4): then ${write}.` - : `${write}. The policy reads as a comparison and behaves as a refusal of every write that leaves a list ` + - 'or an object in that column.'; +export function listHoldingVerdict(comparisons: readonly ListHoldingComparison[]): string { + const [first] = comparisons; + const holders = first.holders.map((name) => `\`${name}\``).join(' and '); + return ( + `compares \`${first.written}\`, where ${holders} ${first.holders.length > 1 ? 'hold' : 'holds'} a list ` + + `or an object${moreComparisons(comparisons.length)}` + ); } +/** + * What a refused field-to-field comparison costs, per clause — the list-holding + * and the cross-class arm alike. Measured through the real plugin-security on + * driver-sql, every clause and operation (this file's header); the full account + * is the `os explain` entry's. + */ +const REFUSED_COMPARISON: Readonly> = { + using: 'so the SQL drivers refuse every read it scopes (INVALID_FILTER / 400)', + check: 'so the write check refuses the writes it judges (INVALID_FILTER / 400)', +}; + /* ──────────────────────────────────────────────────────────────────────────── * #20347 — a field compared with a field of ANOTHER comparison class (see the * header). The classification is the spec's; this block only names it. @@ -1114,10 +1077,10 @@ const CLASS_PHRASE: Readonly> = { time: 'a time of day', }; -/** Why a single-valued column has no class, as a sentence (a list or an object is the arm above's). */ -const NO_CLASS_PHRASE: Readonly> = { - file: 'a file field, which no row filter can compare with another column', - formula: 'a formula field, which has no stored column a row filter can read', +/** A single-valued column with no class, as a noun (a list or an object is the arm above's). */ +const NO_CLASS_NOUN: Readonly> = { + file: 'a file field', + formula: 'a formula field', }; /** @@ -1143,19 +1106,18 @@ function declaredColumn( return { name, type: verdict.meta.type, multiple: verdict.meta.multiple === true, meta: verdict.meta }; } -/** One column's part in a refused comparison, as the author declared it. */ -function describeColumn(column: { name: string; type: string }, verdict: CrossFieldColumnVerdict): string { - const declared = `\`${column.name}\` is declared \`type: '${column.type}'\``; - if (verdict.kind === 'class') return `${declared}, compared as ${CLASS_PHRASE[verdict.class]}`; - return verdict.reason === 'list-or-object' ? declared : `${declared}, ${NO_CLASS_PHRASE[verdict.reason]}`; +/** One side of a refused comparison: its class, or why it has none. */ +function sidePhrase(verdict: CrossFieldColumnVerdict): string { + if (verdict.kind === 'class') return CLASS_PHRASE[verdict.class]; + return verdict.reason === 'list-or-object' ? 'a list or an object' : NO_CLASS_NOUN[verdict.reason]; } /** One lowered comparison between two columns that share no comparison class. */ export interface CrossClassComparison { /** The comparison as the author wrote it, back in CEL. */ written: string; - /** The column(s) at fault, with what each is declared as. */ - columns: string[]; + /** The two sides, in the written order: each one's class, or why it has none (`text vs a number`). */ + why: string; } /** @@ -1186,59 +1148,34 @@ export function crossClassComparisons( const target = declaredColumn(graph, object, site.field); const ref = declaredColumn(graph, object, referenced); if (!target || !ref) continue; - if (listHoldingDeclaration(target.meta) || listHoldingDeclaration(ref.meta)) continue; + if (holdsListOrObject(target.meta) || holdsListOrObject(ref.meta)) continue; const verdict = crossFieldComparisonVerdict(target, ref); - let columns: string[]; + let why: string; if (verdict.verdict === 'cross-class') { - columns = [ - describeColumn(target, { kind: 'class', class: verdict.left }), - describeColumn(ref, { kind: 'class', class: verdict.right }), - ]; + why = `${CLASS_PHRASE[verdict.left]} vs ${CLASS_PHRASE[verdict.right]}`; } else if (verdict.verdict === 'no-class') { - columns = [ - ...(verdict.left.kind === 'no-class' ? [describeColumn(target, verdict.left)] : []), - ...(verdict.right.kind === 'no-class' && ref.name !== target.name ? [describeColumn(ref, verdict.right)] : []), - ]; + why = `${sidePhrase(verdict.left)} vs ${sidePhrase(verdict.right)}`; } else { continue; } const written = `record.${site.field} ${FIELD_COMPARISON_SYMBOL.get(site.op)} record.${referenced}`; - found.set(written, { written, columns }); + found.set(written, { written, why }); } return [...found.values()]; } -/** The comparisons, quoted back with the declarations behind them. */ -export function describeCrossClassComparisons(comparisons: readonly CrossClassComparison[]): string { - return comparisons.map((c) => `\`${c.written}\`, where ${c.columns.join(' and ')}`).join('; '); -} - -/** The sentence both rules state before their per-surface consequence. */ -export const CROSS_CLASS_SENTENCE = - 'Two columns are compared only within one comparison class — the class decides how their stored values ' + - 'order and equal, and across classes SQL and the in-memory evaluator answer differently — and a file field ' + - 'or a formula field has no class at all, so the platform defines no comparison between these columns'; - /** - * What a cross-class comparison does at request time, per clause. Measured - * through the real plugin-security and ObjectQL on driver-sql (see this file's - * header). - * - * The WRITE half is the in-process write check's behaviour, which reads the - * same classification since the engine lane moved it onto - * `crossFieldComparisonVerdict` (#20355): it refuses where the read refuses. + * [#22161] The cross-class verdict, after its subject: the first comparison, + * why its two fields share no class, and a count of the rest. Why classes + * decide comparability, and what the refusal costs, is the `os explain` + * entry's. Shared with the sharing-rule arm. */ -function crossClassConsequence(clause: 'using' | 'check'): string { - const write = - 'the in-process write check refuses the comparison by the same classification (`INVALID_FILTER` / 400), ' + - 'so every insert or update it judges is refused and nothing is stored'; - return clause === 'using' - ? 'every read this policy scopes is refused on the SQL drivers (`INVALID_FILTER` / 400: driver-sql refuses ' + - 'the comparison by the two columns\' declared types), and every by-id update or delete it scopes fails ' + - 'closed (`PERMISSION_DENIED` / 403). On an `insert`, `update` or `all` policy the same `using` is also ' + - 'the write check whenever no applicable policy for that operation declares a `check` (ADR-0058 D4), and ' + - `there ${write}.` - : `${write[0].toUpperCase()}${write.slice(1)}. The policy reads as a write rule and admits no write at all.`; +export function crossClassVerdict(comparisons: readonly CrossClassComparison[]): string { + const [first] = comparisons; + return ( + `compares \`${first.written}\`, which no comparison class spans (${first.why})` + + moreComparisons(comparisons.length) + ); } /** The prescription both rules share, ahead of their per-surface alternatives. */ @@ -1247,80 +1184,6 @@ export const CROSS_CLASS_REMEDY = 'A file field and a formula field cannot be compared with another column at all. If the two columns do ' + 'hold comparable values, one of them is declared with the wrong type: fix the declaration, not the predicate.'; -/** - * What a reference miss costs at request time, per clause. Measured, not inferred. - * - * ⚠️ This text was rewritten once, and the reason it was wrong is worth keeping: - * it said the field half had TWO directions — fail-closed in the leading - * position `extractTargetField` recognises, fail-OPEN everywhere else — and it - * attributed the WRITE leg's fail-closed to that same safety net. Both halves - * were wrong to leave standing. The runtime now judges column existence on the - * COMPILED predicate, inside `RLSCompiler.compileFilter`, which both the read - * layer and the ADR-0058 D4 write gate pass through: position and polarity are - * normalised away before the check runs, so a miss fails CLOSED everywhere, on - * both clauses. And the write path never had an `extractTargetField` net to - * credit — `computeWriteCheckFilter` compiled `check` with no field-existence - * check at all, which is why a negated miss there PERMITTED the write until the - * compiler-side guard landed. - * - * ⇒ Both KINDS now have one direction each, and it is the same direction. What - * this text still owes an author is that the miss is not a harmless typo: it - * turns the policy into a blanket refusal for every holder of the set. - */ -function referenceConsequence(clause: 'using' | 'check', kind: 'field' | 'variable'): string { - if (kind === 'variable') { - const dropped = - 'The pushdown compiler answers `unresolved-variable` in EVERY position — including under `!` and in ' + - 'a trailing `||` arm — so `RLSCompiler` DROPS the policy at request time, and one WARN line is the ' + - 'only signal. '; - return clause === 'using' - ? dropped + - 'When it is the only applicable policy for that object and operation the layer falls back to the ' + - '`RLS_DENY_FILTER` sentinel, which is AND-ed onto the where clause: every select / update / ' + - 'delete matches ZERO rows, so the object DISAPPEARS for every holder of this permission set — ' + - 'not because they were denied, but because the narrowing they were granted resolves to nothing. ' + - 'When other policies also apply, this one vanishes from the OR and grants none of the access it ' + - 'appears to.' + - USING_INSERT_CONSEQUENCE - : dropped + - 'On the ADR-0058 D4 write path that leaves the post-image `check` unsatisfiable: every ' + - 'single-record insert and by-id update the policy governs fails with `PermissionDeniedError`. ' + - 'The policy reads as a write rule and behaves as a blanket refusal for every holder of this ' + - 'permission set.' + - CHECK_SET_QUALIFIER; - } - - // ── The FIELD half. ONE direction, in every position and every polarity, - // since the runtime moved the column check onto the COMPILED predicate. The - // tracker id stays HERE and never in the returned string, because that string - // reaches authors, operators and generated surfaces, none of whom can resolve - // `#NNNN` (`check:doc-authoring`, maintainer ruling 2026-08-12). - const dropped = - '`RLSCompiler` judges column existence on the COMPILED predicate, so the position and the polarity ' + - 'you wrote it in make no difference — a leading `field ==`, a negation (`field != x`, ' + - '`!(field == x)`, `!(field in [...])`) and any arm after the first all lower to the same tree and ' + - 'all DROP the policy at request time, with one WARN line as the only signal. '; - return clause === 'using' - ? dropped + - 'When it is the only applicable policy for that object and operation the layer falls back to the ' + - '`RLS_DENY_FILTER` sentinel: every select / update / delete matches ZERO rows, so the object ' + - 'DISAPPEARS for every holder of this permission set — not because they were denied, but because ' + - 'the narrowing they were granted names a column that is not there. When other policies also ' + - 'apply, this one vanishes from the OR and grants none of the access it appears to.' + - USING_INSERT_CONSEQUENCE - : dropped + - 'On the ADR-0058 D4 write path that leaves the post-image `check` unsatisfiable: every ' + - 'single-record insert and by-id update the policy governs fails with `PermissionDeniedError`. ' + - 'The policy reads as a write rule and behaves as a blanket refusal for every holder of this ' + - 'permission set.' + - CHECK_SET_QUALIFIER + - ' ⚠️ On a runtime older ' + - 'than that guard this clause failed OPEN rather than closed — the write path had no ' + - 'field-existence check at all, so a negated miss was satisfied VACUOUSLY by the post-image and ' + - 'PERMITTED exactly the writes the policy was written to refuse, on every driver. Fix the name ' + - 'rather than relying on either behaviour.'; -} - /** * The engine's judge-only filter admission, as the caller hands it in * (`IObjectQLEngine.judgeFilter`, bound to its engine). @@ -1391,16 +1254,6 @@ function engineRefusal( return verdict.ok ? null : verdict; } -/** - * What an engine-refused read scope does at request time. The analytics face - * composes the read scope into the `where` it hands the engine, so the same - * admission that produced this verdict refuses the query there. - */ -const ENGINE_REFUSED_CONSEQUENCE = - 'This clause is part of the object\'s row-level READ SCOPE, and the analytics face hands the read ' + - 'scope to this same engine admission before it runs a query: every analytics query over the object ' + - 'that this policy scopes is refused.'; - /** The prescription for an engine refusal, keyed by the engine's `code` (never by its prose). */ function engineRefusalHint(code: string): string { if (code === 'FILTER_TOKEN_UNKNOWN' || code === 'FILTER_TOKEN_UNRESOLVED') { @@ -1458,9 +1311,7 @@ function referenceFindings( rule: RLS_PREDICATE_UNKNOWN_FIELD, where, path, - message: - `RLS ${clause} \`${quote(source)}\` lowers correctly but does not name a real column: ` + - `${account.message} ` + referenceConsequence(clause, 'field'), + message: `${account.message} ${droppedSentence(clause)}`, hint: `${account.detail} Point the predicate at a column the object really declares, or delete the ` + `policy if the narrowing is gone — a policy naming a column that does not exist is not ` + @@ -1480,12 +1331,8 @@ function referenceFindings( where, path, message: - `RLS ${clause} \`${quote(source)}\` reads \`${variablePath}\`, which nothing pre-resolves. The ` + - `kernel-resolved \`current_user\` keys are exactly ${listNames(PRERESOLVED_USER_KEYS)}, and the ` + - `only other keys that can EVER appear are §7.3.1 membership sets, which the runtime stages as ` + - `ARRAYS and which are therefore usable only as \`field in current_user.\` — this reference ` + - `is in a scalar position, so no request can ever supply it. ` + - referenceConsequence(clause, 'variable'), + `RLS ${clause} reads \`${variablePath}\`, which nothing pre-resolves, in a scalar position no request ` + + `can ever fill, ${droppedClause(clause)}`, hint: `Use one of the pre-resolved context values (${listNames(PRERESOLVED_USER_KEYS)})${suggestName( key, @@ -1508,10 +1355,7 @@ function referenceFindings( rule: RLS_PREDICATE_UNENFORCEABLE, where, path, - message: - `RLS ${clause} \`${quote(source)}\` passes the shape check, but the compiler refuses it for the value ` + - `\`${fault.variable}\` holds on every request (${refusal}). ${HOLDING_SENTENCE[fault.holds](fault.variable)} ` + - consequence(clause, DROPPED_EVERY_REQUEST), + message: `RLS ${clause}: ${HOLDING_VERDICT[fault.holds](fault.variable)}, ${droppedClause(clause)}`, hint: `${HOLDING_RULE[fault.holds]} In this predicate: ` + `${fault.sites.map((site) => typeFaultRewrite(fault, site)).join('; ')}.`, @@ -1526,9 +1370,10 @@ function referenceFindings( rule: RLS_PREDICATE_UNENFORCEABLE, where, path, - message: - `RLS ${clause} \`${quote(source)}\` lowers, but to a comparand the platform's shared filter check ` + - `refuses (${firstSentence(faced)}), ${consequence(clause, DROPPED_FACE_REFUSED)}`, + // [#20212] `RLSCompiler.compileFilter` runs the same two faces on every + // compiled policy filter, `using` and `check` alike, so the policy is + // dropped on every request, exactly as a TYPE fault is. + message: `RLS ${clause} lowers to a refused comparand (${firstSentence(faced)}), ${droppedClause(clause)}`, hint: rewrites.length > 0 ? '`null` has no place inside a list or opposite an ordering operator — the platform refuses both in ' + @@ -1548,12 +1393,7 @@ function referenceFindings( rule: RLS_PREDICATE_UNENFORCEABLE, where, path, - message: - `RLS ${clause} \`${quote(source)}\` lowers, but compares a field with a field that holds a list or an ` + - `object: ${listComparisons.map((c) => `\`${c.written}\`, where ${c.columns.join(' and ')}`).join('; ')}. ` + - 'A column that holds a list or an object is not one comparable value, on either side of a ' + - 'field-to-field comparison, so the platform refuses the comparison instead of evaluating it: ' + - listHoldingConsequence(clause), + message: `RLS ${clause} ${listHoldingVerdict(listComparisons)}, ${REFUSED_COMPARISON[clause]}`, hint: 'A field compared with a `json` or `multiple` field has no row-filter form: a row filter compares ' + 'one value with one value, and cannot test membership in a list another column holds. Compare ' + @@ -1574,9 +1414,7 @@ function referenceFindings( rule: RLS_PREDICATE_UNENFORCEABLE, where, path, - message: - `RLS ${clause} \`${quote(source)}\` lowers, but compares two fields that share no comparison class: ` + - `${describeCrossClassComparisons(crossClass)}. ${CROSS_CLASS_SENTENCE}: ${crossClassConsequence(clause)}`, + message: `RLS ${clause} ${crossClassVerdict(crossClass)}, ${REFUSED_COMPARISON[clause]}`, hint: `${CROSS_CLASS_REMEDY} To keep the rule without a second column, compare with a literal or a ` + '`current_user` value, test a file field with `!= null`, or store the value you mean in a field of the ' + @@ -1586,8 +1424,9 @@ function referenceFindings( // [#20158] The engine's own admission of the lowered read scope — run only // on a clause every pass above left clean, so one defect earns one finding. - // The verdict and its sentence are the engine's; this rule adds the clause, - // the object and what the refusal costs. + // The verdict and its sentence are the engine's; this rule adds the clause. + // What the refusal costs (every analytics query the read scope reaches) is + // the `os explain` entry's. const refused = judge && filter && findings.length === 0 ? engineRefusal(judge, object, bridged, filter, probe) : null; @@ -1598,8 +1437,7 @@ function referenceFindings( where, path, message: - `RLS ${clause} \`${quote(source)}\` lowers, but the engine refuses to run the lowered filter on ` + - `"${object}" (${refused.code} / ${refused.status}): ${refused.message} ${ENGINE_REFUSED_CONSEQUENCE}`, + `RLS ${clause} (${refused.code} / ${refused.status}): ${refusalVerdict(firstSentence(refused.message))}`, hint: engineRefusalHint(refused.code), }); } @@ -1700,9 +1538,8 @@ export function validateRlsPredicateEnforceability( 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), + `RLS ${clause} is valid CEL but overruns the parse bound ${bound}${budget}${measured} ` + + `(${overrun.summary}), ${droppedClause(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 ` + @@ -1727,8 +1564,8 @@ export function validateRlsPredicateEnforceability( where, path, message: - `RLS ${clause} \`${source}\` does not parse as CEL even after the legacy SQL bridge ` + - `(\`=\` → \`==\`, \`IN\` → \`in\`) has been applied (${detail}), ` + consequence(clause), + `RLS ${clause} does not parse as CEL, even after the legacy SQL bridge (${detail}), ` + + droppedClause(clause), hint: 'Author the predicate in canonical CEL (ADR-0058 D1). The bridge covers only the historic ' + 'SQL subset — a bare `=` and `IN` — so everything else must already be CEL: combine with ' + @@ -1745,9 +1582,7 @@ export function validateRlsPredicateEnforceability( rule: RLS_PREDICATE_UNENFORCEABLE, where, path, - message: - `RLS ${clause} \`${source}\` is outside the pushdown subset the runtime can compile ` + - `(${detail}), ` + consequence(clause), + message: `RLS ${clause} is not lowerable (${refusalVerdict(detail)}), ${droppedClause(clause)}`, hint: 'Rewrite the predicate inside the lowerable subset. ' + PUSHDOWN_SUBSET + ' Three traps in ' + 'particular: (1) a function call — `size(record.tags) > 0`, `has(record.x)` — is correct in an ' + diff --git a/packages/lint/src/validate-sharing-rule-enforceability.cross-class-field.test.ts b/packages/lint/src/validate-sharing-rule-enforceability.cross-class-field.test.ts index b2ff2b0d86f..7f300b99e81 100644 --- a/packages/lint/src/validate-sharing-rule-enforceability.cross-class-field.test.ts +++ b/packages/lint/src/validate-sharing-rule-enforceability.cross-class-field.test.ts @@ -19,10 +19,23 @@ import { describe, expect, it } from 'vitest'; import { - validateSharingRuleEnforceability, + validateSharingRuleEnforceability as validateSharingRuleEnforceabilityUnrecorded, SHARING_RULE_UNLOWERABLE_CONDITION, } from './validate-sharing-rule-enforceability.js'; import { runAuthoringRules } from './authoring-rules.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding is one verdict sentence; the reasoning it used to +// carry is the id's `os explain` entry. Every call below records what it +// fired, and the last case in this file holds each recorded verdict to one +// line of at most 200 characters. Run the whole file: that case reads what the +// cases above fired. +const fired: Array<{ rule: string; message: string }> = []; +const validateSharingRuleEnforceability: typeof validateSharingRuleEnforceabilityUnrecorded = (...args) => { + const findings = validateSharingRuleEnforceabilityUnrecorded(...args); + fired.push(...findings); + return findings; +}; const deal = { name: 'deal', @@ -70,15 +83,16 @@ const OPERATORS: ReadonlyArray<{ op: string; spell: (a: string, b: string) => st { op: '<=', spell: (a, b) => `record.${a} <= record.${b}`, quoted: '<=' }, ]; +/** How the finding names each cell's column: its class, or why it has none. */ const CELLS = [ - { cell: 'text vs number', field: 'amount', names: ["`status` is declared `type: 'text'`, compared as text", "`amount` is declared `type: 'number'`, compared as a number"] }, - { cell: 'text vs a single image', field: 'photo', names: ["`photo` is declared `type: 'image'`, a file field"] }, - { cell: 'text vs a formula field', field: 'is_open', names: ["`is_open` is declared `type: 'formula'`, a formula field"] }, + { cell: 'text vs number', field: 'amount', side: 'a number' }, + { cell: 'text vs a single image', field: 'photo', side: 'a file field' }, + { cell: 'text vs a formula field', field: 'is_open', side: 'a formula field' }, ] as const; describe('validateSharingRuleEnforceability — a field compared with a field of another class is REFUSED (#20347)', () => { for (const { op, spell, quoted } of OPERATORS) { - for (const { cell, field, names } of CELLS) { + for (const { cell, field, side } of CELLS) { for (const order of ['text first', 'text second'] as const) { const [left, right] = order === 'text first' ? ['status', field] : [field, 'status']; const condition = spell(left, right); @@ -89,12 +103,11 @@ describe('validateSharingRuleEnforceability — a field compared with a field of ]); const [finding] = findings; expect(finding.where).toBe('sharing rule "deal_desk_share" on object "deal"'); - expect(finding.message).toContain( - `Sharing-rule condition \`${condition}\` lowers, but compares two fields that share no comparison ` + - `class: \`record.${left} ${quoted} record.${right}\`, where `, + const why = order === 'text first' ? `text vs ${side}` : `${side} vs text`; + expect(finding.message).toBe( + `condition compares \`record.${left} ${quoted} record.${right}\`, which no comparison class spans ` + + `(${why}), so its criteria query is refused (INVALID_FILTER / 400) and it grants nothing`, ); - for (const name of names) expect(finding.message).toContain(name); - expect(finding.message).toContain('The rule is declared and grants nothing.'); expect(finding.hint).toMatch(/^Compare a field only with a field of the same comparison class: /); }); } @@ -126,14 +139,16 @@ describe('validateSharingRuleEnforceability — one comparison, one finding (#20 const findings = validateSharingRuleEnforceability(stackWith('record.photo != record.tags')); expect(findings).toHaveLength(1); expect(findings[0].message).toContain('holds a list or an object'); - expect(findings[0].message).not.toContain('share no comparison class'); + expect(findings[0].message).not.toContain('which no comparison class spans'); }); it('two defects in one condition — a list and a class mismatch — earn one finding each', () => { const findings = validateSharingRuleEnforceability(stackWith('record.status != record.tags && record.close_date < record.signed_at')); expect(findings.map((f) => f.rule)).toEqual([SHARING_RULE_UNLOWERABLE_CONDITION, SHARING_RULE_UNLOWERABLE_CONDITION]); expect(findings[0].message).toContain('holds a list or an object'); - expect(findings[1].message).toContain('share no comparison class: `record.close_date < record.signed_at`'); + expect(findings[1].message).toContain( + 'compares `record.close_date < record.signed_at`, which no comparison class spans (a date vs a datetime)', + ); }); it('judges nothing the graph cannot answer: an anchor outside the stack, a field map it cannot read', () => { @@ -149,3 +164,22 @@ describe('validateSharingRuleEnforceability — one comparison, one finding (#20 ]); }); }); + +describe('[#22161] one-line verdicts', () => { + it('every verdict the cases above fired is one line of at most 200 characters', () => { + // The coverage control first: the cases above fired the cross-class + // verdict, so the shape assertion cannot pass over an empty record. + expect(fired.some((f) => f.message.includes('which no comparison class spans'))).toBe(true); + for (const f of fired) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` carries the class rule and the seeded-but-refused consequence the verdict no longer states', () => { + const text = explainRule(SHARING_RULE_UNLOWERABLE_CONDITION)?.paragraphs.join('\n') ?? ''; + expect(text).toContain('Two columns are compared only within one comparison class'); + expect(text).toContain('so the rule IS seeded into `sys_sharing_rule`, but every criteria query it runs is refused'); + expect(text).toContain('The rule is declared and grants nothing.'); + }); +}); diff --git a/packages/lint/src/validate-sharing-rule-enforceability.list-holding-field.test.ts b/packages/lint/src/validate-sharing-rule-enforceability.list-holding-field.test.ts index 19d894174db..7e6b42ed2a7 100644 --- a/packages/lint/src/validate-sharing-rule-enforceability.list-holding-field.test.ts +++ b/packages/lint/src/validate-sharing-rule-enforceability.list-holding-field.test.ts @@ -23,11 +23,24 @@ import { describe, expect, it } from 'vitest'; import { - validateSharingRuleEnforceability, + validateSharingRuleEnforceability as validateSharingRuleEnforceabilityUnrecorded, SHARING_RULE_UNLOWERABLE_CONDITION, } from './validate-sharing-rule-enforceability.js'; import { validateRlsPredicateEnforceability } from './validate-rls-predicate-enforceability.js'; import { runAuthoringRules } from './authoring-rules.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding is one verdict sentence; the reasoning it used to +// carry is the id's `os explain` entry. Every call below records what it +// fired, and the last case in this file holds each recorded verdict to one +// line of at most 200 characters. Run the whole file: that case reads what the +// cases above fired. +const fired: Array<{ rule: string; message: string }> = []; +const validateSharingRuleEnforceability: typeof validateSharingRuleEnforceabilityUnrecorded = (...args) => { + const findings = validateSharingRuleEnforceabilityUnrecorded(...args); + fired.push(...findings); + return findings; +}; const deal = { name: 'deal', @@ -76,26 +89,16 @@ const OPERATORS: ReadonlyArray<{ op: string; spell: (a: string, b: string) => st ]; const COLUMNS = [ - { column: 'a json field', field: 'tags', declared: "`tags` is declared `type: 'json'`" }, - { column: 'a multiple lookup', field: 'reviewers', declared: "`reviewers` is declared `type: 'lookup'`, `multiple: true`" }, + { column: 'a json field', field: 'tags' }, + { column: 'a multiple lookup', field: 'reviewers' }, ] as const; -/** The family's class sentence — the RLS arm's words, kept identical. */ -const CLASS_SENTENCE = - 'A column that holds a list or an object is not one comparable value, on either side of a field-to-field ' + - 'comparison, so the platform refuses the comparison instead of evaluating it'; - -/** The measured runtime consequence on the sharing path. */ -const CONSEQUENCE = - 'the rule is seeded into `sys_sharing_rule`, but every criteria query it runs is refused on the SQL drivers ' + - '(`INVALID_FILTER` / 400: driver-sql refuses a cross-field comparison against such a column by its declared ' + - 'type), and `SharingRuleService` reads a refused query as matching no record. No `sys_record_share` grant is ' + - 'ever materialised, at boot or on any later write, and the only signal is a WARN line in the server log. The ' + - 'rule is declared and grants nothing.'; +/** What the refusal costs on the sharing path — the verdict's closing clause. */ +const CONSEQUENCE = 'so its criteria query is refused (INVALID_FILTER / 400) and it grants nothing'; describe('validateSharingRuleEnforceability — a condition comparing a field with a json / multiple field is REFUSED (#19886)', () => { for (const { op, spell, quoted } of OPERATORS) { - for (const { column, field, declared } of COLUMNS) { + for (const { column, field } of COLUMNS) { for (const order of ['scalar first', 'list first'] as const) { const [left, right] = order === 'scalar first' ? ['status', field] : [field, 'status']; const condition = spell(left, right); @@ -110,8 +113,8 @@ describe('validateSharingRuleEnforceability — a condition comparing a field wi }, ]); expect(findings[0].message).toBe( - `Sharing-rule condition \`${condition}\` lowers, but compares a field with a field that holds a list or ` + - `an object: \`record.${left} ${quoted} record.${right}\`, where ${declared}. ${CLASS_SENTENCE}: ${CONSEQUENCE}`, + `condition compares \`record.${left} ${quoted} record.${right}\`, where \`${field}\` holds a list or an ` + + `object, ${CONSEQUENCE}`, ); expect(findings[0].hint).toMatch(/^A field compared with a `json` or `multiple` field has no row-filter form/); }); @@ -126,7 +129,7 @@ describe('validateSharingRuleEnforceability — a condition comparing a field wi it('the parsed tier (`{ dialect, source }`) is refused identically', () => { const findings = validateSharingRuleEnforceability(stackWith({ dialect: 'cel', source: 'record.status != record.tags' })); expect(findings.map((f) => f.rule)).toEqual([SHARING_RULE_UNLOWERABLE_CONDITION]); - expect(findings[0].message).toContain('Sharing-rule condition `record.status != record.tags` lowers'); + expect(findings[0].message).toContain('condition compares `record.status != record.tags`, where `tags` holds'); }); it('an offending comparison inside a compound condition is refused, and a second one is named in the same finding', () => { @@ -135,11 +138,7 @@ describe('validateSharingRuleEnforceability — a condition comparing a field wi const two = validateSharingRuleEnforceability(stackWith('record.status != record.tags || record.reviewers == record.tags')); expect(two).toHaveLength(1); - expect(two[0].message).toContain( - "object: `record.status != record.tags`, where `tags` is declared `type: 'json'`; " + - "`record.reviewers == record.tags`, where `reviewers` is declared `type: 'lookup'`, `multiple: true` and " + - "`tags` is declared `type: 'json'`. ", - ); + expect(two[0].message).toContain('compares `record.status != record.tags`, where `tags` holds a list or an object (and 1 more), '); }); it('an inactive rule is judged too — the seeder compiles and seeds it regardless of `active`', () => { @@ -147,14 +146,19 @@ describe('validateSharingRuleEnforceability — a condition comparing a field wi expect(findings.map((f) => f.rule)).toEqual([SHARING_RULE_UNLOWERABLE_CONDITION]); }); - it('speaks the RLS arm\'s class sentence, word for word — one class, one sentence, two surfaces', () => { + it('speaks the RLS arm\'s verdict and class paragraph, word for word — one class, one sentence, two surfaces', () => { const rls = validateRlsPredicateEnforceability({ objects: [deal, account], permissions: [{ name: 'sales', label: 'Sales', rowLevelSecurity: [{ name: 'p', object: 'deal', operation: 'select', using: 'record.status != record.tags' }] }], }); const sharing = validateSharingRuleEnforceability(stackWith('record.status != record.tags')); - expect(rls[0].message).toContain(`${CLASS_SENTENCE}: `); - expect(sharing[0].message).toContain(`${CLASS_SENTENCE}: `); + const verdict = 'compares `record.status != record.tags`, where `tags` holds a list or an object, '; + expect(rls[0].message).toContain(verdict); + expect(sharing[0].message).toContain(verdict); + const classParagraph = (rule: string) => + explainRule(rule)?.paragraphs.find((p) => p.startsWith('A column that holds a list or an object')); + expect(classParagraph(SHARING_RULE_UNLOWERABLE_CONDITION)).toBeDefined(); + expect(classParagraph(SHARING_RULE_UNLOWERABLE_CONDITION)).toBe(classParagraph(rls[0].rule)); }); }); @@ -180,12 +184,12 @@ describe('validateSharingRuleEnforceability — every declared list-or-object cl ['multiple file', { type: 'file', multiple: true }, "`type: 'file'`, `multiple: true`"], ['multiple image', { type: 'image', multiple: true }, "`type: 'image'`, `multiple: true`"], ]; - for (const [label, def, declared] of LIST_HOLDING) { - it(`${label}: refused, naming the declaration`, () => { + for (const [label, def] of LIST_HOLDING) { + it(`${label}: refused, naming the column`, () => { const objects = [{ ...deal, fields: { ...deal.fields, subject: { label: 'Subject', ...def } } }, account]; const findings = validateSharingRuleEnforceability(stackWith('record.status != record.subject', objects)); expect(findings.map((f) => f.rule)).toEqual([SHARING_RULE_UNLOWERABLE_CONDITION]); - expect(findings[0].message).toContain(`\`record.status != record.subject\`, where \`subject\` is declared ${declared}.`); + expect(findings[0].message).toContain('`record.status != record.subject`, where `subject` holds a list or an object,'); }); } }); @@ -223,7 +227,7 @@ describe('validateSharingRuleEnforceability — the one-value spellings stay CLE const findings = validateSharingRuleEnforceability(stackWith('record.status != record.subject', objects)); expect(findings.map((f) => f.rule)).toEqual([SHARING_RULE_UNLOWERABLE_CONDITION]); expect(findings[0].message).not.toContain('holds a list or'); - expect(findings[0].message).toContain('share no comparison class'); + expect(findings[0].message).toContain('which no comparison class spans (text vs a file field)'); }); }); @@ -238,8 +242,8 @@ describe('validateSharingRuleEnforceability — the arm is the graph\'s, and rep const findings = validateSharingRuleEnforceability(stackWith('size(record.tags) > 0 && record.status != record.tags')); expect(findings).toHaveLength(1); expect(findings[0].rule).toBe(SHARING_RULE_UNLOWERABLE_CONDITION); - expect(findings[0].message).toMatch(/is outside the pushdown subset/); - expect(findings[0].message).not.toMatch(/lowers, but compares/); + expect(findings[0].message).toMatch(/^condition is not lowerable/); + expect(findings[0].message).not.toMatch(/holds a list or an object/); }); it('reports beside the anchor arm, independently — two fields, two findings', () => { @@ -257,6 +261,24 @@ describe('validateSharingRuleEnforceability — the arm is the graph\'s, and rep expect(cli.map((f) => ({ rule: f.rule, path: f.path }))).toEqual([ { rule: SHARING_RULE_UNLOWERABLE_CONDITION, path: 'sharingRules[0].condition' }, ]); - expect(cli[0].message).toContain('`record.reviewers == record.status`, where `reviewers` is declared'); + expect(cli[0].message).toContain('`record.reviewers == record.status`, where `reviewers` holds a list or an object'); + }); +}); + +describe('[#22161] one-line verdicts', () => { + it('every verdict the cases above fired is one line of at most 200 characters', () => { + // The coverage control first: the cases above fired the list-holding + // verdict, so the shape assertion cannot pass over an empty record. + expect(fired.some((f) => f.message.includes('holds a list or an object'))).toBe(true); + for (const f of fired) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); + + it('`os explain` carries the measured consequence the verdict no longer states', () => { + const text = explainRule(SHARING_RULE_UNLOWERABLE_CONDITION)?.paragraphs.join('\n') ?? ''; + expect(text).toContain('`SharingRuleService` reads a refused query as matching no record'); + expect(text).toContain('at boot or on any later write'); }); }); diff --git a/packages/lint/src/validate-sharing-rule-enforceability.test.ts b/packages/lint/src/validate-sharing-rule-enforceability.test.ts index dcfd2305058..93702127001 100644 --- a/packages/lint/src/validate-sharing-rule-enforceability.test.ts +++ b/packages/lint/src/validate-sharing-rule-enforceability.test.ts @@ -8,13 +8,36 @@ import { compileCelToFilter } from '@objectstack/formula'; import { OWDModel, ShareRecipientType, ObjectAccessScopeSchema } from '@objectstack/spec/security'; import { - validateSharingRuleEnforceability, + validateSharingRuleEnforceability as validateSharingRuleEnforceabilityUnrecorded, SHARING_RULE_UNLOWERABLE_CONDITION, SHARING_RULE_RUNTIME_VARIABLE_CONDITION, SHARING_RULE_OBJECT_NOT_SHAREABLE, SHARING_RULE_OBJECT_CONTROLLED_BY_PARENT, } from './validate-sharing-rule-enforceability.js'; import { AUTHORING_RULES, runAuthoringRules } from './authoring-rules.js'; +import { explainRule } from './rule-explanations.js'; + +// [#22161] Each finding of the four ids is one verdict sentence; the reasoning +// it used to carry is the id's `os explain` entry. Every call below records +// what it fired, and the last cases in this file hold each recorded verdict to +// one line of at most 200 characters — so the pin covers every firing variant +// this suite exercises, not a chosen few. Run the whole file: those cases read +// what the cases above fired. +const SHARING_RULE_IDS: readonly string[] = [ + SHARING_RULE_UNLOWERABLE_CONDITION, + SHARING_RULE_RUNTIME_VARIABLE_CONDITION, + SHARING_RULE_OBJECT_NOT_SHAREABLE, + SHARING_RULE_OBJECT_CONTROLLED_BY_PARENT, +]; +const fired: Array<{ rule: string; message: string }> = []; +const validateSharingRuleEnforceability: typeof validateSharingRuleEnforceabilityUnrecorded = (...args) => { + const findings = validateSharingRuleEnforceabilityUnrecorded(...args); + fired.push(...findings); + return findings; +}; + +/** The `os explain` text of `rule`, one string. */ +const explanationOf = (rule: string): string => explainRule(rule)?.paragraphs.join('\n') ?? ''; const ids = (stack: unknown) => validateSharingRuleEnforceability(stack).map((f) => f.rule); @@ -50,9 +73,11 @@ describe('validateSharingRuleEnforceability — the declared-but-never-read case path: 'sharingRules[0].condition', where: 'sharing rule "high_value_opps" on object "opportunity"', }); - // It must say what actually happens at boot, not merely "unsupported". - expect(findings[0].message).toMatch(/SKIPS the rule at boot/); - expect(findings[0].message).toMatch(/never written to `sys_sharing_rule`/); + // It must say what actually happens at boot, not merely "unsupported"… + expect(findings[0].message).toMatch(/^condition is not lowerable \(.+\), so the rule is never seeded and grants nothing$/); + // …and `os explain` says how. + expect(explanationOf(SHARING_RULE_UNLOWERABLE_CONDITION)).toMatch(/the rule is SKIPPED/); + expect(explanationOf(SHARING_RULE_UNLOWERABLE_CONDITION)).toMatch(/never written to `sys_sharing_rule`/); // …and prescribe the fix that works on THIS surface. expect(findings[0].hint).toMatch(/record\.x != null/); expect(findings[0].hint).toMatch(/INTERPRETED/); @@ -303,11 +328,12 @@ describe('validateSharingRuleEnforceability — an anchor no gate would consult where: 'sharing rule "high_value_opps" on object "crm_opportunity"', }); // An author must be able to act on this: it has to name the object, the - // rule, and WHY the rule cannot take effect. - expect(findings[0].message).toMatch(/high_value_opps/); - expect(findings[0].message).toMatch(/crm_opportunity/); - expect(findings[0].message).toMatch(/sharingModel 'public_read_write'/); - expect(findings[0].message).toMatch(/SHARING_NOT_ENABLED/); + // rule (the `where` above), and WHY the rule cannot take effect. + expect(findings[0].message).toBe( + 'anchor object "crm_opportunity" declares sharingModel \'public_read_write\', so the grant is refused ' + + '(SHARING_NOT_ENABLED) and the rule restricts nothing', + ); + expect(explanationOf(SHARING_RULE_OBJECT_NOT_SHAREABLE)).toMatch(/`buildReadFilter` returns `null`/); // …and both honest fixes, because which one is right is the author's call. expect(findings[0].hint).toMatch(/delete it/); expect(findings[0].hint).toMatch(/sharingModel: 'private'/); @@ -333,7 +359,8 @@ describe('validateSharingRuleEnforceability — an anchor no gate would consult // `controlled_by_parent` first and answers "share the master record // instead"; this arm mirrors that order, and names the master it resolved. expect(findings[0].message).toMatch(/derived from its master/i); - expect(findings[0].message).toMatch(/share the master record instead/); + expect(findings[0].message).toMatch(/SHARING_NOT_ENABLED/); + expect(explanationOf(SHARING_RULE_OBJECT_CONTROLLED_BY_PARENT)).toMatch(/share the master record instead/); expect(findings[0].hint).toMatch(/crm_account/); }); @@ -590,3 +617,34 @@ describe('validateSharingRuleEnforceability — an unreadable master carrier is expect(ids(detailAnchoredOn(carrier))).toContain(SHARING_RULE_OBJECT_CONTROLLED_BY_PARENT); }); }); + +describe('[#22161] one-line verdicts — the four ids', () => { + it('every verdict the cases above fired is one line of at most 200 characters', () => { + // The coverage control first: each id fired at least once, so the shape + // assertion below cannot pass over an empty record. + expect([...new Set(fired.map((f) => f.rule))].sort()).toEqual([...SHARING_RULE_IDS].sort()); + for (const f of fired) { + expect(f.message, f.rule).not.toContain('\n'); + expect(f.message.length, `${f.rule}: ${f.message}`).toBeLessThanOrEqual(200); + } + }); + + // What each verdict stopped saying, which `os explain RULE_ID` now prints. + const MOVED: Record = { + [SHARING_RULE_UNLOWERABLE_CONDITION]: ['`bootstrapDeclaredSharingRules`', 'one WARN line in the boot log', 'ADR-0049', 'SharingRuleService', 'INVALID_FILTER'], + [SHARING_RULE_RUNTIME_VARIABLE_CONDITION]: ['MATERIALISED', 'no "current user"', 'the rule is SKIPPED'], + [SHARING_RULE_OBJECT_NOT_SHAREABLE]: ['WIDENS', 'ADR-0090 D1', '`assertNotInertGrant`', 'not under record-sharing enforcement', 'restriction that does not exist'], + [SHARING_RULE_OBJECT_CONTROLLED_BY_PARENT]: ['ADR-0055', 'no shares to widen', '`assertNotInertGrant`', 'whatever the MASTER grants them'], + }; + + it('covers exactly the four ids', () => { + expect(Object.keys(MOVED).sort()).toEqual([...SHARING_RULE_IDS].sort()); + }); + + it.each([...SHARING_RULE_IDS])('`os explain %s` carries what its verdict no longer says', (rule) => { + const explanation = explainRule(rule); + expect(explanation, `no \`os explain ${rule}\` entry`).toBeDefined(); + const text = explanation!.paragraphs.join('\n'); + for (const fact of MOVED[rule]) expect(text, `${rule} explanation names ${fact}`).toContain(fact); + }); +}); diff --git a/packages/lint/src/validate-sharing-rule-enforceability.ts b/packages/lint/src/validate-sharing-rule-enforceability.ts index 355c46e3bf5..f3c366fe7d8 100644 --- a/packages/lint/src/validate-sharing-rule-enforceability.ts +++ b/packages/lint/src/validate-sharing-rule-enforceability.ts @@ -184,10 +184,11 @@ import { referenceCarrierOf } from '@objectstack/spec/data'; import { indexObjectGraph, recordsOf, type ObjectGraph } from './object-graph.js'; import { CROSS_CLASS_REMEDY, - CROSS_CLASS_SENTENCE, crossClassComparisons, - describeCrossClassComparisons, + crossClassVerdict, listHoldingComparisons, + listHoldingVerdict, + refusalVerdict, } from './validate-rls-predicate-enforceability.js'; /** @@ -249,18 +250,26 @@ function toCompilerInput(condition: unknown): string | { source?: string } | nul return null; } -/** What the author sees quoted back at them. */ -function sourceOf(condition: unknown): string { - const input = toCompilerInput(condition); - if (typeof input === 'string') return input; - return str(input?.source); -} - const PUSHDOWN_SUBSET = 'The lowerable subset is: `==` `!=` `>` `<` `>=` `<=`, `in`, `&&` `||` `!`, `== null` / `!= null`, ' + 'and the string methods `startsWith` / `endsWith` / `contains` — over SINGLE-column `record.` ' + 'paths (ADR-0058 D2).'; +/* + * [#22161] The verdict clauses. Every finding of the four ids prints one + * verdict sentence; what the seeder, the criteria query and the grant + * pre-flight do with the rule — and the measurements behind it — is each id's + * `os explain` entry (`rule-explanations.ts`, the "Row-level security and + * sharing rules" block). The finding does not echo the condition: its `path` + * names it, and the condition is author text of any length. + */ + +/** A condition that does not lower: the seeder skips the rule — both condition ids. */ +const NEVER_SEEDED = 'so the rule is never seeded and grants nothing'; + +/** A lowered condition the driver refuses on every criteria query — both comparison arms. */ +const CRITERIA_QUERY_REFUSED = 'so its criteria query is refused (INVALID_FILTER / 400) and it grants nothing'; + /** * The finding for a lowered `condition` that compares a field with a field * holding a list or an object, or `null` when it compares none (see this @@ -271,7 +280,7 @@ function listHoldingFinding( graph: ObjectGraph, object: string, filter: Record, - at: { where: string; path: string; source: string }, + at: { where: string; path: string }, ): SharingRuleEnforceabilityFinding | null { const comparisons = listHoldingComparisons(graph, object, filter); if (comparisons.length === 0) return null; @@ -280,16 +289,7 @@ function listHoldingFinding( rule: SHARING_RULE_UNLOWERABLE_CONDITION, where: at.where, path: at.path, - message: - `Sharing-rule condition \`${at.source}\` lowers, but compares a field with a field that holds a list or ` + - `an object: ${comparisons.map((c) => `\`${c.written}\`, where ${c.columns.join(' and ')}`).join('; ')}. ` + - 'A column that holds a list or an object is not one comparable value, on either side of a ' + - 'field-to-field comparison, so the platform refuses the comparison instead of evaluating it: the rule ' + - 'is seeded into `sys_sharing_rule`, but every criteria query it runs is refused on the SQL drivers ' + - '(`INVALID_FILTER` / 400: driver-sql refuses a cross-field comparison against such a column by its ' + - 'declared type), and `SharingRuleService` reads a refused query as matching no record. No ' + - '`sys_record_share` grant is ever materialised, at boot or on any later write, and the only signal is ' + - 'a WARN line in the server log. The rule is declared and grants nothing.', + message: `condition ${listHoldingVerdict(comparisons)}, ${CRITERIA_QUERY_REFUSED}`, hint: 'A field compared with a `json` or `multiple` field has no row-filter form: a row filter compares one ' + 'value with one value, and cannot test membership in a list another column holds. Compare with a ' + @@ -310,7 +310,7 @@ function crossClassFinding( graph: ObjectGraph, object: string, filter: Record, - at: { where: string; path: string; source: string }, + at: { where: string; path: string }, ): SharingRuleEnforceabilityFinding | null { const comparisons = crossClassComparisons(graph, object, filter); if (comparisons.length === 0) return null; @@ -319,14 +319,7 @@ function crossClassFinding( rule: SHARING_RULE_UNLOWERABLE_CONDITION, where: at.where, path: at.path, - message: - `Sharing-rule condition \`${at.source}\` lowers, but compares two fields that share no comparison class: ` + - `${describeCrossClassComparisons(comparisons)}. ${CROSS_CLASS_SENTENCE}: the rule is seeded into ` + - '`sys_sharing_rule`, but every criteria query it runs is refused on the SQL drivers (`INVALID_FILTER` / ' + - "400: driver-sql refuses the comparison by the two columns' declared types), and `SharingRuleService` " + - 'reads a refused query as matching no record. No `sys_record_share` grant is ever materialised, at boot ' + - 'or on any later write, and the only signal is a WARN line in the server log. The rule is declared and ' + - 'grants nothing.', + message: `condition ${crossClassVerdict(comparisons)}, ${CRITERIA_QUERY_REFUSED}`, hint: `${CROSS_CLASS_REMEDY} To keep the rule without a second column, compare with a literal — "one of these ` + "values\" is `record.status in ['open', 'pending']` — or test a file field with `!= null`.", @@ -487,15 +480,11 @@ function anchorFindings( rule: SHARING_RULE_OBJECT_CONTROLLED_BY_PARENT, where, path, + // [#22161] One verdict sentence; why a detail holds no shares, and what + // the refused grant leaves the recipients, is the `os explain` entry's. message: - `Sharing rule "${name}" is anchored on object "${object}", which declares ` + - `sharingModel 'controlled_by_parent'. A detail record has no record-level access of its own — ` + - `its visibility is DERIVED from its master (ADR-0055), so it holds no shares to widen. ` + - `\`SharingService.assertNotInertGrant\` refuses the grant with ` + - `SHARING_NOT_ENABLED ("'${object}' is controlled by its parent (master-detail); share the ` + - `master record instead"), so the rule's boot backfill fails, no \`sys_record_share\` row is ` + - `ever written, and the recipients this rule names get whatever the MASTER grants them — ` + - `which may be nothing. The grant is declared and does not exist.`, + `anchor object "${object}" declares sharingModel 'controlled_by_parent', so its access is derived ` + + 'from its master and the grant is refused (SHARING_NOT_ENABLED)', hint: `Move the rule onto the MASTER object` + (master ? ` — "${object}" derives from "${master}" through its master_detail field, so share ` + @@ -515,22 +504,18 @@ function anchorFindings( const declared = owd === 'public_read_write' ? `declares sharingModel 'public_read_write'` - : `declares no sharingModel and is a system object (\`isSystem: true\` or a \`sys_\` name), ` + - `which ADR-0090 D1 resolves to public`; + : 'declares no sharingModel and is a system object, which resolves to public'; return [{ severity: 'error', rule: SHARING_RULE_OBJECT_NOT_SHAREABLE, where, path, + // [#22161] One verdict sentence; why a public baseline has nothing to + // widen, and the measured read filter, is the `os explain` entry's. message: - `Sharing rule "${name}" is anchored on object "${object}", which ${declared}. Its effective ` + - `sharing model is therefore \`public\`, and sharing only ever WIDENS an OWD baseline — on the ` + - `widest baseline there is nothing left to widen. \`SharingService.assertNotInertGrant\` refuses ` + - `the grant with SHARING_NOT_ENABLED ("'${object}' is not under record-sharing enforcement"), so ` + - `the rule's boot backfill fails and no \`sys_record_share\` row is ever written. Measured: ` + - `\`buildReadFilter\` returns \`null\` for this object, i.e. NO record-level filter at all — every ` + - `principal already reads every row, so this rule advertises a restriction that does not exist.`, + `anchor object "${object}" ${declared}, so the grant is refused (SHARING_NOT_ENABLED) and the rule ` + + 'restricts nothing', hint: `Decide which half is wrong. If the ACCESS is right — everyone should read and write these ` + `records — the rule is dead metadata: delete it (ADR-0049 enforce-or-remove). If the RULE is ` + @@ -585,7 +570,6 @@ export function validateSharingRuleEnforceability(stack: unknown): SharingRuleEn const object = str(rule.object); const where = `sharing rule "${name}"${object ? ` on object "${object}"` : ''}`; const path = `sharingRules[${index}].condition`; - const source = sourceOf(rule.condition); // The seeder's exact call: `compileCelToFilter(r.condition, { variables: {} })` // in `bootstrap-declared-sharing-rules.ts`. Same function, same options — @@ -597,13 +581,13 @@ export function validateSharingRuleEnforceability(stack: unknown): SharingRuleEn // when it compares with a list- or object-holding field (file header). const graph = object ? anchorGraph(object) : null; const listHolding = graph - ? listHoldingFinding(graph, object, result.filter as Record, { where, path, source }) + ? listHoldingFinding(graph, object, result.filter as Record, { where, path }) : null; if (listHolding) findings.push(listHolding); // [#20347] …and when it compares two fields that share no comparison // class — refused by the same driver check, the same way (file header). const crossClass = graph - ? crossClassFinding(graph, object, result.filter as Record, { where, path, source }) + ? crossClassFinding(graph, object, result.filter as Record, { where, path }) : null; if (crossClass) findings.push(crossClass); return; @@ -612,12 +596,6 @@ export function validateSharingRuleEnforceability(stack: unknown): SharingRuleEn // same field with a message written about syntax. if (result.reason === 'parse-error') return; - const skipped = - 'so `bootstrapDeclaredSharingRules` SKIPS the rule at boot: it is never written to ' + - '`sys_sharing_rule`, no `sys_record_share` grant is ever materialised, and the only signal is one ' + - 'WARN line in the boot log. The rule is declared and grants nothing (ADR-0049: an unlowerable ' + - 'condition is never seeded as a permissive match-all).'; - if (result.reason === 'unresolved-variable') { // The record-relative-recipient gap the hint's last sentence describes is // tracked at #14103. The id lives HERE and not in the string: a runtime @@ -629,8 +607,7 @@ export function validateSharingRuleEnforceability(stack: unknown): SharingRuleEn rule: SHARING_RULE_RUNTIME_VARIABLE_CONDITION, where, path, - message: - `Sharing-rule condition \`${source}\` reads a runtime variable (${result.detail}), ` + skipped, + message: `condition reads a runtime variable (${result.detail}), ${NEVER_SEEDED}`, hint: 'A criteria sharing rule is MATERIALISED: the seeder compiles ONE static `criteria_json` per ' + 'rule and the evaluator writes `sys_record_share` rows from it, so there is no "current user" ' + @@ -667,9 +644,7 @@ export function validateSharingRuleEnforceability(stack: unknown): SharingRuleEn rule: SHARING_RULE_UNLOWERABLE_CONDITION, where, path, - message: - `Sharing-rule condition \`${source}\` is outside the pushdown subset the runtime can compile ` + - `(${result.detail}), ` + skipped, + message: `condition is not lowerable (${refusalVerdict(result.detail)}), ${NEVER_SEEDED}`, hint: 'Rewrite the predicate inside the lowerable subset. ' + PUSHDOWN_SUBSET + ' Two traps in ' + 'particular: (1) `has(record.x)` is correct in an object VALIDATION rule, which is INTERPRETED, ' +