diff --git a/.changeset/current-user-scope-root.md b/.changeset/current-user-scope-root.md new file mode 100644 index 0000000000..ab698d1099 --- /dev/null +++ b/.changeset/current-user-scope-root.md @@ -0,0 +1,56 @@ +--- +"@objectstack/formula": patch +"@objectstack/lint": patch +--- + +fix(formula,lint): `current_user` becomes a declared root, and its field-level rejection becomes a real rule (#6290) + +`@objectstack/formula` told two stories about one root. `introspectScope` handed +`current_user` to authors as a legal namespace and `checkRoleCatalog`'s four +position-membership regexes all lead with it — both correct, because ADR-0068 D1 +makes `current_user` THE canonical spelling and `buildScope` really does mount +the same `EvalUser` under it. Only `cel-engine.ts`'s `SCOPE_ROOTS` disagreed, so +the strict environment read the blessed spelling as a BARE FIELD REFERENCE while +its two aliases (`user`, `ctx`) passed unremarked. + +Three things change. + +**1. `SCOPE_ROOTS` declares `current_user`.** That list is a "never faults" +baseline, not a per-surface contract, and it now advertises exactly what the +package advertises elsewhere. A new pin asserts the property directly: every +root `introspectScope` reports must resolve in the strict env. + +**2. The wrong prescription is gone.** Because the rejection used to fall out of +the baseline's omission, the author got the GENERIC bare-field diagnostic — +"Write `record.current_user`". That shape binds on no layer of the platform, so +an author who followed the message ended up with something strictly worse than +what they started with, still silent. The field-level verdict now comes from a +rule of its own in `@objectstack/lint`, which names the real failure (unbound ⇒ +fault ⇒ visibility falls back to `true` ⇒ the field a `current_user` test was +meant to hide stays visible for everyone, #6146) and prescribes surfaces that +exist: move the predicate to the option's own `visibleWhen`, declare field-level +security on a permission set (`fields: { '.': { readable: false } }`), +or rewrite it against `record`. It covers `visibleWhen`, `readonlyWhen` and +`requiredWhen`, which share the one evaluator. + +**3. Per-option `visibleWhen` is validated at all.** `validate-expressions.ts` +walked field-level conditional rules and stopped there, so `SelectOption.visibleWhen` +— an authorable CEL slot the client filters on AND the server enforces — reached +compile, validate and run time checked by nobody. A bare field reference, a +reference to a field that does not exist, a syntax error or a template-dialect +predicate in an option all shipped in silence, and the option simply never +offered itself. Options are now walked, located by option value, on the same +`record` scope as their host field. + +The two surfaces deliberately give opposite verdicts on `current_user`, because +their evaluators differ: field-level rules go through `evalFieldPredicate` +(`record` + `previous` + `parent`, never a user), options through +`resolveCascadingOptions` against the host's predicate scope, which does bind it +(ADR-0068 / objectui#2284). The showcase's role-gated option +(`'admin' in current_user.positions`) had never met this rule before and is now +pinned as the legal usage it is. + +Sweep: `objectstack validate` is clean on all three example apps +(`app-showcase`, `app-crm`, `app-todo`) with the option walk active — zero new +findings, including the showcase object that carries both a record-scoped +cascade and the role-gated option. diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 757a742b1a..f702c02add 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -66,8 +66,40 @@ const SCOPE_ROOTS = [ // (the submit-time snapshot) and `vars`. Declared here so the strict lint // env doesn't misread `current.x` as a bare field reference. 'current', + // ADR-0068 D1's CANONICAL user root, and the last one this list was missing + // (#6290). `buildScope` mounts the same `EvalUser` object under + // `current_user` / `user` / `ctx.user` / `os.user` whenever the evaluation + // carries a user, and this package already told the rest of the platform so: + // `introspectScope` lists `current_user` among the roots it hands an author, + // and `checkRoleCatalog`'s four position-membership regexes all lead with it. + // Only this list disagreed — so the one spelling ADR-0068 calls canonical was + // the one spelling the strict env read as a BARE FIELD, while its two aliases + // (`user`, `ctx`) passed. One package, two accounts of the same root. + 'current_user', ] as const; +/* + * Why widening this list is the safe direction, and where the narrow verdict + * lives instead (#6290). + * + * This list is a "never faults" BASELINE, not a per-surface contract — the + * doc-comment above says so, and every entry is generous by construction. A + * surface that binds a CLOSED set of roots does not express that by hoping the + * baseline omits the others; it says so at the surface, through + * `collectCelRootIdentifiers` (that helper reads the AST and is completely + * independent of this list — see the approval-node approvers in #3447 P2, and + * `@objectstack/lint`'s field-level `*When` gate for `current_user`). + * + * That matters here because field- and section-level `visibleWhen` genuinely do + * NOT bind `current_user` (#6146, measured at both ends: `evalFieldPredicate` + * binds `record` + `previous` + `parent` and nothing else). Before #6290 that + * surface's rejection came out of this list's omission as a SIDE EFFECT, and it + * showed: the diagnostic was the generic bare-field one, so it prescribed + * "Write `record.current_user`" — a shape that binds on no layer at all. A + * verdict that belongs to one surface now reads as that surface's own rule, + * with that surface's own prescription. + */ + /** * A `record`-scoped environment (`unlistedVariablesAreDyn: false`) for detecting * bare field references. It reuses the real stdlib so function calls don't fault; diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index e363361e27..ac02ec2634 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest'; import { validateExpression, introspectScope, expectedDialect, inferExpressionType } from './validate'; +import { firstUndeclaredReference } from './cel-engine'; describe('validateExpression (ADR-0032)', () => { describe('predicates (CEL)', () => { @@ -343,6 +344,51 @@ describe('validateExpression (ADR-0032)', () => { expect(scope.roots).toContain('record'); expect(scope.functions).toContain('daysFromNow'); }); + + /** + * [#6290] The package must give ONE answer about what a root is. + * + * `introspectScope` is the roots list this package HANDS an author (and the + * agent authoring tool); `firstUndeclaredReference` is the strict env that + * JUDGES what an author wrote, off `cel-engine.ts`'s `SCOPE_ROOTS`. Nothing + * kept the two in step, and they had drifted on exactly the root ADR-0068 + * D1 calls canonical: `current_user` was advertised here and read as a bare + * field reference there — so the one spelling the ADR blesses was the one + * spelling the validator refused, while its two aliases (`user`, `ctx`) + * passed. + * + * Pinned as behaviour rather than as list equality: `SCOPE_ROOTS` is a + * generous baseline and stays free to declare MORE than it advertises (it + * carries `trigger`, `step`, `parent`, … for sites this introspection does + * not describe). What it may never do again is advertise a root it then + * faults on. Delete `'current_user'` from `SCOPE_ROOTS` and this goes red. + */ + it('every root `introspectScope` advertises really resolves in the strict env (#6290)', () => { + const advertised = introspectScope('predicate').roots; + expect(advertised).toContain('current_user'); + const faulting = advertised.filter((root) => firstUndeclaredReference(`${root}.x`) !== null); + expect(faulting).toEqual([]); + }); + + /** + * [#6290] The same drift, seen from `checkRoleCatalog`'s side: its four + * position-membership regexes accept `current_user` / `user` / `ctx.user` + * as the user subject, so a role-catalog verdict on a `current_user` + * predicate was only ever reachable at sites that do not run the + * `record`-scope bare-ref check. All three spellings now reach it. + */ + it('a role-catalog verdict is reachable through every ADR-0068 user spelling (#6290)', () => { + for (const subject of ['current_user', 'user', 'ctx.user']) { + const r = validateExpression('predicate', `'org_admni' in ${subject}.positions`, { + scope: 'record', + roleCatalog: ['org_admin', 'org_member'], + }); + expect(r.ok).toBe(false); + // The role typo is the finding — not a bare reference to the subject. + expect(r.errors.map((e) => e.message).join('\n')).toContain('unknown role `org_admni`'); + expect(r.errors.map((e) => e.message).join('\n')).not.toContain('bare reference'); + } + }); }); }); diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index b8d77b663a..2ae16de090 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -2,7 +2,7 @@ import { readFileSync } from 'node:fs'; import { describe, it, expect } from 'vitest'; import { ExpressionInputSchema, ObjectStackSchema } from '@objectstack/spec'; -import { FieldSchema, ObjectSchema } from '@objectstack/spec/data'; +import { FieldSchema, ObjectSchema, SelectOptionSchema } from '@objectstack/spec/data'; import { SharingRuleSchema } from '@objectstack/spec/security'; import { validateStackExpressions } from './validate-expressions.js'; @@ -634,6 +634,209 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { }); }); + /** + * ── `current_user` is a per-SURFACE verdict, not a missing root (#6290) ─── + * + * `@objectstack/formula`'s `SCOPE_ROOTS` now declares `current_user` + * (ADR-0068 D1's canonical spelling, which `buildScope` really mounts and + * which `introspectScope` already advertised). That deliberately removes + * the field-level rejection's OLD cause — an omission in a global baseline, + * doing a per-surface job by accident, and prescribing + * "Write `record.current_user`" while it did so: a shape that binds on no + * layer, so an author who obeyed the message ended up worse off. + * + * The verdict itself is not removed. It moves to the surface that owns it, + * with the prescriptions that exist — and the two surfaces now disagree on + * purpose, because their evaluators disagree (#6146): + * + * field level → `evalFieldPredicate`: `record` + `previous` + `parent`. + * `current_user` unbound ⇒ fault ⇒ fail-OPEN ⇒ rejected here. + * option level → `resolveCascadingOptions` against the host predicate + * scope, which binds `current_user` ⇒ accepted. + */ + describe('`current_user` at field level vs option level (#6290)', () => { + const gated = "'admin' in current_user.positions"; + + it('still REJECTS a field-level `visibleWhen` that reads `current_user`', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { amount: { type: 'number', visibleWhen: gated } }, + }], + }); + const hit = issues.filter((i) => i.where.includes("field 'amount' visibleWhen")); + expect(hit).toHaveLength(1); + expect(hit[0]!.severity).toBe('error'); + }); + + /** + * The prescription is the half this issue was filed for. Asserted as a + * NEGATIVE plus three positives, because "the message changed" is not the + * property that matters — "the message names a shape that actually binds" + * is. `record.current_user` is the shape that binds nowhere. + */ + it('prescribes surfaces that exist — and never `record.current_user`', () => { + const [issue] = validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { amount: { type: 'number', visibleWhen: gated } }, + }], + }).filter((i) => i.where.includes('visibleWhen')); + expect(issue!.message).not.toContain('record.current_user'); + // 1. what actually goes wrong, in the direction it goes wrong + expect(issue!.message).toMatch(/falls back to VISIBLE/); + // 2. the one `*When` surface that binds `current_user` + expect(issue!.message).toMatch(/option's own `visibleWhen`/); + // 3. the surface that hides a whole field by role — FLS on a permission + // set (`PermissionSetSchema.fields`, `permission.zod.ts:455`) + expect(issue!.message).toMatch(/readable: false/); + }); + + it('rejects it on `readonlyWhen` / `requiredWhen` too — same evaluator, same unbound root', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'showcase_deal', + fields: { + amount: { type: 'number', readonlyWhen: gated }, + note: { type: 'text', requiredWhen: gated }, + }, + }], + }); + expect(issues.filter((i) => /current_user/.test(i.message)).map((i) => i.where.replace(/.*· /, '')).sort()) + .toEqual(["field 'amount' readonlyWhen", "field 'note' requiredWhen"]); + }); + + /** + * The pin for the legal usage. Shape copied from + * `examples/app-showcase/src/data/objects/cascading-select.object.ts:85`, + * the role-gated option that has been shipping since ADR-0068 and had + * never met this rule — because until #6290 nothing walked options at all. + */ + it('ACCEPTS the showcase role-gated OPTION — per-option binds `current_user`', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'showcase_cascading_select', + fields: { + tier: { + type: 'select', + options: [ + { label: 'Standard', value: 'standard', default: true }, + { label: 'Restricted (admin only)', value: 'restricted', visibleWhen: gated }, + ], + }, + }, + }], + }); + expect(issues).toHaveLength(0); + }); + + it('accepts the cascading (record-scoped) options from the same showcase field', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'showcase_cascading_select', + fields: { + country: { type: 'select', options: [{ label: 'China', value: 'cn' }] }, + province: { + type: 'select', + dependsOn: ['country'], + options: [ + { label: 'Zhejiang', value: 'zj', visibleWhen: "record.country == 'cn'" }, + { label: 'California', value: 'ca', visibleWhen: "record.country == 'us'" }, + ], + }, + }, + }], + }); + expect(issues).toHaveLength(0); + }); + }); + + /** + * ── The option-level traversal itself (#6290 half 3) ──────────────────── + * + * Accepting `current_user` at the option level cannot be the only evidence + * the walk runs — an absent walk accepts everything. These are the findings + * the walk PRODUCES; delete the loop and every one of them disappears. + */ + describe('per-option `visibleWhen` is walked at all (#6290)', () => { + const optionIssues = (visibleWhen: unknown) => validateStackExpressions({ + objects: [{ + name: 'shop_item', + fields: { + country: { type: 'text' }, + tier: { + type: 'select', + options: [{ label: 'Restricted', value: 'restricted', visibleWhen }], + }, + }, + }], + }); + + it('flags a BARE field reference in an option predicate', () => { + const issues = optionIssues("country == 'cn'"); + expect(issues).toHaveLength(1); + expect(issues[0]!.where).toBe("object 'shop_item' · field 'tier' option 'restricted' visibleWhen"); + expect(issues[0]!.message).toMatch(/bare reference `country`/); + }); + + it('flags a reference to a field the object does not declare', () => { + const issues = optionIssues("record.no_such_field == 'cn'"); + expect(issues).toHaveLength(1); + expect(issues[0]!.message).toMatch(/no_such_field/); + }); + + it('flags a syntactically broken option predicate', () => { + const issues = optionIssues("record.country == 'cn' &&&"); + expect(issues).toHaveLength(1); + expect(issues[0]!.severity).toBe('error'); + }); + + it('flags a template-dialect option predicate (wrong dialect for a bare-CEL slot)', () => { + const issues = optionIssues('{{ record.country }}'); + expect(issues).toHaveLength(1); + }); + + /** + * The finding is located by the option's `value`, not by its index, so an + * author with eight options is told WHICH one to edit. The index fallback + * exists for a valueless option (which the schema rejects, but the rule + * runs on pre-parse stacks too and must not report `option 'undefined'`). + */ + it('locates each finding by the offending option, one finding per option', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'shop_item', + fields: { + tier: { + type: 'select', + options: [ + { label: 'Ok', value: 'ok', visibleWhen: "record.kind == 'a'" }, + { label: 'Bad', value: 'bad', visibleWhen: "kind == 'a'" }, + { label: 'Worse', value: 'worse', visibleWhen: "other == 'b'" }, + ], + }, + kind: { type: 'text' }, + }, + }], + }); + expect(issues.map((i) => i.where.replace(/.*· field 'tier' /, ''))).toEqual([ + "option 'bad' visibleWhen", + "option 'worse' visibleWhen", + ]); + }); + + it('is silent on options that carry no predicate at all', () => { + expect(validateStackExpressions({ + objects: [{ + name: 'shop_item', + fields: { + tier: { type: 'select', options: [{ label: 'A', value: 'a' }, { label: 'B', value: 'b', default: true }] }, + }, + }], + })).toHaveLength(0); + }); + }); + it('flags a bare-field sharing-rule condition', () => { const issues = validateStackExpressions({ objects: [{ name: 'crm_account', fields: { region: { type: 'text' } } }], @@ -1441,6 +1644,15 @@ const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: s declaredBy: 'ExpressionInputSchema', keys: () => shapeKeysOf(ExpressionInputSchema), }, + { + // [#6290] Per-option `visibleWhen` — the option surface had no traversal at + // all until then, so it enters the guard with its first two reads. `value` + // only locates the finding; `visibleWhen` is the predicate being checked. + receiver: 'opt', + expected: ['value', 'visibleWhen'], + declaredBy: 'SelectOptionSchema', + keys: () => Object.keys(SelectOptionSchema.shape), + }, ]; /** @@ -1474,7 +1686,11 @@ describe('validateStackExpressions — reads only keys the spec declares (meta-t it('the field receiver reads only declared keys — the tracked debt is now empty (#5026)', () => { const read = keysReadOff('f'); - expect(read).toEqual(['expression', 'name', 'readonlyWhen', 'requiredWhen']); + // `options` joined in #6290 — the per-option `visibleWhen` traversal. It is + // a LITERAL `f.options` read on purpose, for the same reason the two + // `*When` slots below are literal: an `(f as AnyRec).options` cast would + // have hidden the new surface from this very scan. + expect(read).toEqual(['expression', 'name', 'options', 'readonlyWhen', 'requiredWhen']); const declared = Object.keys(FieldSchema.shape); const tracked = TRACKED_UNDECLARED_READS.filter((t) => t.receiver === 'f').map((t) => t.key); expect(tracked).toEqual([]); diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index cd3752db32..7d7688ad6e 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -37,6 +37,8 @@ * | `validations[].condition` / `.when` / `.then` / `.otherwise` | the six `*ValidationSchema` variants | * | `objects[].fields[].reference` | `FieldSchema` | * | `objects[].fields[].expression` | `FieldSchema` | + * | `objects[].fields[].options` | `FieldSchema` | + * | `options[].value` / `.visibleWhen` | `SelectOptionSchema` | * | `actions[].objectName` | the action schema | * | `sharingRules[].condition` / `.object` | `SharingRuleSchema` | * @@ -389,6 +391,69 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { for (const w of res.warnings) issues.push({ where, message: w.message, source: w.source, severity: 'warning' }); }; + /** + * A FIELD-level conditional rule (`visibleWhen` / `readonlyWhen` / + * `requiredWhen`) that reaches for `current_user` — the one root the field + * level does not bind (#6146, measured at both ends: `evalFieldPredicate` / + * `resolveFieldRuleState` bind `record` + `previous` + `parent` and nothing + * else, and objectui#1582's authoring autocomplete pins the same three). + * + * ## Why this is a rule of its own rather than a missing root + * + * Until #6290 the same rejection fell out of `@objectstack/formula`'s + * `SCOPE_ROOTS` not listing `current_user` — a global baseline, doing a + * per-surface job by accident. Two things were wrong with that: + * + * 1. The rest of the package said the opposite. `introspectScope` hands + * `current_user` to authors as a legal root and `checkRoleCatalog`'s four + * position-membership regexes all lead with it, because ADR-0068 D1 makes + * it THE canonical spelling and `buildScope` really does mount it. One + * package, two accounts of one root. + * 2. The diagnostic was the generic bare-field one, so it prescribed + * "Write `record.current_user`" — a shape that binds on NO layer. An + * author who followed it wrote something strictly worse than what they + * started with, and the failure stayed silent. + * + * So the baseline now declares the root (a reference through it never faults + * platform-wide) and the surface that genuinely cannot bind it says so here, + * in its own words, with the prescriptions that actually exist. + * + * ## Why it is an error and not a warning + * + * The failure direction is the worst one available: an unbound identifier + * faults, the fault falls back, and visibility's fallback is `true` — so a + * predicate written to HIDE a field leaves it permanently visible, which is + * the opposite of what the author wrote and the half nobody notices (#6146). + * + * Verdict scope is the field level only. Per-option `visibleWhen` is checked + * by the loop in the field walk and deliberately NOT passed through here: + * options resolve against the host's predicate scope, which binds + * `current_user` (ADR-0068 / objectui#2284) — that surface is where such a + * predicate belongs, which is why it is also the first prescription below. + */ + const checkFieldRuleUserRoot = (where: string, slot: string, raw: unknown): void => { + const source = celSourceOf(raw); + if (!source) return; + const roots = collectCelRootIdentifiers(source); + if (!roots.ok || !roots.roots.includes('current_user')) return; + issues.push({ + where, + message: + `\`${slot}\` reads \`current_user\`, but a field-level conditional rule binds only ` + + `\`record\` (plus \`previous\`, and \`parent\` on a master-detail line item) — ` + + `\`current_user\` is unbound here, so the predicate faults and falls back to VISIBLE, ` + + `leaving the field the test was meant to hide showing for everyone (#6146). ` + + `To gate the CHOICES of a select by user, move the predicate to the option's own ` + + `\`visibleWhen\` (\`options: [{ …, visibleWhen: … }]\`) — per-option is the one \`*When\` ` + + `surface that binds \`current_user\`. To hide the FIELD by role, declare field-level ` + + `security on a permission set (\`fields: { '.': { readable: false } }\`), ` + + `which the server enforces. To gate on record state, rewrite the predicate against ` + + `\`record\`.`, + source, + severity: 'error', + }); + }; + /** * A declared bare-CEL slot (#4027). No object schema is passed: these slots * bind the *screen's own* collected values, not the trigger record's fields, so @@ -571,6 +636,34 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // not enforced = data-integrity hole). #1928 class, same as actions. for (const key of ['requiredWhen', 'readonlyWhen', 'conditionalRequired', 'visibleWhen'] as const) { check(`object '${objectName}' · field '${fname}' ${key}`, (f as AnyRec)[key], objectName, 'record'); + checkFieldRuleUserRoot(`object '${objectName}' · field '${fname}' ${key}`, key, (f as AnyRec)[key]); + } + // [#6290] Per-OPTION `visibleWhen` — a `select`/`multiselect`/`radio` + // option's own predicate (`SelectOptionSchema.visibleWhen`, + // `field.zod.ts:143`). It had no traversal here at all, so the whole + // option surface reached compile, validate and run time unvalidated: + // a bare field ref (`country == 'cn'`) or a reference to a field that + // does not exist produced no build error, and the option then simply + // never offered itself. + // + // Deliberately checked on the SAME `record` scope as the field-level + // slots, and that is the whole of the difference between the two faces + // after #6290: `current_user` is a declared root platform-wide (ADR-0068 + // D1), so it passes here — options resolve through + // `resolveCascadingOptions` against the host's predicate scope, which + // binds it (ADR-0068 / objectui#2284), and the showcase's + // `'admin' in current_user.positions` is the pinned legal usage — while + // `checkFieldRuleUserRoot` above rejects it one level up, where nothing + // binds it. Same helper, two verdicts, because the two surfaces have two + // evaluators; neither verdict is a side effect of a shared root list. + for (const [oi, opt] of asArray(f.options).entries()) { + const label = typeof opt.value === 'string' ? `'${opt.value}'` : `#${oi}`; + check( + `object '${objectName}' · field '${fname}' option ${label} visibleWhen`, + opt.visibleWhen, + objectName, + 'record', + ); } // [#4889] A `parent`-scoped `readonlyWhen` is a SERVER-enforced lock: // the write path resolves the object's master-detail header and binds it