diff --git a/.changeset/app-area-fail-open-gates-removed.md b/.changeset/app-area-fail-open-gates-removed.md new file mode 100644 index 0000000000..8d66af1971 --- /dev/null +++ b/.changeset/app-area-fail-open-gates-removed.md @@ -0,0 +1,58 @@ +--- +'@objectstack/spec': major +--- + +feat(spec)!: retire the two fail-open app-area gates — `app.areas[].visible` and `app.areas[].requiredPermissions` (#4651) + +These were **not** inert authoring keys. They were capability gates that **failed +open**: an author wrote `requiredPermissions: ['sales.admin']` on a navigation +area, got a clean parse and a stored value, and the area — with everything under +it — was served and rendered to **every user**. + +**This is a breaking change with a real migration.** Both keys are authorable +metadata keys on a `.strict()` schema, so existing `app` metadata that declares +either one now **fails to parse** with the prescription below. `authorable-surface.json` +is net **−2 keys**. This is not the "zero metadata migration" shape of the +same-window renames (#4661 C8, #4684 C9) — those kept every key. + +**The retirement kit:** + +| FROM | TO | Fix | +|---|---|---| +| `app.areas[].requiredPermissions` | *(removed)* | Delete the key. Gate each of the area's `navigation` items with `requiredPermissions` / `requiresService`, or gate the whole app with `requiredPermissions` on the AppSchema. | +| `app.areas[].visible` | *(removed)* | Delete the key. Move the same CEL expression onto the area's `navigation` items — a navigation **item**'s `visible` is evaluated per item by the shell. | + +The retired alias spellings `visibleWhen` / `visibleOn` / `permissions` carry the +same prescriptions rather than renaming onto keys that are themselves gone. + +Run `os migrate meta --from 16` to rewrite existing sources automatically +(ADR-0087 D2 conversion `app-area-fail-open-gates-removed`, wired into the +protocol-17 D3 chain step). + +**Why they read alive — and why that made them worse than dead.** The *same key +names* are genuinely enforced one level up and one level down: + +- **app-level** `requiredPermissions` — server-side: an app whose required + permissions the caller lacks is dropped from `/meta` entirely; +- **item-level** `requiredPermissions` / `requiresService` — stripped server-side + from the app's top-level `navigation` tree, and re-checked in the shell; + item-level `visible` is a real CEL gate in the shell. + +Three layers, of which the middle one was theatre — `filterAppForUser` reads the +app's `requiredPermissions` and then walks **only** `item.navigation`; it never +touches `item.areas`, and the client renders every area in the switcher. ADR-0078 +false compliance, the same shape as `capabilities.readOnly` (#4583). + +**Removed rather than enforced (ADR-0049), deliberately.** Enforcing area gates +is not wrong, it is unscoped: it needs semantics settled first — when an area is +filtered out, do its items disappear everywhere, or still participate in other +areas? does the server bind `user` for area-level CEL? — and a retirement must +not invent an authorization mechanism. Removing a gate that never gated is +strictly safer than shipping a major with it still declared, which would have +kept authors writing it for all of 17.x. + +**One caveat the prescription carries rather than hides:** per-item gating +*inside* an area is enforced by the shell only, because the server does not walk +`areas`. Anything that must never reach the browser belongs in the app's +top-level `navigation` tree, or in its own app. Trading one false belief for a +weaker one would have repeated the defect this removal exists to end. diff --git a/content/docs/references/ui/app.mdx b/content/docs/references/ui/app.mdx index 98256ec670..80405b8237 100644 --- a/content/docs/references/ui/app.mdx +++ b/content/docs/references/ui/app.mdx @@ -211,8 +211,6 @@ const result = ActionNavItemSchema.parse(data); | **label** | `string` | ✅ | Area display label | | **icon** | `string` | optional | Area icon name | | **description** | `string` | optional | Area description | -| **visible** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Visibility predicate (CEL) for this area. | -| **requiredPermissions** | `string[]` | optional | Permissions required to access this area | | **navigation** | `{ id: string; label: string; icon?: string; order?: number; … } \| { id: string; label: string; icon?: string; order?: number; … } \| { id: string; label: string; icon?: string; order?: number; … } \| { id: string; label: string; icon?: string; order?: number; … } \| { id: string; label: string; icon?: string; order?: number; … } \| { id: string; label: string; icon?: string; order?: number; … } \| { id: string; label: string; icon?: string; order?: number; … } \| { type: 'separator'; id?: string; order?: number } \| { id: string; label: string; icon?: string; order?: number; … }[]` | ✅ | Navigation items within this area | diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 90d86bc5c5..415c884a42 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -168,6 +168,8 @@ Separately, `object.managedBy: 'system'` is retired in favour of `'system-data'` Finally, five keys retire because the advisory lint could never have warned about them (#4509): mapping `extractQuery` / `errorPolicy` / `batchSize`, and app `contextSelectors[].includeAll` / `.placement`. Four of the five carry schema DEFAULTS, and a default materialises at parse time — so the liveness lint cannot tell a value the author wrote from one the schema supplied, and marking them would have warned on every mapping and every selector in existence. For a key in that state removal is not the escalation after a warning; it is the only channel that ever reaches the author, which is why they ship inside the 17.0.0 window rather than after a deprecation cycle. What they claimed: `extractQuery` promised an export path no exporter implements (exports go through the ordinary query API); `errorPolicy` offered skip/abort/retry where error handling belongs to the import REQUEST; `batchSize` sized batches the write path sizes itself; `placement` offered a topbar that places nothing. `includeAll` is the one worth reading twice — it was not unread but deliberately DISOBEYED, because context selectors are mandatory-scope and an "All" row would clear the scope: on Studio's package selector that means listing the platform's own system/cloud kernel packages to a developer who scoped to their package. `STUDIO_APP` authored `includeAll: true` against a renderer that ignored it. The mapping prescription for `batchSize` deliberately offers no rename: bulk-action, connector, sync, offline, seed-loader and NoSQL-cursor `batchSize` are all live, but each is a different key sizing its own path — the same trap `datasource.retryPolicy` vs `hook`/`job` `retryPolicy` had to defuse one issue earlier. +The sharpest removal in this step is two keys wide: `app.areas[].visible` and `app.areas[].requiredPermissions` (#4651). Read the class before the count — these were not inert authoring keys but FAIL-OPEN access gates. The server-side authority (`filterAppForUser`) checks the app's `requiredPermissions` and then walks ONLY the top-level `navigation` tree; it never reads `item.areas` at all, and the client renders every area in the switcher. So an author writing `requiredPermissions: ['sales.admin']` on an area got a clean parse, a stored value, and an area visible to everybody — and had every reason to believe otherwise, because the SAME key names are genuinely enforced one level up and one level down: app-level `requiredPermissions` drops the whole app server-side, and a navigation ITEM's `requiredPermissions` / `requiresService` are stripped server-side and re-checked in the shell, whose item-level `visible` is a real CEL gate. Three layers, of which the middle one was theatre. Enforcing instead was weighed and deliberately not taken here: it needs semantics decided first (does filtering an area remove its items everywhere? does the server bind `user` for area CEL?), and a retirement must not invent an authorization mechanism — while shipping a major with the gate still declared would have kept authors writing it for all of 17.x. The rewrite is lossless in outcome (the keys changed nothing), so what an upgrading author has to re-decide is only where the gate really goes: onto the items inside the area, or onto the app. One honest caveat the prescription carries rather than hides — per-item gating INSIDE an area is enforced by the shell only, since the server does not walk `areas`, so anything that must never reach the browser belongs in the top-level tree or in its own app. + The same window converges the retry policy (#4661). `@objectstack/spec/automation` and `@objectstack/spec/system` each exported a `RetryPolicy`/`RetryPolicySchema` resolving to a DIFFERENT declaration, so which shape a consumer got depended only on the import path (#4411) — yet both computed `delay = base * multiplier^(retry-1)` and both executors implemented that same formula. One declaration now serves both entries with the union of their capabilities, so `job.retryPolicy` gains the `maxRetryDelayMs` ceiling and `jitter` (both enforced in `runWithPolicy`, not merely declared — jitter is what stops a fleet of jobs that failed on one outage from retrying in lockstep). The single authorable casualty is the automation spelling of the base delay: `retryDelayMs` → `backoffMs`, a pure rename that replays losslessly and is what the already-enforced retry policies (`job.retryPolicy`, `hook.retryPolicy`) call it. The subtle half is the defaults, and it is worth stating because no gate can see it: `job.retryPolicy` defaulted `maxRetries: 3` / `backoffMultiplier: 2` while the automation shape defaulted 0 / 1, and the authorable-surface gate compares KEY SETS — a changed default is invisible to it, to the tombstone mechanism and to `spec_changes` alike. The merged declaration takes 0 / 1 (retry replays side effects, so it is opt-in — the same reading already recorded in `flow-retry-max-retries-required`), and the conversion writes the pre-17 numbers into every existing `job.retryPolicy` that omitted them. Deployed stacks therefore keep their exact behaviour; what changes is only what a NEWLY authored omission means. @@ -192,6 +194,7 @@ The same enforce-or-remove pass reaches the event vocabulary: `DataEventType` dr | `permission-rls-priority-removed` | `permission.rowLevelSecurity.priority` | RLS-policy key 'priority' removed (#3896 audit — policies OR-combine, so the promised conflict-resolution semantics cannot exist; dropping it changes no outcome) | retired — `migrate meta` only | | `tool-inert-authoring-keys-removed` | `tool.category / tool.permissions / tool.active / tool.builtIn` | tool keys 'category'/'permissions'/'active'/'builtIn' removed (#3896 close-out — authorable and inert; permissions gated nothing, active:false withdrew nothing) | retired — `migrate meta` only | | `app-dead-authoring-keys-removed` | `app.version / app.aria / app.objects / app.apis / app.sharing / app.embed / app.mobileNavigation / app.contextSelectors.includeAll / app.contextSelectors.placement / app.homePageId / app.areas.order` | app keys 'version'/'aria'/'objects'/'apis'/'sharing'/'embed'/'mobileNavigation'/'homePageId' plus contextSelectors 'includeAll'/'placement' and areas 'order' removed (liveness audits #4001, #4509, #4667 — never read; sharing/embed declared a public surface no route enforced, mobileNavigation was fully unimplemented, includeAll was deliberately disobeyed because an 'All' row would clear a mandatory scope, the landing page IS the first nav item, and no renderer ever sorted areas) | retired — `migrate meta` only | +| `app-area-fail-open-gates-removed` | `app.areas.visible / app.areas.requiredPermissions` | navigation-area keys 'visible'/'requiredPermissions' removed (#4651, ADR-0049 — FAIL-OPEN access gates: no layer ever read them, so a 'hidden' or permission-gated area was served and rendered to every user, while the identically named keys on a navigation ITEM and on the APP are enforced; gate the items inside the area, or gate the app) | retired — `migrate meta` only | | `field-required-notnull-explicit` | `object.fields.*.required / object.fields.*.storage.notNull` | required fields gain explicit 'storage.notNull: true' (ADR-0113 — pre-17 'required' implied the column constraint; post-17 it is only the write contract) | retired — `migrate meta` only | | `action-inert-keys-removed` | `action.shortcut / action.bulkEnabled` | action keys 'shortcut'/'bulkEnabled' removed (#3896 close-out — no keydown path dispatches shortcuts; the multi-select toolbar reads the view's bulkActions) | retired — `migrate meta` only | | `flow-inert-keys-removed` | `flow.active / flow.template / flow.nodes[].outputSchema / flow.errorHandling.fallbackNodeId` | flow keys 'active'/'template', node 'outputSchema' and errorHandling 'fallbackNodeId' removed (#3896 close-out — active:false never stopped a flow; status is the enforced lifecycle) | retired — `migrate meta` only | diff --git a/packages/cli/src/utils/lint-liveness-properties.test.ts b/packages/cli/src/utils/lint-liveness-properties.test.ts index 320ea9c03e..169c188fea 100644 --- a/packages/cli/src/utils/lint-liveness-properties.test.ts +++ b/packages/cli/src/utils/lint-liveness-properties.test.ts @@ -258,15 +258,19 @@ describe('lintLivenessProperties', () => { // ── #4488 — the nine remaining types, governed. Pins run against the REAL // ledgers, one per finding class the audit surfaced. - // The app ledger's most important entries: area-level gating keys that FAIL - // OPEN (nothing evaluates them, so a "hidden"/"gated" area shows for - // everyone), on the surface whose item-level siblings ARE enforced. - // `homePageId` and `areas.order` used to be asserted here too. Both were - // RETIRED in 17.0.0 (#4667) — the schema owns them now (a tombstone and a - // strict rejection respectively), so this advisory lint correctly says - // nothing about them. The two that remain are the ones #4651 still has to - // decide, and they are the reason this test exists. - it('warns on the fail-open area gates (#4488, tracked as #4651)', () => { + // The app ledger's most important entries were the area-level gating keys + // that FAILED OPEN — nothing evaluated them, so a "hidden"/"gated" area + // showed for everyone, on the surface whose item-level siblings ARE enforced. + // This test used to assert the WARNING. #4651 removed the keys (route B), + // so the advisory lint must now say nothing about them: `NavigationAreaSchema` + // is strict and rejects them at parse with the prescription, which reaches an + // author harder and earlier than an advisory line, and warning about a key + // that no longer parses is noise. Same disposition `homePageId` and + // `areas.order` reached in #4667. + // + // Kept as a SILENCE pin rather than deleted: a half-reverted retirement + // (ledger rows restored without the schema, or vice versa) shows up here. + it('is silent on the fail-open area gates — retired in 17.0.0 (#4651)', () => { const findings = lintLivenessProperties({ apps: [{ name: 'crm', @@ -281,12 +285,36 @@ describe('lintLivenessProperties', () => { }], }); const msgs = paths(findings); - expect(msgs.some((m) => m.includes('areas.visible'))).toBe(true); - expect(msgs.some((m) => m.includes('areas.requiredPermissions'))).toBe(true); - // The gating hints must point at the enforced alternative (per-item gates), - // or the warning just relocates the author's confusion. - const perms = findings.find((f) => f.message.includes('areas.requiredPermissions')); - expect(perms!.hint).toMatch(/per item|Per-item/i); + expect(msgs.some((m) => m.includes('areas.visible'))).toBe(false); + expect(msgs.some((m) => m.includes('areas.requiredPermissions'))).toBe(false); + expect(findings).toEqual([]); + }); + + // Anti-vacuity guard for the pin above. `lintLivenessProperties` resolves the + // shipped ledgers off `@objectstack/spec/package.json` and returns [] when it + // cannot find them — so "no findings" is also what a BROKEN lint returns, and + // the silence pin alone would pass on a lint that had stopped reading ledgers + // entirely. This asserts it still warns on a property that is still marked + // `authorWarn` (`object.externalSharingModel`, the last one in tree), in the + // same call that authors the retired area gates: same process, same ledger + // load, one warning and not three. + it('the area-gate silence is a real verdict, not a lint that stopped loading ledgers', () => { + const findings = lintLivenessProperties({ + objects: [{ name: 'widget', externalSharingModel: 'read' }], + apps: [{ + name: 'crm', + label: 'CRM', + areas: [{ + id: 'area_sales', + label: 'Sales', + visible: "'sales' in current_user.positions", + requiredPermissions: ['crm.access'], + navigation: [], + }], + }], + }); + expect(paths(findings).some((m) => m.includes('externalSharingModel'))).toBe(true); + expect(paths(findings).some((m) => m.includes('areas.'))).toBe(false); }); // email_template used to carry a per-artifact warn on `name`: the WHOLE diff --git a/packages/lint/src/validate-capability-references.ts b/packages/lint/src/validate-capability-references.ts index 6316c2bc9e..751c916489 100644 --- a/packages/lint/src/validate-capability-references.ts +++ b/packages/lint/src/validate-capability-references.ts @@ -166,8 +166,13 @@ export function validateCapabilityReferences(stack: AnyRec): CapabilityRefFindin } } - // ── Apps: requiredPermissions can appear at the app, area/tab, and nav-item - // (recursively through groups) levels. Walk each app subtree. ── + // ── Apps: requiredPermissions can appear at the app and nav-item + // (recursively through groups) levels. Walk each app subtree. `areas` is + // still traversed, but only to REACH the nav items nested inside it: the + // area itself stopped carrying `requiredPermissions` in 17.0.0 (#4651 — it + // was a fail-open gate nothing enforced), so the generic check below no + // longer fires on an area node. Dropping the traversal would strand every + // area-nested item. ── const apps = asArray(stack.apps); for (let i = 0; i < apps.length; i++) { const app = apps[i]; diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index 8fb6b2b887..ceb50597c7 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -2892,6 +2892,98 @@ describe('filterAppForUser — ADR-0045 hidden-app gate', () => { }); }); +// --------------------------------------------------------------------------- +// #4651 — the gates that ARE enforced, pinned after the fake one was removed +// +// `app.areas[].visible` / `app.areas[].requiredPermissions` left the spec in +// 17.0.0 because nothing evaluated them: this function reads the APP's +// `requiredPermissions` and then walks ONLY `item.navigation`. Removing a gate +// that never gated is safe exactly and only while the gates that DO exist keep +// working — otherwise the change trades "a gate that fails open" for "nobody +// checks whether the real gates are still there". Both surviving layers are +// pinned here, at the server that is the authority for them. +// --------------------------------------------------------------------------- + +describe('filterAppForUser — the enforced permission layers (#4651)', () => { + const make = () => new RestServer(createMockServer() as any, createMockProtocol() as any, ANON_API as any); + const ids = (a: any): string[] => (a?.navigation ?? []).map((e: any) => e.id); + + it('APP level: an app whose requiredPermissions the caller lacks is dropped entirely', () => { + const rest: any = make(); + const app = { name: 'crm', requiredPermissions: ['crm.access'], navigation: [] }; + expect(rest.filterAppForUser(app, new Set())).toBeNull(); + expect(rest.filterAppForUser(app, new Set(['other.perm']))).toBeNull(); + expect(rest.filterAppForUser(app, new Set(['crm.access']))?.name).toBe('crm'); + }); + + it('APP level: every declared permission is required, not any of them', () => { + const rest: any = make(); + const app = { name: 'crm', requiredPermissions: ['crm.access', 'crm.admin'], navigation: [] }; + expect(rest.filterAppForUser(app, new Set(['crm.access']))).toBeNull(); + expect(rest.filterAppForUser(app, new Set(['crm.access', 'crm.admin']))?.name).toBe('crm'); + }); + + it('ITEM level: nav entries the caller cannot satisfy are stripped from the served tree', () => { + const rest: any = make(); + const app = () => ({ + name: 'crm', + navigation: [ + { id: 'nav_leads', type: 'object' }, + { id: 'nav_forecast', type: 'object', requiredPermissions: ['sales.admin'] }, + { + id: 'grp_admin', type: 'group', children: [ + { id: 'nav_users', type: 'object', requiredPermissions: ['admin.access'] }, + { id: 'nav_about', type: 'url' }, + ], + }, + ], + }); + const out = rest.filterAppForUser(app(), new Set()); + expect(ids(out)).toEqual(['nav_leads', 'grp_admin']); + expect(out.navigation[1].children.map((c: any) => c.id)).toEqual(['nav_about']); + + const admin = rest.filterAppForUser(app(), new Set(['sales.admin', 'admin.access'])); + expect(ids(admin)).toEqual(['nav_leads', 'nav_forecast', 'grp_admin']); + expect(admin.navigation[2].children.map((c: any) => c.id)).toEqual(['nav_users', 'nav_about']); + }); + + it('a group left empty by the item gate is dropped, not served as a bare label', () => { + const rest: any = make(); + const app = { + name: 'crm', + navigation: [{ + id: 'grp_admin', type: 'group', + children: [{ id: 'nav_users', type: 'object', requiredPermissions: ['admin.access'] }], + }], + }; + expect(ids(rest.filterAppForUser(app, new Set()))).toEqual([]); + expect(ids(rest.filterAppForUser(app, new Set(['admin.access'])))).toEqual(['grp_admin']); + }); + + it('characterises the boundary the #4651 prescription warns about: `areas` is not walked', () => { + // NOT an endorsement — a characterisation. The server filters the top-level + // `navigation` tree only, so an item gate nested under `areas[]` is enforced + // by the shell alone. That is exactly why the retirement's guidance tells an + // author to put anything that must never reach the browser in the top-level + // tree or its own app, and why route A (enforce area gates server-side) is a + // separate decision with its own semantics to settle. Whoever makes this + // walk areas should see THIS expectation fail and rewrite it deliberately, + // updating the `areas.navigation` ledger note in the same change. + const rest: any = make(); + const app = { + name: 'crm', + navigation: [{ id: 'nav_home', type: 'object' }], + areas: [{ + id: 'area_admin', label: 'Admin', + navigation: [{ id: 'nav_users', type: 'object', requiredPermissions: ['admin.access'] }], + }], + }; + const out = rest.filterAppForUser(app, new Set()); + expect(ids(out)).toEqual(['nav_home']); + expect(out.areas[0].navigation.map((e: any) => e.id)).toEqual(['nav_users']); + }); +}); + // --------------------------------------------------------------------------- // ADR-0057 D10 — requiresService capability gate (filterAppForUser) // --------------------------------------------------------------------------- diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index e1eeaa303b..b14b4a10d1 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -7800,8 +7800,6 @@ "ui/NavigationArea:id", "ui/NavigationArea:label", "ui/NavigationArea:navigation", - "ui/NavigationArea:requiredPermissions", - "ui/NavigationArea:visible", "ui/NavigationConfig:mode", "ui/NavigationConfig:openNewTab", "ui/NavigationConfig:preventNavigation", diff --git a/packages/spec/liveness/README.md b/packages/spec/liveness/README.md index 73e9a95416..84820e3b4f 100644 --- a/packages/spec/liveness/README.md +++ b/packages/spec/liveness/README.md @@ -508,7 +508,7 @@ for t, v in r['types'].items(): | query | 16 | 1 | 4 | – | **not a metadata type** — the REQUEST surface (`QuerySchema`: client SDK QueryBuilder output; the `POST /data/:object/query` body), governed via `SPEC_ONLY_SCHEMAS` (#4286). The gate's one-level walk resolves 1 experimental; the 7 marker-experimental search affordances sit one level deeper, below the walk — resolved from `[EXPERIMENTAL — not enforced]` describe markers, not ledger entries (search `fuzzy`/`operator`/`boost`/`minScore`/`language`/`highlight` + `aggregations[].filter` — declared engine affordances no executor receives). The #4286 sweep closed out same-release: `having` ENFORCED 2026-07-31 (engine-side post-aggregation filter, both paths; was finding 1); dead 4 = the tombstoned removals `joins`/`windowFunctions`/`cursor`/`distinct` — REMOVED 2026-07-31 (retiredKey keeps each in the walked shape so the rows stay; protocol-17 semantic migrations; the JoinNode + WindowFunctionNode clusters and the `QueryBuilder.cursor()`/`.distinct()` producers deleted with their keys; `distinct`'s mis-wired REST count suppression deleted too — finding 2) | | datasource | 30 | 0 | 0 | 0 | seeded 2026-08-01 (#4487) — the **highest dead ratio of any governed type** (20 of 43), and it was ungoverned until now, which is not a coincidence: #4410/#4465/#4481 found six inert keys here by hand, two security-shaped (`schemaMode` left an external DB constructible as `managed` with DDL ungated; `ssl` configured nothing while looking configured). Dead set = `capabilities.*` (all 11 — the engine gates pushdown on the runtime driver's `supports.*` object, a non-overlapping vocabulary), `healthCheck.*` (3 — nothing schedules a datasource probe; the 20 `healthCheck` hits in the repo all belong to the PLUGIN health monitor and other surfaces), `retryPolicy.*` (4 — `retryPolicy` IS enforced on `hook` and `job`, which is what makes this one read alive; the shapes differ), `external.label`, `external.requirePermission`. **`capabilities.readOnly` is the one to know**: it reads as a safety switch, gates nothing, and two shipped prescriptions pointed authors at it until #4487 — `external.allowWrites: false` is the enforced write gate. `config` is a `z.record`, so its per-driver keys sit outside the walk (recorded in the entry's note, not silently skipped) **批 A CLOSED 2026-08-02 (#4583)**: the `capabilities` block — 11 flags, every one dead and authorWarn'd — was REMOVED rather than bridged; pushdown comes from the runtime driver's own `supports.*`, so there was nothing to connect it to. Its rows are deleted (strict-removal route), which is why dead falls 20 → 9. `readOnly` was the reason the audit was worth doing: it read as a safety switch, gated nothing, and had already been MOVED twice toward somewhere it might be enforced (#4410, #4465) — the shipped CRM example called a datasource a read replica on the strength of it while the datasource took writes. Removing it does NOT hand the author a working alternative: `external.allowWrites` only gates FEDERATED datasources, so a managed one has no read-only gate at all (#4584). Remaining 9 = healthCheck ×3 + retryPolicy ×4 + external ×2, batches B/C/D of #4583 **BATCHES B/C/D CLOSED 2026-08-02 — datasource now has ZERO dead properties**, down from the 20 it was seeded with (the highest dead ratio of any governed type). `retryPolicy` ×4 and `healthCheck` ×3 went as whole blocks, `external.label` / `external.requirePermission` as keys. None was bridgeable: each already had a different LIVE mechanism doing the job — the boot policy, the driver handle's on-demand `ping()`/`checkHealth()`, the top-level `label`, and ordinary permission sets + RLS. The `retryPolicy` rejection deliberately refuses to offer a rename: `hook`/`job` retryPolicy ARE enforced but spell the delay `backoffMs`, and that inconsistency is itself the evidence nothing read the datasource one (#4488's sharpest trap) | | webhook | 11 | 0 | 0 | – | **not a registered metadata type** — governed via the gate's spec-only schema override (`SPEC_ONLY_SCHEMAS`), not `getMetadataTypeSchema`; folding it onto the registry is the #3490 reassessment. This row once read 0/1/16 ("the ENTIRE authoring surface is dead", #3461) and both halves of that were CLOSED same-quarter: #3489 built the materializer bridge (authored `webhooks:` entries now land as `sys_webhook` dispatcher rows) and #3494 pruned the aspirational props outright — so the surviving surface is fully live. Kept in the table as the worked example that a dead verdict is a worklist entry, not a tombstone: enforce-or-remove resolved this one by ENFORCING | -| app | 45 | – | 11 | – | seeded 2026-08-01 (#4488). Dead 12 = the seven #4142 `retiredKey` tombstones (version/aria/objects/apis/sharing/embed/mobileNavigation — rows stay while the tombstones hold the keys in the walked shape) + `homePageId` (the landing IS the first nav item; root landing follows `isDefault` routing) + the **fail-open area gates** `areas.visible` / `areas.requiredPermissions` — nothing evaluates them while the per-ITEM siblings are enforced server- and client-side; `filterAppForUser` never reads `item.areas` at all (rest-server.ts:1823), so a "hidden" area shows to everyone. Both authorWarn'd, the audit's most important app finding, now tracked for decision as #4651 + `areas.order`/`description`. RETIRED 17.0.0 (#4509, rows deleted — the selector schema is strict): selector `includeAll` (deliberately DISOBEYED, not merely unread — selectors are mandatory-scope and an "All" row would clear the scope, leaking system metadata through Studio's package filter; STUDIO_APP authored it against a renderer that ignored it) and `placement` (no renderer read it; "topbar" placed nothing). Nav walk covers the union's `object` variant; other variants hand-verified live, and the `actionDef` dispatch gap closed in #4509 | **#4667**: `homePageId` TOMBSTONED (row stays — retiredKey keeps it in the walked shape) and `areas.order` row DELETED (strict removal). `homePageId` was described with its own hedge ("if not set, usually defaults to the first navigation item") — that WAS the only behaviour; `areas.order` read alive because the per-ITEM `order` really is sorted (NavigationRenderer.tsx:1154) while no renderer ever sorted areas. | +| app | 45 | – | 9 | – | seeded 2026-08-01 (#4488). Dead 9 = the seven #4142 `retiredKey` tombstones (version/aria/objects/apis/sharing/embed/mobileNavigation — rows stay while the tombstones hold the keys in the walked shape) + `homePageId` (#4667 tombstone — the landing IS the first nav item; root landing follows `isDefault` routing) + `areas.description` (benign, docs-shaped, kept and not warned). RETIRED 17.0.0 (#4509, rows deleted — the selector schema is strict): selector `includeAll` (deliberately DISOBEYED, not merely unread — selectors are mandatory-scope and an "All" row would clear the scope, leaking system metadata through Studio's package filter; STUDIO_APP authored it against a renderer that ignored it) and `placement` (no renderer read it; "topbar" placed nothing). Nav walk covers the union's `object` variant; other variants hand-verified live, and the `actionDef` dispatch gap closed in #4509 | **#4651**: the **fail-open area gates** `areas.visible` / `areas.requiredPermissions` — this ledger's most important app finding — are REMOVED, rows DELETED (strict removal; retained rows would report ORPHAN). They were not merely unread: `filterAppForUser` never reads `item.areas` at all and the shell renders every area, so a "hidden" or permission-gated area was served to everyone, while the identically named per-ITEM and per-APP keys ARE enforced. Route B (remove) over route A (enforce): enforcing needs semantics decided first (does filtering an area remove its items everywhere? does the server bind `user` for area CEL?), which the 17.0.0 window could not hold. Boundary unchanged and still recorded on `areas.navigation`: per-item gating inside an area is shell-side only. **#4667**: `homePageId` TOMBSTONED (row stays — retiredKey keeps it in the walked shape) and `areas.order` row DELETED (strict removal); `areas.order` read alive because the per-ITEM `order` really is sorted (NavigationRenderer.tsx:1154) while no renderer ever sorted areas. | | book | 20 | – | 1 | – | seeded 2026-08-01 (#4488). ADR-0046 §6 spine; `audience` is ENFORCED and fail-closed (tree 401/403 + per-doc effective-audience union on both list and tree). Dead 2 = BOTH inline `translations` maps (book-level and per-group): no resolver reads them and the bundle translator doesn't cover `book` — the trap is that `doc.translations` two files over works on every read path. Also recorded: the `include: { tag }` rule variant can never match (DocSchema declares no `tags`) | **#4667**: both inline translation maps retired — book-level row DELETED (BookSchema is strictObject), group-level row KEPT as a tombstone (BookGroupSchema is a plain z.object with no .strict(), so a bare delete would have zod silently strip it). No resolver read either; the trap was proximity to `doc.translations`, which is live on every doc render path. | | doc | 15 | – | 0 | – | seeded 2026-08-01 (#4488). Fully live: the kernel stores `content` unparsed, but the REST read layer localizes (resolveDocLocale), audience-gates, list-strips `content`, and the book resolver consumes name/label/description/order/group — plus the objectui console portal renders it all. The schema's own "docs are inert data" header describes the kernel, not the type. **`tags` DECLARED in 17.0.0 (#4509)** — the enforce half of enforce-or-remove: the book resolver's `include: { tag }` matcher, the REST transport and `ResolverDoc.tags` all already existed, but DocSchema is strict and had no `tags` key, so authoring one was a parse error and the variant could never match. Live on arrival | | email_template | 21 | 0 | 0 | 0 | this row read 8/–/13/– for one day (seeded 2026-08-01, #4488: "every authorable property is dead", the webhook shape on AUTH mail) and #4509 CLOSED it by ENFORCING — the second worked example, after `webhook`, that a dead verdict is a worklist entry rather than a tombstone. `bootstrapDeclaredEmailTemplates` materializes declared items into the `sys_email_template` rows `sendTemplate` reads, sharing `mapTemplateToRow` with the built-in seeder so the two doors cannot drift, and re-materializes on live metadata writes (`email_template` is `allowRuntimeCreate: true`, so boot-only would have left Studio saves inert). Three breaks had to close, not one: the engine never registered `emailTemplates:` into the registry, built-in seeds masqueraded as `managed_by: admin` and outranked declared templates, and nothing materialized. ADR-0054 proof bound on `subject` (`email-template-materialization`) | diff --git a/packages/spec/liveness/app.json b/packages/spec/liveness/app.json index 81e18b8910..47067c83de 100644 --- a/packages/spec/liveness/app.json +++ b/packages/spec/liveness/app.json @@ -1,6 +1,6 @@ { "type": "app", - "_note": "AppSchema — the navigation shell, the densest hand-authored surface on the platform. Consumers: the REST read layer's filterAppForUser (packages/rest/src/rest-server.ts:1796-1847 — the SERVER-side authority for app/nav permission + capability gating and ADR-0045 hidden-app visibility), the spec i18n translateApp (i18n-resolver.ts:472), and objectui's shell (@940ba24: app-shell AppSidebar/ConsoleLayout/ContextSelectors, layout NavigationRenderer, console RootLandingRedirect). The #4001/#4142 app step already retired seven dead keys as retiredKey tombstones — they stay in the walked shape, so their rows stay here (tombstone rule, orphans.mts). WALK BOUNDARY (#3095 union rule): `navigation` drills into the union's FIRST member (the `object` variant + base keys); the other variants' payload keys sit outside the walk and were verified by hand — dashboardName (NavigationRenderer.tsx:433), pageName (:435-442), url/target (:462), reportName (:460), componentRef (:464,:644), group `expanded` (:856) all live. The one GAP found there is now CLOSED (#4509, objectui @e8bec83): an `action` item's click dispatches through a host-supplied `onAction` prop that no shipped shell passed, so `actionDef.actionName` reached no dispatcher and every such item dead-clicked. objectui's `useNavActionDispatch` (objectui: packages/app-shell/src/hooks/useNavActionDispatch.ts) resolves the name against `action` metadata and dispatches through the console action runtime, and UnifiedSidebar passes it (objectui: packages/app-shell/src/layout/UnifiedSidebar.tsx:473). A shell that still passes no handler now HIDES action items rather than rendering them dead (objectui: packages/layout/src/NavigationRenderer.tsx:971) — the renderer stops manufacturing the trap. Also note filterAppForUser walks ONLY the top-level `navigation` tree — it never reads `item.areas` at all (rest-server.ts:1823 returns early when `navigation` is absent), and the client area switcher renders every area. So area-level `visible` / `requiredPermissions` are FAIL-OPEN gates, not merely unread: a \"hidden\" or permission-gated area shows to everyone. Confirmed 2026-08-02 while removing the contextSelectors keys; filed as #4651 (enforce-or-remove decision, deliberately NOT taken in #4509 so a retirement PR does not invent an authorization mechanism). Seeded 2026-08-01 (#4488). CONTEXT SELECTORS, 17.0.0 (#4509): `includeAll` and `placement` rows DELETED — AppContextSelectorSchema is strict, so the keys left the walked shape and retained rows would report ORPHAN. Both were unwarnable (schema defaults materialize at parse, so the lint could not tell authored from supplied), which made removal the only channel that could reach an author. `includeAll` was the sharp one: not unread but deliberately DISOBEYED — selectors are mandatory-scope, and an All row would clear the scope, which on Studio's package selector means listing the platform's own system/cloud kernel packages. STUDIO_APP authored `includeAll: true` against a renderer that ignored it, and that authoring site went with the key.", + "_note": "AppSchema — the navigation shell, the densest hand-authored surface on the platform. Consumers: the REST read layer's filterAppForUser (packages/rest/src/rest-server.ts:1796-1847 — the SERVER-side authority for app/nav permission + capability gating and ADR-0045 hidden-app visibility), the spec i18n translateApp (i18n-resolver.ts:472), and objectui's shell (@940ba24: app-shell AppSidebar/ConsoleLayout/ContextSelectors, layout NavigationRenderer, console RootLandingRedirect). The #4001/#4142 app step already retired seven dead keys as retiredKey tombstones — they stay in the walked shape, so their rows stay here (tombstone rule, orphans.mts). WALK BOUNDARY (#3095 union rule): `navigation` drills into the union's FIRST member (the `object` variant + base keys); the other variants' payload keys sit outside the walk and were verified by hand — dashboardName (NavigationRenderer.tsx:433), pageName (:435-442), url/target (:462), reportName (:460), componentRef (:464,:644), group `expanded` (:856) all live. The one GAP found there is now CLOSED (#4509, objectui @e8bec83): an `action` item's click dispatches through a host-supplied `onAction` prop that no shipped shell passed, so `actionDef.actionName` reached no dispatcher and every such item dead-clicked. objectui's `useNavActionDispatch` (objectui: packages/app-shell/src/hooks/useNavActionDispatch.ts) resolves the name against `action` metadata and dispatches through the console action runtime, and UnifiedSidebar passes it (objectui: packages/app-shell/src/layout/UnifiedSidebar.tsx:473). A shell that still passes no handler now HIDES action items rather than rendering them dead (objectui: packages/layout/src/NavigationRenderer.tsx:971) — the renderer stops manufacturing the trap. Also note filterAppForUser walks ONLY the top-level `navigation` tree — it never reads `item.areas` at all (rest-server.ts returns early when `navigation` is absent), and the client area switcher renders every area. That made area-level `visible` / `requiredPermissions` FAIL-OPEN gates, not merely unread: a \"hidden\" or permission-gated area showed to everyone. AREA GATES, 17.0.0 (#4651): both keys REMOVED and their rows DELETED — NavigationAreaSchema is strict, so the keys left the walked shape and retained rows would report ORPHAN. Route B (remove) over route A (enforce) was the maintainer's call: enforcing needs semantics decided first (does filtering an area remove its items everywhere? does the server bind `user` for area CEL?), which the 17.0.0 window could not hold, and a gate that never gated is strictly safer removed than shipped for a whole major. The strict rejection carries the prescription (ui/app.zod.ts AREA_VISIBLE_RETIRED / AREA_REQUIRED_PERMISSIONS_RETIRED) and names the layers that DO enforce. The underlying boundary is unchanged and still recorded on `areas.navigation` below: per-item gating inside an area is shell-side only. Seeded 2026-08-01 (#4488). CONTEXT SELECTORS, 17.0.0 (#4509): `includeAll` and `placement` rows DELETED — AppContextSelectorSchema is strict, so the keys left the walked shape and retained rows would report ORPHAN. Both were unwarnable (schema defaults materialize at parse, so the lint could not tell authored from supplied), which made removal the only channel that could reach an author. `includeAll` was the sharp one: not unread but deliberately DISOBEYED — selectors are mandatory-scope, and an All row would clear the scope, which on Studio's package selector means listing the platform's own system/cloud kernel packages. STUDIO_APP authored `includeAll: true` against a renderer that ignored it, and that authoring site went with the key.", "props": { "name": { "status": "live", @@ -91,7 +91,7 @@ "status": "live", "verifiedAt": "2026-08-01", "evidence": "objectui @940ba24: packages/layout/src/NavigationRenderer.tsx:891 (item gate); objectui packages/app-shell/src/layout/AppSidebar.tsx:236 (CEL evaluation via ExpressionProvider)", - "note": "the CEL visibility gate — enforced per item. Note the contrast with `areas[].visible`, which is NOT." + "note": "the CEL visibility gate — enforced per item. This is the layer the retired `areas[].visible` (#4651) prescribes moving to: same CEL dialect, one level down, and actually evaluated." }, "requiredPermissions": { "status": "live", @@ -177,20 +177,6 @@ "verifiedAt": "2026-08-01", "note": "display annotation no surface renders. Benign — docs-shaped, kept, not warned (hook.label precedent)." }, - "visible": { - "status": "dead", - "verifiedAt": "2026-08-01", - "authorWarn": true, - "authorHint": "Delete it, or gate the items INSIDE the area — nothing evaluates an area-level `visible` predicate, so a 'hidden' area renders for everyone: a capability gate that fails open, the worst shape of the silent no-op (#4001's own words). Per-ITEM `visible` IS enforced (NavigationRenderer.tsx:891).", - "note": "The schema declares it with the same CEL wording as the enforced item-level key, which is exactly what makes it a trap." - }, - "requiredPermissions": { - "status": "dead", - "verifiedAt": "2026-08-01", - "authorWarn": true, - "authorHint": "Delete it, or gate per item / per app — no layer checks area-level permissions (the server's filterAppForUser walks only the top-level `navigation` tree, and the client area switcher renders every area). Per-item `requiredPermissions` are enforced server + client, and app-level `requiredPermissions` are enforced server-side (rest-server.ts:1814).", - "note": "Fail-open access gate — same class as `visible` above; the two are this ledger's most important app findings." - }, "navigation": { "status": "live", "verifiedAt": "2026-08-01", @@ -198,7 +184,7 @@ "note": "the active area's tree replaces the top-level navigation. NOTE: area trees are NOT server-side permission-stripped (filterAppForUser reads only `item.navigation`) — per-item gating inside an area is client-side only." } }, - "note": "Drilled because the gating keys diverge sharply from the live identity/tree keys." + "note": "Drilled because the gating keys diverged sharply from the live identity/tree keys — and they are gone: `visible` and `requiredPermissions` were RETIRED in 17.0.0 (#4651), rows DELETED because NavigationAreaSchema is strict, so the keys left the walked shape and retained rows would report ORPHAN. Keep drilling: `description` is the surviving benign dead key, and the drill is what would catch a new gate being added here." }, "contextSelectors": { "children": { diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index 7c0d515b95..e0e084b1b4 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -152,6 +152,12 @@ "conversionId": "app-dead-authoring-keys-removed", "toMajor": 17 }, + { + "surface": "app.areas.visible / app.areas.requiredPermissions", + "to": "navigation-area keys 'visible'/'requiredPermissions' removed (#4651, ADR-0049 — FAIL-OPEN access gates: no layer ever read them, so a 'hidden' or permission-gated area was served and rendered to every user, while the identically named keys on a navigation ITEM and on the APP are enforced; gate the items inside the area, or gate the app)", + "conversionId": "app-area-fail-open-gates-removed", + "toMajor": 17 + }, { "surface": "object.fields.*.required / object.fields.*.storage.notNull", "to": "required fields gain explicit 'storage.notNull: true' (ADR-0113 — pre-17 'required' implied the column constraint; post-17 it is only the write contract)", @@ -729,6 +735,12 @@ "conversionId": "app-dead-authoring-keys-removed", "toMajor": 17 }, + { + "surface": "app.areas.visible / app.areas.requiredPermissions", + "to": "navigation-area keys 'visible'/'requiredPermissions' removed (#4651, ADR-0049 — FAIL-OPEN access gates: no layer ever read them, so a 'hidden' or permission-gated area was served and rendered to every user, while the identically named keys on a navigation ITEM and on the APP are enforced; gate the items inside the area, or gate the app)", + "conversionId": "app-area-fail-open-gates-removed", + "toMajor": 17 + }, { "surface": "object.fields.*.required / object.fields.*.storage.notNull", "to": "required fields gain explicit 'storage.notNull: true' (ADR-0113 — pre-17 'required' implied the column constraint; post-17 it is only the write contract)", diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index bad05e2c94..e128e2a594 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -1694,6 +1694,122 @@ const appDeadAuthoringKeysRemoved: MetadataConversion = { }, }; +/** + * `app.areas[].visible` / `app.areas[].requiredPermissions` removed + * (protocol 17, #4651, ADR-0049). + * + * Deliberately its own conversion rather than two more keys on + * `app-dead-authoring-keys-removed` above. That entry's summary is a list of + * inert authoring keys; these two are a **security** finding, and the summary + * string is what `spec-changes.json`, the generated upgrade guide and the + * `spec_changes` MCP tool serve to an upgrading consumer. Folded into the + * catch-all, "a gate that never gated has been removed" would arrive buried in + * a sentence about `version` and `mobileNavigation` — and it is the one line of + * this release an author with a gated area has to read. + * + * The defect: `filterAppForUser` (`packages/rest/src/rest-server.ts`) checks the + * APP's `requiredPermissions`, then walks ONLY `item.navigation` — it never + * reads `item.areas` — while the client renders every area in the switcher. An + * area declaring `requiredPermissions: ['sales.admin']` parsed, stored, served + * and rendered for every user. Fail open, on the surface whose per-ITEM and + * per-APP siblings of the same name ARE enforced (ADR-0078 false compliance). + * + * Stripping is lossless in the only sense that matters: the keys changed no + * outcome, so metadata behaves identically with or without them. What the + * author loses is the BELIEF that the area was gated — which is the point of + * the removal, and why the strict schema's `guidance` prescription (see + * `ui/app.zod.ts`) names the two layers that do enforce instead of just saying + * "removed". + * + * `retiredFromLoadPath`: the strict schema refuses the keys outright, so there + * is no alias window; the entry exists so `spec-changes.json` carries the + * removal and `os migrate meta --from 16` can rewrite authored sources. + */ +const appAreaFailOpenGatesRemoved: MetadataConversion = { + id: 'app-area-fail-open-gates-removed', + toMajor: 17, + retiredFromLoadPath: true, + surface: 'app.areas.visible / app.areas.requiredPermissions', + summary: "navigation-area keys 'visible'/'requiredPermissions' removed (#4651, ADR-0049 — FAIL-OPEN access gates: no layer ever read them, so a 'hidden' or permission-gated area was served and rendered to every user, while the identically named keys on a navigation ITEM and on the APP are enforced; gate the items inside the area, or gate the app)", + apply(stack, emit) { + const RETIRED_AREA_GATES = ['visible', 'requiredPermissions']; + return mapCollection(stack, 'apps', (app, path) => { + // `areas` is an ARRAY one level down, so `stripKeys` (top-level only) + // cannot reach it. Copy-on-write at both levels, so an app with nothing + // to strip keeps its identity for change detection. + const areas = app.areas; + if (!Array.isArray(areas)) return app; + let touched = false; + const mapped = areas.map((el, i) => { + if (!isDict(el)) return el; + const stripped = stripKeys(el, RETIRED_AREA_GATES, emit, `${path}.areas[${i}]`); + if (stripped !== el) touched = true; + return stripped; + }); + return touched ? { ...app, areas: mapped } : app; + }); + }, + fixture: { + // DISJOINT from `app-dead-authoring-keys-removed`'s fixture above and from + // every other entry: this app carries none of the keys another conversion + // strips, and that fixture's area carries neither gate — so each replays + // through the whole table hitting only its own entry. + before: { + apps: [{ + name: 'sales_portal', + label: 'Sales Portal', + areas: [ + { + id: 'area_admin', + label: 'Admin', + visible: "'sales_admin' in current_user.positions", + requiredPermissions: ['sales.admin'], + navigation: [{ id: 'nav_forecast', label: 'Forecast', type: 'object', objectName: 'forecast' }], + }, + // Untouched on purpose: the per-ITEM gate inside this area is the + // ENFORCED layer the prescription points at, and it spells the two + // key names identically. A sweep by key name alone would delete the + // working gate along with the fake one. + { + id: 'area_sales', + label: 'Sales', + navigation: [{ + id: 'nav_leads', label: 'Leads', type: 'object', objectName: 'lead', + visible: "'sales' in current_user.positions", + requiredPermissions: ['sales.access'], + }], + }, + ], + }], + }, + after: { + apps: [{ + name: 'sales_portal', + label: 'Sales Portal', + areas: [ + { + id: 'area_admin', + label: 'Admin', + navigation: [{ id: 'nav_forecast', label: 'Forecast', type: 'object', objectName: 'forecast' }], + }, + { + id: 'area_sales', + label: 'Sales', + navigation: [{ + id: 'nav_leads', label: 'Leads', type: 'object', objectName: 'lead', + visible: "'sales' in current_user.positions", + requiredPermissions: ['sales.access'], + }], + }, + ], + }], + }, + // Two notices: both gates on the ONE area that declared them. The second + // area and the nav item inside it must produce none. + expectedNotices: 2, + }, +}; + /** * RLS-policy `priority` removed (protocol 17, #3896 security audit). * @@ -3255,6 +3371,7 @@ export const CONVERSIONS_BY_MAJOR: Readonly { }); it('should accept area with full properties', () => { + // The area's own `visible` / `requiredPermissions` went out in 17.0.0 + // (#4651) — they gated nothing. Every gate this area declares now sits on + // an ITEM, which is the layer that is actually evaluated. const area = NavigationAreaSchema.parse({ id: 'area_service', label: 'Service', icon: 'headset', description: 'Customer service management', - visible: 'user.has_permission("service.access")', - requiredPermissions: ['service.access'], navigation: [ - { id: 'nav_cases', type: 'object', label: 'Cases', objectName: 'case' }, - { id: 'nav_knowledge', type: 'page', label: 'Knowledge Base', pageName: 'knowledge_base' }, + { + id: 'nav_cases', type: 'object', label: 'Cases', objectName: 'case', + requiredPermissions: ['service.access'], + }, + { + id: 'nav_knowledge', type: 'page', label: 'Knowledge Base', pageName: 'knowledge_base', + visible: 'user.has_permission("service.access")', + }, ], }); expect(area.id).toBe('area_service'); expect(area.icon).toBe('headset'); - expect(area.requiredPermissions).toEqual(['service.access']); expect(area.navigation).toHaveLength(2); + expect(area.navigation[0]).toMatchObject({ requiredPermissions: ['service.access'] }); }); it('should enforce snake_case for area id', () => { @@ -942,9 +949,14 @@ describe('AppSchema with areas', () => { id: 'area_settings', label: 'Settings', icon: 'settings', - requiredPermissions: ['admin.access'], navigation: [ - { id: 'nav_users', type: 'object', label: 'Users', objectName: 'user' }, + // The gate moved DOWN one level in 17.0.0 (#4651): on the area it + // was fail-open theatre, on the item it is stripped server-side + // and re-checked in the shell. + { + id: 'nav_users', type: 'object', label: 'Users', objectName: 'user', + requiredPermissions: ['admin.access'], + }, ], }, ], @@ -953,7 +965,7 @@ describe('AppSchema with areas', () => { expect(app.areas).toHaveLength(3); expect(app.areas![0].id).toBe('area_sales'); expect(app.areas![0].navigation).toHaveLength(2); - expect(app.areas![2].requiredPermissions).toEqual(['admin.access']); + expect(app.areas![2].navigation[0]).toMatchObject({ requiredPermissions: ['admin.access'] }); }); it('should accept app with both navigation and areas (backward compatibility)', () => { @@ -1335,4 +1347,141 @@ describe('unknown keys are rejected, not stripped (#4001 PR B)', () => { expect(app.areas![0]).not.toHaveProperty('order'); }); }); + + // ── areas[].visible / areas[].requiredPermissions, 17.0.0 (#4651) ───────── + // + // These were FAIL-OPEN gates, which is why the rejections below assert the + // reason and not just the refusal: an author who reads only "removed" goes + // looking for the replacement knob on the area, and there is none. The + // message has to move them one level down (per item) or one level up (per + // app) — the two layers that are actually enforced. + describe('retired fail-open area gates (#4651)', () => { + const withArea = (extra: Record) => ({ + name: 'crm', label: 'CRM', + areas: [{ id: 'area_admin', label: 'Admin', navigation: [], ...extra }], + }); + + it('rejects `areas[].requiredPermissions` and names the layers that DO enforce', () => { + const msg = unknownKeyIssue(AppSchema, withArea({ requiredPermissions: ['sales.admin'] }))!.message; + expect(msg).toMatch(/requiredPermissions.*removed.*17\.0\.0/s); + // The class, not just the fact: fail-open is why this is a defect rather + // than dead weight, and it is what tells the author to re-audit anything + // they believed this key protected. + expect(msg).toMatch(/fail-open|every user/is); + // The two enforced layers, named. + expect(msg).toMatch(/on the APP/s); + expect(msg).toMatch(/navigation ITEM/s); + // The honest caveat: the server does not walk areas, so an item gate + // INSIDE an area is shell-side only. Without this the prescription would + // trade one false belief for a weaker one. + expect(msg).toMatch(/shell only|does not walk/is); + }); + + it('rejects `areas[].visible` and points at the item-level CEL gate that is evaluated', () => { + const msg = unknownKeyIssue(AppSchema, withArea({ visible: "'admin' in current_user.positions" }))!.message; + expect(msg).toMatch(/visible.*removed.*17\.0\.0/s); + expect(msg).toMatch(/fails open|EVERYONE/s); + expect(msg).toMatch(/navigation ITEM's `visible`/s); + }); + + it('routes the retired gating aliases to the same prescriptions', () => { + // `visibleWhen` / `visibleOn` / `permissions` used to RENAME onto the two + // keys this issue removed. An alias pointing at a key that is itself gone + // answers an unknown key with a second unknown key, so each became a + // prescription instead (the #4667 `sort` precedent). + for (const alias of ['visibleWhen', 'visibleOn']) { + expect(unknownKeyIssue(AppSchema, withArea({ [alias]: 'x' }))!.message) + .toMatch(/`areas\[\]\.visible` was removed/s); + } + expect(unknownKeyIssue(AppSchema, withArea({ permissions: ['x'] }))!.message) + .toMatch(/`areas\[\]\.requiredPermissions` was removed/s); + }); + + it('an area gating nothing still parses, and the ITEM-level gates are untouched', () => { + // The removal must not take the working gate with it: the item keys are + // spelled identically, one level down, and they are the prescription's + // destination. + const app = AppSchema.parse({ + name: 'crm', label: 'CRM', + areas: [{ + id: 'area_admin', label: 'Admin', + navigation: [{ + id: 'nav_users', type: 'object', label: 'Users', objectName: 'user', + requiredPermissions: ['admin.access'], + visible: "'admin' in current_user.positions", + requiresService: 'org-scoping', + }], + }], + }); + expect(app.areas![0]).not.toHaveProperty('visible'); + expect(app.areas![0]).not.toHaveProperty('requiredPermissions'); + expect(app.areas![0].navigation[0]).toMatchObject({ + requiredPermissions: ['admin.access'], + requiresService: 'org-scoping', + }); + expect(app.areas![0].navigation[0]).toHaveProperty('visible'); + }); + + it('the APP-level `requiredPermissions` gate is untouched by this removal', () => { + // The other enforced layer the prescription names (rest-server's + // filterAppForUser drops the whole app). Pinned here so "we removed the + // area gate" can never quietly become "we removed the app gate too". + const app = AppSchema.parse({ + name: 'crm', label: 'CRM', requiredPermissions: ['crm.access'], + navigation: [{ id: 'nav_home', type: 'object', label: 'Home', objectName: 'account' }], + }); + expect(app.requiredPermissions).toEqual(['crm.access']); + }); + + // The type-level half, and the only way to measure it that is not a no-op. + // + // `NavigationArea` is a TYPE — erased before any runtime assertion can see + // it — so an `Assert< Equal< … > >` pin written in this file would never + // run: `packages/spec/tsconfig.json` excludes `**/*.test.ts` and vitest does + // not type-check (#4642). Every assertion above would stay green if the two + // keys were re-added to the schema's TS shape while the parse kept + // rejecting them. This resolves the exported type through the TypeScript + // compiler API and reads its members — the same symbol-identity measurement + // `check:dual-source-exports` makes, over `src/`, so it runs in `pnpm test` + // without a build. + it('the retired gates are gone from the TYPE, not only from the parse (compiler API)', async () => { + const ts = (await import('typescript')).default; + const { resolve, dirname } = await import('node:path'); + const { fileURLToPath } = await import('node:url'); + + const entry = resolve(dirname(fileURLToPath(import.meta.url)), 'app.zod.ts'); + const program = ts.createProgram([entry], { + module: ts.ModuleKind.ESNext, + moduleResolution: ts.ModuleResolutionKind.Bundler, + strict: true, + skipLibCheck: true, + noEmit: true, + }); + const checker = program.getTypeChecker(); + const sf = program.getSourceFile(entry); + // Guard #1: a resolution failure would make every assertion below pass + // vacuously — the exact way a gate goes dormant (#4642). + expect(sf, 'app.zod.ts must resolve').toBeTruthy(); + const moduleSym = checker.getSymbolAtLocation(sf!); + expect(moduleSym, 'app.zod.ts module symbol must resolve').toBeTruthy(); + + const areaSym = checker + .getExportsOfModule(moduleSym!) + .find((s) => s.getName() === 'NavigationArea'); + expect(areaSym, 'NavigationArea must be exported').toBeTruthy(); + + const props = checker + .getDeclaredTypeOfSymbol(areaSym!) + .getProperties() + .map((p) => p.getName()) + .sort(); + + // Guard #2: the alias really resolved to the object shape. Without this, + // a `z.infer` that degraded to `any`/`unknown` would report ZERO members + // and the two negatives below would pass while the type said nothing. + expect(props).toEqual(['description', 'icon', 'id', 'label', 'navigation']); + expect(props).not.toContain('visible'); + expect(props).not.toContain('requiredPermissions'); + }); + }); }); diff --git a/packages/spec/src/ui/app.zod.ts b/packages/spec/src/ui/app.zod.ts index d1d601638b..fc04e86579 100644 --- a/packages/spec/src/ui/app.zod.ts +++ b/packages/spec/src/ui/app.zod.ts @@ -614,6 +614,55 @@ const AREA_ORDER_RETIRED = + '`order` is genuinely sorted — this removal does not touch it. Run ' + '`os migrate meta --from 16` to rewrite existing sources automatically.'; +/** + * `app.areas[].visible` and `app.areas[].requiredPermissions`, retired in + * 17.0.0 (#4651, ADR-0049). + * + * These were not ordinary dead keys. They were **fail-open capability gates**: + * the authoritative server-side filter (`filterAppForUser`, + * `packages/rest/src/rest-server.ts`) reads the app's `requiredPermissions` and + * then walks ONLY `item.navigation` — it returns early when that tree is + * absent and never touches `item.areas` at all — while the client renders every + * area in the switcher. So an author who wrote `requiredPermissions: + * ['sales.admin']` on an area got a clean parse, a stored value, and an area + * visible to everyone. + * + * What made them read alive is that the SAME key names are genuinely enforced + * one level up and one level down: app-level `requiredPermissions` drops the + * whole app server-side, and a navigation ITEM's `requiredPermissions` / + * `requiresService` are stripped server-side and re-checked in the shell, whose + * item-level `visible` is a real CEL gate. Three layers, of which the middle one + * was theatre — ADR-0078 false compliance, the `capabilities.readOnly` shape + * (#4583). + * + * Enforcing them instead (route A) was considered and deliberately not taken in + * the 17.0.0 window: it needs semantics decided first (does filtering an area + * remove its items everywhere? does the server bind `user` for area CEL?), and + * a retirement PR must not invent an authorization mechanism. Removing a gate + * that never gated is strictly safer than shipping a major with it in place. + */ +const AREA_VISIBLE_RETIRED = + '`areas[].visible` was removed in @objectstack/spec 17.0.0 (#4651, ADR-0049) — nothing ever ' + + 'evaluated an area-level predicate, so an area "hidden" by one rendered for EVERYONE: a ' + + 'gate that fails open, which is worse than no gate at all. Delete the key and gate the ' + + 'items INSIDE the area — a navigation ITEM\'s `visible` takes the same CEL expression and ' + + 'IS evaluated per item by the shell. For a gate the SERVER enforces, use ' + + '`requiredPermissions`: on the app itself, or on items of the app\'s top-level ' + + '`navigation` tree. Run `os migrate meta --from 16` to rewrite existing sources ' + + 'automatically.'; + +const AREA_REQUIRED_PERMISSIONS_RETIRED = + '`areas[].requiredPermissions` was removed in @objectstack/spec 17.0.0 (#4651, ADR-0049) — ' + + 'no layer ever checked it, so a "permission-gated" area was served to, and rendered for, ' + + 'every user: a fail-open access gate, not merely an unread key. Delete it and move the ' + + 'gate to a layer that is actually enforced. `requiredPermissions` on the APP is checked ' + + 'server-side (the app is dropped from /meta entirely for a caller who lacks them), and ' + + '`requiredPermissions` / `requiresService` on a navigation ITEM are stripped server-side ' + + 'from the app\'s top-level `navigation` tree and re-checked in the shell. Items nested ' + + 'under `areas[]` are gated in the shell only — the server does not walk `areas` — so ' + + 'anything that must never reach the browser belongs in the top-level tree, or in its own ' + + 'app. Run `os migrate meta --from 16` to rewrite existing sources automatically.'; + /** * Navigation Area Schema * @@ -624,17 +673,24 @@ const AREA_ORDER_RETIRED = * Areas allow large applications to partition navigation by business function while * keeping a single AppSchema definition. The runtime may render areas as top-level tabs, * sidebar sections, or a switchable navigation context. - * + * + * An area is a LAYOUT grouping, not an access boundary: it carries no gate of + * its own. Gate the items inside it (`visible` / `requiredPermissions` on a + * navigation item) or gate the app (`requiredPermissions` on the AppSchema) — + * see AREA_VISIBLE_RETIRED / AREA_REQUIRED_PERMISSIONS_RETIRED above for why + * the area-level keys were removed in 17.0.0. + * * @example * ```ts * const salesArea: NavigationArea = { * id: 'area_sales', * label: 'Sales', * icon: 'briefcase', - * order: 1, * navigation: [ * { id: 'nav_leads', type: 'object', label: 'Leads', objectName: 'lead' }, - * { id: 'nav_opportunities', type: 'object', label: 'Opportunities', objectName: 'opportunity' }, + * // gate per ITEM — the layer that is actually enforced + * { id: 'nav_forecast', type: 'object', label: 'Forecast', objectName: 'forecast', + * requiredPermissions: ['sales.admin'] }, * ], * }; * ``` @@ -655,24 +711,33 @@ export const NavigationAreaSchema = lazySchema(() => z.object({ /** Area description */ description: I18nLabelSchema.optional().describe('Area description'), - /** - * Visibility condition. - * Formula expression returning boolean. - */ - visible: ExpressionInputSchema.optional().describe('Visibility predicate (CEL) for this area.'), - - /** Permissions required to access this area */ - requiredPermissions: z.array(z.string()).optional().describe('Permissions required to access this area'), + // `visible` and `requiredPermissions` removed in 17.0.0 (#4651) — see + // AREA_VISIBLE_RETIRED / AREA_REQUIRED_PERMISSIONS_RETIRED. Both were + // FAIL-OPEN gates: no layer read them, while the identically named keys on a + // navigation ITEM and on the APP are enforced. Gate the items inside the area + // (or the app) instead. /** Navigation items within this area */ navigation: z.array(NavigationItemSchema).describe('Navigation items within this area'), }, { error: strictUnknownKeyError({ surface: 'this navigation area', - knownKeys: ['id', 'label', 'icon', 'description', 'visible', 'requiredPermissions', 'navigation'], - // `sort: 'order'` retired with the key it pointed at (#4667). - aliases: { visiblewhen: 'visible', visibleon: 'visible', title: 'label', name: 'id', permissions: 'requiredPermissions', items: 'navigation', children: 'navigation' }, - guidance: { order: AREA_ORDER_RETIRED, sort: AREA_ORDER_RETIRED }, + knownKeys: ['id', 'label', 'icon', 'description', 'navigation'], + // `sort: 'order'` retired with the key it pointed at (#4667); the three + // gating aliases (`visibleWhen`/`visibleOn`/`permissions`) retired with + // theirs (#4651). An alias must never rename onto a key that is itself + // gone — it would answer "unknown key" with a second unknown key — so each + // moves to `guidance` and carries the prescription instead. + aliases: { title: 'label', name: 'id', items: 'navigation', children: 'navigation' }, + guidance: { + order: AREA_ORDER_RETIRED, + sort: AREA_ORDER_RETIRED, + visible: AREA_VISIBLE_RETIRED, + visibleWhen: AREA_VISIBLE_RETIRED, + visibleOn: AREA_VISIBLE_RETIRED, + requiredPermissions: AREA_REQUIRED_PERMISSIONS_RETIRED, + permissions: AREA_REQUIRED_PERMISSIONS_RETIRED, + }, history: 'Until #4001 these were dropped silently — the area still parsed, so its gating or ' + 'ordering was quietly ignored.',