19 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 10-admin-vue-spa | 2026-09-27T16:34:04Z | standard | 118 |
|
|
issues_found |
Phase 10: Code Review Report
Reviewed: 2026-09-27T16:34:04Z Depth: standard Files Reviewed: 118 Status: issues_found
Summary
Reviewed the Phase 10 admin SPA (admin/), the boardwalk SPA server, and the cabana admin API changes: cookie transport and CSRF, the backend.uri prefix, relation field options and saves, filter options, messages, the phrasebook override layer, and the fonoteka controller wiring. The review read the changed files and traced the auth calls into bouncer (jwt.go, refresh.go, mint.go) and cabana/commands.go.
The main controls hold up:
- CSRF: every unsafe admin route is wrapped in
requireAjax, and the cookie isSameSite=Strict,HttpOnlyandSecure. - XSS: the SPA has no raw-HTML sink, so it renders plugin and server strings as text only. The CSP is
script-src 'self'. - Redirects:
safeRedirectandmapWinterUrlonly produce in-app routes. - Relation saves: submitted ids go back through the scoped options query inside the save transaction.
- Admin prefix: other plugins' routes under the prefix are refused at boot.
There is one blocker. POST /auth/refresh does not check the tokens_valid_after cutoff that admin:reset-password uses to revoke sessions. It also mints a token with a fresh iat. The Phase 10 SPA refreshes automatically on any 401, so a password reset no longer ends existing browser sessions.
The warnings cover:
- logout leaving the cookie in place when the token is rejected
- a relation-scope bypass the activation check does not catch
- validation running before relation keys are assigned
- filter options that cannot be scoped per admin
- missing error handling in SPA loaders
- pagination after deletes
- an unbounded sliding refresh window
Critical Issues
CR-01: Refresh ignores the tokens_valid_after cutoff, so the SPA undoes session revocation
File: cabana/auth.go:217-241, bouncer/refresh.go:52-71, bouncer/mint.go:48-60, admin/src/api/client.ts:77-92
Issue: summer admin:reset-password (cabana/commands.go:128-133) sets tokens_valid_after, and the backend guard rejects any token whose iat is before it (bouncer/jwt.go:144). The refresh handler never loads the user. RefreshAudience checks only the signature, the audience, iat + refresh_ttl and the blacklist, then calls MintAudience, which stamps iat = now. The new token's iat is after the cutoff, so it passes the guard.
Before Phase 10 a Bearer client had to call refresh on purpose. Now the SPA transport does it for every 401: the guard rejects the revoked cookie, refreshSession() gets a fresh cookie, and the request is replayed. Anyone holding a copied or stolen summer_admin cookie (or an old Bearer token) keeps full admin access for up to refresh_ttl (14 days) after a password reset. Refresh also skips is_activated. The guard still blocks a deactivated user, but refresh keeps minting tokens for them.
Fix: Load the principal in the refresh handler and apply the same checks as the guard before minting:
func (s *service) refresh(w http.ResponseWriter, r *http.Request) {
raw, fromCookie := sessionToken(r)
// parse without exp validation to read sub and iat (reuse the refresh parser)
sub, iat, err := bouncer.PeekRefreshClaims(s.secret, raw, bouncer.AudienceBackend)
if err != nil { /* 401 */ }
id, _ := strconv.ParseUint(sub, 10, 64)
principal, err := lazyBackendUsers{app: s.app, reg: s.reg}.FindByID(r.Context(), uint(id))
if err != nil || principal == nil ||
(!principal.TokensValidAfter.IsZero() && iat.Before(principal.TokensValidAfter)) {
s.expireSessionCookie(w)
WriteError(w, http.StatusUnauthorized, "unauthenticated", msgUnauthenticated)
return
}
next, err := bouncer.RefreshAudience(...)
...
}
Also add a test: reset the password, then refresh with a token issued before the reset, and expect a 401.
Warnings
WR-01: Logout does not expire the cookie when the token is rejected
File: cabana/http.go:196, cabana/auth.go:243-269
Issue: The comment says logout "always expires the admin cookie". But /auth/logout sits behind the backend guard. An expired, blacklisted or cutoff-rejected token gets the guard's 401 before the handler runs. Inside the handler, a VerifyClaimsAudience failure also returns 401 before expireSessionCookie. In both cases no Set-Cookie is sent, and the cookie stays in the browser for its refresh-window Max-Age.
The SPA only recovers through its refresh-and-retry path. If refresh fails, the stale cookie stays behind. With CR-01 unfixed, a later visit can bring that session back to life.
Fix: Mount logout outside the guard (keep requireAjax), and call s.expireSessionCookie(w) first on every path:
func (s *service) logout(w http.ResponseWriter, r *http.Request) {
s.expireSessionCookie(w) // unconditionally
raw, _ := sessionToken(r)
_, iat, exp, jti, err := bouncer.VerifyClaimsAudience(raw, s.secret, bouncer.AudienceBackend)
...
}
For expired tokens, parse them without claims validation so their jti can still be blacklisted.
WR-02: A belongsTo foreign key exposed as a scalar field skips the relation scope check
File: cabana/relation_field.go:136-152, cabana/crud.go:93-117, cabana/registry.go:76-92
Issue: compileFieldRelations binds type: relation fields. BindWritableFields separately makes every scalar form field that names a model column writable. Nothing stops fields.yaml from declaring both genre (relation) and genre_id (for example the older Winter type: dropdown pattern). In that case genre_id goes through ProjectWritableFields and lagoon.Fill with no RelationExtendOptionsQuery check, so an out-of-scope id can be stored. That breaks the stated invariant that "the options hook is never only cosmetic". The same applies to a pivot foreign key column.
Fix: After BindWritableFields, fail activation if any cc.Writable[i].FillKey equals a FieldRelations[*].Contract.ForeignKey, with an error such as field genre_id duplicates the foreign key of relation field genre.
WR-03: Model rules run before relation values are assigned
File: cabana/crud.go:336 vs cabana/crud.go:353-356
Issue: lagoon.Validate runs on target before assignBelongsTo writes the submitted foreign key. A model rule on the foreign key column (a common Winter pattern such as category_id: required or exists:...) checks the stale value. On create, a valid submitted genre fails required. On update, the rule checks the old id and the new one is never validated. Relation shape errors (liftRelationValues) and scope errors also come back separately from scalar validation errors, so a bad form needs several round trips.
Fix: Run checkRelationScope and assignBelongsTo before mergedRules/lagoon.Validate, and merge relation ValidationError details with the scalar validation messages into one 422.
WR-04: Scope filter choices cannot be scoped to the signed-in admin
File: pact/capabilities.go:265-270, cabana/filter_schema.go:264-294
Issue: FilterOptions(scope string) []Option gets no context.Context and no *gorm.DB. A model cannot see the principal, so it cannot narrow choices the way ListExtendQuery/FormExtendQuery narrow rows (for example "collections I can see"). It also has no request-scoped DB handle. Any model-backed filter that lists tenant data returns every tenant's labels to any admin who can open the list. The contract is new in this phase, so changing it now is cheap.
Fix: Change the capability to FilterOptions(ctx context.Context, db *gorm.DB, scope string) []Option, and pass r.Context() (with the principal) and s.db() from filterOptions. Optionally run the result through the controller's ListExtendQuery.
WR-05: SPA loaders have no error handling, so network failures leave views stuck loading
File: admin/src/views/ListView.vue:96-115, admin/src/views/FormView.vue:141-159, admin/src/views/SettingsFormView.vue:34-44, admin/src/components/list/FilterBar.vue:44-49, admin/src/views/LoginView.vue:19-37
Issue: openapi-fetch rejects on network errors or aborted fetches, and on a thrown replay inside transport.onResponse. loadList, load (list schema), FormView load, SettingsFormView load and FilterBar loadScope have no try/catch. loading stays true forever, the skeleton never clears, no failure message appears, and each case is an unhandled promise rejection. In LoginView, login() throwing skips failed.value = true, so the form silently does nothing.
Fix: Wrap each loader in try { ... } catch { failed.value = true } finally { loading.value = false }, keeping the generation guard in ListView. In LoginView, catch and set failed.
WR-06: After a delete or unlink, the list can stay on a page past the last page
File: admin/src/views/ListView.vue:199-203, admin/src/components/relation/RelationManager.vue:172-179
Issue: Both reload with the same page after removing rows. Deleting or unlinking every row on the last page (for example page 3 of 3) reloads page 3, which is now empty. The user sees the "no records" state even though pages 1-2 still hold records.
Fix: After reload, clamp the page: if rows.length === 0 && meta.page > 1, go to meta.last_page. In ListView use replaceQuery({...query.value, page: meta.value.last_page}); in RelationManager set page.value = meta.value.last_page and call loadRows() again.
WR-07: Refresh re-mints iat, so the refresh window slides with no upper bound
File: bouncer/refresh.go:52-71, bouncer/mint.go:48-60, admin/src/state/useAuth.ts:117-127
Issue: refresh_ttl is measured from the presented token's iat, and each refresh issues a token with iat = now. The SPA refreshes on its own at 80% of the access lifetime. A browser session, or a stolen cookie refreshed at least once every 14 days, therefore never reaches an absolute expiry. tymon/jwt-auth, which the PHP side follows, keeps the original iat by default (refresh_iat: false), which caps the session at refresh_ttl after login. Check this against the PHP contract before changing it.
Fix: Carry an orig_iat (or auth_time) claim through refresh, and measure refresh_ttl from it.
Info
IN-01: A large access TTL makes the proactive refresh fire in a loop
File: admin/src/state/useAuth.ts:117-127
Issue: setTimeout(..., seconds * 800) overflows the 32-bit browser timer limit when admin.jwt.ttl is above about 44,700 minutes (31 days). The callback then fires at once, onRefreshed reschedules it, and the SPA sends a continuous stream of /auth/refresh calls.
Fix: Math.min(seconds * 800, 2_147_483_647), or reject such a TTL on the server.
IN-02: Nothing enforces the CSRF design's "no preflight on the admin API" assumption
File: cabana/csrf.go:8-15, surf/router.go:356
Issue: The X-Requested-With defence assumes the admin API never answers a CORS preflight. pathScopedCORS wraps the whole mux, and nothing refuses http.cors.paths that match {prefix}/api/* (for example *, or backend.uri: /api). A credentialed origin allow-list would then let a same-site origin send the header.
Fix: Skip CORS handling for paths under the admin prefix, or fail boot when a configured CORS glob matches {prefix}/api/v1/....
IN-03: Choosing the transport by X-Requested-With is fragile for Bearer clients
File: cabana/auth.go:175-184
Issue: A Bearer-mode API client whose HTTP library adds X-Requested-With: XMLHttpRequest by default (Laravel's axios bootstrap, jQuery) gets token_type: cookie, a Set-Cookie, and no access_token from login.
Fix: Select the cookie transport with an explicit signal, such as a transport: "cookie" body field or a dedicated header, rather than a common framework default.
IN-04: Dead genre and style cases in the albums DropdownOptions
File: ../fonoteka.go/plugins/golem15/fonoteka/controllers/albums_admin_controller.go (DropdownOptions)
Issue: genre is now type: relation and the album fields.yaml has no style dropdown, so the genre/style cases and their unscoped SELECT id, name FROM ... queries are unreachable.
Fix: Remove both cases, so later readers do not assume these options apply.
IN-05: Relation id lists have no size cap
File: cabana/relation_field.go:376-402, cabana/relation_field.go:455-457
Issue: A belongsToMany value accepts any number of ids. Past PostgreSQL's 65,535 bind-parameter limit, the IN scope check fails and the save returns a generic 500 instead of a 422.
Fix: Cap the list, for example at 1,000 ids, with a validation message.
IN-06: The gate's required-test check ignores the package
File: scripts/check-phase10.sh:79-80, scripts/check-phase10.sh:103-106
Issue: passed stores bare test names. A required test is satisfied by a passing test of the same name in any package in the run. Also, the trap ... RETURN in run_self_test never runs on the exit 1 paths, so the scratch directory is left behind.
Fix: Key passed as pkg:Test, pass the package with PHASE10_REQUIRE, and use trap ... EXIT in a subshell.
IN-07: Logging out from a dirty form can leave the user on the form without a session
File: admin/src/state/useAuth.ts:171-184, admin/src/views/FormView.vue:279-291
Issue: logout() clears the session, then router.replace({name: 'login'}) triggers FormView's dirty guard. If the admin picks cancel, the navigation is aborted and the error swallowed. The shell stays on the form with currentUser null and a server session that is already gone, so the next save fails with a 401.
Fix: Set a "force leave" flag (as FormView's leaving does) before logout navigation, or bypass route-leave guards for the login redirect.
Reviewed: 2026-09-27T16:34:04Z Reviewer: Claude (gsd-code-reviewer) Depth: standard