validate-permission-group-item

# Validate Permission Group Item Skill The question is not "does this key exist in the right places" — the registry makes most of that compiler-enforced. It is: > **If an organization admin sets this, what refuses, and can I make that refusal happen?** A key with an admin checkbox but no server gate passes every structural audit. Assume nothing enforces until you have found the throw. **`add-permission-group-item` owns the procedure and the rationale for every invariant named below.** Read it for *why*; this skill is the checklist. Its "Read the system first" list is the same one — start there. ## Step 1: Registry entry (`lib/permission-groups/fields.ts`) Record the builder, the `enforcement`, and the position. - **Default permissive?** The builders hardcode `false` / `null` / `[]`, so the risk is a *name* that inverts the meaning — an `allowX` boolean. The checkbox renders `checked={!editingConfig[feature.configKey]}` (ticked = allowed), so a positively-named boolean renders backwards. - **Position stable?** Declaration order is the wire order, and no test catches a reorder. If `git log -p` shows the key was ever *moved* rather than appended, every open editor reads as unsaved (`group-detail.tsx` dirty-checks stringified configs). - **Phrasing accurate?** An allowlist's `{ limited, empty }` and a denylist's string are read by `getActivePermissionGroupRestrictions` in `features.ts` and surface to users through the Copilot workspace VFS and the enterprise platform context. Confirm `empty` says "none allowed", not "unrestricted". - **Does the `hint` tell the truth?** Highest-value read in this step. A `'capability'` key refuses at the API, so a hint saying it hides a tab, module, or nav item "from the sidebar" is a **lie an admin acts on** — they believe they are tidying chrome while withholding a module. The same string is reused as the prose for an *active* restriction, where "hide" is simply false. Any surviving "Hide the …" hint on a `'capability'` key is a finding, not a nit; check `label` and `category` the same way (a "Sidebar" or "Settings Tabs" section makes the claim structurally). ## Step 2: Schemas, type, defaults, parser All derived by `collectFieldProperty` from the same registry. **Do not hand-verify them.** Verify nothing bypasses the derivation: ```bash grep -rn "<configKey>" apps/sim --include='*.ts' --include='*.tsx' \ | grep -vE 'lib/permission-groups/(fields|resolve\.server|config-scope\.server)\.ts' ``` Only the registry and the resolvers are excluded, so `capabilities.ts` stays in the output — its `CAPABILITY_RULES` entry and `deniedBy` are the authoritative reads this step exists to check. Every hit should be a rule's `deniedBy`, an enforcement site, a UI binding, or a test. A route restating the key, a client re-deriving a default, or a second coercion path is a leak. Specifically: - **`z.array(...).catch(...)` anywhere on this key's path** — whole-value tolerant, so one bad member discards every good one, and on an allowlist the `null` fallback means unrestricted. That is fail-**open**. `tolerantArray` filters element-wise. Rank a regression here with the enforcement findings. - **`?? []` applied to an allowlist** — collapses "allows everything" into "allows nothing". - **A hand-rolled comparison against an integration allowlist.** `allowedIntegrations` and the deployment's `ALLOWED_INTEGRATIONS` are written independently, so one names `slack` where the other names `slack_v2`; anything folding only case intersects them to nothing and hides an integration both allow. Both halves must canonicalize through `integration-allowlist.ts` (`intersectAccessControlAllowlists` / `toAccessControlAllowlist` / `resolveAccessControlBlockType`) *before* intersecting, with the checked type resolved the same way. That module reads `block-successors.generated.ts` through `Object.hasOwn` — a bare bracket lookup answers an admin-supplied `constructor` with an inherited function and 500s every path reading that group; `check:block-successors` catches the map going stale. - **Any config read not from `parsePermissionGroupConfig` or a `resolvePermissionGroupConfig` caller.** Two structural guards must still be present: - **`parsePermissionGroupConfig` still tests `Array.isArray(config)`.** `typeof [] === 'object'`, the column is `jsonb` so a row genuinely can hold `[]`, and `z.object().parse([])` throws — the guard is what returns defaults instead of a 500. `tolerantArray` carries the mirror image. - **`CAPABILITY_RULES` still uses `satisfies`, not an annotation.** An annotation collapses `StaticPermissionGroupCapability` to `never`, silently disabling the type system around capabilities with nothing wrong at runtime. `AssertsStaticCapabilityResolves` catches it; any weakening is a top-tier finding. Confirm the assertions at the bottom of `fields.ts` still name a field of this kind (`AssertsAllowlistStaysPrecise`, `AssertsDenylistStaysPrecise`, `AssertsRestrictionStaysPrecise`, `AssertsAuthTypesStayPrecise`, `AssertsParserReturnsTheConfig`) — a zod generic degrading to `unknown` is invisible at runtime and quietly loses every call site's narrowing. ## Step 3: Admin UI (`ee/access-control/components/group-detail.tsx`) - **Boolean:** appears automatically via `PLATFORM_FEATURES`. Confirm its `category` is in `PLATFORM_CATEGORY_ORDER`; an unlisted one renders after every ordered section. - **Nested allowlist / denylist** (one that qualifies a platform-feature boolean): renders **nothing** unless it is in the `featureExtras` map — keyed by the *parent boolean's feature id*, not the config key. No picker there means no admin can ever set it. Report it. Top-level lists — `allowedIntegrations`, `allowedModelProviders`, `deniedModels`, `deniedTools` — are not in `featureExtras` and must not be reported for it; they render from the dedicated Providers and Blocks sections, so check them there. - For an **allowlist** picker, check both behaviors: refuses an empty selection (`if (values.length === 0) return`) and collapses a full one back to `null` (otherwise the allowlist freezes at today's members). A **denylist** picker must do neither: clearing every entry is how an admin denies nothing, and a full selection is a real state that denies everything. - Check the parent is the right one (`allowedKnowledgeConnectors` under `hide-knowledge-base`, not `disable-knowledge-base-creation`). ## Step 4: Capability rule A `'capability'` key must appear in some rule's `configKeys` — the audit asserts this (D) and the converse (E): a key declared `'executor'` or `'ui-only'` that a rule reads is flagged, so a key cannot gain enforcement while staying documented as weaker. Then check what the audit cannot: - **`configKeys` lists every key `deniedBy` reads.** The audit parses it textually and never reads the closure; a key read but unlisted is invisible to D and E. - **`kind` is right.** A rule needing a request value must be `'parameterized'` — and a parameterized rule named on an operation cannot have run in production (`defineWorkspaceOperation` throws at definition time), so something else is wrong. - **A narrower capability subsumes the broader one it replaced.** An operation carries exactly one capability. Precedent: `knowledge.create` / `knowledge.upload` both read `hideKnowledgeBaseTab`, without which a group withholding the whole module could still create a KB through the API. Check `git log` for a re-pointed `capability:` and verify the narrower rule grew the broader key in the same commit. - **`detailCode` matches the remedy** — `FORBIDDEN_DETAIL_CODES` is closed over remedies, not causes; otherwise `PERMISSION_GROUP_CAPABILITY_BLOCKED`. Any code in use needs a TSDoc-documented entry in `FORBIDDEN_DETAIL_CODES`, published to OpenAPI as the `V2ForbiddenDetailCode` enum. - **`describe` reads correctly** as the subject of `"<describe> is not available under your organization's permission group"` — a singular noun or gerund agreeing with "is". Exactly two functions build that sentence, both defined in `capabilities.ts` (`refuseCapability` throws it, `capabilityRefusal` returns it); any call site writing it out is a drift finding. ## Step 5: Prove the enforcement — do not assume it The step the skill exists for. Find the **actual refusal**, name file and line, and say what a caller sees. ```bash grep -rn "'<capability-id>'" apps/sim --include='*.ts' --include='*.tsx' grep -rn "permission-group-enforced: <capability-id>" apps/sim ``` The second grep misses a gate whose annotation sits in a TSDoc block above the enclosing statement — read the surrounding function. Classify into exactly one of: 1. **Declared on operations.** The funnel enforces in `requireCurrentHumanAccess` → `requireCapability`. Verify the set is *complete*: enumerate every route and tool reaching the same behavior. One declaring `capability: 'none'` is the hole. 2. **Asserted at a call site** with a `// permission-group-enforced: <id> — <reason>` annotation. Verify it goes through `capability-assertions.ts` (`assertWorkspaceCapability`, `isWorkspaceCapabilityWithheld`, `isOrganizationCapabilityWithheld`, `capabilityDeniedBy`), through `isCapabilityWithheldForUser` (`lib/permission-groups/user-scope.server.ts` — workspace group first, else the organization's default, for a user-level act that may or may not name a workspace; outside `capability-assertions.ts` on purpose because it reads org membership through the billing graph, a guarded root of `check:application-graph`; `app/api/cli/auth/approve/route.ts` is the shape), or a direct `CAPABILITY_RULES['<id>'].deniedBy(...)` rather than reading `config.disableX` inline, **and** that it *raises* through `refuseCapability` / renders `capabilityRefusal(cap)` rather than building its own `ForbiddenOperationError` with a hand-written message — the easy half to miss, because the decision looks right. Use-case shape: `validatePublicFileSharing`, `validateChatDeployAuth` (`ee/access-control/utils/permission-check.ts`), `assertConnectorTypeAllowed` (`lib/knowledge/application/connectors.ts`). Raw-route shape: `app/api/logs/stats/route.ts`, `app/api/table/[tableId]/export/route.ts`. A raw route should render through `capabilityRefusalResponse` (`lib/permission-groups/capability-response.ts`), which reads `details.code` off the rule — a hand-rolled `NextResponse.json({ error: capabilityRefusal(cap) }, { status: 403 })` drops it, reporting the four specifically-coded capabilities (`deploy.chat.auth_mode`, `file_share.publish`, `file_share.auth_mode`, `personal_api_key.use`) as the generic block. Convergence is partial: `grep -rln "capabilityRefusal(" apps/sim/app --include=route.ts` lists the raw routes that still hand-roll it (ignore `*.test.ts`, `app/api/v1/middleware.ts`, `app/api/table/utils.ts`, and the v2 envelope, which are not raw-route responses). A raw route you add or touch renders through `capabilityRefusalResponse`; report an untouched hand-rolled one as a finding when its capability carries a specific code, otherwise as a note. v1 is deliberately not converged on it (`resolveCapabilityRefusal` in `app/api/v1/middleware.ts`). 3. **Executor-gated** by `assertPermissionsAllowed`, per block / tool / model, matching through the shared primitives in `lib/permission-groups/` — `block-access.ts`, `operation-access.ts`, `model-access.ts`, `integration-allowlist.ts` — which the editor and Copilot projections read too, so a second copy of a match rule is a finding. Verify the branch throws a real error and that the id it compares against is the vocabulary the admin UI writes — `deniedTools` holds block `tools.access` ids verbatim, version suffix included. `allowedIntegrations` is *also* enforced off the run, by `assertSelectorIntegrationAllowed` (`lib/selectors/server/integration-access.ts`), so an executor key's coverage is not complete until every non-run path that reaches the third party is checked too. 4. **A field projection, not a gate.** `logs.trace_spans` and `logs.cost` withhold fields, so the logs routes correctly declare `capability: 'none'`. Single owner: `lib/logs/projection.ts` (`resolveLogFieldProjection`, `projectExecutionData`, `projectCostTotal`), which carries both annotations. A **second** implementation of the same redaction is the finding — as is a query that lets a caller filter or sort on a withheld field, which turns the projection into an oracle. 5. **Nothing.** Report as a defect: "an organization that sets this believes it applied a restriction that does not exist". Ahead of all five: the principal-wide capabilities (`personal_api_key.use`, `oauth_apps.use`; `PRINCIPAL_WIDE_CAPABILITIES` in `lib/core/application/operation.ts`) fit none of them. They withhold a *principal kind* across every operation — the funnel's `personal_api_key` / `oauth_access_token` branches (`lib/core/application/workspace-authorization.ts`) and `app/api/v1/middleware.ts` — and declaring one on an operation throws, so their absence from every `capability:` field is correct, not a hole. Then **make the refusal happen**: write a failing case, or remove the gate (the `capability:` field, the `deniedBy` body, the assertion call) and confirm an existing test goes red. A test that still passes with the gate removed proves nothing. Restore afterward. **Check the fixture's `workspaceOrganizationId` first**: `requireCapability` short-circuits when it is `null` (`requireCapability` in `lib/core/application/workspace-authorization.ts`), so a context that leaves it unset passes either way and the existing test proves nothing even before you touch it. For an allowlist the three states must be tested separately — `null` permits every member, a populated list only the named ones, `[]` permits **none**. `capabilities.test.ts` pins all three for `knowledge.connectors`; less than that elsewhere is a gap. ### Who the gate runs against **Read the subject, not the nearest user id.** Every capability sink must take its subject from the `capabilityGoverned*` helper for the identity the surface holds — `capabilityGovernedPrincipalUserId` for a `Principal` (`lib/core/application`), `capabilityGovernedUserId` for a v1 `RateLimitResult` or a `TableAccessPrincipal`, `capabilityGovernedAuthUserId` for a `checkSessionOrInternalAuth` result. Each returns `null` where no group governs, and `null` is a pass. Reading `rateLimit.userId`, `auth.userId`, `subjectUserId` or `triggeredByUserId` into a sink is the finding: for a workspace key the first is the key's *creator*, for an internal JWT the second is the run's actor, and the last is a billing *attribution*. `check-capability-subject.ts` audits **v1 only**, so every other surface is on you. Where the subject is persisted and read back later (`capabilityGovernedUserId` on `table_run_dispatches` / `table_row_executions`), it must be declared required as `string | null` — an optional field with a fallback is exactly how producers re-inherited `triggeredByUserId`, so a proposal to make it optional is a finding. - **`/api/v1`** authorizes in `app/api/v1/middleware.ts`, not through `authorizeWorkspaceOperation`; `capabilityGovernedUserId(rateLimit)` branches on `keyType`, never on the presence of a user id. Each route also threads a required, spelled-out `V1RouteCapability`. - **Raw internal table routes** gate `tables.use` in `checkAccess` (`app/api/table/utils.ts`) via a `TableAccessPrincipal` union — `{ kind: 'user'; userId }` or `{ kind: 'workspace_api_key'; keyCreatorUserId }` — so a bare id does not type-check. `tableAccessPrincipal(rateLimit)` is the one place v1 builds it. - **The definition-time `undefined` guard** on `defineWorkspaceOperation` is not redundant even though `capability` is required on the `ApplicationOperation` **base type** (the `capability` field in `lib/core/application/operation.ts`, not merely on the builder — which is what stops a bare-literal factory from compiling): `apps/sim/tsconfig.json` excludes `*.test.ts` / `*.test.tsx` and the enforcement audit walks past test files, so a fixture is the one construction site no static check reads. Without it a capability-less operation defines cleanly and then throws `Cannot read properties of undefined` inside `capabilityDeniedBy` **only for tenants that actually have a permission group**, passing CI and every personal workspace. A proposal to drop it is a finding. ## Step 6: Tests - **`fields.test.ts`** — the key must be in both the `input` and `expected` halves of the `'a fully populated config'` fixture; that corpus is pinned so a changed row is defended rather than slipping through. The rest of the file derives from `DEFAULT_PERMISSION_GROUP_CONFIG` and needs no per-key edit. - **`capabilities.test.ts`** — a case for any rule with logic beyond reading one key: subsumption, allowlist three-state, auth-mode membership. - **`features.test.ts`** — no edit for a boolean; a non-boolean key's `limited` / `empty` prose should be pinned here. - **`config-scope.server.test.ts`** — the per-request memo. A gate resolving the config outside `resolvePermissionGroupConfig` is a Step 2 finding, not one here. ## Step 7: Run the checks ```bash bun run check:permission-group-enforcement bun run check:application-graph bun run check:capability-subject cd apps/sim && bun run type-check bun run --cwd apps/sim test lib/permission-groups ``` All three are inside `check:audits`, which derives its list from the `check:*` scripts in `package.json` — a new audit is opted *out* deliberately. Read the output, not the exit codes. Success-line shapes (the counts must include the item under audit): ``` ✓ permission-group enforcement: <N> operations declare a capability, <M> capabilities all enforced ✅ Application graph clean: <N> roots reach none of <M> forbidden module trees check:capability-subject — <N> v1 files, <M> capability subjects resolved through capabilityGovernedUserId. ``` | Audit | What it catches | |---|---| | `check:permission-group-enforcement` | Every operation declares a capability and every capability is enforced. All-or-nothing — no migration mode exits 0 with work outstanding, so do not go looking for a `pending enforcement:` list | | `check:application-graph` | The funnel roots (`lib/core/application/index.ts`, `capabilities.ts`, `capability-assertions.ts`, `config-scope.server.ts`) and `with-route-handler.ts` reach no heavy module tree at *runtime* (`import type` is erased and allowed). A gate that imports a resolver into a guarded root is a finding even if the gate is correct; such an edge can also make unrelated tests fail on partial mocks | | `check:capability-subject` | Every v1 capability sink takes its subject from `capabilityGovernedUserId`, no v1 file outside the middleware imports the permission-group modules, and at least one governed sink was found at all | Two ways the enforcement audit passes without proving what you want: - **Vacuous parse.** It reads source text with regexes, so it refuses success when the three registries parse to nothing, cross-checks rule count against capability count, reports per call any unreadable `id`, fails a file that mints an operation but parses to **zero** declarations, and flags any exported `*Operations` registry member it read no operation from. If one fires the audit is broken, not the code — fix the parsers rather than leaving it green. (That last guard is what catches an operation minted by a factory that never calls the builder, which bypasses the required type *and* the audit.) - **A capability declared on an operation nothing routes to.** Assertion C is satisfied by the declaration alone. The audits prove *reachability*, never correctness — that a capability is named, a key is read by some rule, a subject came from the right helper. Step 5 is what covers the rest. ## Known gaps — recognize these, do not re-report them Each is deliberate and documented in the code; `add-permission-group-item` carries the full reasoning.