From 3bd1d1b91cdca7ec57aa9adb7fcda210fe742f76 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 00:20:47 +0000 Subject: [PATCH] fix(lint): nav-object-ungranted hint drops the gating remedy it cannot honour The hint (and module doc-block) prescribed gating a nav entry with requiredPermissions/visible as a way to clear the finding. Neither key is read by the rule, so following that advice left the warning firing forever. Corrected to the two remedies the rule can prove: grant read on the object, or drop the entry. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .../nav-object-ungranted-hint-drops-gating.md | 5 ++ packages/lint/src/validate-nav-access.test.ts | 58 +++++++++++++++++++ packages/lint/src/validate-nav-access.ts | 14 ++++- 3 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 .changeset/nav-object-ungranted-hint-drops-gating.md diff --git a/.changeset/nav-object-ungranted-hint-drops-gating.md b/.changeset/nav-object-ungranted-hint-drops-gating.md new file mode 100644 index 0000000000..741ada350f --- /dev/null +++ b/.changeset/nav-object-ungranted-hint-drops-gating.md @@ -0,0 +1,5 @@ +--- +"@objectstack/lint": patch +--- + +`nav-object-ungranted`'s hint no longer tells you to gate the nav entry with `requiredPermissions`/`visible` — that never cleared the finding, because the rule never reads either key. Gating restricts who can see the entry; it doesn't grant the object read, so a holder who clears the gate could still hit permission-denied, and the warning kept firing anyway. The hint (and the module doc-block) now name the two remedies that actually clear it: grant read on the object in a permission set (`allowRead: true` or `viewAllRecords`), or drop the nav entry. No behavior change — the rule fires and stays silent on exactly the same inputs as before; only the wording of the hint moved. diff --git a/packages/lint/src/validate-nav-access.test.ts b/packages/lint/src/validate-nav-access.test.ts index 3a0f9b356a..c0eddc5b73 100644 --- a/packages/lint/src/validate-nav-access.test.ts +++ b/packages/lint/src/validate-nav-access.test.ts @@ -190,3 +190,61 @@ describe('validateNavAccess — exemptions (false-positive floor)', () => { expect(findings).toEqual([]); }); }); + +// #16065: the hint used to prescribe a third remedy — "gate the entry with +// requiredPermissions/visible" — that this rule cannot honour: neither key is +// read anywhere in this file, so a gated-but-ungranted entry kept warning +// forever. The fix corrects the hint (and the doc-block) to the two remedies +// the rule can actually prove; it does NOT make the rule start reading those +// keys. These controls pin that: gating must not silence a genuinely +// ungranted object, and must not spuriously flag a genuinely granted one. +describe('validateNavAccess — gating keys are not read (#16065)', () => { + it('hint prescribes only the two remedies it can prove, not gating', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([objectNav('nav_forecast', 'crm_forecast')]), + permissions: [{ name: 'p', label: 'P', objects: { crm_lead: { allowRead: true } } }], + }); + expect(findings).toHaveLength(1); + const { hint } = findings[0]; + // The two remedies the rule can prove. + expect(hint).toContain('allowRead: true'); + expect(hint).toContain('viewAllRecords'); + // The retired third remedy must not return. + expect(hint).not.toContain('gate the entry'); + expect(hint).not.toMatch(/requiredPermissions.*if it is meant for admins only/); + }); + + it('a requiredPermissions-gated but ungranted object still fires (gating is not a remedy)', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([ + { ...objectNav('nav_forecast', 'crm_forecast'), requiredPermissions: ['view_forecast'] }, + ]), + permissions: [{ name: 'p', label: 'P', objects: { crm_lead: { allowRead: true } } }], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(NAV_OBJECT_UNGRANTED); + }); + + it('a visible:false but ungranted object still fires (gating is not a remedy)', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([{ ...objectNav('nav_forecast', 'crm_forecast'), visible: false }]), + permissions: [{ name: 'p', label: 'P', objects: { crm_lead: { allowRead: true } } }], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(NAV_OBJECT_UNGRANTED); + }); + + it('a gated AND granted object stays silent — gating causes no false positive either', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([ + { ...objectNav('nav_forecast', 'crm_forecast'), requiredPermissions: ['view_forecast'] }, + ]), + permissions: [{ name: 'p', label: 'P', objects: { crm_forecast: { allowRead: true } } }], + }); + expect(findings).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-nav-access.ts b/packages/lint/src/validate-nav-access.ts index 1057079f80..c0a4b27361 100644 --- a/packages/lint/src/validate-nav-access.ts +++ b/packages/lint/src/validate-nav-access.ts @@ -25,6 +25,13 @@ * entry — it is only *suspicious*, and the ceiling for a static check is a * warning (the same posture `validate-capability-references` takes). * + * The only remedies that clear this finding are granting read (a permission set's + * `objects` entry with `allowRead: true` or `viewAllRecords`) or dropping the nav + * entry. Gating the item with `requiredPermissions` or `visible` does not: those + * gate on capabilities/visibility, not the object grant, so a holder who clears + * the gate can still lack read and hit permission-denied — honouring either key + * here would silence the rule on an object that is genuinely reachable. + * * Two exemptions keep it quiet: * - **Platform-provided objects** (`sys_user`, `sys_approval_request`, …) are * skipped: the packages that register them ship their own permission sets, @@ -170,9 +177,10 @@ export function validateNavAccess(stack: AnyRec): NavAccessFinding[] { `and breaks for the users the app ships permission sets for.`, hint: `Add "${objectName}" to a permission set's \`objects\` with \`allowRead: true\` ` + - `(or \`viewAllRecords\`), gate the entry with \`requiredPermissions\`/\`visible\` ` + - `if it is meant for admins only, or drop it. Ignore this if a permission set ` + - `from another installed package grants it.`, + `(or \`viewAllRecords\`), or drop the entry. Gating it with \`requiredPermissions\`/` + + `\`visible\` does not clear this: those gate visibility, not the object grant, so a ` + + `holder can still open the entry and hit permission-denied. Ignore this if a ` + + `permission set from another installed package grants it.`, }); }