From eb48b102d4dde740d261d67b0db7b2c466a5d434 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 14:59:14 +0000 Subject: [PATCH] feat(lint): navigation reachability vs. granted access (#3583) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Assessment R5. Joins what an app's navigation exposes against buildAccessMatrix — the first lint consumer of the ADR-0090 D6 matrix, which until now only backed `os compile`'s snapshot gate. Navigation and permissions are separate metadata, each valid alone, so an app can ship a menu entry for an object no permission set grants read on. The entry renders and opening it fails permission-denied. The failure hides during development because the platform's built-in `admin_full_access` carries a wildcard grant, so it works while you browse as an administrator and breaks for exactly the users the app ships permission sets for. Advisory severity: a grant may come from a set another installed package ships, which a static check cannot see. Quiet by construction in three cases — platform-provided objects (granted by their own packages), a stack that declares no permission sets at all (permissions are managed elsewhere, so flagging every entry says nothing), and any stack whose set carries a wildcard `objects: { '*': … }` grant. That last one is a bug found while verifying against the real apps: the access matrix records the wildcard under the literal key `*`, so without it the rule would have fired hardest on the stacks that granted the most. Verification also corrected the finding text. An earlier draft claimed the entry fails "for every user, including admins" — untrue, since admin_full_access is a wildcard set. The message now says what actually happens, because a diagnostic that overstates its case teaches authors to discount it. Findings on the example apps: app-crm 0, app-todo 0 (declares no permission sets), app-showcase 7 — verified TRUE positives, not false ones: showcase binds `everyone` to `showcase_member_default`, which grants 7 objects while the navigation exposes ~20, so a member genuinely cannot open Teams / Categories / Field Zoo / Cascading Select / Expense Reports / Business Units / Preferences. Left for the showcase maintainers rather than silently rewriting an example app's permission model. Suites: lint 447, cli 642 passing; eslint, nul-bytes and api-surface clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GBks5G7AkwrvgF2kL5rfkT --- .changeset/nav-access-lint.md | 24 +++ .../docs/deployment/validating-metadata.mdx | 19 ++ packages/cli/src/commands/compile.ts | 2 + packages/cli/src/commands/lint.ts | 16 ++ packages/cli/src/commands/validate.ts | 2 + packages/lint/src/index.ts | 3 + packages/lint/src/validate-nav-access.test.ts | 192 ++++++++++++++++++ packages/lint/src/validate-nav-access.ts | 187 +++++++++++++++++ 8 files changed, 445 insertions(+) create mode 100644 .changeset/nav-access-lint.md create mode 100644 packages/lint/src/validate-nav-access.test.ts create mode 100644 packages/lint/src/validate-nav-access.ts diff --git a/.changeset/nav-access-lint.md b/.changeset/nav-access-lint.md new file mode 100644 index 0000000000..6693532b08 --- /dev/null +++ b/.changeset/nav-access-lint.md @@ -0,0 +1,24 @@ +--- +'@objectstack/lint': minor +'@objectstack/cli': minor +--- + +Navigation reachability vs. granted access (issue #3583, assessment R5) + +`validate-nav-access` joins what an app's navigation exposes against +`buildAccessMatrix` — the first lint consumer of the ADR-0090 D6 matrix, which +previously only backed `os compile`'s snapshot gate. An object in the menu that +no permission set grants read on renders as an entry and then fails +permission-denied when opened: it works while you browse as an administrator +(the platform's built-in `admin_full_access` carries a wildcard grant) and +breaks for exactly the users the app ships permission sets for. + +Advisory severity — a grant can legitimately come from a permission set another +installed package ships. Quiet by construction in three cases: platform-provided +objects (their own packages grant them), stacks that declare no permission sets +at all (permissions managed elsewhere, so flagging every entry says nothing), +and any stack where a set carries a wildcard `objects: { '*': … }` grant — the +shape `admin_full_access` itself uses, which the access matrix records under the +literal key `*`. + +Wired into `os validate`, `os lint`, and `os compile`. diff --git a/content/docs/deployment/validating-metadata.mdx b/content/docs/deployment/validating-metadata.mdx index a352d32343..83b7db800f 100644 --- a/content/docs/deployment/validating-metadata.mdx +++ b/content/docs/deployment/validating-metadata.mdx @@ -159,6 +159,24 @@ An axis naming a measure the dataset declares but this chart does not *select* nothing. The react `` block is object-bound rather than dataset-bound and is not checked here. +### 7. Navigation exposing objects nobody can read + +Navigation and permissions are separate metadata, each valid on its own — so an +app can put an object in its menu that no permission set grants read on. The +entry renders; opening it fails permission-denied for everyone except a holder +of the platform's built-in wildcard admin set. It works while you browse as an +administrator and breaks for the users the app ships permission sets for. + +```ts +navigation: [{ id: 'nav_forecast', type: 'object', objectName: 'crm_forecast' }] +// …and no permission set lists `crm_forecast` under `objects` → warning +``` + +Advisory: the grant may come from a permission set another installed package +ships. Skipped for platform-provided objects (whose own packages grant them), +for stacks that declare no permission sets at all, and when any set carries a +wildcard (`objects: { '*': … }`) grant. + ## The one gate, two entry points `os validate` and `os build` (alias of `os compile`) run the **same** validator: @@ -172,6 +190,7 @@ dataset-bound and is not checked here. | Object & action name references (#3583) | ✓ | ✓ | | Page-component field bindings (#3583) | ✓ | ✓ | | Chart bindings outside dashboards (#3583) | ✓ | ✓ | +| Navigation vs. granted access (ADR-0090 D6) | ✓ | ✓ | | Security posture (ADR-0090 — e.g. every custom object declares `sharingModel`) | ✓ | ✓ | | Emits `dist/objectstack.json` | — | ✓ | diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index 063c4de125..082737da45 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -15,6 +15,7 @@ import { validateDashboardActionRefs } from '@objectstack/lint'; import { validateFilterTokens } from '@objectstack/lint'; import { validateObjectReferences, validateActionNameRefs } from '@objectstack/lint'; import { validatePageFieldBindings, validateChartBindings } from '@objectstack/lint'; +import { validateNavAccess } from '@objectstack/lint'; import { validateResponsiveStyles } from '@objectstack/lint'; import { validateSecurityPosture, validateOrgAxisRedLines, buildAccessMatrix, diffAccessMatrix } from '@objectstack/lint'; import { validateReadonlyFlowWrites } from '@objectstack/lint'; @@ -340,6 +341,7 @@ export default class Compile extends Command { ...validateActionNameRefs(result.data as Record), ...validatePageFieldBindings(result.data as Record), ...validateChartBindings(result.data as Record), + ...validateNavAccess(result.data as Record), ]; const refErrors = refFindings.filter((f) => f.severity === 'error'); const refWarnings = refFindings.filter((f) => f.severity === 'warning'); diff --git a/packages/cli/src/commands/lint.ts b/packages/cli/src/commands/lint.ts index 8e8d6c3edf..45e050a16c 100644 --- a/packages/cli/src/commands/lint.ts +++ b/packages/cli/src/commands/lint.ts @@ -12,6 +12,7 @@ import { validateWidgetBindings } from '@objectstack/lint'; import { validateRecordTitle, validateSemanticRoles, validateCapabilityReferences, validateSecurityPosture, validateOrgAxisRedLines, validateApprovalApprovers, validateSeedReplaySafety, validateSeedStateMachine } from '@objectstack/lint'; import { validateObjectReferences, validateActionNameRefs } from '@objectstack/lint'; import { validatePageFieldBindings, validateChartBindings } from '@objectstack/lint'; +import { validateNavAccess } from '@objectstack/lint'; import { collectAndLintDocs } from '../utils/collect-docs.js'; import { scoreMetadata } from '../lint/score.js'; import { runMetadataEval } from '../lint/metadata-eval.js'; @@ -551,6 +552,21 @@ export function lintConfig(config: any): LintIssue[] { }); } + // ── Navigation reachability vs. granted access (ADR-0090 D6, issue #3583) ── + // Navigation and permissions are separate metadata, so an app can expose an + // object no permission set grants read on: the entry renders and the click + // fails permission-denied for every user, including admins. Advisory — the + // grant may come from a set another installed package ships. + for (const t of validateNavAccess(config)) { + issues.push({ + severity: t.severity, + rule: t.rule, + message: `${t.where}: ${t.message}`, + path: t.path, + fix: t.hint, + }); + } + return issues; } diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index bf96ff7d8c..a0c75ba440 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -16,6 +16,7 @@ import { validateDashboardActionRefs } from '@objectstack/lint'; import { validateFilterTokens } from '@objectstack/lint'; import { validateObjectReferences, validateActionNameRefs } from '@objectstack/lint'; import { validatePageFieldBindings, validateChartBindings } from '@objectstack/lint'; +import { validateNavAccess } from '@objectstack/lint'; import { validateResponsiveStyles } from '@objectstack/lint'; import { validateJsxPages, validateReactPages, validateReactPageProps, validatePageSourceStyling } from '@objectstack/lint'; import { validateCapabilityReferences } from '@objectstack/lint'; @@ -302,6 +303,7 @@ export default class Validate extends Command { ...validateActionNameRefs(result.data as Record), ...validatePageFieldBindings(result.data as Record), ...validateChartBindings(result.data as Record), + ...validateNavAccess(result.data as Record), ]; const refErrors = refFindings.filter((f) => f.severity === 'error'); const refWarnings = refFindings.filter((f) => f.severity === 'warning'); diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index ef4fc71a60..251266ecbd 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -203,4 +203,7 @@ export { } from './validate-chart-bindings.js'; export type { ChartBindingFinding, ChartBindingSeverity } from './validate-chart-bindings.js'; +export { validateNavAccess, NAV_OBJECT_UNGRANTED } from './validate-nav-access.js'; +export type { NavAccessFinding, NavAccessSeverity } from './validate-nav-access.js'; + export { buildAccessMatrix, diffAccessMatrix } from './build-access-matrix.js'; diff --git a/packages/lint/src/validate-nav-access.test.ts b/packages/lint/src/validate-nav-access.test.ts new file mode 100644 index 0000000000..3a0f9b356a --- /dev/null +++ b/packages/lint/src/validate-nav-access.test.ts @@ -0,0 +1,192 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { validateNavAccess, NAV_OBJECT_UNGRANTED } from './validate-nav-access.js'; + +const objects = [ + { name: 'crm_lead', fields: { name: { type: 'text' } } }, + { name: 'crm_forecast', fields: { name: { type: 'text' } } }, +]; + +const navApp = (items: unknown[]) => [{ name: 'crm', label: 'CRM', navigation: items }]; + +const objectNav = (id: string, objectName: string) => ({ + id, + type: 'object', + label: objectName, + objectName, +}); + +describe('validateNavAccess', () => { + // The HotCRM instance: an object in the menu that no set grants. + it('warns on a nav-exposed object no permission set grants read on', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([objectNav('nav_leads', 'crm_lead'), objectNav('nav_forecast', 'crm_forecast')]), + permissions: [ + { name: 'crm_user', label: 'CRM User', objects: { crm_lead: { allowRead: true } } }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].rule).toBe(NAV_OBJECT_UNGRANTED); + expect(findings[0].path).toBe('apps[0].navigation[1].objectName'); + expect(findings[0].message).toContain('crm_forecast'); + }); + + // The platform's own `admin_full_access` uses this shape; a stack may too. + // Without wildcard support the matrix records the literal key `*` and the + // rule would fire on exactly the stacks that granted the most. + it('accepts a wildcard read grant as covering every object', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([objectNav('nav_forecast', 'crm_forecast')]), + permissions: [ + { name: 'full', label: 'Full', objects: { '*': { allowRead: true, viewAllRecords: true } } }, + ], + }); + expect(findings).toEqual([]); + }); + + it('accepts read granted through viewAllRecords or modifyAllRecords', () => { + for (const bit of ['viewAllRecords', 'modifyAllRecords']) { + const findings = validateNavAccess({ + objects, + apps: navApp([objectNav('nav_forecast', 'crm_forecast')]), + permissions: [ + { name: 'admin', label: 'Admin', objects: { crm_forecast: { [bit]: true } } }, + ], + }); + expect(findings, `${bit} should grant read`).toEqual([]); + } + }); + + it('does not treat a write-only grant as read', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([objectNav('nav_forecast', 'crm_forecast')]), + permissions: [ + { name: 'writer', label: 'Writer', objects: { crm_forecast: { allowCreate: true, allowEdit: true } } }, + ], + }); + expect(findings).toHaveLength(1); + }); + + it('reports an object once even when several nav entries expose it', () => { + const findings = validateNavAccess({ + objects, + apps: [ + { + name: 'crm', + navigation: [objectNav('nav_a', 'crm_forecast')], + areas: [{ id: 'area', label: 'Area', navigation: [objectNav('nav_b', 'crm_forecast')] }], + }, + ], + permissions: [{ name: 'p', label: 'P', objects: { crm_lead: { allowRead: true } } }], + }); + expect(findings).toHaveLength(1); + }); + + it('walks area navigation and nested children', () => { + const findings = validateNavAccess({ + objects, + apps: [ + { + name: 'crm', + areas: [ + { + id: 'area_sales', + label: 'Sales', + navigation: [ + { + id: 'grp', + type: 'group', + label: 'Group', + children: [objectNav('nav_forecast', 'crm_forecast')], + }, + ], + }, + ], + }, + ], + permissions: [{ name: 'p', label: 'P', objects: { crm_lead: { allowRead: true } } }], + }); + expect(findings).toHaveLength(1); + expect(findings[0].path).toBe('apps[0].areas[0].navigation[0].children[0].objectName'); + }); +}); + +describe('validateNavAccess — exemptions (false-positive floor)', () => { + it('skips platform-provided objects — their own packages grant them', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([ + objectNav('nav_users', 'sys_user'), + objectNav('nav_approvals', 'sys_approval_request'), + ]), + permissions: [{ name: 'p', label: 'P', objects: { crm_lead: { allowRead: true } } }], + }); + expect(findings).toEqual([]); + }); + + it('skips a stack that declares no permission sets at all', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([objectNav('nav_forecast', 'crm_forecast')]), + }); + expect(findings).toEqual([]); + }); + + it('skips an object this stack does not define', () => { + // A dangling nav target is `validate-object-references`' finding, not this + // rule's — reporting it here would double-report one mistake. + const findings = validateNavAccess({ + objects, + apps: navApp([objectNav('nav_ghost', 'crm_ghost')]), + permissions: [{ name: 'p', label: 'P', objects: { crm_lead: { allowRead: true } } }], + }); + expect(findings).toEqual([]); + }); + + it('ignores non-object nav entries', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([ + { id: 'nav_dash', type: 'dashboard', label: 'D', dashboardName: 'd' }, + { id: 'nav_url', type: 'url', label: 'U', url: '/x' }, + { id: 'nav_sep', type: 'separator' }, + ]), + permissions: [{ name: 'p', label: 'P', objects: { crm_lead: { allowRead: true } } }], + }); + expect(findings).toEqual([]); + }); + + it('is silent when every exposed object is granted, and tolerates empty input', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([objectNav('nav_leads', 'crm_lead'), objectNav('nav_forecast', 'crm_forecast')]), + permissions: [ + { + name: 'p', + label: 'P', + objects: { crm_lead: { allowRead: true }, crm_forecast: { allowRead: true } }, + }, + ], + }); + expect(findings).toEqual([]); + expect(validateNavAccess({})).toEqual([]); + expect(validateNavAccess(null as unknown as Record)).toEqual([]); + }); + + it('accepts a grant coming from any one of several sets', () => { + const findings = validateNavAccess({ + objects, + apps: navApp([objectNav('nav_forecast', 'crm_forecast')]), + permissions: [ + { name: 'a', label: 'A', objects: { crm_lead: { allowRead: true } } }, + { name: 'b', label: 'B', 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 new file mode 100644 index 0000000000..70dec7c02f --- /dev/null +++ b/packages/lint/src/validate-nav-access.ts @@ -0,0 +1,187 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [ADR-0090 D6] Navigation reachability vs. granted access (issue #3583, + * assessment R5). + * + * An app can put an object in its navigation without any permission set + * granting read on it. Nothing rejects that: navigation and permissions are + * separate metadata, each valid on its own. At runtime the entry renders, the + * user clicks it, and the list view comes back permission-denied — for EVERY + * user, including the admin, because a permission nobody was granted is a + * permission nobody has. The HotCRM audit shipped two such objects + * (`crm_forecast`, `crm_knowledge_article`). + * + * This is the first lint consumer of `buildAccessMatrix` (ADR-0090 D6), which + * already derives one row per (permission set × object) with `read` folded + * across `allowRead` / `viewAllRecords` / `modifyAllRecords`. The rule is that + * matrix joined against what navigation exposes. + * + * ── Advisory, deliberately ────────────────────────────────────────────── + * + * A grant can legitimately live outside this stack: a permission set shipped by + * another installed package, a platform default, or an org-level assignment + * made after install. A stack is therefore not *wrong* to ship an ungranted nav + * entry — it is only *suspicious*, and the ceiling for a static check is a + * warning (the same posture `validate-capability-references` takes). + * + * 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, + * which this stack never sees. + * - **A stack that declares no permission sets at all** is skipped entirely. + * Flagging every nav entry there says nothing useful — it means permissions + * are managed elsewhere, not that each entry is broken (the same + * "empty collection ⇒ don't judge" gate `defineStack` applies to nav + * dashboard/page/report references). + */ + +import { isPlatformProvidedObjectName } from '@objectstack/spec/system'; +import { buildAccessMatrix } from './build-access-matrix.js'; + +export const NAV_OBJECT_UNGRANTED = 'nav-object-ungranted'; + +export type NavAccessSeverity = 'error' | 'warning'; + +export interface NavAccessFinding { + /** Always `warning` — a grant may come from a package this stack cannot see. */ + severity: NavAccessSeverity; + /** Diagnostic rule id. */ + rule: string; + /** Human-readable location, e.g. `app "crm" · nav "nav_forecast"`. */ + where: string; + /** Config path, e.g. `apps[0].navigation[3].objectName`. */ + path: string; + /** What is wrong. */ + message: string; + /** How to fix it. */ + hint: string; +} + +type AnyRec = Record; + +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v as AnyRec[]; + if (v && typeof v === 'object') { + return Object.entries(v as AnyRec).map(([name, def]) => ({ name, ...(def as AnyRec) })); + } + return []; +} + +function strName(v: unknown): string | undefined { + return typeof v === 'string' && v.length > 0 ? v : undefined; +} + +/** One navigation entry that exposes an object. */ +interface NavExposure { + objectName: string; + where: string; + path: string; +} + +/** Collect every object a stack's navigation exposes, across areas and children. */ +function collectNavExposures(stack: AnyRec): NavExposure[] { + const out: NavExposure[] = []; + const apps = asArray(stack.apps); + + for (let ai = 0; ai < apps.length; ai++) { + const app = apps[ai]; + if (!app || typeof app !== 'object') continue; + const appName = strName(app.name) ?? `#${ai}`; + + const walk = (items: unknown, basePath: string) => { + const navItems = asArray(items); + for (let ni = 0; ni < navItems.length; ni++) { + const nav = navItems[ni]; + if (!nav || typeof nav !== 'object') continue; + const navPath = `${basePath}[${ni}]`; + const objectName = strName(nav.objectName); + if (nav.type === 'object' && objectName) { + out.push({ + objectName, + where: `app "${appName}" · nav "${strName(nav.id) ?? `#${ni}`}"`, + path: `${navPath}.objectName`, + }); + } + if (Array.isArray(nav.children)) walk(nav.children, `${navPath}.children`); + } + }; + + walk(app.navigation, `apps[${ai}].navigation`); + const areas = asArray(app.areas); + for (let ri = 0; ri < areas.length; ri++) { + walk(areas[ri]?.navigation, `apps[${ai}].areas[${ri}].navigation`); + } + } + + return out; +} + +/** + * Validate that every object a stack's navigation exposes is readable by at + * least one permission set the stack declares. Returns findings (empty = clean). + */ +export function validateNavAccess(stack: AnyRec): NavAccessFinding[] { + const findings: NavAccessFinding[] = []; + if (!stack || typeof stack !== 'object') return findings; + + // No permission sets in this stack ⇒ permissions are managed elsewhere. + const permissionSets = asArray(stack.permissions); + if (permissionSets.length === 0) return findings; + + const exposures = collectNavExposures(stack); + if (exposures.length === 0) return findings; + + // Objects this stack actually defines — the only ones whose grants must be + // present here. A nav target that resolves nowhere is a different bug, owned + // by `validate-object-references` / `defineStack`. + const ownObjects = new Set(); + for (const obj of asArray(stack.objects)) { + const n = strName(obj.name); + if (n) ownObjects.add(n); + } + + // `read` is already folded across allowRead / viewAllRecords / modifyAllRecords. + const readable = new Set(); + for (const entry of buildAccessMatrix(stack).entries) { + if (entry.read) readable.add(entry.object); + } + // A wildcard grant (`objects: { '*': { allowRead: true } }`) covers every + // object — the shape the platform's own `admin_full_access` uses. Without + // this the matrix records the literal key `*` and every object looks + // ungranted, which would make the rule fire on exactly the stacks that + // granted the most. + if (readable.has('*')) return findings; + + // De-duplicate: one finding per object, not per nav entry that exposes it. + const reported = new Set(); + + for (const exposure of exposures) { + const { objectName } = exposure; + if (reported.has(objectName)) continue; + if (isPlatformProvidedObjectName(objectName)) continue; // granted by its own package + if (!ownObjects.has(objectName)) continue; // not ours to grant + if (readable.has(objectName)) continue; + + reported.add(objectName); + findings.push({ + severity: 'warning', + rule: NAV_OBJECT_UNGRANTED, + where: exposure.where, + path: exposure.path, + message: + `navigation exposes object "${objectName}", but no permission set this stack ` + + `declares grants read on it — the entry renders, and opening it fails ` + + `permission-denied for every principal except one holding the platform's ` + + `built-in wildcard admin set. It works when you browse as an administrator ` + + `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.`, + }); + } + + return findings; +}