diff --git a/.changeset/lint-expressions-security-alias-reads.md b/.changeset/lint-expressions-security-alias-reads.md new file mode 100644 index 0000000000..a2f44e6707 --- /dev/null +++ b/.changeset/lint-expressions-security-alias-reads.md @@ -0,0 +1,44 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): 收敛 `validateStackExpressions` / `validateSecurityPosture` 里读 spec 不声明键的 `??` 别名链 (#5017) + +两条规则都以 `input: 'parsed'` 注册,看到的是 `ObjectStackSchema` 解析后的产物。 +#4984 → #5009 清掉了 sharing rule 字段层和 org-axis 规则里的同形读法;这一轮是同族 +第三轮,落在另外两个文件。议题点名五条,全包 grep 又找出同形的两条,一并处置: + +| 原读法 | spec 事实 | 处置 | +|:--|:--|:--| +| `rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula`(两处) | 四个别名全是 `validation.zod.ts` 的 `aliases: { …: 'condition' }` **按名拒绝**的键;canonical 排第三 | 收敛为 `rule.condition` | +| `obj.validations ?? obj.validationRules` | `ObjectSchema` 只声明 `validations`,strict 按名拒绝 | 收敛为 `obj.validations` | +| `rule.condition ?? rule.criteria ?? rule.predicate` | `criteria` 是运行时编译产物 `criteria_json` 的拼法(#3896),`predicate` 直接拒绝 | 收敛为 `sharingRule.condition` | +| `def.reference ?? def.referenceTo` | `field.zod.ts:331` 把 `referenceTo` 映射为 `reference` | 收敛为 `def.reference` | +| `action.objectName ?? action.object` | canonical 是 `objectName`;`object` 按名拒绝 | 收敛为 `action.objectName` | +| `obj.sharingModel ?? (obj.security)?.sharingModel` | **`ObjectSchema` 根本没有 `security` 键** —— OWD 三个拨盘是平铺的,且 strict:嵌套写法被整包拒绝 | **删除整个 fallback** | +| `def.reference ?? def.reference_to` | 同 `referenceTo` | 收敛为 `def.reference` | + +对任何能解析的 stack,判定结果不变 —— 三个 example(crm / showcase / todo)与平台 +default permission sets 上,改动前后两条规则的 findings 逐字相同。 + +**其中一条不是死代码,是活着的错。** `rule.expression ?? … ?? rule.condition ?? …` +把 canonical 的 `condition` 排在两个被拒别名之后,所以一条同时写了 `condition` 和 +`expression` 的规则,lint 校验的是 schema 会拒绝的那个,而作者声明的那个**从头到尾 +没被看过**:producer 和 consumer 对同一份元数据给出两套说法。测试里重建了旧链来演示 +这个差异,而不是只描述它。 + +真正的代价从来不是漏报,而是误导 —— `object.security.sharingModel` 出现在**安全 +linter**里,足以让下一位作者(人或 AI)相信对象级 `security` 信封是真实的授权面。 + +同时补上两层结构性 meta-guard(#4992 模式,#5018 形状),让下一条死读法在 review +前就红: + +- **declared-key guard** —— 规则源码里从每个 surface 上读的键,必须出现在该 surface + 自己的 Zod `.shape` 里。扫源码不是扫行为是刻意的:不可达分支没有行为可断言。 +- **reachability guard** —— `validateSecurityPosture` 全部 15 个 `findings.push` + 落点都必须被一条 schema **不报 `unrecognized_keys`** 的 fixture 触达。判据不是 + #5018 的 `safeParse` 全绿,而这正是这条规则的特点:它被文档明确设计为也跑在 + parse 前,好让 `os lint` 对 zod 会拒绝的**值**(`sharingModel: 'read'`)给出更 + 好的信息。被拒的**值**和被拒的**键**是两回事 —— 后者在 parsed 路径上压根到不了。 + +七条读法各自做过变异测试:任意一条加回去,都至少有一条测试转红。 diff --git a/packages/cli/test/authoring-rule-command-parity.test.ts b/packages/cli/test/authoring-rule-command-parity.test.ts index 9312d5d80c..55c6bda1a7 100644 --- a/packages/cli/test/authoring-rule-command-parity.test.ts +++ b/packages/cli/test/authoring-rule-command-parity.test.ts @@ -87,7 +87,13 @@ const CASES: ReadonlyArray<{ rule: string; blindTo: readonly AuthoringCommand[]; rule: 'expression-invalid', blindTo: ['lint'], stack: withBaseline({ - objects: [{ name: 'parity_lead', label: 'Lead', sharingModel: 'private', fields: { lead_score: { type: 'number', label: 'Score' } }, validations: [{ name: 'r', expression: 'lead_score > 100' }] }], + // The planted defect is the BARE `lead_score` (a record-scoped predicate + // binds fields under `record`, so this silently evaluates to null) — not + // the key it is written under. Spelled `condition`, which is the only key + // `validation.zod.ts` declares: `expression` is one of the four names it + // rejects outright, so a fixture using it planted TWO defects and let the + // rule under test read a stack `os validate` would never accept (#5017). + objects: [{ name: 'parity_lead', label: 'Lead', sharingModel: 'private', fields: { lead_score: { type: 'number', label: 'Score' } }, validations: [{ type: 'script', name: 'r', message: 'Score out of range', condition: 'lead_score > 100' }] }], }), }, { diff --git a/packages/lint/src/runtime-gate.test.ts b/packages/lint/src/runtime-gate.test.ts index 16e01cf241..e066c79124 100644 --- a/packages/lint/src/runtime-gate.test.ts +++ b/packages/lint/src/runtime-gate.test.ts @@ -122,7 +122,12 @@ describe('runtime publish gate (#4463)', () => { { name: 'leave_request', fields: { owner: { type: 'text' } }, - validationRules: [{ name: 'bad', expression: 'record.owner ==', message: 'x' }], + // Spelled with the two keys the spec declares (`validations` / + // `condition`). Written as `validationRules` / `expression` — both + // rejected aliases — the broken CEL was not reachable by the rule at + // all, so this fixture proved the subtraction worked by having nothing + // to subtract (#5017). + validations: [{ type: 'script', name: 'bad', message: 'x', condition: 'record.owner ==' }], }, ]; @@ -147,7 +152,12 @@ describe('runtime publish gate (#4463)', () => { { name: 'leave_request', fields: { owner: { type: 'text' } }, - validationRules: [{ name: 'bad', expression: 'record.owner ==', message: 'x' }], + // Spelled with the two keys the spec declares (`validations` / + // `condition`). Written as `validationRules` / `expression` — both + // rejected aliases — the broken CEL was not reachable by the rule at + // all, so this fixture proved the subtraction worked by having nothing + // to subtract (#5017). + validations: [{ type: 'script', name: 'bad', message: 'x', condition: 'record.owner ==' }], }, ]; const result = runRuntimeAuthoringRules({ diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index a43ea1970c..a96e9ae0e4 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -1,4 +1,10 @@ +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 { SharingRuleSchema } from '@objectstack/spec/security'; + import { validateStackExpressions } from './validate-expressions.js'; describe('validateStackExpressions (ADR-0032 build-time)', () => { @@ -60,7 +66,7 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { it('validates object validation-rule predicates too', () => { const issues = validateStackExpressions({ objects: [ - { name: 'crm_lead', fields: { rating: {} }, validations: [{ name: 'r1', expression: '{record.rating} > 0' }] }, + { name: 'crm_lead', fields: { rating: {} }, validations: [{ name: 'r1', condition: '{record.rating} > 0' }] }, ], }); expect(issues).toHaveLength(1); @@ -190,7 +196,7 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { objects: [{ name: 'crm_lead', fields: { lead_score: { type: 'number' } }, - validations: [{ name: 'lead_score_range', expression: 'lead_score != null && lead_score > 100' }], + validations: [{ name: 'lead_score_range', condition: 'lead_score != null && lead_score > 100' }], }], }); expect(issues).toHaveLength(1); @@ -207,7 +213,7 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { probability: { type: 'percent' }, expected_revenue: { type: 'formula', name: 'expected_revenue', formula: 'record.amount * record.probability / 100' }, }, - validations: [{ name: 'amt', expression: 'record.amount != null && record.amount >= 0' }], + validations: [{ name: 'amt', condition: 'record.amount != null && record.amount >= 0' }], }], }); expect(issues).toHaveLength(0); @@ -302,7 +308,7 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { objects: [{ name: 'crm_lead', fields: { lead_score: { type: 'number' } }, - validations: [{ name: 'r', expression: 'lead_score > 100' }], + validations: [{ name: 'r', condition: 'lead_score > 100' }], }], }); expect(issues).toHaveLength(1); @@ -347,7 +353,7 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { // rejects that shape at authoring now. Soundness (this block's // subject) and null-guarding are separate verdicts; the predicate has // to satisfy both to produce zero issues. - validations: [{ name: 'future', expression: 'record.close_date != null && record.close_date >= today()' }], + validations: [{ name: 'future', condition: 'record.close_date != null && record.close_date >= today()' }], }], }); expect(issues).toHaveLength(0); @@ -358,7 +364,7 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { objects: [{ name: 'crm_lead', fields: { title: { type: 'text' } }, - validations: [{ name: 'r', expression: 'record.title > 5' }], + validations: [{ name: 'r', condition: 'record.title > 5' }], }], }); const w = issues.filter(i => i.severity === 'warning'); @@ -1170,3 +1176,569 @@ describe('null-guard gate (#4763)', () => { }); }); }); + +/** + * ── The structural meta-guard (#4992 pattern, #5009/#5018 shape) — #5017 ───── + * + * This rule is registered `input: 'parsed'` (`authoring-rules.ts`), so on the + * compile path it sees `ObjectStackSchema`'s output. #5017 found five `??` + * alias chains here reaching for keys the spec does not declare. Four were the + * familiar inert kind. The fifth was not: + * + * rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula + * + * — the canonical `condition` in THIRD position, behind two names + * `validation.zod.ts` rejects. See the reverse-verification block below for + * the behaviour that cost. + * + * Two guards pin the property that made all five removable: + * + * 1. **Declared-key guard** — every key read off a stack, object, field, + * validation rule, sharing rule, action, hook, flow, node or edge must + * appear in that surface's own Zod `.shape`. Scanning the SOURCE rather than + * the behaviour is deliberate: an unreachable branch has no behaviour to + * assert on, which is exactly the problem. + * + * 2. **Reachability guard** — every changed read must be reached by a fixture + * that passes `ObjectStackSchema.safeParse` outright. + * + * **Scope, stated rather than implied.** The declared-key guard covers every + * receiver in the file except the flow-node CONFIG (`cfg` / `startCfg`), which + * is excused for a reason that is itself a schema fact — see + * `NOT_A_SINGLE_SHAPE` below. The reachability guard is scoped to the reads + * #5017 changed plus the surfaces they sit on; this rule is 660 lines with + * `issues.push` sites that carry no rule id or path template (its finding shape + * predates `{ rule, path, hint }`), so the #5018-style push-site scan does not + * transfer, and a full push-site inventory belongs to #5017's own suggestion 3 + * — one shared guard across every `input: 'parsed'` rule. + */ +const RULE_SOURCE = readFileSync(new URL('./validate-expressions.ts', import.meta.url), 'utf8'); + +/** The rule's CODE — comments stripped, since the guards scan reads, not prose. */ +const RULE_CODE = RULE_SOURCE.replace(/\/\*[\s\S]*?\*\//g, '').replace(/\/\/[^\n]*/g, ''); + +function keysReadOff(receiver: string): string[] { + const re = new RegExp(`\\b${receiver}\\??\\.([A-Za-z_$][\\w$]*)`, 'g'); + return [...new Set([...RULE_CODE.matchAll(re)].map((m) => m[1]))].sort(); +} + +/** + * Declared keys of a schema, unwrapping optional / array / record / lazy / + * union layers. A union answers the UNION of its members' keys: a validation + * rule is exactly one of six variants, and a key any variant declares is one + * some author may legitimately write. + * + * `lazySchema` proxies a FUNCTION target, so the `typeof` guard admits both — + * miss that and every lazily-built schema answers "declares nothing", which + * would silently make this guard vacuous. + */ +function shapeKeysOf(schema: unknown, depth = 0): string[] { + const s = schema as { shape?: Record; _def?: Record; unwrap?: () => unknown }; + if (!s || (typeof s !== 'object' && typeof s !== 'function') || depth > 12) return []; + if (s.shape) return Object.keys(s.shape); + const d = (s._def ?? {}) as Record; + if (d.type === 'union' && Array.isArray(d.options)) { + return [...new Set((d.options as unknown[]).flatMap((o) => shapeKeysOf(o, depth + 1)))]; + } + const getter = d.getter as (() => unknown) | undefined; + for (const next of [d.innerType, d.element, d.valueType, getter?.(), d.in, d.out]) { + const r = shapeKeysOf(next, depth + 1); + if (r.length) return r; + } + if (typeof s.unwrap === 'function') return shapeKeysOf(s.unwrap(), depth + 1); + return []; +} + +const flowShape = () => { + const s = ObjectStackSchema.shape.flows as unknown as { _def?: Record }; + // flows: optional(array(lazy(object))) — walk to the element's own shape. + let cur: unknown = s; + for (let i = 0; i < 12; i++) { + const c = cur as { shape?: Record; _def?: Record }; + if (c?.shape) return c.shape; + const d = (c?._def ?? {}) as Record; + cur = d.innerType ?? d.element ?? (d.getter as (() => unknown) | undefined)?.(); + } + return {} as Record; +}; + +/** + * The one receiver this table does not cover, and why it is a schema fact + * rather than an exemption of convenience. + * + * A flow node's `config` has no single `.shape`: it is discriminated on the + * node's `type`, and the expression slots inside it are resolved through the + * descriptor registry (`resolveFlowNodeExpressions`, #4027) rather than read by + * name. The named reads that do remain — `cfg.function` / `cfg.functionName` + * and the retired `actionType` / `template` / `recipients` / `variables` / + * `script` — are NOT the #5017 defect: `functionName` is a key + * `schemaless-node-config.zod.ts` DECLARES and the ADR-0087 D2 conversion + * `flow-node-script-config-aliases` (#3796) rewrites at load, and the retired + * five are deliberately detected on the pre-parse tier so `os lint` can hand + * the author a named replacement instead of a bare "unrecognized key" (#4343). + * Reading a key to REJECT it is the opposite of reading a key to honour it. + */ +const NOT_A_SINGLE_SHAPE = ['cfg', 'startCfg']; + +const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: string; keys: () => string[] }> = [ + { + receiver: 'stack', + expected: ['actions', 'flows', 'hooks', 'objects', 'sharingRules'], + declaredBy: 'ObjectStackSchema', + keys: () => Object.keys(ObjectStackSchema.shape), + }, + { + receiver: 'obj', + // `validationRules` is absent — one of the five #5017 removed. + expected: ['actions', 'fields', 'name', 'validations'], + declaredBy: 'ObjectSchema', + keys: () => Object.keys(ObjectSchema.shape), + }, + { + receiver: 'def', + // `referenceTo` is absent — likewise. + expected: ['defaultValue', 'options', 'reference', 'required', 'type'], + declaredBy: 'FieldSchema', + keys: () => Object.keys(FieldSchema.shape), + }, + { + receiver: 'rule', + // `expression` / `predicate` / `formula` are absent — the chain that put + // the canonical `condition` in third place (#5017). + expected: ['condition', 'name', 'when'], + declaredBy: 'the ObjectSchema.validations[] union', + keys: () => shapeKeysOf(ObjectSchema.shape.validations), + }, + { + receiver: 'sharingRule', + // `criteria` / `predicate` are absent — likewise. + expected: ['condition', 'name', 'object'], + declaredBy: 'SharingRuleSchema', + keys: () => Object.keys(SharingRuleSchema.shape), + }, + { + receiver: 'action', + // `object` is absent — the action schema's canonical key is `objectName`. + expected: ['disabled', 'name', 'objectName', 'visible'], + declaredBy: 'ObjectStackSchema.actions[]', + keys: () => shapeKeysOf(ObjectStackSchema.shape.actions), + }, + { + receiver: 'hook', + expected: ['condition', 'name', 'object'], + declaredBy: 'ObjectStackSchema.hooks[]', + keys: () => shapeKeysOf(ObjectStackSchema.shape.hooks), + }, + { + receiver: 'flow', + expected: ['name', 'nodes'], + declaredBy: 'ObjectStackSchema.flows[]', + keys: () => shapeKeysOf(ObjectStackSchema.shape.flows), + }, + { receiver: 'node', expected: ['config', 'id', 'type'], declaredBy: 'flows[].nodes[]', keys: () => shapeKeysOf(flowShape().nodes) }, + { receiver: 'startNode', expected: ['config'], declaredBy: 'flows[].nodes[]', keys: () => shapeKeysOf(flowShape().nodes) }, + { + receiver: 'edge', + expected: ['condition', 'id', 'source', 'target'], + declaredBy: 'flows[].edges[]', + keys: () => shapeKeysOf(flowShape().edges), + }, + { + receiver: 'rec', + expected: ['dialect', 'source'], + declaredBy: 'ExpressionInputSchema', + keys: () => shapeKeysOf(ExpressionInputSchema), + }, +]; + +/** + * The ONE read in this file that is still undeclared, tracked rather than + * silently tolerated. + * + * `FieldSchema` declares the computed slot as `expression` and rejects + * `formula` by name ("Did you mean `formula` → `expression`?"), so the + * field-formula pass has never run against a stack that parses. It is NOT + * fixed here because converging it would ACTIVATE a check rather than delete a + * dead branch — a coverage change with its own verification to do, not + * dead-code removal. Filed as #5026. + * + * This list must shrink, never grow: a second entry means the next author + * treated it as a place to put exceptions rather than a debt to pay down. + */ +const TRACKED_UNDECLARED_READS: Array<{ receiver: string; key: string; issue: number }> = [ + { receiver: 'f', key: 'formula', issue: 5026 }, +]; + +describe('validateStackExpressions — reads only keys the spec declares (meta-test, #5017)', () => { + it.each(READ_SURFACES)('every key read off `$receiver` is declared by $declaredBy', (surface) => { + const read = keysReadOff(surface.receiver); + // Exact match, so ADDING a read (or renaming a loop variable, which would + // silently disarm the scan) forces a deliberate visit to this table. + expect(read).toEqual(surface.expected); + const declared = surface.keys(); + expect(declared.length, `${surface.declaredBy} resolved to no keys — the guard would be vacuous`).toBeGreaterThan(0); + expect(read.filter((k) => !declared.includes(k))).toEqual([]); + }); + + it('the field receiver reads only declared keys, plus exactly the tracked debt', () => { + const read = keysReadOff('f'); + expect(read).toEqual(['formula', 'name', '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(['formula']); + expect(read.filter((k) => !declared.includes(k) && !tracked.includes(k))).toEqual([]); + // And the debt is real, not a stale entry: `formula` genuinely is not there. + expect(declared).not.toContain('formula'); + expect(declared).toContain('expression'); + }); + + it('the computed key lists are spelled from the declaring schema', () => { + // `rule[branch]` and `(f as AnyRec)[key]` are COMPUTED reads the scan above + // cannot see; the word lists they index with are checked here instead. + const branches = /for \(const branch of \[([^\]]*)\] as const\)/.exec(RULE_CODE); + expect(branches, 'the nested-rule branch loop moved — update this guard').not.toBeNull(); + const branchKeys = [...branches![1].matchAll(/'([^']+)'/g)].map((m) => m[1]); + expect(branchKeys).toEqual(['then', 'otherwise']); + expect(branchKeys.filter((k) => !shapeKeysOf(ObjectSchema.shape.validations).includes(k))).toEqual([]); + + const fieldKeys = /for \(const key of \[([^\]]*)\] as const\)/.exec(RULE_CODE); + expect(fieldKeys, 'the field-predicate loop moved — update this guard').not.toBeNull(); + const slots = [...fieldKeys![1].matchAll(/'([^']+)'/g)].map((m) => m[1]); + expect(slots).toEqual(['requiredWhen', 'readonlyWhen', 'conditionalRequired', 'visibleWhen']); + expect(slots.filter((k) => !Object.keys(FieldSchema.shape).includes(k))).toEqual([]); + }); + + it('covers every receiver in the source that is not explicitly excused', () => { + const receivers = [...new Set([...RULE_CODE.matchAll(/\b([a-z][\w$]*)\??\.[A-Za-z_$]/g)].map((m) => m[1]))]; + const tabled = new Set([...READ_SURFACES.map((s) => s.receiver), ...NOT_A_SINGLE_SHAPE, 'f']); + // Locals whose "keys" are JS methods / this file's own plumbing. + const PLUMBING = new Set([ + 'issues', 'idx', 'out', 'kept', 'seen', 'seenActions', 'nullable', 'nullableFields', 'nullableIndex', + 'fieldIndex', 'fieldTypeIndex', 'fields', 'nodes', 'options', 'targets', 'retired', 'ref', 'roots', + 'res', 'graph', 'found', 'e', 'w', 'p', 'n', 'issue', 'guards', 'config', + ]); + expect(receivers.filter((r) => !tabled.has(r) && !PLUMBING.has(r))).toEqual([]); + }); +}); + +/** + * ── The alias spellings this rule deliberately does NOT read (#5017) ───────── + */ +const MANIFEST = { id: 'expr_probe', name: 'expr_probe', version: '1.0.0', type: 'app' } as const; + +/** A fixture is only a fixture if the spec accepts it. */ +function specValid(stack: Record): Record { + const result = ObjectStackSchema.safeParse({ manifest: MANIFEST, ...stack }); + if (!result.success) { + throw new Error( + `fixture is not spec-valid (#5017): ` + + result.error.issues.map((i) => `${i.path.join('.') || '(root)'}: ${i.message}`).join('; '), + ); + } + return result.data as unknown as Record; +} + +/** + * The alias spellings, the canonical key each is rejected in favour of, and + * what this rule says about a stack that uses one AFTER #5017. + * + * `lintAfter` is spelled per case rather than assumed uniform, because it is + * not uniform, and the difference is the honest part. Six of the seven fall + * silent — the read they fed is gone, so the author's only diagnostic comes + * from the schema, by name. The seventh (`actions[].object`) keeps a + * diagnostic: dropping the alias costs the rule the action's OBJECT, not the + * predicate, so the object-INDEPENDENT half of the check (bare-reference scope) + * still fires while the field-existence half no longer can. + */ +const REJECTED_ALIASES: Array<{ label: string; stack: Record; refusal: RegExp; lintAfter: RegExp | null }> = [ + { + label: 'objects[].validationRules → validations', + stack: { + objects: [{ name: 'crm_lead', label: 'Lead', sharingModel: 'private', fields: { lead_score: { type: 'number', label: 'S' } }, + validationRules: [{ type: 'script', name: 'r', message: 'm', condition: 'lead_score > 100' }] }], + }, + refusal: /Unrecognized key\(s\) on this object: `validationRules`.*Did you mean `validationRules` → `validations`/s, + lintAfter: null, + }, + { + label: 'validations[].expression → condition', + stack: { + objects: [{ name: 'crm_lead', label: 'Lead', sharingModel: 'private', fields: { lead_score: { type: 'number', label: 'S' } }, + validations: [{ type: 'script', name: 'r', message: 'm', expression: 'lead_score > 100' }] }], + }, + refusal: /Did you mean `expression` → `condition`/, + lintAfter: null, + }, + { + label: 'validations[].predicate → condition', + stack: { + objects: [{ name: 'crm_lead', label: 'Lead', sharingModel: 'private', fields: { lead_score: { type: 'number', label: 'S' } }, + validations: [{ type: 'script', name: 'r', message: 'm', predicate: 'lead_score > 100' }] }], + }, + refusal: /Did you mean `predicate` → `condition`/, + lintAfter: null, + }, + { + label: 'validations[].formula → condition', + stack: { + objects: [{ name: 'crm_lead', label: 'Lead', sharingModel: 'private', fields: { lead_score: { type: 'number', label: 'S' } }, + validations: [{ type: 'script', name: 'r', message: 'm', formula: 'lead_score > 100' }] }], + }, + refusal: /Did you mean `formula` → `condition`/, + lintAfter: null, + }, + { + label: 'sharingRules[].criteria → condition', + stack: { + sharingRules: [{ name: 'sr', type: 'criteria', object: 'crm_lead', sharedWith: { type: 'team', value: 't' }, criteria: 'lead_score > 1' }], + }, + refusal: /Unrecognized key\(s\) on this sharing rule: `criteria`.*Did you mean `criteria` → `condition`/s, + lintAfter: null, + }, + { + label: 'fields[].referenceTo → reference', + stack: { + objects: [{ name: 'child', label: 'C', sharingModel: 'controlled_by_parent', + fields: { p: { type: 'master_detail', label: 'P', referenceTo: 'invoice' } } }], + }, + refusal: /Unrecognized key\(s\) on this field: `referenceTo`.*Did you mean `referenceTo` → `reference`/s, + lintAfter: null, + }, + { + label: 'actions[].object → objectName', + stack: { + actions: [{ name: 'mark_done', label: 'Mark', object: 'crm_lead', type: 'script', target: 'fn', visible: 'lead_score > 1' }], + }, + refusal: /Unrecognized key\(s\) on this action: `object`.*Did you mean `object` → `objectName`/s, + // Object-INDEPENDENT half survives; the field-existence half cannot. + lintAfter: /bare reference `lead_score`/, + }, +]; + +describe('validateStackExpressions — undeclared keys are the schema’s job, not this rule’s (#5017)', () => { + it.each(REJECTED_ALIASES)('$label: the schema refuses it BY NAME, so no consumer needs a fallback', ({ stack, refusal }) => { + const result = ObjectStackSchema.safeParse({ manifest: MANIFEST, ...stack }); + expect(result.success).toBe(false); + expect(result.error!.issues.map((i) => i.message).join(' ')).toMatch(refusal); + }); + + it.each(REJECTED_ALIASES)('$label: the lint defers to the schema', ({ stack, lintAfter }) => { + // Reverse verification, canonical-first direction (#5018's lesson): before + // #5017 each of these produced a lint diagnostic keyed on the ALIAS, for a + // stack `os validate` refuses by name. Alias tolerance belongs at the + // producer's refusal, not in a consumer (Prime Directive #12). + const issues = validateStackExpressions(stack); + if (lintAfter === null) { + expect(issues).toEqual([]); + } else { + expect(issues.map((i) => i.message).join(' ')).toMatch(lintAfter); + } + }); +}); + +/** + * ── Where removing an alias limb ADDS a diagnostic rather than removing one ── + * + * #5018's reverse verification ran one way: canonical-first chains over-reached + * on illegal spellings, and the fix handed those back to the schema. One of + * #5017's chains feeds a COUNT rather than a predicate, so removing the alias + * limb moves the count and can make a downstream gate fire that did not before. + * Pinned here rather than left to be discovered, and it only ever applies to a + * stack `os validate` already refuses by name. + */ +describe('validateStackExpressions — the alias limb that fed a count, not a predicate (#5017)', () => { + const withRefKey = (key: string) => ({ + objects: [{ + name: 'line_item', + fields: { + invoice_ref: { type: 'master_detail', [key]: 'invoice' }, + locked: { type: 'text', readonlyWhen: 'parent.posted == true' }, + }, + }], + }); + + it('`reference`: one master ⇒ `parent` binds ⇒ silent, exactly as before', () => { + expect(validateStackExpressions(withRefKey('reference'))).toEqual([]); + }); + + it('`referenceTo`: the master is no longer counted, so the parent-scope gate now fires', () => { + // BEFORE #5017 the alias limb counted this field, the object looked like it + // had exactly one master, and the gate stayed silent. Now the count is 0 + // and the author is told `parent` cannot bind. On a stack that PARSES the + // two agree — a parsing stack cannot spell `referenceTo` at all (see the + // rejected-alias block above) — so the difference exists only on the + // pre-parse tier, where the stack is already refused by name for this very + // key. An added diagnostic on an invalid stack, not a new verdict on a + // valid one: acceptable, and pinned so it stays a decision rather than a + // surprise. + const issues = validateStackExpressions(withRefKey('referenceTo')); + expect(issues).toHaveLength(1); + expect(issues[0].message).toMatch(/declares no `master_detail` relationship/); + }); +}); + +/** + * ── Reverse verification for the one chain whose behaviour really changed ──── + * + * rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula + * + * The other four chains put the canonical key FIRST, so their alias limbs were + * simply unreachable. This one put it THIRD. For a rule carrying both spellings + * the alias SHORT-CIRCUITED the canonical key away: the lint validated the + * predicate the schema rejects and never looked at the one the author declared. + * Producer and consumer gave two different accounts of the same metadata. + * + * The old chain is reconstructed here rather than described, so the difference + * is demonstrated rather than asserted. + */ +const OLD_CHAIN = (rule: Record): unknown => + rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula; + +describe('validateStackExpressions — the alias short-circuit that #5017 removed', () => { + const ruleWithBoth = { + type: 'script', name: 'r', message: 'm', + condition: 'record.no_such_field > 0', // the CANONICAL predicate — and it is broken + expression: 'record.lead_score > 0', // a REJECTED alias — and it is clean + }; + const stackWithBoth = { + objects: [{ name: 'crm_lead', fields: { lead_score: { type: 'number' } }, validations: [ruleWithBoth] }], + }; + + it('the old chain picked the rejected alias over the declared key', () => { + expect(OLD_CHAIN(ruleWithBoth)).toBe('record.lead_score > 0'); + expect(OLD_CHAIN(ruleWithBoth)).not.toBe(ruleWithBoth.condition); + }); + + it('so the broken CANONICAL predicate went unreported — the rule read the alias instead', () => { + // What the rule used to say about this stack: nothing about + // `no_such_field`. Its verdict was entirely about the alias's source. + const oldVerdict = validateStackExpressions({ + objects: [{ name: 'crm_lead', fields: { lead_score: { type: 'number' } }, + validations: [{ ...ruleWithBoth, condition: OLD_CHAIN(ruleWithBoth) }] }], + }); + expect(oldVerdict.map((i) => i.message).join(' ')).not.toMatch(/no_such_field/); + }); + + it('now it reads the declared key, and the real defect surfaces', () => { + const issues = validateStackExpressions(stackWithBoth); + expect(issues).toHaveLength(1); + expect(issues[0].message).toMatch(/unknown field `no_such_field`/); + }); + + it('and either way the stack does not parse — the alias is refused by name', () => { + const result = ObjectStackSchema.safeParse({ + manifest: MANIFEST, + objects: [{ name: 'crm_lead', label: 'Lead', sharingModel: 'private', + fields: { lead_score: { type: 'number', label: 'S' } }, validations: [ruleWithBoth] }], + }); + expect(result.success).toBe(false); + expect(result.error!.issues.map((i) => i.message).join(' ')).toMatch(/Did you mean `expression` → `condition`/); + }); + + it('on every stack that DOES parse the verdict is unchanged: the alias limbs were dead', () => { + // The same defect, spelled the only way the spec accepts. + const issues = validateStackExpressions( + specValid({ + objects: [{ name: 'crm_lead', label: 'Lead', sharingModel: 'private', + fields: { lead_score: { type: 'number', label: 'S' } }, + validations: [{ type: 'script', name: 'r', message: 'm', condition: 'record.no_such_field > 0' }] }], + }), + ); + expect(issues).toHaveLength(1); + expect(issues[0].message).toMatch(/unknown field `no_such_field`/); + }); +}); + +/** + * ── Reachability: every changed read fires from a stack that parses ────────── + * + * Removing a fallback is only safe if the canonical limb is genuinely live. + * Each fixture below goes through `specValid` (a real `ObjectStackSchema.parse`) + * and must still produce the finding the changed read feeds. + */ +describe('validateStackExpressions — every changed read is reachable from a spec-valid stack (meta-test, #5017)', () => { + const LEAD = { name: 'crm_lead', label: 'Lead', sharingModel: 'private', fields: { lead_score: { type: 'number', label: 'S' } } }; + + it('objects[].validations[].condition — the canonical predicate is read', () => { + const issues = validateStackExpressions( + specValid({ objects: [{ ...LEAD, validations: [{ type: 'script', name: 'r', message: 'm', condition: 'record.nope > 0' }] }] }), + ); + expect(issues.map((i) => i.message).join(' ')).toMatch(/unknown field `nope`/); + expect(issues[0].where).toContain("validation 'r'"); + }); + + it('nested conditional `then` still reaches the null-guard gate through `condition`', () => { + const issues = validateStackExpressions( + specValid({ + objects: [{ + ...LEAD, + validations: [{ + type: 'conditional', name: 'c', message: 'm', when: 'record.lead_score != null', + then: { type: 'script', name: 'inner', message: 'm2', condition: 'record.lead_score > 0' }, + }], + }], + }), + ); + // `lead_score` is nullable (no `required`, no default) and `>` is applied to + // it inside the nested rule — the #4763 verdict, reached via `rule.condition`. + expect(issues.map((i) => i.message).join(' ')).toMatch(/compares a value that is null/); + }); + + it('sharingRules[].condition — the canonical predicate is read', () => { + const issues = validateStackExpressions( + specValid({ + objects: [LEAD], + sharingRules: [{ name: 'sr', type: 'criteria', object: 'crm_lead', sharedWith: { type: 'team', value: 't' }, condition: 'nope > 0' }], + }), + ); + expect(issues.map((i) => i.message).join(' ')).toMatch(/bare reference `nope`/); + expect(issues[0].where).toContain("sharingRule 'sr'"); + }); + + it('actions[].objectName — the canonical key still binds the action to its object', () => { + const issues = validateStackExpressions( + specValid({ + objects: [LEAD], + actions: [{ name: 'mark_done', label: 'Mark', objectName: 'crm_lead', type: 'script', target: 'fn', visible: 'record.nope' }], + }), + ); + // The object binding is what makes `nope` reportable as an unknown FIELD + // rather than merely unparsed — i.e. `objectName` really was read. + expect(issues.map((i) => i.message).join(' ')).toMatch(/unknown field `nope`/); + }); + + it('fields[].reference — the master-detail count still sees the parent', () => { + // Exactly one master ⇒ `parent` binds ⇒ NO finding. That verdict depends on + // `masterDetailCount` reading `reference`; if the read were broken the count + // would be 0 and this would fire. + expect( + validateStackExpressions( + specValid({ + objects: [{ + name: 'line_item', label: 'Line', sharingModel: 'controlled_by_parent', + fields: { + invoice_ref: { type: 'master_detail', label: 'Invoice', reference: 'invoice' }, + locked: { type: 'text', label: 'Locked', readonlyWhen: 'parent.posted == true' }, + }, + }], + }), + ), + ).toEqual([]); + + // Two masters ⇒ no single `parent` ⇒ the gate fires. Same read, other side. + const issues = validateStackExpressions( + specValid({ + objects: [{ + name: 'line_item', label: 'Line', sharingModel: 'controlled_by_parent', + fields: { + invoice_ref: { type: 'master_detail', label: 'Invoice', reference: 'invoice' }, + order_ref: { type: 'master_detail', label: 'Order', reference: 'sales_order' }, + locked: { type: 'text', label: 'Locked', readonlyWhen: 'parent.posted == true' }, + }, + }], + }), + ); + expect(issues).toHaveLength(1); + expect(issues[0].message).toMatch(/declares 2 `master_detail` relationships/); + }); +}); diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 7a2531423c..ab6f53f1cc 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -21,6 +21,55 @@ * test dominates is rejected here, so the `has(a) && has(b) && a < b` trap * (which reads as a guard and is not one) never reaches a production write. * See `validate-null-guards.ts` for the decision procedure and its scope. + * + * ## Scope — the keys this rule reads, and the ones it deliberately does not + * + * This rule is registered `input: 'parsed'` (`authoring-rules.ts`), so what it + * sees on the compile path is what `ObjectStackSchema` returned. Every key it + * reads is one `@objectstack/spec` DECLARES. That is a contract, not a style + * preference: the strict sub-schemas reject an undeclared key by NAME, so a + * branch keyed on one is inert for every stack an author can ship (#4984, + * #5009, #5017). + * + * | Read | Declared by | + * |----------------------------------------|-----------------------------------| + * | `objects[].validations[]` | `ObjectSchema` | + * | `validations[].condition` / `.when` / `.then` / `.otherwise` | the six `*ValidationSchema` variants | + * | `objects[].fields[].reference` | `FieldSchema` | + * | `actions[].objectName` | the action schema | + * | `sharingRules[].condition` / `.object` | `SharingRuleSchema` | + * + * NOT read, and each for a reason that is a schema fact (all verified against + * the live `.shape` in `validate-expressions.test.ts`): + * + * - `objects[].validationRules` — `ObjectSchema` declares `validations` and is + * strict; the alias is refused with "Did you mean `validationRules` → + * `validations`?". + * - `validations[].expression` / `.predicate` / `.formula` / `.rule` — the four + * names `validation.zod.ts` lists in `aliases: { … : 'condition' }`, i.e. the + * ones it rejects by name. This one was the worst of the family: the chain + * read them BEFORE `condition`, so for a rule carrying both spellings the + * canonical predicate was short-circuited away and the rejected alias + * validated in its place. Producer and consumer gave two different accounts + * of the same metadata (#5017). + * - `sharingRules[].criteria` / `.predicate` — `criteria` is the runtime's own + * spelling of the COMPILED predicate (`criteria_json`), mapped back to the + * authored `condition` in the schema's rejection (#3896); `predicate` is + * refused outright. #4984 removed the same pair from the org-axis rule. + * - `objects[].fields[].referenceTo` — `field.zod.ts:331` maps it (with + * `relatedTo` / `target` / `targetObject` / `lookupObject`) to `reference`. + * - `actions[].object` — the action schema's own rejection says "Did you mean + * `object` → `objectName`?". + * + * Alias tolerance belongs at the schema's refusal, not in a consumer (Prime + * Directive #12) — in a consumer it also converts a loud, named rejection into + * a silently-inert (or, above, silently-WRONG) gate. + * + * One read here is still undeclared and is tracked rather than fixed in place: + * the field-formula pass below reads `f.formula`, which `FieldSchema` rejects + * in favour of `expression`. Converging it would ACTIVATE a check that has + * never run against a parsing stack, which is a coverage change, not dead-code + * removal — see the note at that call site and the tracking issue. */ import { validateExpression, collectCelRootIdentifiers } from '@objectstack/formula'; @@ -175,7 +224,12 @@ function masterDetailCount(obj: AnyRec): number { let n = 0; for (const [, def] of fieldEntries(obj)) { if (def.type !== 'master_detail') continue; - const ref = def.reference ?? def.referenceTo; + // `reference` is the ONLY spelling `FieldSchema` declares. `referenceTo` + // (with `relatedTo` / `target` / `targetObject` / `lookupObject`) is a + // rejected alias — `field.zod.ts:331` maps it to `reference` in the strict + // error map, so a field spelling it does not parse (#5017). See the + // `## Scope` table on this module for why a consumer must not re-admit it. + const ref = def.reference; if (typeof ref === 'string' && ref.trim() !== '') n += 1; } return n; @@ -202,7 +256,15 @@ function rulePredicates(rule: AnyRec, path: string): Array<{ label: string; raw: const out: Array<{ label: string; raw: unknown }> = []; const name = typeof rule.name === 'string' ? rule.name : '?'; const here = path ? `${path} → '${name}'` : `'${name}'`; - const main = rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula; + // `condition` is the declared predicate key on every validation-rule variant + // that has one (`script`, `cross_field`). `expression` / `predicate` / + // `formula` / `rule` are the four names `validation.zod.ts` REJECTS by name + // (`aliases: { formula: 'condition', expression: 'condition', predicate: + // 'condition', rule: 'condition' }`), so a rule spelling any of them does not + // parse. Reading them here put the canonical key in THIRD position — an + // author who wrote both `condition` and a rejected alias had their canonical + // predicate short-circuited away and the alias validated instead (#5017). + const main = rule.condition; if (main != null) out.push({ label: `validation rule ${here}`, raw: main }); if (rule.when != null) out.push({ label: `validation rule ${here} when-predicate`, raw: rule.when }); for (const branch of ['then', 'otherwise'] as const) { @@ -413,12 +475,15 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // ── Object validation-rule + formula predicates ──────────────────── for (const obj of objects) { const objectName = typeof obj.name === 'string' ? obj.name : undefined; - const validations = obj.validations ?? obj.validationRules; + // `validations` is the key `ObjectSchema` declares; `validationRules` is a + // rejected alias of it (#5017) — see the `## Scope` table above. + const validations = obj.validations; for (const rule of asArray(validations)) { const where = `object '${objectName}' · validation '${(rule.name as string) ?? '?'}'`; - // Common predicate keys across rule shapes. Validation predicates are - // `record`-scoped — no field flattening — so bare refs are flagged (#1928). - check(where, rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula, objectName, 'record'); + // The declared predicate key is `condition` (see `rulePredicates`). + // Validation predicates are `record`-scoped — no field flattening — so + // bare refs are flagged (#1928). + check(where, rule.condition, objectName, 'record'); // `conditional` rules carry a nested `when` predicate (record-scoped). check(`${where} when`, (rule as AnyRec).when, objectName, 'record'); // #4763 — null-guard gate over every predicate the rule carries, nested @@ -551,9 +616,11 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // `disabled` may be a boolean (skip) or a predicate (check). const seenActions = new Set(); const checkAction = (where: string, action: AnyRec, objectName?: string): void => { + // `objectName` is the declared key on an action; `object` is the rejected + // alias the strict error map maps back to it ("Did you mean `object` → + // `objectName`?"), so an action spelling it does not parse (#5017). const obj = objectName - ?? (typeof action.objectName === 'string' ? action.objectName : undefined) - ?? (typeof action.object === 'string' ? action.object : undefined); + ?? (typeof action.objectName === 'string' ? action.objectName : undefined); const name = typeof action.name === 'string' ? action.name : '?'; const key = `${obj ?? ''}:${name}`; if (seenActions.has(key)) return; // de-dup (actions are merged onto objects AND kept top-level) @@ -589,10 +656,19 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // ── Sharing-rule predicates (security-critical, record-scoped) ───── // A criteria sharing rule's `condition` decides which rows a principal sees. // It is evaluated against the record, so a bare ref silently changes access. - for (const rule of asArray(stack.sharingRules)) { - const ruleObj = typeof rule.object === 'string' ? rule.object : undefined; - const where = `sharingRule '${(rule.name as string) ?? '?'}'${ruleObj ? ` (${ruleObj})` : ''} condition`; - check(where, rule.condition ?? rule.criteria ?? rule.predicate, ruleObj, 'record'); + // Named `sharingRule` rather than `rule` so the declared-key guard in the + // test can tell this receiver apart from the VALIDATION rule one — the two + // are governed by different schemas, and a scan that merged them would let a + // key declared by either schema pass on both (#5017). + for (const sharingRule of asArray(stack.sharingRules)) { + const ruleObj = typeof sharingRule.object === 'string' ? sharingRule.object : undefined; + const where = `sharingRule '${(sharingRule.name as string) ?? '?'}'${ruleObj ? ` (${ruleObj})` : ''} condition`; + // `condition` is the authored key `SharingRuleSchema` declares. `criteria` + // is the RUNTIME spelling of the compiled predicate (`criteria_json`) and + // `sharing.zod.ts` maps it back to `condition` in its rejection message; + // `predicate` is refused with no rename at all. #4984 removed exactly this + // pair one file over; this was the same read left behind (#5017). + check(where, sharingRule.condition, ruleObj, 'record'); } // ── Hook `condition` predicates (record-scoped gate) ─────────────── diff --git a/packages/lint/src/validate-security-posture.test.ts b/packages/lint/src/validate-security-posture.test.ts index b02f1dde67..9978cfc2c2 100644 --- a/packages/lint/src/validate-security-posture.test.ts +++ b/packages/lint/src/validate-security-posture.test.ts @@ -6,7 +6,12 @@ * it"), plus the clean-stack fixture that must stay silent. */ +import { readFileSync } from 'node:fs'; + import { describe, it, expect } from 'vitest'; +import { ObjectStackSchema } from '@objectstack/spec'; +import { FieldSchema, ObjectSchema } from '@objectstack/spec/data'; +import { ObjectPermissionSchema, PermissionSetSchema } from '@objectstack/spec/security'; import { SECURITY_FLS_UNQUALIFIED_KEY, validateSecurityPosture, @@ -64,10 +69,17 @@ describe('validateSecurityPosture (ADR-0090 D7)', () => { expect(rulesOf({ objects: [{ name: 'sys_thing' }, { name: 'custom', isSystem: true }] })).toEqual([]); }); - it('honors sharingModel nested under security.*', () => { + // Was: "honors sharingModel nested under security.*". There is no such + // envelope and there never was — `ObjectSchema` declares the OWD dials flat + // and is strict, so `objects[].security` is REFUSED by name, not stripped. + // The fallback that test pinned could not run for any stack an author can + // ship; all it did was advertise an authorization surface that does not + // exist (#5017). Schema evidence: the "undeclared keys are the schema's job" + // block at the end of this file. + it('does not read an OWD nested under a `security` envelope — no such key', () => { expect( rulesOf({ objects: [{ name: 'ok_obj', security: { sharingModel: 'private' } }] }), - ).toEqual([]); + ).toEqual([SECURITY_OWD_UNSET]); }); // ── Rule: security-owd-alias (ADR-0090 D4) ─────────────────────────── @@ -466,3 +478,393 @@ describe('validateSecurityPosture · book audience (ADR-0046 §6.7 / ADR-0090)', ).toEqual([]); }); }); + +/** + * ── The structural meta-guard (#4992 pattern, #5009/#5018 shape) — #5017 ───── + * + * #4984 removed the `??` alias reads from a sharing rule's fields; #5009 + * removed four more one file over. #5017 found two of the same shape HERE, and + * one of them was the worst of the family: `obj.security?.sharingModel`, a + * fallback onto an object-level `security` envelope that **does not exist** — + * in the security linter, where the next reader is most likely to believe it. + * + * These guards pin the PROPERTY that made those reads removable, so the next + * one fails before review rather than after: + * + * 1. **Declared-key guard** — every key this rule reads off a stack, object, + * field, permission set, permission entry, app, position, book or seed must + * appear in that surface's own Zod `.shape`. Scanning the SOURCE rather than + * the behaviour is deliberate: an unreachable branch has no behaviour to + * assert on, which is exactly the problem. + * + * 2. **Reachability guard** — every `findings.push` site must be reached by a + * fixture the schema raises no `unrecognized_keys` issue on. + * + * Note the criterion, which is NOT #5018's flat `safeParse` success, and the + * difference is this rule's whole point. It is registered `input: 'parsed'` + * but documented to run pre-parse too, so that `os lint` can answer a value + * the zod gate would reject — with a better message (`sharingModel: 'read'` + * is `invalid_value`, and rule `security-owd-alias` exists to name the + * canonical replacement). Demanding a fully-parsing fixture would delete + * four legitimate rules. A rejected KEY is a different animal from a + * rejected VALUE: a key the schema does not declare cannot reach this rule + * on the parsed path at all, and its presence in the source asserts that an + * authoring surface exists. So the corpus below is allowed to carry values + * the schema refuses, and never a key it refuses. + * + * Scope: BOTH guards cover the whole rule (all fifteen `findings.push` sites, + * every receiver except the two named in `NOT_SCHEMA_RECEIVERS` below). + */ +const RULE_SOURCE = readFileSync(new URL('./validate-security-posture.ts', import.meta.url), 'utf8'); + +/** The rule's CODE — comments stripped, since the guards scan reads, not prose. */ +const RULE_CODE = RULE_SOURCE.replace(/\/\*[\s\S]*?\*\//g, '').replace(/\/\/[^\n]*/g, ''); + +/** Distinct property names read off `receiver` in the rule's code. */ +function keysReadOff(receiver: string): string[] { + const re = new RegExp(`\\b${receiver}\\??\\.([A-Za-z_$][\\w$]*)`, 'g'); + return [...new Set([...RULE_CODE.matchAll(re)].map((m) => m[1]))].sort(); +} + +/** + * The declared keys of a schema, unwrapping the optional / array / record / + * lazy / union layers between a collection and its element. + * + * A union answers the UNION of its members' keys, which is the right reading: + * a validation rule or a field is exactly one variant, and a key any variant + * declares is a key some author can legitimately write. + * + * `lazySchema` wraps schemas in a Proxy whose target is a FUNCTION, so the + * `typeof` guard has to admit both — miss that and every lazily-built schema + * silently answers "declares nothing", which would make this guard vacuous. + */ +function shapeKeysOf(schema: unknown, depth = 0): string[] { + const s = schema as { shape?: Record; _def?: Record; unwrap?: () => unknown }; + if (!s || (typeof s !== 'object' && typeof s !== 'function') || depth > 12) return []; + if (s.shape) return Object.keys(s.shape); + const d = (s._def ?? {}) as Record; + if (d.type === 'union' && Array.isArray(d.options)) { + return [...new Set((d.options as unknown[]).flatMap((o) => shapeKeysOf(o, depth + 1)))]; + } + const getter = d.getter as (() => unknown) | undefined; + for (const next of [d.innerType, d.element, d.valueType, getter?.(), d.in, d.out]) { + const r = shapeKeysOf(next, depth + 1); + if (r.length) return r; + } + if (typeof s.unwrap === 'function') return shapeKeysOf(s.unwrap(), depth + 1); + return []; +} + +/** + * Receivers whose keys are NOT a spec `.shape`, and why. Kept as an explicit, + * reasoned list rather than "whatever the table forgot": a receiver that drops + * out of the table silently is how an undeclared read gets back in. + */ +const NOT_SCHEMA_RECEIVERS: Record = { + rec: 'a seed RECORD — its keys are COLUMNS of `sys_user_position` / `sys_user_permission_set` (ADR-0091), not keys of a metadata schema.', + md: "this file's own `firstMasterDetailField` return type, not an authored surface.", +}; + +const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: string; keys: () => string[] }> = [ + { + receiver: 'stack', + expected: ['apps', 'books', 'data', 'objects', 'permissions', 'positions'], + declaredBy: 'ObjectStackSchema', + keys: () => Object.keys(ObjectStackSchema.shape), + }, + { + receiver: 'obj', + // `security` is absent, and that is the #5017 fix: `ObjectSchema` declares + // the OWD dials FLAT and has no `security` envelope to nest them under. + expected: ['actions', 'externalSharingModel', 'fields', 'isSystem', 'label', 'name', 'sharingModel'], + declaredBy: 'ObjectSchema', + keys: () => Object.keys(ObjectSchema.shape), + }, + { receiver: 'o', expected: ['name'], declaredBy: 'ObjectSchema', keys: () => Object.keys(ObjectSchema.shape) }, + { + receiver: 'ps', + expected: ['fields', 'isDefault', 'label', 'name', 'objects'], + declaredBy: 'PermissionSetSchema', + keys: () => Object.keys(PermissionSetSchema.shape), + }, + // `reference_to` is absent — the other half of the #5017 fix. + { receiver: 'def', expected: ['reference'], declaredBy: 'FieldSchema', keys: () => Object.keys(FieldSchema.shape) }, + { receiver: 'f', expected: ['label', 'name', 'type'], declaredBy: 'FieldSchema', keys: () => Object.keys(FieldSchema.shape) }, + { + receiver: 'p', + expected: ['allowCreate', 'allowDelete', 'allowEdit', 'allowRead', 'modifyAllRecords', 'readScope', 'viewAllRecords'], + declaredBy: 'ObjectPermissionSchema', + keys: () => shapeKeysOf(ObjectPermissionSchema), + }, + { + receiver: 'wildcard', + expected: ['modifyAllRecords', 'viewAllRecords'], + declaredBy: 'ObjectPermissionSchema', + keys: () => shapeKeysOf(ObjectPermissionSchema), + }, + { + receiver: 'action', + expected: ['label', 'name'], + declaredBy: 'ObjectSchema.actions[]', + keys: () => shapeKeysOf(ObjectSchema.shape.actions), + }, + { + receiver: 'app', + expected: ['label', 'name'], + declaredBy: 'ObjectStackSchema.apps[]', + keys: () => shapeKeysOf(ObjectStackSchema.shape.apps), + }, + { + receiver: 'pos', + expected: ['label', 'name'], + declaredBy: 'ObjectStackSchema.positions[]', + keys: () => shapeKeysOf(ObjectStackSchema.shape.positions), + }, + { + receiver: 'book', + expected: ['label', 'name'], + declaredBy: 'ObjectStackSchema.books[]', + keys: () => shapeKeysOf(ObjectStackSchema.shape.books), + }, + { + receiver: 'audience', + expected: ['permissionSet'], + declaredBy: 'books[].audience', + keys: () => shapeKeysOf((shapeOf(ObjectStackSchema.shape.books) as Record).audience), + }, + { + receiver: 'seed', + expected: ['object', 'records'], + declaredBy: 'ObjectStackSchema.data[]', + keys: () => shapeKeysOf(ObjectStackSchema.shape.data), + }, +]; + +/** The `.shape` object itself (not just its keys) of a wrapped collection. */ +function shapeOf(schema: unknown, depth = 0): Record { + const s = schema as { shape?: Record; _def?: Record; unwrap?: () => unknown }; + if (!s || (typeof s !== 'object' && typeof s !== 'function') || depth > 12) return {}; + if (s.shape) return s.shape; + const d = (s._def ?? {}) as Record; + const getter = d.getter as (() => unknown) | undefined; + for (const next of [d.innerType, d.element, d.valueType, getter?.(), d.in, d.out]) { + const r = shapeOf(next, depth + 1); + if (Object.keys(r).length) return r; + } + if (typeof s.unwrap === 'function') return shapeOf(s.unwrap(), depth + 1); + return {}; +} + +describe('validateSecurityPosture — reads only keys the spec declares (meta-test, #5017)', () => { + it.each(READ_SURFACES)('every key read off `$receiver` is declared by $declaredBy', (surface) => { + const read = keysReadOff(surface.receiver); + // Exact match, so ADDING a read (or renaming a loop variable, which would + // silently disarm the scan) forces a deliberate visit to this table. + expect(read).toEqual(surface.expected); + const declared = surface.keys(); + expect(declared.length, `${surface.declaredBy} resolved to no keys — the guard would be vacuous`).toBeGreaterThan(0); + expect(read.filter((k) => !declared.includes(k))).toEqual([]); + }); + + it('covers every receiver in the source that is not explicitly excused', () => { + // Without this, a NEW receiver (a new loop variable over a new collection) + // would carry undeclared reads with nothing to notice — the table only + // guards what the table lists. + const receivers = [...new Set([...RULE_CODE.matchAll(/\b([a-z][\w$]*)\??\.[A-Za-z_$]/g)].map((m) => m[1]))]; + const tabled = new Set([...READ_SURFACES.map((s) => s.receiver), ...Object.keys(NOT_SCHEMA_RECEIVERS)]); + // Locals whose "keys" are JS methods / this file's own plumbing, not metadata. + const PLUMBING = new Set([ + 'findings', 'objects', 'permissionSets', 'privateObjects', 'grantedObjects', 'stackSetNames', + 'records', 'reason', 'until', 'setName', 'flsKey', 'opts', 'path', 'i', 'e', 'fields', 'crm_opportunity', + ]); + expect(receivers.filter((r) => !tabled.has(r) && !PLUMBING.has(r))).toEqual([]); + }); +}); + +/** + * ── The two surfaces this rule deliberately does NOT read (#5017) ─────────── + * + * Each is pinned against the schema fact that makes it unreachable, so "put + * the fallback back, just in case" fails a test with the evidence attached + * rather than passing quietly. + */ +const MANIFEST = { id: 'security_probe', name: 'security_probe', version: '1.0.0', type: 'app' } as const; + +/** Does the schema refuse any KEY in this stack (as opposed to any VALUE)? */ +function unrecognizedKeysIn(stack: unknown): string[] { + const result = ObjectStackSchema.safeParse(stack); + if (result.success) return []; + return result.error.issues + .filter((i) => i.code === 'unrecognized_keys') + .map((i) => `${i.path.join('.') || '(root)'}: ${i.message}`); +} + +describe('validateSecurityPosture — undeclared keys are the schema’s job, not this rule’s (#5017)', () => { + it('there is no `objects[].security` envelope: the OWD dials are declared FLAT', () => { + const objectKeys = Object.keys(ObjectSchema.shape); + expect(objectKeys).not.toContain('security'); + expect(objectKeys).toEqual(expect.arrayContaining(['sharingModel', 'externalSharingModel', 'publicSharing'])); + + // And `ObjectSchema` is strict, so this is not a silent strip: a stack + // nesting the OWD under `security` is REFUSED, by name. + expect( + unrecognizedKeysIn({ + manifest: MANIFEST, + objects: [{ name: 'ok_obj', label: 'OK', fields: { a: { type: 'text', label: 'A' } }, security: { sharingModel: 'private' } }], + }).join(' '), + ).toMatch(/Unrecognized key\(s\) on this object: `security`/); + + // So the lint reads `sharingModel` and nothing else. An author who nested + // it gets `security-owd-unset` from here (the OWD really is unset on the + // only key that carries one) and a named refusal from the schema — instead + // of a silent all-clear from a fallback onto a surface that is not there. + expect(rulesOf({ objects: [{ name: 'ok_obj', security: { sharingModel: 'private' } }] })).toEqual([ + SECURITY_OWD_UNSET, + ]); + expect(rulesOf({ objects: [{ name: 'ok_obj', sharingModel: 'private' }] })).toEqual([]); + }); + + it('`fields[].reference_to` is a rejected alias of `reference`', () => { + const fieldKeys = Object.keys(FieldSchema.shape); + expect(fieldKeys).toContain('reference'); + expect(fieldKeys).not.toContain('reference_to'); + expect( + unrecognizedKeysIn({ + manifest: MANIFEST, + objects: [ + { + name: 'child', label: 'Child', sharingModel: 'controlled_by_parent', + fields: { parent_ref: { type: 'master_detail', label: 'Parent', reference_to: 'parent_obj' } }, + }, + ], + }).join(' '), + ).toMatch(/Unrecognized key\(s\) on this field: `reference_to`/); + + // The finding still fires either way — `reference_to` only ever fed the + // master's NAME into the message. On the canonical spelling that name is + // there; on the rejected one the rule now says "master_detail" without a + // target, and the schema says which key to fix. + const withAlias = validateSecurityPosture({ + objects: [{ name: 'child', sharingModel: 'private', fields: [{ name: 'p', type: 'master_detail', reference_to: 'parent_obj' }] }], + permissions: [{ name: 'ps', objects: { other: { allowRead: true } } }], + }).filter((f) => f.rule === SECURITY_MASTER_DETAIL_UNGRANTED); + expect(withAlias).toHaveLength(1); + expect(withAlias[0].message).not.toContain('parent_obj'); + + const withCanonical = validateSecurityPosture({ + objects: [{ name: 'child', sharingModel: 'private', fields: [{ name: 'p', type: 'master_detail', reference: 'parent_obj' }] }], + permissions: [{ name: 'ps', objects: { other: { allowRead: true } } }], + }).filter((f) => f.rule === SECURITY_MASTER_DETAIL_UNGRANTED); + expect(withCanonical).toHaveLength(1); + expect(withCanonical[0].message).toContain('"parent_obj"'); + }); +}); + +/** + * ── Reachability: every branch is reachable without an undeclared key ──────── + */ +function pushedRuleIds(): string[] { + const sites = RULE_CODE.split('findings.push({').slice(1); + return sites.map((block, i) => { + const ruleConst = /rule:\s*([A-Z_][A-Z0-9_]*)/.exec(block)?.[1]; + if (!ruleConst) throw new Error(`findings.push site #${i} has no literal \`rule:\` — the guard cannot map it`); + const id = RULE_IDS[ruleConst]; + if (!id) throw new Error(`findings.push site #${i} emits unknown rule id \`${ruleConst}\``); + return id; + }); +} + +const RULE_IDS: Record = { + SECURITY_OWD_UNSET, + SECURITY_OWD_ALIAS, + SECURITY_EXTERNAL_WIDER, + SECURITY_WILDCARD_VAMA, + SECURITY_ANCHOR_HIGH_PRIVILEGE, + SECURITY_ROLE_WORD, + SECURITY_BOOK_AUDIENCE_UNKNOWN_SET, + SECURITY_PRIVATE_NO_READSCOPE, + SECURITY_MASTER_DETAIL_UNGRANTED, + SECURITY_FLS_UNQUALIFIED_KEY, + SECURITY_GRANT_EXPIRED_AT_AUTHORING, + SECURITY_DELEGATION_MISSING_REASON, +}; + +const TEXT_FIELD = { a: { type: 'text', label: 'A' } } as const; +const objectFixture = (extra: Record) => ({ label: 'X', fields: TEXT_FIELD, ...extra }); + +/** + * One fixture per rule. Every one is a full stack put through + * `unrecognizedKeysIn` below — values the schema refuses are allowed (that is + * what half these rules are FOR), keys it refuses are not. + */ +const REACHABILITY_CORPUS: Array<{ label: string; stack: Record }> = [ + { label: 'owd-unset', stack: { objects: [objectFixture({ name: 'leave_request' })] } }, + { label: 'owd-alias (retired value)', stack: { objects: [objectFixture({ name: 'leave_request', sharingModel: 'read' })] } }, + { label: 'owd-alias (non-canonical value)', stack: { objects: [objectFixture({ name: 'leave_request', sharingModel: 'nonsense' })] } }, + { label: 'owd-alias (external retired value)', stack: { objects: [objectFixture({ name: 'o', sharingModel: 'private', externalSharingModel: 'full' })] } }, + { + label: 'external-wider', + stack: { objects: [objectFixture({ name: 'o', sharingModel: 'private', externalSharingModel: 'public_read_write' })] }, + }, + { label: 'fls-unqualified-key', stack: { permissions: [{ name: 'ps', label: 'PS', objects: {}, fields: { budget: { readable: true } } }] } }, + { label: 'wildcard-vama', stack: { permissions: [{ name: 'ps', label: 'PS', objects: { '*': { viewAllRecords: true } } }] } }, + { + label: 'anchor-high-privilege', + stack: { permissions: [{ name: 'ps', label: 'PS', isDefault: true, objects: { '*': { modifyAllRecords: true } } }] }, + }, + { label: 'role-word (identifier)', stack: { objects: [objectFixture({ name: 'user_role', sharingModel: 'private' })] } }, + { label: 'role-word (label)', stack: { objects: [objectFixture({ name: 'user_duty', label: 'User Role', sharingModel: 'private' })] } }, + { + label: 'book-audience-unknown-set', + stack: { books: [{ name: 'guide', label: 'Guide', slug: 'guide', groups: [], audience: { permissionSet: 'nobody_declares_this' } }] }, + }, + { + label: 'private-no-readscope', + stack: { + objects: [objectFixture({ name: 'todo', sharingModel: 'private' })], + permissions: [{ name: 'ps', label: 'PS', objects: { todo: { allowRead: true } } }], + }, + }, + { + label: 'master-detail-ungranted', + stack: { + objects: [ + objectFixture({ + name: 'line_item', sharingModel: 'controlled_by_parent', + fields: { parent_ref: { type: 'master_detail', label: 'Parent', reference: 'invoice' } }, + }), + ], + permissions: [{ name: 'ps', label: 'PS', objects: { invoice: { allowRead: true } } }], + }, + }, + { + label: 'grant-expired-at-authoring', + stack: { data: [{ object: 'sys_user_position', records: [{ user_id: 'u1', position: 'p', valid_until: '2020-01-01T00:00:00Z' }] }] }, + }, + { + label: 'delegation-missing-reason', + stack: { data: [{ object: 'sys_user_permission_set', records: [{ user_id: 'u1', permission_set: 'ps', delegated_from: 'u2' }] }] }, + }, +]; + +describe('validateSecurityPosture — every branch is reachable without an undeclared key (meta-test, #5017)', () => { + it.each(REACHABILITY_CORPUS)('$label: the fixture uses only keys the spec declares', ({ stack }) => { + expect(unrecognizedKeysIn({ manifest: MANIFEST, ...stack })).toEqual([]); + }); + + it('maps every `findings.push` site in the source', () => { + expect(pushedRuleIds()).toHaveLength(15); + }); + + it('reaches every `findings.push` site from that corpus', () => { + const emitted = new Set( + REACHABILITY_CORPUS.flatMap(({ stack }) => + validateSecurityPosture(stack, { nowMs: Date.parse('2026-07-10T12:00:00Z') }).map((f) => f.rule), + ), + ); + expect( + [...new Set(pushedRuleIds())].filter((id) => !emitted.has(id)), + 'a branch no key-legal stack can reach must be deleted, not kept "just in case" (#5017)', + ).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-security-posture.ts b/packages/lint/src/validate-security-posture.ts index 420cb8cefe..cac68ae0d7 100644 --- a/packages/lint/src/validate-security-posture.ts +++ b/packages/lint/src/validate-security-posture.ts @@ -31,6 +31,27 @@ * Pure `(stack) => Finding[]`; accepts the NORMALIZED stack input (works both * pre- and post-zod-parse, so `os lint` catches what the zod gate would * reject in `os compile` — with a better message). + * + * ## Scope — the keys this rule reads, and the ones it deliberately does not + * + * Registered `input: 'parsed'` (`authoring-rules.ts`), so on the compile path + * it sees `ObjectStackSchema`'s output. Every key it reads is one the spec + * DECLARES, checked structurally against the live `.shape` in + * `validate-security-posture.test.ts`. Two reads that were not, until #5017: + * + * - `objects[].security.sharingModel` — **there is no `security` envelope on an + * object.** `ObjectSchema` declares the OWD dials flat (`sharingModel`, + * `externalSharingModel`, `publicSharing`) and is strict, so a stack nesting + * one under `security` is refused by name rather than stripped. Nothing could + * reach that fallback; what it did instead was describe an authorization + * surface that does not exist, in the security linter of all places. + * - `objects[].fields[].reference_to` — a rejected alias of `reference` + * (`field.zod.ts:331`). + * + * Alias tolerance belongs at the schema's refusal, not in a consumer (Prime + * Directive #12). Here it also silently downgraded a NAMED rejection into an + * inert branch — and an inert branch in a security linter reads, to the next + * author, as a gate that is watching (#4984, #5009, #5017). */ import { describeAnchorForbiddenBits } from '@objectstack/spec/security'; @@ -90,8 +111,20 @@ function asArray(v: unknown): AnyRec[] { return []; } +/** + * The object's org-wide default. + * + * `sharingModel` is the whole of it. There is no `objects[].security` envelope + * to fall back to and there never was: `ObjectSchema.shape` carries + * `sharingModel` / `externalSharingModel` / `publicSharing` flat, declares no + * `security` key, and is strict — a stack nesting the OWD under `security` is + * REFUSED ("Unrecognized key(s) on this object: `security`"), not stripped. So + * the fallback removed in #5017 could not run for any stack an author can ship; + * what it could do is tell the next reader that `object.security.sharingModel` + * is a real authorization surface. See the `## Scope` note on this module. + */ function owdOf(obj: AnyRec): unknown { - return obj.sharingModel ?? (obj.security as AnyRec | undefined)?.sharingModel; + return obj.sharingModel; } function isSystemObject(obj: AnyRec): boolean { @@ -113,9 +146,15 @@ function labelHasRoleWord(label: unknown): boolean { return /\brole(s)?\b/i.test(label); } -/** The `reference`/`reference_to` target a relationship field points at. */ +/** + * The `reference` target a relationship field points at. + * + * `reference` is the only spelling `FieldSchema` declares; `reference_to` (like + * `referenceTo` / `relatedTo` / `target`) is a rejected alias the strict error + * map renames for the author, so a field carrying it does not parse (#5017). + */ function refOf(def: AnyRec): string | undefined { - const r = (def.reference ?? def.reference_to) as unknown; + const r = def.reference as unknown; return typeof r === 'string' && r ? r : undefined; }