golem15/summercms
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(10): re-review CR-01 fix and record approved UAT

Browse Source
This commit is contained in:
Jakub Zych
2026-09-27 20:38:18 +02:00
parent 7e015638f7
commit c7487f6799
3 changed files with 95 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
.planning/phases/10-admin-vue-spa/10-REVIEW-DISPOSITION.md
View File

@@ -23,3 +23,7 @@ created: 2026-09-27T16:36:06Z
| IN-05 | info | open | Relation id lists have no size cap |
| IN-06 | info | open | The gate's required-test check ignores the package |
| IN-07 | info | open | Logging out from a dirty form can leave the user on the form without a session |
| WR-08 | warning | open | The frontend user refresh still ignores tokens_valid_after, so the CR-01 gap remains for site users (added by re-review 2026-09-27) |
| IN-08 | info | open | bouncer.Middleware still inlines the subject lookup that subjectPrincipal now owns (added by re-review 2026-09-27) |
| IN-09 | info | open | RefreshAudienceFor takes a request context but the blacklist calls ignore it (added by re-review 2026-09-27) |
| IN-10 | info | open | service.users is documented as the backend guard's provider, which is not guaranteed (added by re-review 2026-09-27) |

83
.planning/phases/10-admin-vue-spa/10-REVIEW.md
View File

@@ -1,6 +1,8 @@
---
phase: 10-admin-vue-spa
reviewed: 2026-09-27T16:34:04Z
re_reviewed: 2026-09-27T18:37:03Z
re_review_base: 6918ec5
depth: standard
files_reviewed: 118
files_reviewed_list:
@@ -122,20 +124,54 @@ files_reviewed_list:
- ../fonoteka.go/plugins/golem15/fonoteka/lang/en/lang.yaml
- ../fonoteka.go/plugins/golem15/fonoteka/lang/pl/lang.yaml
- ../fonoteka.go/scripts/check-openapi.sh
re_review_files:
- bouncer/jwt.go
- bouncer/refresh.go
- cabana/auth.go
- cabana/http.go
- admin/src/views/FormView.vue
- admin/src/views/SettingsFormView.vue
- bouncer/jwt_guard_test.go
- bouncer/refresh_test.go
- cabana/phase10_coverage_test.go
- cabana/refresh_revocation_test.go
findings:
critical: 1
warning: 7
info: 7
total: 15
critical: 0
warning: 8
info: 10
total: 18
resolved: 1
status: issues_found
---
# Phase 10: Code Review Report
**Reviewed:** 2026-09-27T16:34:04Z
**Re-reviewed:** 2026-09-27T18:37:03Z (delta `6918ec5..HEAD`: be4a923, a13a121, 2585671)
**Depth:** standard
**Files Reviewed:** 118
**Status:** issues_found
**Files Reviewed:** 118 (+10 in the re-review, 4 of them tests)
**Status:** issues_found (0 open critical, CR-01 resolved)
## Re-review (2026-09-27T18:37:03Z)
Scope: `git diff 6918ec5..HEAD` of bouncer/jwt.go, bouncer/refresh.go, cabana/auth.go, cabana/http.go, admin/src/views/FormView.vue and admin/src/views/SettingsFormView.vue, plus the four changed test files. `go vet ./bouncer ./cabana` is clean. The refresh, guard and Phase 10 tests pass, including `TestAdminRefreshRevocation` against Postgres.
**CR-01 is resolved.** Evidence:
- `cabana/auth.go:224` now calls `bouncer.RefreshAudienceFor(r.Context(), s.users, ...)`. `s.users` is the same `lazyBackendUsers` value handed to `NewBackendJWTGuard` (`cabana/http.go:113-114,137`).
- `bouncer/refresh.go:98-102` runs the subject check after every token-only check and before `MintAudience`. A refusal returns before the mint and before the old jti is blacklisted.
- The check (`refresh.go:41-50`) reuses `subjectPrincipal` and `issuedBeforeCutoff` (`jwt.go:152-174`), the same code the guard now runs. `BackendUsers.FindByID` (`cabana/auth.go:37-62`) returns nil for a missing row, a soft-deleted row (`gorm.DeletedAt` on `BackendUser`) or `!IsActivated`, so all three map to `ErrSubjectRejected`.
- It fails closed. A provider or DB error becomes `errAuthentication`, which gives a 401, mints nothing, blacklists nothing and leaves the cookie in place. A nil provider, a zero sub or a non-numeric sub is `ErrSubjectRejected`.
- There is no race with a reset. `admin:reset-password` sets the cutoff to `now+1s` (`cabana/commands.go:131`), and `iat` is truncated to whole seconds. A refresh that loaded the user just before the reset committed therefore mints a token whose `iat` is still before the cutoff, and the guard rejects that token on its next use.
- Cookie handling: on `ErrSubjectRejected` over the cookie transport, the handler sends an expiring `summer_admin` with the admin Path (`auth.go:229-231`). A Bearer refusal sets no cookie. The tests cover the reset (cookie and Bearer), deactivated, soft-deleted, provider-error and still-active cases (`cabana/refresh_revocation_test.go`, `cabana/phase10_coverage_test.go:384-482`, `bouncer/refresh_test.go:125-261`).
**No regressions found in the guard or the user plugin:**
- The guard's order is unchanged: verify, load subject, blacklist, cutoff (`jwt.go:113-137`).
- The guard's messages are unchanged. A subject or cutoff refusal is still "User not found" (now the `ErrSubjectRejected` sentinel), and a provider error is still "Authentication error".
- `bouncer.Refresh` and `bouncer.RefreshAudience` pass `check == nil`, so their flow and errors are the same. The only external caller, `fonoteka.go/plugins/golem15/user/controllers/api_controller.go:190`, uses `bouncer.Refresh` and is untouched. No caller compares these errors by identity.
The form width change (2585671) only removes `mx-auto max-w-[980px]` in both views, and `boardwalk/dist` was rebuilt in the same commit. No issue.
New findings from the re-review: WR-08 and IN-08 to IN-10. WR-01 and WR-07 are still open, and their text is updated below where CR-01 changed their impact.
## Summary
@@ -161,7 +197,11 @@ The warnings cover:
## Critical Issues
### CR-01: Refresh ignores the tokens_valid_after cutoff, so the SPA undoes session revocation
### CR-01: Refresh ignores the tokens_valid_after cutoff, so the SPA undoes session revocation — RESOLVED (be4a923, a13a121)
**Resolution (re-review 2026-09-27T18:37:03Z):** Fixed as proposed, through a new opt-in `bouncer.RefreshAudienceFor` that runs the guard's subject checks (`subjectPrincipal` and `issuedBeforeCutoff`) immediately before minting (`bouncer/refresh.go:37-52,98-102`). The admin refresh handler uses it with the guard's provider (`cabana/auth.go:224`, `cabana/http.go:113,137`). A token issued before the cutoff, or held by a deleted or deactivated admin, gets a 401. The fix mints nothing, leaves the old jti un-blacklisted, and expires the cookie when the token came from it. A provider error fails closed. The requested regression test exists: `TestAdminRefreshRevocation` resets the password and refreshes the old cookie and the old Bearer. The sliding-iat part (WR-07) is a separate finding and stays open. The same gap on the frontend user refresh is recorded as WR-08.
*Original finding:*
**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.
@@ -198,6 +238,8 @@ Also add a test: reset the password, then refresh with a token issued before the
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.
*Re-review note:* CR-01's fix removes the "back to life" path for reset, deactivated and deleted admins, and a refused cookie refresh now expires the cookie (`cabana/auth.go:229-231`). This finding is still open. Logout still sits behind the guard (`cabana/http.go:199`) and returns 401 without `Set-Cookie`. A cookie refresh that fails for a token-only reason (blacklisted jti, past the refresh window, bad signature) also leaves the cookie in place on purpose (`auth.go:226-228`).
**Fix:** Mount logout outside the guard (keep `requireAjax`), and call `s.expireSessionCookie(w)` first on every path:
```go
func (s *service) logout(w http.ResponseWriter, r *http.Request) {
@@ -245,6 +287,14 @@ For expired tokens, parse them without claims validation so their jti can still
**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.
*Re-review note:* Still open. be4a923 leaves the mint unchanged (`bouncer/refresh.go:103`). A password reset now ends a sliding session. Without a reset, a session refreshed at least once every 14 days still never expires.
### WR-08: The frontend user refresh still ignores tokens_valid_after, so the CR-01 gap remains for site users
**File:** `bouncer/refresh.go:17-19`, `../fonoteka.go/plugins/golem15/user/controllers/api_controller.go:175-199`, `../fonoteka.go/plugins/golem15/user/controllers/api_controller.go:537-549`
**Issue:** The user plugin's change-password handler sets `tokens_valid_after = iat - 1s`. That keeps the caller's own session and revokes every other session, and `classes/user_lookup.go:42-43` feeds the cutoff to the frontend guard. `POST /_user/api/v1/refresh` still calls `bouncer.Refresh`, which by design does no subject checks. A token stolen before the password change keeps being refreshed for the whole `refresh_ttl`, and each refreshed token has a new `iat` that passes the guard. This is the CR-01 bug on the frontend audience. It is not a regression from this delta, because `Refresh` was deliberately left unchanged for the core user plugin's contract. It is a real revocation bypass that the CR-01 fix makes easy to close.
**Fix:** Do not change `bouncer.Refresh`. In the user plugin's `Refresh` handler, opt in to the subject checks with a frontend variant that keeps the missing-aud (`allowMissing`) behaviour, for example an exported `bouncer.RefreshFor(ctx, users, secret, raw, ...)` that passes a `check` to `refreshAudience(..., AudienceUser, true, ..., check)`. It should use the provider the frontend guard uses. First confirm with the owners of the user plugin contract that PHP's refresh also rejects a revoked subject. If it does, this restores parity. If it does not, record the choice in a decision note.
## Info
### IN-01: A large access TTL makes the proactive refresh fire in a loop
@@ -289,8 +339,27 @@ For expired tokens, parse them without claims validation so their jti can still
**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.
### IN-08: bouncer.Middleware still inlines the subject lookup that subjectPrincipal now owns
**File:** `bouncer/jwt.go:45-62` vs `bouncer/jwt.go:152-168`
**Issue:** be4a923 moved the guard's sub parse, nil-provider check, `FindByID` call and nil-user check into `subjectPrincipal`, but `Middleware` keeps its own copy of the same logic. The guard's blacklist-store error also still builds a fresh `errors.New("Authentication error")` (`jwt.go:128`) instead of reusing `errAuthentication`. The three paths now share messages only by convention, so a later change to one can leave the others behind.
**Fix:** In `Middleware`, call `subjectPrincipal(r.Context(), users, sub)` and `write401(w, err.Error())`. Return `errAuthentication` at `jwt.go:128`.
### IN-09: RefreshAudienceFor takes a request context but the blacklist calls ignore it
**File:** `bouncer/refresh.go:85`, `bouncer/refresh.go:116`
**Issue:** The subject lookup now runs with the request `ctx`, but `IsBlacklisted` and `Add` in the same flow still use `context.Background()`. A cancelled or timed-out admin refresh can still block on a slow blacklist store, and request-scoped values such as tracing do not reach it. The old `Refresh` and `RefreshAudience` have no ctx, so they need the Background fallback. The new ctx-aware entry point does not.
**Fix:** Pass a `ctx` parameter into `refreshAudience`. `Refresh` and `RefreshAudience` keep passing `context.Background()` so their behaviour does not change, and `RefreshAudienceFor` passes its `ctx`.
### IN-10: service.users is documented as the backend guard's provider, which is not guaranteed
**File:** `cabana/http.go:44`, `cabana/http.go:113-119`, `cabana/http.go:137`
**Issue:** `Activate` registers its own guard only when no `backend` guard exists yet. If another plugin, or a second `Activate` on the same app, registered `backend` first, requests are authenticated by that guard's provider while refresh checks subjects against cabana's `lazyBackendUsers`. The field comment "the backend guard's provider, reused by refresh" would then be false, and refresh could mint tokens for subjects the active guard refuses, or refuse subjects it accepts. Today only cabana registers `backend`, so this is latent.
**Fix:** Fail activation when a `backend` guard is already registered by another owner. Alternatively, reword the comment to say refresh always checks cabana's `backend_users`, independent of the registered guard.
---
_Reviewed: 2026-09-27T16:34:04Z_
_Re-reviewed: 2026-09-27T18:37:03Z (delta 6918ec5..HEAD)_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_

31
.planning/phases/10-admin-vue-spa/10-UAT.md
View File

@@ -1,56 +1,55 @@
---
status: testing
status: complete
phase: 10-admin-vue-spa
source: [10-VERIFICATION.md]
started: 2026-09-27T16:44:38Z
updated: 2026-09-27T16:44:38Z
updated: 2026-09-27T18:34:33Z
---
## Current Test
number: 1
name: Decide CR-01 (refresh ignores tokens_valid_after / is_activated)
expected: |
Secure behavior: after `summer admin:reset-password <login>` and access-token expiry, the SPA lands on the login screen. Current code silently refreshes and keeps working for up to refresh_ttl (14 days). Decide: fix now, or accept with override plus follow-up.
awaiting: user response
[testing complete]
## Tests
### 1. Decide CR-01 (refresh ignores tokens_valid_after / is_activated)
expected: Fix now or accept with override. Secure behavior is a redirect to login after password reset and token expiry.
result: [pending]
result: pass
note: fixed in quick task 260927-q23 (be4a923, a13a121)
### 2. Limited admin vs superuser navigation at /plytadmin
expected: Limited admin (only golem15.fonoteka.access_genres) sees only fonoteka > Genres, no Ustawienia. Superuser sees Albums, Collections, Genres, Styles, Artists plus Ustawienia.
result: [pending]
result: pass
### 3. Walkthrough of the five controllers and Ustawienia
expected: Every list and form renders from its YAML. Search, sort, filter, paging and bulk delete work. Save shows a toast. 422 errors appear under the right fields. Albums genre/artists relations and Ustawienia > search_use_typesense work.
result: [pending]
result: pass
### 4. Collection editors link/unlink round trip
expected: The picker shows 5 per page with the owner excluded. Dodaj (N) is disabled at 0. Linking refreshes the list and shows a plural toast. Unlink asks for confirmation and removes the row. The focus trap and Esc work. The relation manager does not appear on the create form.
result: [pending]
result: pass
### 5. Visual check in light/dark at desktop and ~900px
expected: Matches .planning/phases/10-admin-vue-spa/design. The sidebar stays dark. Below ~1100px the panel collapses to the rail, and the flyout opens on hover/focus, closes on Esc and returns focus.
result: [pending]
result: pass
### 6. Secure cookie on http://localhost (assumption A3)
expected: With backend.cookie_secure at its default, Chrome and Firefox store summer_admin on http://localhost and the session works.
result: [pending]
result: pass
### 7. Review the 17 judgment-tier prohibitions in 10-VERIFICATION.md
expected: Accept or reject the verifier's "not violated" verdicts.
result: [pending]
result: pass
## Summary
total: 7
passed: 0
passed: 7
issues: 0
pending: 7
pending: 0
skipped: 0
blocked: 0
## Gaps
None. The user approved all items in a local browser session on 2026-09-27. A follow-up change made the record and settings forms full width (2585671).