golem15/summercms
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
1f228f85bdaa0aa301b288afab540311feab3ace
summercms/.planning/phases/12.1-user-plugin-admin-screens/12.1-SECURITY-REVIEW.md
Jakub Zych 93f0171e9c docs(12.1-05): security review and validation sign-off for Phase 12.1 
- 12.1-SECURITY-REVIEW.md: every threat T-12.1-01 to T-12.1-40 and T-12.1-SC with its mitigation, test and observed result; T-12-18 revisited; the D-30 guard and its boundary; the eleven handed-over items; five findings that need a decision
- 12.1-VALIDATION.md: per-task map with real task ids and measured run times, signed off
- deferred-items.md: older framework files that name an application
2026-10-05 16:13:50 +02:00

198 lines
40 KiB
Markdown
Raw Blame History

---
phase: "12.1"
reviewed: "2026-10-05"
reviewer: "gsd-executor, plan 12.1-05 (self-performed code-and-test review of plans 12.1-01 to 12.1-05; no reviewer agent could be spawned from the executor's runtime, per the 08-10 and Phase 12 precedent). The orchestrator may additionally run /gsd-secure-phase 12.1."
threats_open: 0
findings_for_decision: 5
gate: "scripts/check-phase12.1.sh --all"
removal_harness: "scripts/check-phase12.1.sh --removal"
---
# Phase 12.1 Security Review
This is a code-and-test review of every threat in the registers of plans 12.1-01 to 12.1-05: T-12.1-01 to T-12.1-40 and T-12.1-SC. Severity and disposition are copied from the originating plan. T-12.1-SC is declared by every plan and is listed once. The Phase 12 threat T-12-18 (the user groups table), which this phase revisits, is cited by its original id and has its own section.
`threats_open: 0` counts the registered threats: each is mitigated with a passing test or accepted with its rationale. It does not mean nothing is left to decide. `findings_for_decision: 5` counts review findings that are not vulnerabilities at the severity of this register but need the owner's decision; they are listed under "Findings that need a decision" and are not silently closed.
A high or critical threat counts as mitigated only when its named test fails with the protection removed. `scripts/check-phase12.1.sh --removal` does this: it refuses a file with uncommitted changes, applies an anchor-exact mutation, runs the named test, requires it to fail on an assertion (a build failure does not count), and restores the file byte for byte, checked with `cmp`. Results are under "Removal checks".
Repositories: `summercms.go` (framework: `modules/cabana`, `modules/pact`, `admin/`), sm-user-plugin (the user plugin, mounted in the application workspace at `plugins/golem15/user`). Framework tests are in `modules/cabana`; plugin tests run inside the application workspace. Gate stages are modes of `scripts/check-phase12.1.sh`.
## Threat register
| Threat | Category | Component | Severity | Disposition | Production mitigation | Test or gate stage | Observed result | Residual risk |
|--------|----------|-----------|----------|-------------|-----------------------|--------------------|-----------------|---------------|
| T-12.1-01 | Elevation of Privilege | bulk action ids (IDOR) | high | mitigate | `modules/cabana/crud.go` `CRUDService.BulkAction` resolves ids with `lockScoped` (the controller's list scope, `FOR UPDATE`) inside the action's transaction and answers `partialSelection` (409) unless every id resolved; plugin code receives records, never ids | `TestPhase121Threats/T-12.1-01`, `TestBulkActionScope`, `TestBulkActionPartial`, `TestBulkActionAbsent`; stage `--security` | pass; removal checks RC-01 (scope) and RC-02 (partial selection) fail the subtest | None known |
| T-12.1-02 | Elevation of Privilege | running an action without its permission, or an undeclared action | high | mitigate | `modules/cabana/actions.go` `bulkActionOf` and `recordActionOf` answer 404 unless the action is declared in YAML and registered; `allowAction` checks the action's own permissions (403); the list schema and `meta.actions` are filtered per principal | `TestPhase121Threats/T-12.1-02`, `TestBulkActionPermissions`, `TestBulkActionUndeclared`, `TestRecordActionPermissions`, `TestListSchemaBulkActionsFiltered` | pass; removal check RC-03 fails the subtest | None known |
| T-12.1-03 | Tampering | CSRF on the two new POST routes | high | mitigate | `modules/cabana/http.go` mounts both routes behind `requireAjax`; a cookie request without `X-Requested-With` is refused | `TestPhase121Threats/T-12.1-03`, `TestBulkActionCSRF`, `TestPhase10CSRF` (route count) | pass; removal checks RC-04 and RC-05 fail the subtest | None known |
| T-12.1-04 | Elevation of Privilege | record action on a record outside the form scope or in the wrong state | high | mitigate | `CRUDService.RecordAction` loads the record with `loadRecord` (form scope, row lock) and answers 404 outside it; `Applies` is asked again inside the transaction and a false answer is 409 | `TestPhase121Threats/T-12.1-04`, `TestRecordActionScope`, `TestRecordActionApplies` | pass; removal checks RC-06 (scope) and RC-07 (Applies) fail the subtest | None known |
| T-12.1-05 | Tampering | half-applied bulk mutation | medium | mitigate | One transaction per bulk action (`CRUDService.transaction`); any error from `Run` rolls every row back | `TestPhase121Threats/T-12.1-05`, `TestBulkActionRollback`, `TestBulkActionConcurrent` | pass | None known |
| T-12.1-06 | Information Disclosure | internal error text in 403 or 500 bodies | medium | mitigate | `actionFailure` and `lifecycleFailure` log the cause and answer the fixed 500 body; only `*cabana.ForbiddenError` and `*cabana.ValidationError` carry text, localized | `TestPhase121Threats/T-12.1-06`, `TestForbiddenFromEveryHook`, `TestRecordActionAppliesError`, `TestRowStateHookError` | pass | A plugin that puts internal detail into a ForbiddenError message shows it |
| T-12.1-07 | Tampering | plugin-supplied row state or label used as markup or CSS class | low | mitigate | The server sends only the three known states (`ListMeta.RowStates`); the SPA maps a state to a fixed class table and renders labels as text (`RowStateBadges.vue`, `DataTable.vue` `statesOf`) | `TestPhase121Threats/T-12.1-07`, `TestRowStateUnknownDropped`; `RowStateBadges.test.ts` and `DataTable.test.ts` (the backstop case "a row state outside the fixed set renders no badge and no class") | pass | None known |
| T-12.1-08 | Repudiation | bulk and record actions leave no trace | low | mitigate | One `slog.Info` line per run with controller, action, admin id and affected count; no record contents | `TestPhase121Threats/T-12.1-08` | pass | The line is an application log, not a tamper-proof audit trail |
| T-12.1-09 | Tampering | mass assignment through virtual or password fields | high | mitigate | `BindWritableFields` (`crud.go`) never binds a field listed in `FormVirtualFields`; `liftVirtualValues` takes virtual values out of the body, honours the field's context and hands them to hooks on the transaction context only | `TestPhase121Threats/T-12.1-09`, `TestVirtualFieldsContext`, `TestVirtualFieldsNested`, `TestPasswordFieldNeverProjected` | pass; removal checks RC-08 (context) and RC-09 (binding) fail | None known |
| T-12.1-10 | Information Disclosure | password value in a response, log or URL | high | mitigate | A `type: password` field must be virtual (boot error otherwise, `extension.go` `checkVirtualFields`), so it is never bound or projected; `projectRecord` also skips protected keys; the SPA sends it only in the save body | `TestPhase121Threats/T-12.1-10`, `TestPasswordFieldNeverProjected`, `TestPhase121BootErrors`; `PasswordField.test.ts`, `FormView.test.ts` | pass; removal check RC-10 fails the subtest | The SPA shows whatever a record response carries for the field: the guarantee is the server's (observation OB-1) |
| T-12.1-11 | Elevation of Privilege | protected foreign key written through a relation field | high | mitigate | `relation_field.go`: a belongsTo field on a protected key is read-only unless its contract sets `WritableForeignKey`; the submitted id still passes the scoped options query | `TestPhase121Threats/T-12.1-11`, `TestWritableForeignKeyOptIn`, `TestWritableForeignKeyScope` | pass; removal check RC-11 fails the subtest | None known |
| T-12.1-12 | Elevation of Privilege | locked relation ids changed by a crafted request (mechanism behind T-12-18) | high | mitigate | `crud.go` calls `checkRelationLocks` inside the save transaction, before any row write, on create and update; the locked subset before and after must be equal, else 403 and rollback | `TestPhase121Threats/T-12.1-12`, `TestRelationLockCreate`, `TestRelationLockUpdate`, `TestRelationLockBelongsTo`, `TestRelationLockAbsentField`; `RelationField.test.ts` (backstop) | pass; removal check RC-12 fails the subtest | The lock covers form saves only; a relation manager on the same relation is not covered (H-01, now documented) |
| T-12.1-13 | Tampering | permission code injection or out-of-range value | high | mitigate | `field_permission.go`: a submitted code must be offered by the controller for this request (422), a value must be in the mode's set (422), a locked code must keep its stored value (403); stored codes that are not offered are kept untouched | `TestPhase121Threats/T-12.1-13`, `TestPermissionEditorModes`, `TestPermissionEditorUnknownCode`, `TestPermissionEditorLocked`, `TestPermissionEditorKeepsUnoffered`; `PermissionEditorField.test.ts` (backstop) | pass; removal checks RC-13 (code) and RC-14 (lock) fail the subtest | A locked code stored with a value outside the mode's set blocks every save of that record (H-02, now documented) |
| T-12.1-14 | Tampering | a preview-only field written by a crafted body | medium | mitigate | A field whose context is `preview` only is never writable: `contextAllows` leaves it out of the binding for create and update | `TestPhase121Threats/T-12.1-14`, `TestPreviewFieldNeverWritten` | pass | None known |
| T-12.1-15 | Elevation of Privilege | script or markup through the status hint or callout classes | medium | mitigate | Partials are served as a node tree with an attribute allowlist (`partial_render.go`); the SPA builds elements from nodes and has no raw-HTML sink | `TestPhase121Threats/T-12.1-15`, `TestPreviewHeaderPartialScoped`; `check-phase12.1.sh --hygiene` (no `v-html` or `innerHTML` in `admin/src`) | pass | None known |
| T-12.1-16 | Tampering | open redirect or foreign route through `preview/:id` in plugin YAML | medium | mitigate | `admin/src/app/winterUrl.ts` `mapWinterUrl` emits only the current controller's list, create, record and preview routes, digits only, else the list | `winterUrl.test.ts` (the backstop case and the foreign-controller and non-numeric-id cases), `router.test.ts` ("preview route"); stage `--spa` | pass | None known |
| T-12.1-17 | Tampering | an unreviewed contract published by the tag | high | mitigate | Plan 02 Task 5 was a blocking decision checkpoint listing the published names and six contract properties; the owner chose `tag-local`. The tag `v0.1.3` is annotated, local, on `df5cace`, where every plan gate passed | `check-phase12.1.sh --openapi` (`TestPhase09ContractInventory`, `TestPhase10OpenAPIConformance`), `--hygiene` (the tag exists); the tag is not on origin (`git ls-remote --tags origin v0.1.3` is empty) | pass; no removal row (NR-01) | The tag is still local and can be re-cut; nothing depends on it publicly |
| T-12.1-18 | Elevation of Privilege | Users screens and actions without the users permission | high | mitigate | `usersAdminController.RequiredPermissions` (`golem15.users.access_users`), `usergroupsAdminController` (`access_groups`), `organisationsAdminController` (`access_users`); every bulk and record action names its permission as well | plugin `TestPhase121Threats/T-12.1-18` (every route of the three screens 403 without the permission), `TestAdminUsersTracer`, `TestAdminGroups`, `TestAdminOrganisations`, `TestAdminRegistration` | pass; removal check RC-15 fails the subtest | None known |
| T-12.1-19 | Information Disclosure | password hash, plain password or reset codes in admin responses | high | mitigate | The user form's password fields are virtual; no column of `models/user/columns.yaml` or `fields.yaml` is a secret; relation contracts list `name` and `email` only | plugin `TestPhase121Threats/T-12.1-19` (list, show, partial, update, 422, create, bulk and record action bodies scanned for the plain text, the hash and both codes) | pass; removal check RC-16 (a `password` column added to the list) fails the subtest | None known |
| T-12.1-20 | Tampering | mass assignment of is_activated, permissions or organisation_id through the user form | high | mitigate | Only declared form fields are bound; `is_activated`, `organisation_id` and `permissions` are protected fill keys; the organisation is written through its relation field, permissions through the validated editor; `FormBeforeCreate` forces `is_activated` false | plugin `TestPhase121Threats/T-12.1-20`, `TestAdminUserFormFields` | pass; removal check RC-17 (a protected column offered as a form field) fails the subtest | None known |
| T-12.1-21 | Spoofing | tokens issued before an admin password reset stay valid | medium | mitigate | `FormBeforeUpdate` stamps `tokens_valid_after` when a password was submitted | plugin `TestPhase121Threats/T-12.1-21`, `TestAdminUserPassword` | pass | None known |
| T-12.1-22 | Denial of Service | invitation mail abuse | low | mitigate | One invitation per create, only with `send_invite` true in the body, sent after the commit (`lagoon.AfterCommit`); none on update | plugin `TestPhase121Threats/T-12.1-22`, `TestAdminUserInvite` | pass | An admin with the users permission can create many users; there is no per-admin rate limit |
| T-12.1-23 | Tampering | permanent delete leaves orphans or half-deleted users | medium | mitigate | `FormAfterDelete` runs `classes.ForceDeleteCleanup` (throttle rows, memberships, attachments) and the row delete in the delete's transaction; blobs go after the commit | plugin `TestPhase121Threats/T-12.1-23`, `TestAdminUserForceDelete`, `TestAdminActions` | pass | A blob removal that fails after the commit is logged and leaves the blob |
| T-12.1-24 | Information Disclosure | last_seen, permissions, timestamps or groups leak into user API payloads | high | mitigate | `models.User`: `LastSeen`, `Permissions`, `CreatedAt`, `UpdatedAt` and `Groups` are `json:"-"`; the user API builds its payload field by field (`groups` an empty list, `permissions` null) | plugin `TestPhase121Threats/T-12.1-24` (every user API body, and the marshalled model), `TestLastSeen`, `TestAdminFieldsNeverMarshal`; application `TestUserAPINuxtFlows`, `TestParityCorpus`; stage `--app` | pass; removal checks RC-18 and RC-27 fail the subtest (after the subtest was extended, see FX-3) | None known |
| T-12.1-25 | Denial of Service | last_seen write fails or amplifies writes on the auth path | low | mitigate | `classes.TouchLastSeen` writes at most once per five minutes; login and refresh log and ignore its error | plugin `TestPhase121Threats/T-12.1-25`, `TestLastSeen`, `TestTouchLastSeen` | pass | None known |
| T-12.1-26 | Elevation of Privilege | a deactivated site admin is re-enabled by restore or activate | medium | accept | PHP behaves the same (the membership survives a soft delete) and the action needs golem15.users.access_users (RESEARCH T-12-18 path 8). | none (accepted) | accepted | An administrator with the users permission can re-enable a deactivated member of a privileged group, and can deactivate or ban one (see "D-30: boundary") |
| T-12.1-27 | Elevation of Privilege | a wrong port of the permission merge grants a frontend permission | medium | mitigate | `classes/permissions.go` `MergedPermissions` (highest group value, user value overrides) and `HasPermission` (exactly 1; wildcards as Winter) | plugin `TestPhase121Threats/T-12.1-27`, `TestMergedPermissions` (a table of stored shapes, group sets and wildcards), `TestPermissionSetScan` | pass | Boolean values in stored JSON are not grants here, unlike the API-token reader (H-05) |
| T-12.1-28 | Elevation of Privilege | T-12-18 revisited: adding or removing a privileged group through the user form's groups field | critical | mitigate | `usersAdminController.AdminRelationLocks` locks the ids of the groups whose code is in `golem15.user.privileged_groups` for an administrator without `golem15.users.manage_privileged_groups`; the framework refuses a create or update that changes the locked subset with 403 before any row is written | plugin `TestPhase121Threats/T-12.1-28` (add, remove and create matrix with and without the permission, a crafted body that also changes other fields, `classes.HasGroupCode` before and after), `TestAdminPrivilegedGroups`, `TestAdminUserGroupsField` | pass; removal check RC-19 fails the subtest | None known |
| T-12.1-29 | Elevation of Privilege | creating, renaming to or from, or deleting a privileged group code | high | mitigate | `usergroupsAdminController` `FormBeforeCreate`, `FormBeforeUpdate` (stored code read through the write's transaction) and `FormBeforeDelete` call `requirePrivilegedPermission` | plugin `TestPhase121Threats/T-12.1-29`, `TestAdminPrivilegedGroups`, `TestAdminGroupsEdge`, `TestAdminGroupsControllerGuards` | pass; removal check RC-20 fails the subtest | Validation answers before the guard (H-09) |
| T-12.1-30 | Elevation of Privilege | a second, unguarded writer of users_groups | high | mitigate | The groups field is the only writer besides the two deletes; no relation manager for groups exists on any controller; bulk and record actions and the organisation members manager do not touch the table | plugin `TestPhase121Threats/T-12.1-30` (the table byte-identical before and after every bulk action, record action, members link and unlink; no groups relation route answers), `TestAdminOrganisationMembers` | pass; removal check RC-21 (a membership write added to an action) fails the subtest | None known |
| T-12.1-31 | Tampering | privileged list cached, mis-compared or bypassed through a NULL code | medium | mitigate | `classes/privileged.go`: the list is read from config on every call, compared case-sensitively, blank entries dropped; a group without a code is never privileged | plugin `TestPhase121Threats/T-12.1-31`, `TestPrivilegedGroupCodes`, `TestIsPrivilegedCode`, `TestIsPrivilegedMember` | pass | An application that sets the list to empty has no privileged group |
| T-12.1-32 | Tampering | organisation members manager moves users between organisations | low | accept | PHP's add and remove behave the same; the routes need `golem15.users.access_users`, write only organisation_id and are scoped by the relation contract (RESEARCH T-12-18 path 6). | none (accepted) | accepted | The role column outlives the membership (H-06); a member of another organisation cannot be linked until released |
| T-12.1-33 | Tampering | a published application pointer references an unpublished plugin commit, the plugin is published while the framework contract it needs is not, or the app is built against a framework without v0.1.3 | low | mitigate | Nothing was pushed by plans 03 to 05; the pointer bumps are local commits; the publication step is T-12.1-40 | `check-phase12.1.sh --app` (the recorded pointer equals the plugin head; both trees clean), the plans' verify commands | pass; the plugin's remote head is unchanged (`0fe5b91`) | The application repository must not be pushed before the plugin |
| T-12.1-34 | Information Disclosure | group membership reaches a user API payload | high | mitigate | `User.Groups` is `json:"-"`; the user API's payload builder writes `groups` as an empty list | plugin `TestPhase121Threats/T-12.1-34`, application `TestPhase12Threats` (subtest T-12-18), `TestHiddenNeverMarshals` | pass; removal check RC-22 fails the subtest | None known |
| T-12.1-35 | Repudiation | a gate that passes without measuring (zero tests, skips, a filter that matches nothing) | medium | mitigate | `detect` in the gate refuses a failure, a build failure, a skip, zero tests, "no tests to run", non-JSON output and a required name without a passing top-level test | `check-phase12.1.sh --self-test` (planted inputs for each) | pass; a plugin test renamed by hand made `--security` refuse with "missing named test" | The whole-module runs of `--go` and `--app` accept packages without tests |
| T-12.1-36 | Information Disclosure | a consuming-application name in framework fixtures, tests, docs or gate output | low | mitigate | `check-phase12.1.sh --hygiene` scans the phase's framework files, every module README, the root README, `docs` and `admin/src`; gate output about the application workspace is masked | `check-phase12.1.sh --hygiene`, `--self-test` (four planted spellings found, the mask checked) | pass | Thirteen older Go files in other modules name the application; they predate this phase (OB-3) |
| T-12.1-37 | Tampering | a mitigation silently removed by a later change | medium | mitigate | `TestPhase121Threats` in both repositories, the required name prefixes of `--security`, and the removal harness | `check-phase12.1.sh --security`, `--removal`, `--evidence` | pass; 27 removal rows fail as required | The removal stage is not part of `--all`; it must be run on purpose |
| T-12.1-38 | Elevation of Privilege | takeover of a privileged-group member's account: an admin holding only golem15.users.access_users changes the member's email or sets a password | critical | mitigate | `usersAdminController.FormBeforeUpdate` starts with `guardCredentials`: a changed email or a submitted password on a member of a privileged group needs `golem15.users.manage_privileged_groups` (`guardPrivilegedMember`, `classes.IsPrivilegedMember` through the write's transaction); refusal is a 403 with details and nothing is written | plugin `TestPhase121Threats/T-12.1-38`, `TestAdminPrivilegedMember`, `TestAdminUsersEdge` (superuser; unreadable membership is the opaque 500), `TestAdminUsersControllerGuards`, `TestIsPrivilegedMember` | pass; removal checks RC-23 (the guard) and RC-25 (the clean statement) fail | See "D-30: boundary" |
| T-12.1-39 | Tampering | permanent delete of a privileged-group member, by the form button or inside a bulk delete | high | mitigate | `usersAdminController.FormBeforeDelete` calls `guardPrivilegedMember`; the framework calls it per record inside the bulk delete's transaction, so one refused record rolls the selection back | plugin `TestPhase121Threats/T-12.1-39`, `TestAdminPrivilegedMember`, `TestAdminUsersEdge` | pass; removal check RC-24 fails the subtest | See "D-30: boundary" |
| T-12.1-40 | Tampering | the shared plugin is published before the security review, or while the framework contract it builds on is not published | medium | mitigate | One push point in the phase, after the gate and this review, and only when the framework tag is on origin and no framework production code changed after it | `check-phase12.1.sh --app` (pointer and clean trees); the four conditions are recorded in 12.1-05-SUMMARY.md | pass; condition (c) does not hold, so nothing was pushed and the push is recorded as pending | The push is a manual step the owner takes after pushing the framework tag |
| T-12.1-SC | Tampering | npm/pip/cargo installs | high | mitigate | No package was installed and no manifest changed in the phase | `check-phase12.1.sh --hygiene` (`go.mod`, `go.sum`, `admin/package.json` and `admin/package-lock.json` are unchanged since `v0.1.3` and have no local change) | pass; no removal row (NR-02) | None known |
## Removal checks
Each row is one anchor-exact mutation from `scripts/check-phase12.1.sh --removal`. The anchor occurs exactly once in its file; the test must fail on an assertion; the file is restored and compared with `cmp`. All 27 rows were run on 2026-10-05 and all failed as required; both working trees were clean afterwards. RC-18 first survived: see FX-3.
| Check | Threat | File | What the mutation removes | Test that must fail | Result |
|-------|--------|------|---------------------------|---------------------|--------|
| RC-01 | T-12.1-01 | `modules/cabana/crud.go` | the list scope inside `lockScoped` | `TestPhase121Threats/T-12.1-01` | fails as required |
| RC-02 | T-12.1-01 | `modules/cabana/crud.go` | the partial-selection refusal in `BulkAction` | `TestPhase121Threats/T-12.1-01` | fails as required |
| RC-03 | T-12.1-02 | `modules/cabana/actions.go` | the action's own permission check (`allowAction`) | `TestPhase121Threats/T-12.1-02` | fails as required |
| RC-04 | T-12.1-03 | `modules/cabana/http.go` | `requireAjax` on the bulk action route | `TestPhase121Threats/T-12.1-03` | fails as required |
| RC-05 | T-12.1-03 | `modules/cabana/http.go` | `requireAjax` on the record action route | `TestPhase121Threats/T-12.1-03` | fails as required |
| RC-06 | T-12.1-04 | `modules/cabana/crud.go` | the form scope of `loadRecord` in `RecordAction` | `TestPhase121Threats/T-12.1-04` | fails as required |
| RC-07 | T-12.1-04 | `modules/cabana/crud.go` | the `Applies` refusal in `RecordAction` | `TestPhase121Threats/T-12.1-04` | fails as required |
| RC-08 | T-12.1-09 | `modules/cabana/crud.go` | the context check when virtual values are lifted | `TestVirtualFieldsContext`, `TestVirtualFieldsSmoke` | fails as required |
| RC-09 | T-12.1-09 | `modules/cabana/crud.go` | the virtual-field skip in `BindWritableFields` | `TestPhase121Threats/T-12.1-09` | fails as required (the controller no longer boots) |
| RC-10 | T-12.1-10 | `modules/cabana/crud.go` | the same skip, seen from the password field | `TestPhase121Threats/T-12.1-10` | fails as required (the controller no longer boots) |
| RC-11 | T-12.1-11 | `modules/cabana/relation_field.go` | the read-only rule of a protected foreign key without the opt-in | `TestPhase121Threats/T-12.1-11` | fails as required |
| RC-12 | T-12.1-12 | `modules/cabana/crud.go` | the relations handed to `checkRelationLocks` | `TestPhase121Threats/T-12.1-12` | fails as required |
| RC-13 | T-12.1-13 | `modules/cabana/field_permission.go` | the offered-code check | `TestPhase121Threats/T-12.1-13` | fails as required |
| RC-14 | T-12.1-13 | `modules/cabana/field_permission.go` | the locked-code check | `TestPhase121Threats/T-12.1-13` | fails as required |
| RC-15 | T-12.1-18 | `controllers/users_admin_controller.go` | the Users controller's required permission | plugin `TestPhase121Threats/T-12.1-18` | fails as required |
| RC-16 | T-12.1-19 | `models/user/columns.yaml` | the absence of a `password` list column | plugin `TestPhase121Threats/T-12.1-19` | fails as required |
| RC-17 | T-12.1-20 | `models/user/fields.yaml` | the absence of `organisation_role` from the form | plugin `TestPhase121Threats/T-12.1-20` | fails as required |
| RC-18 | T-12.1-24 | `models/user.go` | `json:"-"` on `LastSeen` | plugin `TestPhase121Threats/T-12.1-24` | fails as required (survived before FX-3) |
| RC-19 | T-12.1-28 | `controllers/users_admin_controller.go` | the locked ids returned by `AdminRelationLocks` | plugin `TestPhase121Threats/T-12.1-28` | fails as required |
| RC-20 | T-12.1-29 | `controllers/usergroups_admin_controller.go` | the permission check of `requirePrivilegedPermission` | plugin `TestPhase121Threats/T-12.1-29` | fails as required |
| RC-21 | T-12.1-30 | `classes/admin_actions.go` | the rule that no action writes `users_groups` (a delete is added to `ActivateUsers`) | plugin `TestPhase121Threats/T-12.1-30` | fails as required |
| RC-22 | T-12.1-34 | `models/user.go` | `json:"-"` on `Groups` | plugin `TestPhase121Threats/T-12.1-34` | fails as required |
| RC-23 | T-12.1-38 | `controllers/users_admin_controller.go` | the `guardCredentials` call at the top of `FormBeforeUpdate` (D-30) | plugin `TestPhase121Threats/T-12.1-38` | fails as required |
| RC-24 | T-12.1-39 | `controllers/users_admin_controller.go` | the `guardPrivilegedMember` call in `FormBeforeDelete` (D-30) | plugin `TestPhase121Threats/T-12.1-39` | fails as required |
| RC-25 | T-12.1-38 | `classes/admin_actions.go` | the clean statement of `fresh` (fix `f496959`) | plugin `TestIsPrivilegedMember` | fails as required |
| RC-26 | D-23 | `models/user.go` | the group filter value check (fix `43fabfa`) | plugin `TestAdminUsersGroupFilterValue` | fails as required |
| RC-27 | T-12.1-24 | `models/user.go` | `json:"-"` on `Permissions` | plugin `TestPhase121Threats/T-12.1-24` | fails as required |
High threats without a removal row:
| Waiver | Threat | Reason |
|--------|--------|--------|
| NR-01 | T-12.1-17 | A process control (the owner's decision checkpoint before the tag); there is no line of code to remove. The contract inventory and conformance tests run in `--openapi`. |
| NR-02 | T-12.1-SC | A process control (no install); `--hygiene` compares the four dependency manifests with the tag. |
Where a protection has two layers, one mutation removes one of them. RC-09 and RC-10 remove the binding skip, after which the controller refuses to boot: the second layer (a password field must be virtual; `projectRecord` skips protected keys) fails closed. The review did not build a mutation that removes both layers at once.
## T-12-18 revisited
Phase 12 recorded T-12-18 as "no route writes users_groups; Groups is never serialized; the site-admin predicate reads group codes server-side only". This phase adds writers. The eight paths of 12.1-RESEARCH.md ("T-12-18: Every users_groups Write Path"):
| Path | What it writes | Guard or disposition | Evidence |
|------|----------------|----------------------|----------|
| 1. User form `groups` field (create and update) | replaces the user's pivot rows | `AdminRelationLocks` plus the framework's `checkRelationLocks`: the privileged subset must not change without `golem15.users.manage_privileged_groups`; 403 and rollback | `TestPhase121Threats/T-12.1-28`, `TestAdminPrivilegedGroups`; RC-12, RC-19 |
| 2. User permanent delete (form and bulk) | removes the user's pivot rows | `access_users`; for a privileged member also the extra permission (D-30) | `TestPhase121Threats/T-12.1-23` and `/T-12.1-39`; RC-24 |
| 3. Group create or re-code with a privileged code | no pivot write; changes who is privileged | extra permission in `FormBeforeCreate` and `FormBeforeUpdate` | `TestPhase121Threats/T-12.1-29`; RC-20 |
| 4. Delete of a privileged group | removes that group's pivot rows | extra permission in `FormBeforeDelete`; `FormAfterDelete` removes the rows | `TestPhase121Threats/T-12.1-29`, `TestAdminGroups` |
| 5. Bulk and record actions | nothing in `users_groups` | none needed; pinned | `TestPhase121Threats/T-12.1-30`; RC-21 |
| 6. Organisation members manager | `users.organisation_id` only | accepted (T-12.1-32) | `TestAdminOrganisationMembers`, `TestPhase121Threats/T-12.1-30` |
| 7. Config change of the privileged list | which codes are privileged | operational; read per request | `TestPhase121Threats/T-12.1-31`, `TestPrivilegedGroupCodes` |
| 8. Restore or activate of a deactivated site admin | no pivot write; re-enables the account | accepted (T-12.1-26) | none (accepted) |
`Groups` still never reaches a user API payload (T-12.1-24, T-12.1-34; the application's `TestPhase12Threats` subtest T-12-18 still passes). The consumer of the membership is unchanged: `classes.HasGroupCode`.
## D-30: takeover guard for privileged-group members
D-30 added T-12.1-38 (critical) and T-12.1-39 (high) at the plan check. For an administrator who holds `golem15.users.access_users` and not `golem15.users.manage_privileged_groups`, three operations on a member of a privileged group are refused with 403 and change nothing:
1. changing the member's email (`FormBeforeUpdate`, `guardCredentials`);
2. submitting a password for the member (the same check; the two together in one body, with other fields, are refused as a whole);
3. permanently deleting the member, by the form's delete and inside a bulk delete, which is then refused as a whole (`FormBeforeDelete`).
The guard landed in sm-user-plugin commit `b410a7f` (plan 12.1-03, with `classes/privileged.go`) and was corrected in `f496959` during this plan (FX-1). Both checks have a removal row: with `guardCredentials` removed, `TestPhase121Threats/T-12.1-38` fails; with the delete guard removed, `TestPhase121Threats/T-12.1-39` fails (RC-23, RC-24). A superuser passes; a membership that cannot be read is an opaque 500 and nothing changes (`TestAdminUsersEdge`): the guard never answers "not privileged" on an error.
### D-30: boundary
What an administrator with only `golem15.users.access_users` may still do to a member of a privileged group, and why:
| Operation | Allowed | Why |
|-----------|---------|-----|
| Change name, surname and other profile fields | yes | not a credential; D-30 names email and password |
| Change the organisation, the avatar, the frontend permissions | yes | not a credential; D-30 does not cover them. Frontend permissions are what the application's own features check, so this can change what the member may do in the app |
| Add or remove an ordinary group | yes | D-04: ordinary groups need only the users permission; the privileged membership itself stays locked |
| Deactivate (soft delete), restore, activate | yes | D-13 and T-12.1-26 (accepted): reversible, as in PHP |
| Ban, unban, unsuspend | yes | D-14; reversible |
| Change email, set a password, permanently delete | no | D-30 |
The guard protects against takeover and permanent removal. It does not protect availability: deactivating or banning a site administrator locks that person out until someone with the users permission reverses it. This is the accepted behaviour of D-30 and T-12.1-26; it is listed as FD-5 because the owner may want the two actions guarded too.
## Items handed over by plans 02 to 04
Each item was checked against code and, where a test could show it, pinned.
| Item | Statement | Disposition | Evidence |
|------|-----------|-------------|----------|
| H-01 | Relation locks are enforced on form create and update only; relation-manager link and unlink routes do not ask `RelationLockProvider` | Contract property, accepted by the owner at the tag decision. Not reachable in the plugin: no relation manager exists for groups. The framework page did not say so; it does now | `TestPhase121Threats/T-12.1-30` (no groups relation route); `docs/backend/relation-manager.md` ("The lock covers the form field only"); decision item FD-2 |
| H-02 | A locked permission code whose stored value is outside the mode's set makes every save by that administrator a 403 | Contract property, accepted at the tag decision. Not reachable in the plugin: neither controller locks a code. Now documented | `TestPermissionEditorLocked`; `docs/backend/forms.md`; `usersAdminController.AdminPermissionOptions` ("None is locked") |
| H-03 | A relation hook returning `cabana.ForbiddenError` is untested | Closed | `TestForbiddenFromEveryHook` (before and after create, update and delete, bulk delete, relation link and the child hooks), `TestForbiddenRollsBack` |
| H-04 | A `users.permissions` value that is not a JSON object fails login, fetch and the admin form for that row | By decision (plan 03): a damaged value must not silently lose its denials. Only the validated editor writes the column, so a damaged value needs direct SQL or a faulty import | `TestPermissionSetScan`, `TestMergedPermissions` ("a damaged row is an error"); cutover note in the summary |
| H-05 | `UserPermissionGrants` accepts booleans in `user_groups.permissions`; `PermissionSet` skips them and an edit drops them | Known difference, towards fewer grants. Pinned so a change is deliberate; decision item FD-3 | `TestAdminGroupsLegacyBooleanPermissions`, `TestMergedPermissions` ("a boolean value is not a grant") |
| H-06 | Unlinking an organisation member clears `organisation_id` and keeps `organisation_role` | Real gap, low: the actor already holds the users permission, which can do more than this. The fix needs a framework unlink hook or a plugin route, so it was not improvised. Pinned; decision item FD-1 | `TestAdminOrganisationRoleOutlivesMembership` |
| H-07 | Deleting an organisation cascades into the application's credential tables, reachable with the users permission | As in PHP; the screen is the place organisations are administered. No confirm text says so. Accepted; mentioned under FD-1 | `TestAdminOrganisations` (delete releases the members), `TestAdminOrganisationsEdge` |
| H-08 | The seeded `guest` and `registered` groups can be deleted or re-coded with the groups permission | Accepted: no Go code reads the two codes. An application protects them by naming their codes in `golem15.user.privileged_groups` | `TestAdminGroupsEdge` (with `guest` in the list, delete and re-code are 403 and the row stays) |
| H-09 | A request that would be refused with 403 answers 422 when it also fails validation | Accepted: nothing is saved either way, and the 422 reveals only that a code exists, which the list shows | `TestAdminGroupsEdge` ("validation answers before the guard") |
| H-10 | A host application loading the plugin needs `admin.jwt.secret` or fails to start | Compatibility note for a shared plugin, not a vulnerability: failing closed is the right behaviour. In the plugin README | `TestAdminNeedsSecret`; decision item FD-4 |
| H-11 | `"password": ""` without confirmation answers 422 | Framework rule order (`confirmed` before `nullable`). The SPA leaves an empty password out, so the admin never sends it. Accepted | `TestPhase121Threats/T-12.1-20`; `formState.test.ts` ("leaves an empty password out on update") |
## Findings that need a decision
These are not open threats of the register. They are recorded here so they are decided, not forgotten.
| Finding | What | Suggested step |
|---------|------|----------------|
| FD-1 | `organisation_role` outlives the membership after an unlink (H-06), and an organisation delete removes the application's organisation credentials without a warning (H-07) | Clear the role on unlink (needs a framework unlink hook or a plugin-side route) and add a confirm text to the organisation delete |
| FD-2 | Relation locks do not cover a relation manager on the same relation (H-01) | Either enforce the lock on the manager's link and unlink routes or make "a locked field plus a manager on the same relation" a boot error; a framework change after v0.1.3 |
| FD-3 | Two readers of `user_groups.permissions` differ on boolean values (H-05) | Normalize booleans to 1 in the cutover import, or teach `ParsePermissionSet` to read `true` as 1 |
| FD-4 | Every host application of the shared plugin now needs `admin.jwt.secret` (H-10) | Tell the other projects before they update the plugin |
| FD-5 | Deactivating or banning a member of a privileged group needs only the users permission (D-30 boundary, T-12.1-26) | Decide whether `deactivate` and `ban` should refuse a privileged member as the permanent delete does |
Smaller observations, no decision needed:
- **OB-1 (T-12.1-10).** The SPA's password input shows whatever a record response carries for that field. The server never sends one (`TestPhase121Threats/T-12.1-10`, RC-10). A defensive `''` on load in `FormView.vue` would be a framework change after the tag; it was not made.
- **OB-2, sorting by an invisible column.** `sort=surname` on the Users list is accepted although the column is not shown. It reveals order, not values, and the column is searchable by design.
- **OB-3.** Thirteen Go files in other framework modules (tests of `modules/tide`, `modules/wristband` and others) name the application. They predate this phase and are outside its hygiene scope; recorded in `deferred-items.md`.
## Fixes made during the review
| Fix | Repository | Commit | What |
|-----|------------|--------|------|
| FX-1 (T-12.1-38, T-12.1-39) | sm-user-plugin | `f496959` | `classes.fresh` chained `WithContext` onto a lazy `NewDB` session, which clones the caller's statement with its conditions. On a handle that carried a condition `IsPrivilegedMember` answered false for a privileged member, so the takeover guard would have passed. No caller passes such a handle today (the hooks hand over the write's transaction). `fresh` now builds the empty statement first; `IsPrivilegedMember` and `PrivilegedGroupIDs` read through it. Tests: `TestIsPrivilegedMember`, `TestAdminActions` (both fail without the fix; RC-25) |
| FX-2 (D-23) | sm-user-plugin | `43fabfa` | `User.FilterScope` handed the group filter value to the database as it came; `filter[groups]=abc` made the Users list answer 500. A value that is not a positive whole number now lists nobody. Test: `TestAdminUsersGroupFilterValue` (fails with a 500 without the fix; RC-26) |
| FX-3 (T-12.1-24) | sm-user-plugin | `2b04cda` | Test only. The removal check for the `json:"-"` tag of `LastSeen` survived: the subtest read the user API payloads, which are built field by field. It now marshals the model too |
| Documentation (H-01, H-02) | summercms.go | see 12.1-05-SUMMARY.md | Two sentences in `docs/backend/relation-manager.md` and `docs/backend/forms.md` state the limits of relation locks and locked permission codes. No Go or SPA source changed |
No framework production code (`modules`, `cmd`, `admin/src`) changed in this plan: `git diff --name-only v0.1.3 HEAD -- modules cmd admin/src` lists only Go test files and files under `testdata`.
Reference in New Issue View Git Blame Copy Permalink