From ff2555949f05a4c6545c8f8fc1babbbcfad4a8fc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 02:58:05 +0000 Subject: [PATCH] fix(lint): report a controlled_by_parent object whose master is decided by field declaration order `SecurityPlugin.resolveCbpRelation` resolves a `controlled_by_parent` object's master through three tiers -- a required `master_detail`, then any `master_detail`, then a required `lookup` -- and picks inside a tier with `Array.prototype.find`. Two or more candidates in the tier that WINS therefore resolve by field declaration order, and nothing reports it: not `os validate`, not `os lint`, not a boot warning. Reordering fields is a review-invisible edit that silently repoints every row's record-level access to another object. New error id `security-controlled-by-parent-ambiguous-relation`, the mirror image of `security-controlled-by-parent-no-relation` (#7503): that one reports ZERO candidates, this one reports two or more. The message names every candidate -- field, type and master -- in declaration order, the tier tested, and the candidate that wins today. Only the WINNING tier is judged: the runtime's `??` chain stops at the first tier that resolves, so a tie in a lower tier is masked and is not a decision the platform ever makes. `resolveCbpRelation` here now reads its tiers from one shared table so the two rules cannot disagree about which tier wins; its answer is unchanged by construction (`find` over a tier is the first element `filter` keeps). The mirror's deliberate `reference`-only divergence (#5017) is kept, so a field carrying the rejected `reference_to` alias is not a candidate and cannot create a tie. The `check:doc-security-posture` gate's partial-evaluation suppression is extended to the new rule for the reason its own docblock already gives: an opaque `Field.master_detail(...)` factory call is invisible to every tier predicate, so a masked higher tier would hand the win to a lower one and report a tie the platform never resolves. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01WLJQhde67SeTccsmnBVarV --- .changeset/lint-cbp-ambiguous-master.md | 57 +++++ .../scripts/check-doc-security-posture.mjs | 18 +- packages/lint/src/index.ts | 1 + .../src/validate-security-posture.test.ts | 229 +++++++++++++++++- .../lint/src/validate-security-posture.ts | 160 ++++++++++-- 5 files changed, 434 insertions(+), 31 deletions(-) create mode 100644 .changeset/lint-cbp-ambiguous-master.md diff --git a/.changeset/lint-cbp-ambiguous-master.md b/.changeset/lint-cbp-ambiguous-master.md new file mode 100644 index 0000000000..8adc8a002a --- /dev/null +++ b/.changeset/lint-cbp-ambiguous-master.md @@ -0,0 +1,57 @@ +--- +'@objectstack/lint': minor +--- + +security lint: report a `controlled_by_parent` object whose master is decided by FIELD DECLARATION ORDER (#14747) + +`SecurityPlugin.resolveCbpRelation` resolves the master a `controlled_by_parent` +object derives record-level access from through three tiers — a required +`master_detail`, then any `master_detail`, then a required `lookup` — and picks +inside a tier with `Array.prototype.find`. So when two or more candidates sit in +the tier that wins, the master is whichever one the field map happens to list +first. Measured on a real kernel: an object declaring two required lookups +resolved its security master to the first-declared one, and swapping the two +field declarations — nothing else — repointed every row's record-level access +to the other object. Nothing reported it: not `os validate`, not `os lint`, not +a boot warning. + +New error id **`security-controlled-by-parent-ambiguous-relation`**, the mirror +image of `security-controlled-by-parent-no-relation` (#7503): that one reports +ZERO candidates, this one reports two or more. The message names every +candidate — field, type and master — in declaration order, says which tier was +tested, and says which candidate wins today and therefore which object access +derives from right now. + +Only the **winning** tier is judged, and that is not a shortcut: the runtime's +`??` chain stops at the first tier that resolves, so a tie in a lower tier is +masked by a higher tier's single winner and is not a decision the platform ever +makes. An object with one required `master_detail` and two required lookups is +silent, and stays silent. + +`error` rather than advisory, for the inverse of the usual reason. The other +error rules in this linter mirror a hard runtime refusal; this one has none to +mirror precisely BECAUSE the runtime does not refuse — it silently picks — so +author time is the only place the ambiguity can ever surface. What it does meet +is the admissibility bar the #7503 rule states: a self-contained property of the +object document, no per-permission-set nuance to adjudicate, and no legitimate +reading, since two tied candidates is not an author saying which master they +meant. + +This **narrows the accept set of a gating rule** — error findings fail +`os validate` / `os compile`. Measured over the shipped corpus: the three +`controlled_by_parent` objects in the example apps (`showcase_invoice_line`, +`showcase_expense_line`, `crm_opportunity_line_item`) plus the 27 +`ObjectSchema.create` sites the `check:doc-security-posture` gate reads across +226 marked prose blocks — **0 findings before and 0 after**. Each of the three +declares exactly one required `master_detail`, so tier 1 wins with a single +candidate. `showcase_invoice_line` is the interesting one: it also carries a +required `lookup`, and the rule is silent because that tie-free lower tier is +never reached. + +No runtime behaviour changes. `resolveCbpRelation` in this package now reads its +tiers from one shared table so the two rules cannot disagree about which tier +wins, and its answer is unchanged by construction: `find` over a tier is the +first element `filter` over that tier keeps. The mirror's one deliberate +divergence from the runtime is kept — `reference` is the only spelling accepted +here (#5017), so a field carrying the rejected `reference_to` alias is not a +candidate and cannot create a tie. diff --git a/packages/lint/scripts/check-doc-security-posture.mjs b/packages/lint/scripts/check-doc-security-posture.mjs index cc67d341e8..b50381da05 100644 --- a/packages/lint/scripts/check-doc-security-posture.mjs +++ b/packages/lint/scripts/check-doc-security-posture.mjs @@ -62,10 +62,14 @@ * - `SECURITY_OWD_ALIAS` / `SECURITY_EXTERNAL_WIDER` fire only on values that * are static strings (the rule itself requires `typeof === 'string'`; the * sentinel is not a string). - * - `SECURITY_CBP_NO_RELATION` reads the `fields` subtree, so when that - * subtree is not fully static its findings are SUPPRESSED with a printed - * notice — a `Field.master_detail(...)` factory call must not read as "no - * relation". (No marked block declares `controlled_by_parent` today; the + * - `SECURITY_CBP_NO_RELATION` and `SECURITY_CBP_AMBIGUOUS_RELATION` read the + * `fields` subtree, so when that subtree is not fully static their findings + * are SUPPRESSED with a printed notice — a `Field.master_detail(...)` factory + * call must not read as "no relation", and it must not read as "absent from + * the master_detail tiers" either: an opaque field is invisible to every tier + * predicate, so a single real `master_detail` masked by a factory call would + * hand the win to a lower tier and report a tie the platform never resolves + * (#14747). (No marked block declares `controlled_by_parent` today; the * suppression exists so the first one that does cannot false-red.) * * Two shapes are refused loudly rather than skipped, because a silent skip is @@ -125,7 +129,7 @@ import { tmpdir } from 'node:os'; import { requireDefaultExport, requireDependency } from '../../../scripts/import-prerequisite.mjs'; const ts = await requireDefaultExport('typescript', () => import('typescript'), import.meta.url); -const { validateSecurityPosture, SECURITY_CBP_NO_RELATION } = await requireDependency('@objectstack/lint', () => import('@objectstack/lint'), import.meta.url); +const { validateSecurityPosture, SECURITY_CBP_NO_RELATION, SECURITY_CBP_AMBIGUOUS_RELATION } = await requireDependency('@objectstack/lint', () => import('@objectstack/lint'), import.meta.url); const HERE = dirname(fileURLToPath(import.meta.url)); const REPO_ROOT = resolve(HERE, '../../..'); @@ -344,10 +348,10 @@ export function judgeFile(fileAbs, relPath, marker) { const findings = validateSecurityPosture({ objects: [obj] }).filter((f) => f.severity === 'error'); const kept = []; for (const f of findings) { - if (f.rule === SECURITY_CBP_NO_RELATION && !fieldsComplete) { + if ((f.rule === SECURITY_CBP_NO_RELATION || f.rule === SECURITY_CBP_AMBIGUOUS_RELATION) && !fieldsComplete) { notices.push( `${relPath}:${pageLine} object "${obj.name}": ${f.rule} suppressed — ` + - `the fields subtree is not statically evaluable (factory calls), so "no relation" would be a guess`, + `the fields subtree is not statically evaluable (factory calls), so the verdict would be a guess`, ); continue; } diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index fd64d1aa13..a60cd732ff 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -297,6 +297,7 @@ export { SECURITY_GRANT_EXPIRED_AT_AUTHORING, SECURITY_DELEGATION_MISSING_REASON, SECURITY_CBP_NO_RELATION, + SECURITY_CBP_AMBIGUOUS_RELATION, } from './validate-security-posture.js'; export type { SecurityFinding, SecuritySeverity } from './validate-security-posture.js'; diff --git a/packages/lint/src/validate-security-posture.test.ts b/packages/lint/src/validate-security-posture.test.ts index e0b1a674da..e8e1c2b465 100644 --- a/packages/lint/src/validate-security-posture.test.ts +++ b/packages/lint/src/validate-security-posture.test.ts @@ -28,6 +28,7 @@ import { SECURITY_GRANT_EXPIRED_AT_AUTHORING, SECURITY_DELEGATION_MISSING_REASON, SECURITY_CBP_NO_RELATION, + SECURITY_CBP_AMBIGUOUS_RELATION, } from './validate-security-posture.js'; const rulesOf = (stack: Record) => @@ -506,6 +507,205 @@ describe('validateSecurityPosture · controlled_by_parent with no relation (#750 }); }); +// ── Rule: security-controlled-by-parent-ambiguous-relation (#14747) ── +// +// The mirror image of the rule above: not ZERO candidates, but two or more in +// the tier that WINS. The runtime picks with `find`, so the master is decided +// by field declaration order and nothing says so. +// +// The accept bar has two halves, and the second is the one that makes this +// rule worth having. It must FIRE on a tie in each of the three tiers, and it +// must stay SILENT whenever the tie sits in a tier the `??` chain never +// reaches — a masked tie is not a decision the platform makes, and reporting it +// would send authors to edit fields that change nothing. +describe('validateSecurityPosture · controlled_by_parent with an AMBIGUOUS master (#14747)', () => { + const ambiguousOnly = (stack: Record) => + validateSecurityPosture(stack).filter((f) => f.rule === SECURITY_CBP_AMBIGUOUS_RELATION); + + /** A cbp object carrying exactly the fields under test, plus two masters. */ + const cbpStack = (fields: unknown): Record => ({ + objects: [ + { name: 'crm_account', label: 'Account', sharingModel: 'private', fields: { name: { name: 'name', label: 'Name' } } }, + { name: 'crm_contact', label: 'Contact', sharingModel: 'private', fields: { name: { name: 'name', label: 'Name' } } }, + { name: 'crm_contract', label: 'Contract', sharingModel: 'controlled_by_parent', fields }, + ], + }); + + const REQ_ACCOUNT_LOOKUP = { name: 'account', type: 'lookup', reference: 'crm_account', required: true }; + const REQ_CONTACT_LOOKUP = { name: 'contact', type: 'lookup', reference: 'crm_contact', required: true }; + + // ── POSITIVE CONTROLS — one tie per tier ──────────────────────── + // + // The first is the shape measured on the card: HotCRM's `crm_contract`, two + // required lookups and no master_detail, where swapping the two field + // declarations moved the resolved master from `crm_account` to `crm_contact`. + it('errors on tier 3: two required lookups, naming both candidates and the winner', () => { + const findings = ambiguousOnly(cbpStack({ account: REQ_ACCOUNT_LOOKUP, contact: REQ_CONTACT_LOOKUP })); + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ + severity: 'error', + rule: SECURITY_CBP_AMBIGUOUS_RELATION, + where: 'object "crm_contract"', + path: 'objects[2].fields', + }); + // Every candidate is named — field, type and master — in DECLARATION order, + // which is the order that decides the winner. A message naming only the + // winner would describe the pick, not the ambiguity. + expect(findings[0].message).toContain('"account" (lookup -> "crm_account")'); + expect(findings[0].message).toContain('"contact" (lookup -> "crm_contact")'); + expect(findings[0].message.indexOf('"account" (lookup')).toBeLessThan( + findings[0].message.indexOf('"contact" (lookup'), + ); + // The tier that was actually tested, and the winner it currently resolves. + expect(findings[0].message).toContain('required lookup tier'); + expect(findings[0].message).toContain('Today "account" wins'); + expect(findings[0].hint).toContain('master_detail'); + }); + + it('names the OTHER field as the winner when the same two are declared the other way round', () => { + const findings = ambiguousOnly(cbpStack({ contact: REQ_CONTACT_LOOKUP, account: REQ_ACCOUNT_LOOKUP })); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('Today "contact" wins'); + expect(findings[0].message).toContain('derives from "crm_contact"'); + }); + + it('errors on tier 1: two REQUIRED master_detail fields', () => { + const findings = ambiguousOnly( + cbpStack({ + a: { name: 'a', type: 'master_detail', reference: 'crm_account', required: true }, + b: { name: 'b', type: 'master_detail', reference: 'crm_contact', required: true }, + }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('required master_detail tier'); + }); + + it('errors on tier 2: two master_detail fields, NEITHER required', () => { + const findings = ambiguousOnly( + cbpStack({ + a: { name: 'a', type: 'master_detail', reference: 'crm_account' }, + b: { name: 'b', type: 'master_detail', reference: 'crm_contact' }, + }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('any master_detail tier'); + }); + + it('names all three when three candidates tie', () => { + const findings = ambiguousOnly( + cbpStack({ + a: REQ_ACCOUNT_LOOKUP, + b: REQ_CONTACT_LOOKUP, + c: { name: 'c', type: 'lookup', reference: 'crm_account', required: true }, + }), + ); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('3 of its fields tie'); + }); + + it('reports the array field form too', () => { + expect(ambiguousOnly(cbpStack([REQ_ACCOUNT_LOOKUP, REQ_CONTACT_LOOKUP]))).toHaveLength(1); + }); + + // ── NEGATIVE CONTROLS — a tie the runtime never reaches is NOT a defect ── + it('stays silent when a single required master_detail masks a tie in the lookup tier', () => { + expect( + ambiguousOnly( + cbpStack({ + parent: { name: 'parent', type: 'master_detail', reference: 'crm_account', required: true }, + account: REQ_ACCOUNT_LOOKUP, + contact: REQ_CONTACT_LOOKUP, + }), + ), + ).toEqual([]); + }); + + it('stays silent when tier 1 has ONE winner and tier 2 holds the rest', () => { + expect( + ambiguousOnly( + cbpStack({ + parent: { name: 'parent', type: 'master_detail', reference: 'crm_account', required: true }, + other: { name: 'other', type: 'master_detail', reference: 'crm_contact' }, + }), + ), + ).toEqual([]); + }); + + it.each([ + ['one required master_detail', { order: { name: 'order', type: 'master_detail', reference: 'crm_account', required: true } }], + ['one bare master_detail', { order: { name: 'order', type: 'master_detail', reference: 'crm_account' } }], + ['one required lookup', { order: REQ_ACCOUNT_LOOKUP }], + ])('stays silent on %s — one candidate in the winning tier is not a tie', (_label, fields) => { + expect(ambiguousOnly(cbpStack(fields))).toEqual([]); + }); + + // `pick` is `pred(f) && ref(f)` in both halves: a relation naming no target + // is not a candidate, so it cannot make a tie — nor can an OPTIONAL lookup, + // which is no resolution step at all. + it('stays silent when the second "candidate" names no reference target', () => { + expect( + ambiguousOnly(cbpStack({ account: REQ_ACCOUNT_LOOKUP, contact: { name: 'contact', type: 'lookup', required: true } })), + ).toEqual([]); + }); + + it('stays silent when the second lookup is OPTIONAL', () => { + expect( + ambiguousOnly(cbpStack({ account: REQ_ACCOUNT_LOOKUP, contact: { name: 'contact', type: 'lookup', reference: 'crm_contact' } })), + ).toEqual([]); + }); + + it.each(['private', 'public_read', 'public_read_write'])( + 'stays silent for a two-candidate object whose sharingModel is %s — it derives nothing', + (model) => { + expect( + ambiguousOnly({ + objects: [{ name: 'crm_contract', label: 'C', sharingModel: model, fields: { account: REQ_ACCOUNT_LOOKUP, contact: REQ_CONTACT_LOOKUP } }], + }), + ).toEqual([]); + }, + ); + + // ── The two CBP rules partition the space; they never both fire ────── + it('is mutually exclusive with the no-relation rule', () => { + const cbpRules = (stack: Record) => + validateSecurityPosture(stack) + .map((f) => f.rule) + .filter((r) => r === SECURITY_CBP_NO_RELATION || r === SECURITY_CBP_AMBIGUOUS_RELATION); + expect(cbpRules(cbpStack({ note: { name: 'note', type: 'text', label: 'Note' } }))).toEqual([ + SECURITY_CBP_NO_RELATION, + ]); + expect(cbpRules(cbpStack({ account: REQ_ACCOUNT_LOOKUP, contact: REQ_CONTACT_LOOKUP }))).toEqual([ + SECURITY_CBP_AMBIGUOUS_RELATION, + ]); + }); + + // Same reasoning as the no-relation rule: the runtime's pick does not exempt + // system objects, and an order-dependent security boundary is a property of + // the document, not of who authored it. + it('reports system objects too', () => { + expect( + ambiguousOnly({ + objects: [ + { name: 'sys_thing_link', sharingModel: 'controlled_by_parent', fields: { a: REQ_ACCOUNT_LOOKUP, b: REQ_CONTACT_LOOKUP } }, + { name: 'thing_internal', isSystem: true, sharingModel: 'controlled_by_parent', fields: { a: REQ_ACCOUNT_LOOKUP, b: REQ_CONTACT_LOOKUP } }, + ], + }), + ).toHaveLength(2); + }); + + // The mirror's ONE deliberate divergence (#5017), restated from this rule's + // side: `reference_to` is a rejected alias, so a field carrying it is not a + // candidate here and cannot create a tie. Re-introducing the alias fallback + // would restore the inert branch #5017 removed. + it('does not count a `reference_to` field as a candidate', () => { + expect( + ambiguousOnly( + cbpStack({ account: REQ_ACCOUNT_LOOKUP, contact: { name: 'contact', type: 'lookup', reference_to: 'crm_contact', required: true } }), + ), + ).toEqual([]); + }); +}); + describe('validateSecurityPosture · book audience (ADR-0046 §6.7 / ADR-0090)', () => { it('flags the reserved word in book names and labels', () => { const findings = validateSecurityRoleWord({ @@ -722,6 +922,15 @@ function shapeKeysOf(schema: unknown, depth = 0): string[] { 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.", + // [#14747] `cbpMasterCandidates` folds each winning-tier field into a + // `CbpRelation` ({ field, type, master }) BEFORE the rule reads it, so these + // three receivers carry this file's own vocabulary, not FieldSchema's. The + // FieldSchema reads that produce them (`f.name` / `f.type` / `f.required`, + // plus `def.reference` in `refOf`) are still scanned, on `f` and `def` — the + // guard did not lose a surface when `found` went away, it moved one. + cbpTier: "this file's own `cbpMasterCandidates` return type ({ tier, candidates }), not an authored surface.", + winner: 'a `CbpRelation` — the winning-tier candidate this file already derived, not an authored surface.', + cand: 'a `CbpRelation` — the same derived shape, one per candidate named in the ambiguity message.', }; const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: string; keys: () => string[] }> = [ @@ -751,8 +960,6 @@ const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: s // `required` joined this list with #7503: `resolveCbpRelation` mirrors the // runtime's three-step fallback, whose steps 1 and 3 are predicated on it. { receiver: 'f', expected: ['label', 'name', 'required', 'type'], declaredBy: 'FieldSchema', keys: () => Object.keys(FieldSchema.shape) }, - // [#7503] The field `resolveCbpRelation` settled on — a FieldSchema record. - { receiver: 'found', expected: ['name', 'type'], declaredBy: 'FieldSchema', keys: () => Object.keys(FieldSchema.shape) }, { receiver: 'p', expected: ['allowCreate', 'allowDelete', 'allowEdit', 'allowRead', 'modifyAllRecords', 'readScope', 'viewAllRecords'], @@ -840,6 +1047,7 @@ describe('validateSecurityPosture — reads only keys the spec declares (meta-te 'findings', 'objects', 'permissionSets', 'privateObjects', 'grantedObjects', 'stackSetNames', 'records', 'reason', 'until', 'setName', 'flsKey', 'opts', 'path', 'i', 'e', 'fields', 'crm_opportunity', 'entries', // #7503: the rule's own field list — `.find`, a JS method. + 'matched', // #14747: one tier's candidate list — `.length` / `.map`, JS methods. ]); expect(receivers.filter((r) => !tabled.has(r) && !PLUMBING.has(r))).toEqual([]); }); @@ -952,6 +1160,7 @@ const RULE_IDS: Record = { SECURITY_GRANT_EXPIRED_AT_AUTHORING, SECURITY_DELEGATION_MISSING_REASON, SECURITY_CBP_NO_RELATION, + SECURITY_CBP_AMBIGUOUS_RELATION, }; const TEXT_FIELD = { a: { type: 'text', label: 'A' } } as const; @@ -1008,6 +1217,20 @@ const REACHABILITY_CORPUS: Array<{ label: string; stack: Record objects: [objectFixture({ name: 'line_item', sharingModel: 'controlled_by_parent' })], }, }, + { + label: 'cbp-ambiguous-relation', + stack: { + objects: [ + objectFixture({ + name: 'contract', sharingModel: 'controlled_by_parent', + fields: { + account: { type: 'lookup', label: 'Account', reference: 'account', required: true }, + contact: { type: 'lookup', label: 'Contact', reference: 'contact', required: 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' }] }] }, @@ -1028,7 +1251,7 @@ describe('validateSecurityPosture — every branch is reachable without an undec }); it('maps every `findings.push` site in the source', () => { - expect(pushedRuleIds()).toHaveLength(16); + expect(pushedRuleIds()).toHaveLength(17); }); it('reaches every `findings.push` site from that corpus', () => { diff --git a/packages/lint/src/validate-security-posture.ts b/packages/lint/src/validate-security-posture.ts index 8d15a5f6d3..c9d81eed14 100644 --- a/packages/lint/src/validate-security-posture.ts +++ b/packages/lint/src/validate-security-posture.ts @@ -20,12 +20,24 @@ * | security-grant-expired-at-authoring(err)| ADR-0091 D2 resolution filtering| * | security-delegation-missing-reason(err) | ADR-0091 D3 dual audit | * | security-cbp-no-relation (error) | #7503 (runtime refusal #7474) | + * | security-cbp-ambiguous-relation(error) | #14747 order-dependent master | * * Per ADR-0049 discipline these are NOT advisory security: every `error` rule - * mirrors a runtime enforcement point (D1 fail-closed OWD default, D4 zod - * enum + fail-closed evaluator, D5/D9 anchor binding gate, D3 rename wave) — - * the lint moves the failure from runtime-deny to author-time fix-it. The - * non-`error` rules are the ones with NO hard runtime refusal behind them: + * BUT ONE mirrors a runtime enforcement point (D1 fail-closed OWD default, D4 + * zod enum + fail-closed evaluator, D5/D9 anchor binding gate, D3 rename wave) + * — the lint moves the failure from runtime-deny to author-time fix-it. + * + * The exception INVERTS that argument rather than weakening it. + * `security-cbp-ambiguous-relation` has no runtime refusal to mirror precisely + * BECAUSE the runtime does not refuse: it silently PICKS a winner by field + * declaration order, so there is no later runtime verdict for the author to + * discover, and author time is the only place the ambiguity can surface at all + * (#14747). What it does satisfy is the admissibility bar the #7503 push site + * below states — a self-contained property of the object document, with no + * per-permission-set nuance to adjudicate and no legitimate reading. + * + * The non-`error` rules are the ones with NO hard runtime refusal behind them + * AND no such certainty: * master-detail-ungranted mirrors a runtime gate (the ADR-0055 object-level * CRUD check) but flags a *likely* misconfiguration whose per-permission-set * nuance it cannot fully adjudicate; book-audience-unknown-set and @@ -72,6 +84,7 @@ export const SECURITY_FLS_UNQUALIFIED_KEY = 'security-fls-unqualified-key'; export const SECURITY_GRANT_EXPIRED_AT_AUTHORING = 'security-grant-expired-at-authoring'; export const SECURITY_DELEGATION_MISSING_REASON = 'security-delegation-missing-reason'; export const SECURITY_CBP_NO_RELATION = 'security-controlled-by-parent-no-relation'; +export const SECURITY_CBP_AMBIGUOUS_RELATION = 'security-controlled-by-parent-ambiguous-relation'; export type SecuritySeverity = 'error' | 'warning' | 'info'; @@ -187,9 +200,18 @@ function firstMasterDetailField(obj: AnyRec): { name: string; parent?: string } return undefined; } +/** A master relation the platform can derive `controlled_by_parent` access from. */ +interface CbpRelation { + /** The field that answers. */ + field: string; + /** `master_detail` or `lookup`. */ + type: string; + /** The object it references. */ + master: string; +} + /** - * [#7503] The relation a `controlled_by_parent` object derives its access from, - * or `undefined` when the platform has nothing to derive from. + * [#7503] The master-relation precedence, as ONE table both CBP rules read. * * A point-for-point mirror of `resolveCbpRelation` in * `packages/plugins/plugin-security/src/security-plugin.ts` — the SAME @@ -204,23 +226,68 @@ function firstMasterDetailField(obj: AnyRec): { name: string; parent?: string } * `required` is read for truthiness, not `=== true`, because the runtime does * (`f?.required`) — mirroring the gate means mirroring its coercions too. * - * The one DELIBERATE divergence is the reference spelling. The runtime accepts - * `reference ?? reference_to ?? referenceTo`; `refOf` here accepts only - * `reference`, the sole spelling `FieldSchema` declares. The aliases do not - * parse (strict schema, #5017), so for any stack an author can ship the two - * agree; re-introducing the alias fallback here would restore precisely the - * inert branch #5017 removed, and on the pre-parse path the schema already - * names the real defect (the alias key) rather than this rule guessing past it. + * The tiers are a table rather than a `??` chain spelled twice because the + * no-relation rule and the ambiguity rule (#14747) must not be able to disagree + * about which tier wins: one says "no tier matched", the other says "the tier + * that matched holds more than one candidate", and those are answers to the + * same question. The `label` is author-facing — it appears in the ambiguity + * message, so the tier the author is told about is the tier that was tested. + * + * The one DELIBERATE divergence from the runtime is the reference spelling. The + * runtime accepts `reference ?? reference_to ?? referenceTo`; `refOf` here + * accepts only `reference`, the sole spelling `FieldSchema` declares. The + * aliases do not parse (strict schema, #5017), so for any stack an author can + * ship the two agree; re-introducing the alias fallback here would restore + * precisely the inert branch #5017 removed, and on the pre-parse path the + * schema already names the real defect (the alias key) rather than this rule + * guessing past it. + */ +const CBP_TIERS: ReadonlyArray<{ label: string; pred: (f: AnyRec) => boolean }> = [ + { label: 'required master_detail', pred: (f) => f.type === 'master_detail' && !!f.required }, + { label: 'any master_detail', pred: (f) => f.type === 'master_detail' }, + { label: 'required lookup', pred: (f) => f.type === 'lookup' && !!f.required }, +]; + +/** + * [#7503 / #14747] The WINNING tier and EVERY candidate in it, in declaration + * order — `undefined` when no tier matches (nothing to derive access from). + * + * Only the winning tier is reported on, and that is not a simplification: the + * runtime's `??` chain stops at the first tier that resolves, so a tie in a + * lower tier is masked by a higher tier's single winner and is not a decision + * the platform ever makes. Reordering fields inside a masked tier changes + * nothing, and reporting it would be reporting a non-defect. */ -function resolveCbpRelation(obj: AnyRec): { field: string; type: string; master: string } | undefined { +function cbpMasterCandidates(obj: AnyRec): { tier: string; candidates: CbpRelation[] } | undefined { const entries = asArray(obj.fields); - const pick = (pred: (f: AnyRec) => boolean) => entries.find((f) => pred(f) && refOf(f)); - const found = - pick((f) => f.type === 'master_detail' && !!f.required) ?? - pick((f) => f.type === 'master_detail') ?? - pick((f) => f.type === 'lookup' && !!f.required); - if (!found) return undefined; - return { field: String(found.name ?? '?'), type: String(found.type), master: refOf(found) as string }; + for (const { label, pred } of CBP_TIERS) { + const matched = entries.filter((f) => pred(f) && refOf(f)); + if (matched.length > 0) { + return { + tier: label, + candidates: matched.map((f) => ({ + field: String(f.name ?? '?'), + type: String(f.type), + master: refOf(f) as string, + })), + }; + } + } + return undefined; +} + +/** + * [#7503] The relation a `controlled_by_parent` object derives its access from, + * or `undefined` when the platform has nothing to derive from. + * + * The runtime's winner, exactly: `Array.prototype.find` over a tier is the + * first element `filter` over that same tier keeps, and the first tier with a + * candidate is the tier the `??` chain stops at — so `candidates[0]` of the + * winning tier IS `pick(...) ?? pick(...) ?? pick(...)`, with the ambiguity the + * runtime resolves by position now visible to the caller instead of discarded. + */ +function resolveCbpRelation(obj: AnyRec): CbpRelation | undefined { + return cbpMasterCandidates(obj)?.candidates[0]; } /** @@ -340,6 +407,57 @@ export function validateSecurityPosture(stack: AnyRec, opts?: { nowMs?: number } }); } + // ── [#14747] controlled_by_parent whose master is decided by ORDER ──── + // The opposite defect to the one above: not zero candidates, but TWO OR + // MORE in the tier that wins. `pick` is `find` here and in the runtime, so + // the master is whichever candidate the field map happens to list first — + // field declaration order, a property that carries no authored meaning. + // Moving a field up or down a schema file is a review-invisible edit that + // reads as cosmetic, and it silently repoints the record-level security of + // every row of the object. Nothing reports it at runtime: the platform does + // not refuse, it picks (#14747). + // + // `error`, not advisory, and for the INVERSE of the usual reason — there is + // no runtime refusal to mirror BECAUSE the runtime silently decides, which + // makes author time the only place this can ever surface. The + // admissibility bar is the one the rule above states and this shape meets + // identically: a self-contained property of the object document, no + // per-permission-set nuance to adjudicate, and no legitimate reading — two + // tied candidates is not an author saying which master they meant. See the + // ADR-0049 paragraph in the module header. + // + // NOT exempted for system objects, for the same reason the rule above is + // not: the ambiguity is a property of the document, independent of who may + // author it. + const cbpTier = owd === 'controlled_by_parent' ? cbpMasterCandidates(obj) : undefined; + if (cbpTier && cbpTier.candidates.length > 1) { + const winner = cbpTier.candidates[0]; + const roster = cbpTier.candidates + .map((cand) => `"${cand.field}" (${cand.type} -> "${cand.master}")`) + .join(', '); + findings.push({ + severity: 'error', + rule: SECURITY_CBP_AMBIGUOUS_RELATION, + where: `object "${objName}"`, + path: `${objPath}.fields`, + message: + `"${objName}" declares sharingModel 'controlled_by_parent' and ${cbpTier.candidates.length} of its ` + + `fields tie for the master relation: ${roster}. ADR-0055 resolves the master through a required ` + + `master_detail, then any master_detail, then a required lookup, and takes the FIRST match in the ` + + `tier that wins — here the ${cbpTier.tier} tier — so which object this one derives its access from ` + + `is decided by FIELD DECLARATION ORDER. Today "${winner.field}" wins and every row's record-level ` + + `access derives from "${winner.master}"; reordering these fields moves that security boundary to ` + + `another object, and the runtime reports nothing when it does.`, + hint: + `Leave exactly ONE candidate in the winning tier, so the master is authored rather than positional. ` + + `Either promote the intended master into a higher tier — make it the object's only required ` + + `master_detail ({ type: 'master_detail', reference: '', required: true }) — or demote ` + + `the others in this tier (drop required, or change the relation type). Precedence is required ` + + `master_detail, then any master_detail, then a required lookup; only a tie inside the tier that ` + + `WINS is ambiguous.`, + }); + } + // D11: external dial present on any object (system included) must obey // external ≤ internal. controlled_by_parent inherits the master's pair. if (typeof external === 'string') {