29 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 09-backend-admin-authentication-and-schema-pipeline | 2026-09-27T00:00:00Z | standard | 88 |
|
|
issues_found |
Phase 9: Code Review Report
Reviewed: 2026-09-27T00:00:00Z Depth: standard Files Reviewed: 88 Status: issues_found
Summary
I reviewed the Phase 9 backend-admin surface as it stands at HEAD. That covers the audience-separated JWTs in bouncer, the cabana login/refresh/logout/me flow, the permission registry, YAML compilation for forms, lists, filters and relations, generic CRUD, bulk delete, the relation manager, singleton settings, the backend migrations, the scaffolding stubs, the phase gate script, and the five Fonoteka controllers. Phase 10 additions (CSRF, cookie transport, the SPA) were considered only where they change Phase 9 behavior.
The guard, audience separation and schema allowlisting hold up. Identifiers reaching SQL come from compiled schemas and are quoted, and I found no SQL injection path. The main defect is in generic CRUD: any writable numeric field cannot be saved, because request numbers are decoded as json.Number and lagoon.Fill cannot convert that type to an integer or float column. The tests miss this because every numeric field in the Phase 9 fixtures is protected, so none is ever written.
The warnings fall into three groups:
- Authorization semantics that differ from Winter: wildcard required permissions, ALL instead of ANY matching, user-level permission overrides, and the navigation parent entry.
- Schema declarations the server does not enforce: toolbar and bulk actions, and relation link/unlink buttons.
- Fragile compile-time helpers: relation column ordering tied to a fixed indentation, and reflection helpers that skip embedded structs.
There are also auth-hygiene issues: a timing oracle on login, passwords passed as CLI flags, logout that cannot revoke an expired but still refreshable token, and ambiguous login identifiers.
Critical Issues
CR-01: Writable numeric fields cannot be saved; bad field values return 500 instead of 422
File: cabana/crud.go:623-634, cabana/crud.go:327-329, cabana/settings.go:143-145 (root cause in lagoon/fill.go:155-166)
Issue: decodeObject decodes the admin request body with dec.UseNumber(), so every JSON number becomes a json.Number, which is a string type. lagoon.Fill then calls convertValue, which uses only AssignableTo and ConvertibleTo. Go reflection allows a string-kinded value to convert to string, but not to int, uint or float64. I checked this directly: reflect.TypeOf(json.Number("1990")).ConvertibleTo(reflect.TypeOf(0)) is false. As a result, any create or update that sends a value for a writable type: number field bound to an integer or float column fails inside Fill. save wraps that failure as a CapabilityError, and writeCRUDError turns it into a 500 Server error. type: number is a supported, writable form type (scalarFormField), so the generic ADMIN-04 CRUD engine cannot write numbers at all.
Every other type mismatch, such as a string sent for a bool column, also becomes a 500 rather than a 422. The settings PUT path has the same root cause but reports it as a generic 422 "The settings payload is invalid.", so numeric settings are also impossible to save. The tests miss this because the only type: number fields in cabana/crud_test.go (id, scope_id) are protected and never reach Fill. The Fonoteka forms happen to contain no numeric fields today.
Fix: Before calling Fill, normalize json.Number against the target field kind. Also report conversion failures as per-field validation errors, not as capability errors:
// cabana: before lagoon.Fill
for key, val := range projected {
if n, ok := val.(json.Number); ok {
if i, err := n.Int64(); err == nil {
projected[key] = i
} else if f, err := n.Float64(); err == nil {
projected[key] = f
}
}
}
if err := lagoon.Fill(target, fillAllowed(cc, target, op), projected, false); err != nil {
return &ValidationError{Details: map[string]any{"body": []string{"The request body is invalid."}}}
}
Better still, teach lagoon.convertValue to handle json.Number for numeric kinds and return a typed field error that cabana maps to 422 for that field. Add a CRUD test that writes a non-protected type: number field bound to an int column.
Warnings
WR-01: Required-permission wildcards never match, and multiple required codes use ALL rather than Winter's ANY
File: cabana/contracts.go:108-137
Issue: validatePermissions accepts wildcard requirements (permissionCode(code, true)), and D-03 promises that golem15.fonoteka.* "works unchanged". However, granted() handles wildcards only on the grant side. Allows(p, []string{"golem15.fonoteka.*"}) is false for a principal granted golem15.fonoteka.access_genres, and is true only for superusers or principals holding a literal golem15.fonoteka.* grant. Any controller or setting that declares a wildcard requirement therefore denies every ordinary admin. The only reason the Fonoteka parent menu still appears is the child rule in navigation.go:50 (see WR-02).
Separately, Allows requires every listed code. Winter's Controller::$requiredPermissions and NavigationManager use hasAnyAccess, so porting a Winter controller that lists two permissions silently tightens access.
Fix: When a required code ends in .*, match it against any granted key with that prefix. Also switch to ANY semantics to match Winter, or document the difference and reject multi-code requirements at compile time:
func grantedRequirement(grants map[string]bool, code string) bool {
if strings.HasSuffix(code, ".*") {
prefix := strings.TrimSuffix(code, "*")
for key, on := range grants {
if on && strings.HasPrefix(key, prefix) { return true }
}
}
return granted(grants, code)
}
WR-02: Navigation shows a denied parent item, including its label and target controller
File: cabana/navigation.go:42-54
Issue: A parent item whose own permissions fail is still emitted, complete with Label and Controller, whenever any child passes. A user who holds only access_genres receives the parent entry pointing at golem15.fonoteka.albums, which returns 403 when opened. This contradicts the function's own comment ("Denied entries are removed before any response value is constructed") and differs from Winter, which drops main-menu items the user cannot access.
Fix: When the parent is denied but children are allowed, either blank the parent's Controller or repoint it at the first allowed child. Alternatively, fix WR-01 so the parent's golem15.fonoteka.* requirement passes for any Fonoteka grant, and drop the len(children) == 0 escape hatch.
WR-03: Create, update and delete are not gated by the compiled list and form declarations
File: cabana/http.go:217-226, cabana/registry.go:80-83, cabana/crud.go:290-385
Issue: When there is no form, compileRegistry removes create from ToolbarButtons, and showCheckboxes: false removes the bulk delete action. Both changes affect only the JSON schema. The server still accepts POST /{controller} (create), PUT /{id}, DELETE /{id} and POST /bulk-delete for every controller with a record source, whatever toolbar.buttons, showCheckboxes or the presence of config_form.yaml say. For a list-only controller, cc.Writable is empty and a POST creates a row with nothing but model defaults, provided the model rules pass. Schema-declared capabilities are therefore advisory only.
Fix: Record the allowed operations on CompiledController at compile time and check them in protect-wrapped handlers. Reject create and update when cc.Form == nil or create is not in the toolbar, and reject delete and bulk-delete unless delete is declared, returning 403 or 405.
WR-04: A form field that is required but limited by context makes every create fail
File: cabana/crud.go:689-707
Issue: mergedRules adds required for every scalar form field with required: true, regardless of the field's context. projectOperation and fillAllowed correctly drop a context: update field on create, so the value can never be supplied. Any field declared with both context: update and required: true therefore makes every create return 422.
Fix: Apply the form-level required only when contextAllows(cc, field.Name, op) holds (pass op into mergedRules).
WR-05: Relation link and unlink ignore the panel's toolbarButtons
File: cabana/relation.go:638-744, cabana/http.go:442-471
Issue: compileRelationButtons compiles view.toolbarButtons (for example link|unlink), but Link and Unlink never check it. A relation declared with only link, or with no buttons at all, still accepts POST .../relations/{name}/unlink and .../link from any holder of the controller permission.
Fix: In relationMutation, return 403 (or 405) unless the compiled cr.Schema.View.ToolbarButtons contains the requested action.
WR-06: A relation list without an explicit sort fails when the first column is not sortable
File: cabana/relation.go:545-560
Issue: When sort is empty, normalizeRelationQuery defaults sortKey to the first column. It then requires that column to be Sortable, or returns 422 sort: is not a sortable column. A panel whose first column declares sortable: false therefore rejects every linked or candidate request that omits sort, including the SPA's first load.
Fix: Default to the first sortable column, or to the primary key when there is none. Validate only when the client explicitly supplies sort.
WR-07: Relation column order depends on a fixed 16-space YAML indentation
File: cabana/relation.go:276-292
Issue: Nested maps lose their order during decode and re-marshal, so orderRelationColumns restores it by searching the raw file for "\n" + 16 spaces + key + ":". With any other indentation (2-space YAML is common), every lookup misses and the columns fall back to the marshaller's alphabetical key order, silently reordering the relation manager. The search also runs over the whole file, so the view and manage panels, and different relations, share one order: whichever occurrence comes first wins.
Fix: Decode config_relation.yaml with yaml.UseOrderedMap(), or walk the AST as fieldMap.UnmarshalYAML already does for forms, so that list.columns stays a MapSlice at every depth. Then drop orderRelationColumns.
WR-08: List search returns 500 for non-text searchable columns, and the scaffold creates one
File: cabana/query.go:294, internal/build/stubs/artifacts.tmpl:139-142
Issue: Every searchable column is compiled to LOWER("table"."col") LIKE ?. PostgreSQL has no lower(integer) or lower(timestamp), so searching a list with a numeric or date searchable column raises an SQL error and a 500. Winter casts to TEXT (DbDongle::cast($field, 'TEXT')). The make:admin-controller stub emits columns: id: searchable: true, so every scaffolded controller returns 500 on its first search.
Fix: Emit LOWER(CAST(%s AS TEXT)) LIKE ?, or reject non-string searchable columns at compile time using modelColumnType. Change the stub to make a text column searchable, or none.
WR-09: Scaffolded admin controllers have no permissions and no record source
File: internal/build/stubs/artifacts.tmpl:92-106
Issue: The generated controller implements only ID, ModelName and ConfigDir. With no RequiredPermissions(), Allows treats the empty requirement as open, so every backend admin (for example publisher) gets full CRUD once a record source is added. With no NewRecord(), the list endpoint returns 500 ("admin controller has no record source") and CRUD fails closed with a 500 CapabilityError. The generated default is therefore broken and, once completed naively, open to all admins.
Fix: Generate a RequiredPermissions() that returns a <plugin>.access_<snake> code, plus a matching HasPermissions entry or a TODO that fails compilation, and a NewRecord() placeholder. Alternatively, make Activate fail when a controller with a record source declares no permissions.
WR-10: Cabana silently reuses any guard already registered under the name "backend"
File: cabana/http.go:115-119
Issue: When a guard named backend already exists, Activate skips registering its own audience-checking guard and mounts the whole admin API behind the existing one. A plugin that registers backend first, whether by mistake or by name collision, silently replaces admin authentication: a different secret, provider and audience rule. Meanwhile svc.users and refresh still use cabana's provider, so the two paths disagree. The only remaining defense is the principal.Backend check in protect, which any guard can satisfy.
Fix: Always register cabana's guard, and fail boot when the name is taken by any plugin other than summercms.cabana:
if err := guards.Register("summercms.cabana", "backend", guard); err != nil {
return nil, fmt.Errorf("cabana: backend guard: %w", err)
}
WR-11: Login identifier can resolve to the wrong admin; admin:create does not prevent login/email collisions
File: cabana/auth.go:383-394, cabana/commands.go:72
Issue: findBackendLogin runs WHERE login = ? OR lower(email) = ? and takes First (the lowest id). If admin A's login equals admin B's email, or two copied rows differ only in email case (the UNIQUE constraint is case-sensitive), the identifier always resolves to A and B can never log in by email. admin:create checks only login = <new login> OR lower(email) = <new email>, so it permits exactly that cross-field collision. The same pattern in admin:reset-password returns "ambiguous", which shows the case is reachable.
Fix: Fetch up to two matches and reject ambiguity, with the same dummy bcrypt timing as a miss. Extend the admin:create check to login IN (?, ?) OR lower(email) IN (?, ?), and add a unique index on lower(email).
WR-12: The dummy hash cost is fixed at 10 while real hashes use the configured cost (timing oracle)
File: cabana/auth.go:145-149, cabana/auth.go:499-506
Issue: A missing user is checked against dummyPasswordHash, which is always bcrypt cost 10. Existing users are rehashed to admin.password.bcrypt_cost on login (NeedsRehash). With a cost of 12, a request for an existing login costs about 4 times more than one for a missing login, which allows admin account enumeration despite the identical response body.
Fix: Build the dummy hash lazily from s.bcryptCost, once per service, for example in Activate.
WR-13: Admin passwords are passed as command-line flags
File: cabana/commands.go:25, cabana/commands.go:42-44, cabana/commands.go:55, cabana/commands.go:109
Issue: admin:create --password and admin:reset-password --password are the only ways to supply a password. The value is then visible in ps or /proc/<pid>/cmdline to other local users and is stored in shell history. The T-09-04 mitigation ("do not echo passwords") does not cover this exposure.
Fix: Read the password from a TTY prompt with echo off, or from stdin (--password-stdin) or an environment variable. Keep --password only for tests, if at all.
WR-14: Logout cannot revoke a token whose access lifetime has expired but whose refresh window is still open
File: cabana/http.go:199, cabana/auth.go:251-257
Issue: logout is mounted behind the backend guard, which rejects expired tokens with 401 before the handler runs. The handler itself also uses VerifyClaimsAudience, which enforces exp. A token past its 60-minute exp but within the 14-day refresh_ttl therefore cannot be blacklisted through logout, yet /auth/refresh still accepts it. After a user "logs out" with a stale token, a leaked copy stays refreshable for up to two weeks. With cookie transport, the 401 also leaves the cookie in place.
Fix: Mount logout outside the guard (it already reads the token itself). Verify the signature and audience without claims validation, as refreshAudience does, and accept any token still inside iat + refresh_ttl. Always expire the cookie.
WR-15: The editors pivot granted_by stores a backend_users id in a frontend-user column
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/collections_admin_controller.go:96
Issue: RelationBeforeLink sets pivot["granted_by"] = &principal.ID, where the principal is a backend admin. In the PHP application, granted_by holds a frontend users.id (InvitationService.php:145 writes $invitation->invited_by; all fixtures use $owner->id). Each admin link therefore records an unrelated or nonexistent frontend user as the grantor, corrupting the audit column that the API may expose.
Fix: Write nil, which is what Winter's RelationController attach produces, or resolve the backend email to the frontend user id as FormBeforeCreate already does.
WR-16: Reflection helpers skip embedded structs and fall back to case-insensitive Go field names
File: cabana/http.go:749-764, cabana/crud.go:555-572, cabana/crud.go:775-795, cabana/query.go:477-498
Issue: fieldByColumn, modelColumns, primaryColumn and pkUint iterate only over top-level fields. For a model that embeds a base struct (gorm.Model or a shared timestamps struct, which is idiomatic GORM), list rows lose id and timestamps, and BindWritableFields rejects embedded columns at boot. Worse, pkUint(parent) returns 0, so relation Link writes pivots with ParentForeignKey = 0, and Linked and Candidates query owner 0. In addition, fieldByColumn matches strings.EqualFold(field.Name, column) even when that field has an explicit gorm:"column:..." tag naming a different column.
Fix: Resolve columns through GORM's parsed schema (schema.Parse(model, &sync.Map{}, db.NamingStrategy) gives LookUpField and PrioritizedPrimaryField) instead of hand-rolled reflection. At minimum, recurse into anonymous struct fields and use the name fallback only when no column: tag is present.
WR-17: User-level backend_users.permissions is ignored, so Winter denies are lost at cutover
File: cabana/auth.go:37-76
Issue: Grants come only from the role JSON and the code-declared role grants. D-01 says an existing Winter backend_users table "can be copied straight in at cutover", and D-03 says "Permissions keep Winter semantics". Winter merges the user's own permissions over the role's, and a -1 there denies a permission the role grants. Any copied admin with a user-level deny therefore gains that permission in Go, and user-level grants are lost.
Fix: Merge parseGrants(user.Permissions) over the role grants, treating -1 as an explicit deny that removes the code, including wildcard matches. Alternatively, have the migration or boot fail when a copied row has non-empty user-level permissions, so the gap cannot pass unnoticed.
WR-18: The phase gate's zero-test check applies per invocation, not per package
File: scripts/check-phase9.sh:24-55, scripts/check-phase9.sh:116-117
Issue: phase9_detect sets saw = True on the first passing test anywhere in the go test -json stream. run_security runs ./bouncer ./cabana -run '^TestPhase09' in one invocation, so if every TestPhase09* test in cabana were renamed or deleted, the passing bouncer test would satisfy the gate. T-09-20 claims zero-test runs are refused.
Fix: Track passing tests per Package and refuse when any package named in the invocation has zero passes. Alternatively, run each package in its own phase9_go call.
WR-19: Plugin hooks and scopes query outside the CRUD transaction
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/albums_admin_controller.go:99-147, ../fonoteka.go/plugins/golem15/fonoteka/controllers/collections_admin_controller.go:100-126, pact/capabilities.go (FormBeforeCreate, FormBeforeUpdate)
Issue: FormBeforeCreate(ctx, model) and the other hooks receive no transaction. The Fonoteka implementations therefore call c.db() (the app pool), and FormExtendQuery does the same to resolve the collection binding. Each admin request holds a transaction connection with a FOR UPDATE lock on the target row while a second connection runs these lookups, which read outside the transaction's snapshot. If the pool is ever bounded (SetMaxOpenConns), N concurrent admin writes can deadlock, each waiting for a second connection.
Fix: Pass the active *gorm.DB into the hook signatures (as GORM model hooks already receive tx), or place it on ctx from save and loadRecord and have plugins use it.
Info
IN-01: A no-op relation mutation returns {}
File: cabana/relation.go:99-102
Issue: Linked and Removed are tagged omitempty, so an idempotent replay returns data: {}, while the OpenAPI shape and the other counters (BulkResult.deleted) always include the field.
Fix: Drop omitempty.
IN-02: Relation search does not escape LIKE wildcards
File: cabana/relation.go:607-608
Issue: % and _ in the term act as wildcards, unlike list search, which uses escapeLike. The parameter is bound, so this is not injection.
Fix: Use "%" + escapeLike(term) + "%" together with ESCAPE '\'.
IN-03: last_page is inconsistent between the list and relation endpoints
File: cabana/relation.go:475-478
Issue: An empty relation page reports last_page: 0, while an empty list reports last_page: 1 (lagoon.Paginate).
Fix: Use lagoon.Paginate in the relation path as well.
IN-04: Page parsing and the offset computation can overflow
File: cabana/relation.go:571-583, cabana/query.go:135
Issue: parsePositive accumulates digits without an overflow check. A very large page makes (page-1)*per wrap negative, and GORM then drops the OFFSET, returning page 1 data labelled as a huge page number.
Fix: Cap page (for example at 1e6), or check offset < 0 and return 422.
IN-05: Controllers with unroutable IDs are accepted
File: cabana/registry.go:16-35
Issue: An ID equal to the plugin ID, or one with more than three dot-separated segments, passes ownership validation but can never match /{vendor}/{plugin}/{controller}.
Fix: Require exactly three identifier segments.
IN-06: The bulk-delete decoder is looser than the relation decoder
File: cabana/http.go:634-642
Issue: decodeBulk accepts unknown fields and trailing JSON values, while decodeRelationMutation rejects both.
Fix: Add DisallowUnknownFields() and the trailing-token check.
IN-07: Dead dropdown branches and SQL built by string concatenation in the albums controller
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/albums_admin_controller.go:42-53, :149-169
Issue: genre and style are type: relation fields (or absent), so DropdownOptions("genre"/"style") is never called. orderedOptions also concatenates a table name into SQL. The name is a constant, but the pattern invites misuse.
Fix: Remove the unused branches and orderedOptions.
IN-08: The password-reset cutoff also rejects logins made within about 1-2 seconds of the reset
File: cabana/commands.go:130, bouncer/jwt.go:172-174
Issue: The cutoff is now + 1s and iat has one-second resolution, so a login immediately after a reset receives a token that the guard rejects at once.
Fix: Truncate the cutoff to the second (time.Now().Truncate(time.Second)) and compare with !iat.Before(cutoff) semantics as intended.
IN-09: Album scoping hides database errors as an empty list
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/albums_admin_controller.go:103-106
Issue: Any resolveBinding error, including a database failure, becomes WHERE 1 = 0, so an outage looks like an empty collection.
Fix: Keep the fail-closed behavior, but log non-CollectionResolveError failures, or surface them as 500.
IN-10: Read-only GETs take row locks
File: cabana/crud.go:445, cabana/relation.go:453
Issue: ShowRecord, Linked and Candidates all call loadRecord, which issues SELECT ... FOR UPDATE. Plain reads therefore block behind concurrent writers and hold locks while plugin scope queries run. PostgreSQL also rejects FOR UPDATE if a FormExtendQuery adds an outer join.
Fix: Pass a lock bool into loadRecord and lock only on mutating paths.
IN-11: Logout reports success without revoking when no blacklist is configured
File: cabana/auth.go:258-274
Issue: When s.bl == nil, the handler returns logged_out even though the token stays valid.
Fix: Return 500, or fail Activate when admin controllers exist but no database or blacklist is available.
IN-12: The scaffold marks a file that must be edited as "DO NOT EDIT"
File: internal/build/stubs/artifacts.tmpl:92
Issue: The generated admin controller must be edited to add a record source and permissions (see WR-09), but its header says "Code generated by summer make. DO NOT EDIT.", which tools such as linters treat as generated code.
Fix: Drop the header for this one-time scaffold.
Reviewed: 2026-09-27T00:00:00Z Reviewer: Claude (gsd-code-reviewer) Depth: standard