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; +}