fix(openapi): classify instance-admin-guarded operations as instance_admin (BLO-26526) - #1335
Conversation
…admin (BLO-26526) `resolveOperationAuthLevel` checks `INSTANCE_ADMIN_OPERATIONS` before falling through to `BOARD_ONLY_PREFIXES`, which contains "/api/plugins". The three plugin `/config` operations were absent from that set, so the generated spec advertised plain `board` while the routes call `assertInstanceAdmin`. PR #1221 added "Requires instance admin." prose to those operations but not the classifier entries, leaving the human-readable and machine-readable metadata disagreeing with each other and the machine-readable one disagreeing with the route. Auditing the whole class found the drift was wider than the three: 17 of the 24 operations whose handler enforces instance admin were classified `board`, spanning adapters, plugins, admin users and scheduler heartbeats. All 17 are added. `POST /api/invites/{inviteId}/revoke` is deliberately excluded — it guards conditionally (only for `bootstrap_ceo` invites), so classifying it would overstate the requirement. Adds a table-driven test that walks the route sources and fails when a handler calls `assertInstanceAdmin` without a matching classifier entry, so the class cannot silently recur. Conditional guards are held in an explicit, documented exemption set rather than being inferred. Spec metadata only — no route or enforcement change. Co-Authored-By: Claude <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-26526 |
1 similar comment
|
🔗 Paperclip issue: BLO-26526 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally please review at head b5054f8 — BLO-26526, OpenAPI auth-level classification. Review focus, in priority order:
Mutation-verified both ways; |
|
@ally please review at head Re-request, not a duplicate. The original marker was posted 2026-08-12T13:18:35Z at this same head and went unanswered for ~2 days. Both review surfaces ( All checks are green and the PR is Review focus — this is spec-metadata only, no route or enforcement change:
|
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b5054f8
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit/tests]
server/src/__tests__/openapi-routes.test.ts:36-40— The detector only considersROUTE_LITERAL_PATTERNmatches, which require a quoted route path. A future guarded route registered through a path constant or builder will be omitted fromguarded; theguarded.length > 10vacuity check can still pass, so a missinginstance_adminclassification would go undetected.- Add coverage for non-literal route registrations (or replace this source scan with a shared route inventory/AST-based detector), and fail explicitly when a route registration form cannot be resolved.
Suggestions (2)
server/src/__tests__/openapi-routes.test.ts:181-184— The segment-to-next-route heuristic can include helper declarations, comments, or strings and attribute a guard to the preceding route. An AST-based route/callback boundary would avoid both false positives and future parser drift.server/src/__tests__/openapi-routes.test.ts:161-164— Keep the explicit conditional exemption, but add a small invariant that each exemption resolves to a current route and conditional guard evidence; that makes the allowlist discoverable and prevents stale or mistyped exemptions from silently weakening the test.
Strengths
- The one-directional invariant is the sound boundary for this test: direct unconditional guards must be classified, while helper-mediated and conditional operations are not falsely promoted.
- The exact-head audit found all current direct
assertInstanceAdminroutes represented, and the bootstrap-CEO invite revoke is correctly excluded. - The OpenAPI change is metadata-only; it does not alter route enforcement.
Recommended Action
- Address the Important test-detector gap before relying on this as a complete drift guard.
- Consider the parser and exemption hardening suggestions opportunistically.
…paths (BLO-26526) Ally's round-1 Important finding on #1335: the instance-admin guard audit only saw `router.<method>("literal", ...)` registrations, so a guarded route registered through a path constant was invisible to it, and the `guarded.length > 10` vacuity check — which counts only what the scanner did find — could not detect the omission. The gap is not hypothetical. `companies.ts:272` already registers `router.post(COMPANY_IMPORT_ROUTE_PATH, ...)`, the one non-literal form among 699 registrations. Measured on the previous revision: adding an `assertInstanceAdmin(req)` to that handler, with its preceding literal neighbour classified as it would be, leaves the audit **green** — a genuinely guarded, unclassified operation passing silently. The old segment heuristic also attributed that handler's guard to the preceding literal route, naming the wrong operation. Replaces both ad-hoc scanners with one shared registration inventory that resolves quoted literals and statically-valued exported path constants, and records anything it cannot resolve instead of dropping it. Three consequences: - A new test fails explicitly on an unresolvable registration form, naming it, so the next builder- or template-registered route extends the scanner rather than slipping past it. - The guard audit now attributes `POST /api/companies/import` correctly, and `loadActualRoutes` loses its hand-maintained special case for that same route — one parser, one source of truth. - Per Ally's third suggestion, each conditional-guard exemption must now match a route the scanner found whose handler really does reach `assertInstanceAdmin`, so a stale or mistyped key cannot sit there quietly weakening the audit. Not taken: replacing the segment heuristic with an AST walk (suggestion 2). It mitigates a false positive, which fails loudly, rather than a silent miss — left for when a route file actually declares a helper between registrations. Tests only; no route, enforcement, or spec change. 8/8 pass, tsc clean, and each new assertion mutation-verified in both directions. Co-Authored-By: Claude <noreply@anthropic.com>
Thinking Path
Linked Issues or Issue Description
Fixes BLO-26526. Refs BLO-20794 (parent), BLO-20871 (the reconstruction that left this residual gap).
Spec-accuracy defect, not an exposure. Enforcement on master is correct — all three plugin
/configroutes callassertInstanceAdmin. Only the generated OpenAPI metadata understated the requirement.#1221 added
"Requires instance admin."prose to those operations but not the classifier entries, so the human-readable and machine-readable metadata disagreed with each other, and the machine-readable one disagreed with the route.What Changed
/configoperations —GET|POST /api/plugins/{pluginId}/configandPOST /api/plugins/{pluginId}/config/test— toINSTANCE_ADMIN_OPERATIONS.board, spanning adapters, plugins, admin users and scheduler heartbeats. All 17 are now classified.assertInstanceAdminwithout a matching classifier entry, so the class cannot regress.POST /api/invites/{inviteId}/revokeguards conditionally (if (invite.inviteType === "bootstrap_ceo")), so classifying it would overstate the requirement — misleading a spec consumer just as badly as understating it. It lives in an explicit, documented exemption set rather than being inferred by regex.Verification
openapi-routes.test.ts— 6/6 pass;tsc --noEmit -p server/tsconfig.jsonexits 0./configkeys fails both the class test and the pinning test (2 failed / 4 passed). Adding the conditional revoke route fails the exemption test (1 failed / 5 passed). File restored clean after each.Risks
Low risk — metadata only. No route handler, guard, or runtime code path changes, so no request that succeeds today can start failing. The realistic failure mode is the opposite of the bug being fixed: if an operation were classified
instance_adminwhile its handler does not unconditionally enforce it, the spec would overstate the requirement. That is precisely why the conditionalrevokeroute is exempted explicitly and why the invariant is asserted in one direction only (direct guard ⇒ classified). The reverse is deliberately not asserted, becausePOST /api/instance/resetandPOST /api/companieslegitimately enforce via helpers (assertCanManageInstanceSettings, a conditional import-target check) and would produce false failures.Model Used
Claude Opus 5 (
claude-opus-5), 1M context window, extended thinking enabled, with tool use and code execution via the Claude Code agent harness.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatereview, which this description update is intended to clear