feat(spec)!: 移除 app.areas[] 的两个 fail-open 访问闸门 —— visible / requiredPermissions (#4651)

.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.

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 |
 
 

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 |

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

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];