From d7dc2a385ae0f4d6be44bc20fd2e9cdafbdfdf8f Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 22 Oct 2026 13:44:02 +0200 Subject: [PATCH] docs(19.6.1): record gap-closure code review --- .../19.6.1-REVIEW-DISPOSITION.md | 12 +-- .../19.6.1-REVIEW.md | 76 ++++++++++++++++++- 2 files changed, 81 insertions(+), 7 deletions(-) diff --git a/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW-DISPOSITION.md b/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW-DISPOSITION.md index cfebd8fb..9416f336 100644 --- a/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW-DISPOSITION.md +++ b/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW-DISPOSITION.md @@ -5,7 +5,7 @@ titles: json findings: - id: CR-01 severity: critical - disposition: open + disposition: fixed title: "Roles screen lets a non-superuser rename or create a role onto a code with code-declared grants (D-07 ceiling bypass, self-escalation)" - id: WR-01 severity: warning @@ -17,7 +17,7 @@ findings: title: "Stored override values other than 1/-1 are dropped on every save, re-granting what they removed" - id: WR-03 severity: warning - disposition: open + disposition: fixed title: "Record actions contributed to the Administrators screen bypass D-19 (outranking) and D-09 (last superuser)" - id: IN-01 severity: info @@ -43,19 +43,19 @@ findings: severity: info disposition: open title: "Role picker on your own record is editable in the SPA but always refused" -open: 10 +open: 8 total: 10 -recorded: 2026-10-22T10:36:13.024Z +recorded: 2026-10-22T11:50:00.000Z --- # Phase 19.6.1: Code Review Disposition | Finding | Severity | Disposition | Source | |---------|----------|-------------|--------| -| CR-01 | critical | open | - | +| CR-01 | critical | fixed | 19.6.1-04 refuseCodeDeclaredCode | | WR-01 | warning | open | - | | WR-02 | warning | open | - | -| WR-03 | warning | open | - | +| WR-03 | warning | fixed | 19.6.1-04 wrapAdministratorsContributedRun (D-19). D-09 on contributed Run stays out of the gap plan | | IN-01 | info | open | - | | IN-02 | info | open | - | | IN-03 | info | open | - | diff --git a/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW.md b/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW.md index 45a5f645..ef861a74 100644 --- a/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW.md +++ b/.planning/phases/19.6.1-backend-administrators-screen/19.6.1-REVIEW.md @@ -80,6 +80,26 @@ findings: info: 6 total: 10 status: issues_found +gap_closure: + reviewed: 2026-10-22T11:50:00Z + depth: standard + plan: "04" + files_reviewed: 7 + files_reviewed_list: + - modules/cabana/backend_roles_admin.go + - modules/cabana/action_extensions.go + - modules/cabana/action_extensions_test.go + - modules/cabana/roles_postgres_test.go + - modules/cabana/action_extensions_postgres_test.go + - modules/pact/capabilities.go + - scripts/check-phase19.6.1.sh + findings: + critical: 0 + warning: 0 + info: 0 + nice_to_have: 0 + total: 0 + status: clean --- # Phase 19.6.1: Code Review Report @@ -145,6 +165,8 @@ func (c rolesAdmin) checkCodeGrants(ctx context.Context, newCode, storedCode str ``` A stricter option that matches D-16 is to refuse `len(grants) > 0 && newCode != storedCode` for everyone, so a role can never become a code-declared system role through the screen. Add a Postgres test that renames a held role onto a code-declared code as a non-superuser and expects 403. +**Gap-closure (2026-10-22):** addressed by plan 04 (`refuseCodeDeclaredCode`). See the gap-closure pass below. Finding kept for history. + ## Warnings ### WR-01: D-07/D-19 ceilings ignore a scoped personal API token's scope @@ -179,6 +201,8 @@ Winter strips 0 on write, so the precondition needs rows written by SQL or other The first planned consumer is 19.7's frontend-link action, which binds a frontend account to a backend admin that MCP then acts as. Through the seam, any `backend.manage_users` holder could link their own frontend account to an administrator who outranks them, which is the account-takeover path D-19 was written to close. Every contributor would have to re-implement unexported guards it cannot reach. **Fix:** When compiling extensions whose target is `AdministratorsControllerID`, wrap each contributed `Run` (and `Applies`) so that `guardOutranked(ctx, reg, principal, record.(*BackendUser))` runs inside the action transaction before the contributor's `Run`. Alternatively, export a `cabana.GuardAdministratorWrite(ctx, record)` helper and document in `pact.AdminActionExtension` that it is required. Add a test with a contributed action on an outranking target that expects 403. +**Gap-closure (2026-10-22):** D-19 half addressed by plan 04 (`wrapAdministratorsContributedRun`). D-09 (last superuser on a contributed Run) and wrapping `Applies` were out of that plan's contract and are not re-filed. Finding kept for history. + ## Info ### IN-01: Last-superuser guard silently depends on READ COMMITTED @@ -219,6 +243,56 @@ The first planned consumer is 19.7's frontend-link action, which binds a fronten --- -_Reviewed: 2026-10-22T10:34:44Z_ +## Gap-closure pass (2026-10-22) + +**Reviewed:** 2026-10-22T11:50:00Z +**Depth:** standard +**Plan:** 04 (commits `068adaa`, `cb460b6`, `e620860`, `6bd2506`) +**Files Reviewed:** 7 +**Status:** clean + +This pass reviewed only the plan-04 gap-closure sources at HEAD. It does not re-review the original 70-file phase. + +### Gap-closure files + +- `modules/cabana/backend_roles_admin.go` (`refuseCodeDeclaredCode`, `FormBeforeCreate`, `FormBeforeUpdate`) +- `modules/cabana/action_extensions.go` (`wrapAdministratorsContributedRun`, `compileActionExtensions` wrap) +- `modules/cabana/action_extensions_test.go` +- `modules/cabana/roles_postgres_test.go` (`TestRolesCodeDeclaredCodeRefused`) +- `modules/cabana/action_extensions_postgres_test.go` (`TestActionExtensionOutranked`) +- `modules/pact/capabilities.go` (`AdminActionExtension` comment) +- `scripts/check-phase19.6.1.sh` (`DOCKER_TESTS` names) + +### Gap-closure summary + +No new Critical, Warning, Info, or Nice-to-have findings. + +`refuseCodeDeclaredCode` is called from create (stored code `""`) and update (stored code from the save transaction). It refuses when `submitted != stored` and `len(registry.rolePermissions(submitted)) > 0`, including for a superuser. Same-code saves of a system role still pass. `rolePermissions` is nil-safe. The 422 shape reuses `backend::lang.user.role.code_locked` on `code`. `guardOutranked` stays unexported. + +`compileActionExtensions` wraps a contributed `Run` only when `ext.Controller == AdministratorsControllerID`, after `checkContributedAction` has required a non-nil `Run`. The wrapper type-asserts `*BackendUser`, fails closed without calling `next` on a bad record, then calls `guardOutranked` inside `CRUDService.RecordAction`'s transaction. `lifecycleFailure` / `actionFailure` pass `*ForbiddenError` through as 403. Other controller IDs are stored unchanged. `Applies` is not wrapped, as the plan required. + +The boot smoke now supplies a superuser principal so the wrap's `actsAsSuperuser` short-circuit avoids needing `TxFromContext` in a unit test. The Postgres tests cover the HTTP paths: create/rename onto a unique code-declared code with no row (editor and superuser), a unique non-declared create, a same-code system-role save, an outranking contributed `acme_ping` (403, spy empty), built-in deactivate still 403, and a covered target still 200 inside the transaction. Both names are in `DOCKER_TESTS` and not in `SHORT_TESTS`. The `AdminActionExtension` comment states that cabana, not the contributor, applies `user.outranked`. No consuming-application names, no new dependency, YAML parser unchanged. + +Prior findings WR-01, WR-02, and IN-01..IN-06 were out of this pass. WR-03's D-09 clause (contributed Run still does not call `guardLastSuperuser`) remains as originally written; a non-superuser still cannot load a superuser row (`scopeAdministrators` → 404), so that leftover is the superuser-on-superuser path the plan did not take. + +### Gap-closure Critical Issues + +None. + +### Gap-closure Warnings + +None. + +### Gap-closure Info + +None. + +### Gap-closure Nice-to-have + +None. + +--- + +_Reviewed: 2026-10-22T10:34:44Z (original); 2026-10-22T11:50:00Z (gap-closure plan 04)_ _Reviewer: Claude (msd-code-reviewer)_ _Depth: standard_