diff --git a/.planning/STATE.md b/.planning/STATE.md index bf16226..ae2a837 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -31,7 +31,7 @@ See: .planning/PROJECT.md (updated 2026-09-16) Phase: 10 (Admin Vue SPA) — EXECUTING Plan: 5 of 5 Status: Phase complete — ready for verification -Last activity: 2026-09-27 — Phase 10 plan 05 unit tests, assembled acceptance and phase gate completed +Last activity: 2026-09-27 - Completed quick task 260927-q23: fix CR-01: /auth/refresh must enforce tokens_valid_after and is_activated Progress: [██████████] 100% @@ -348,6 +348,12 @@ None yet. - Phase 11 (River dual-driver split) is documented but unverified against a real build — plan with `--research-phase` and budget a timed-latency test. - Phase 8 UI gate: scripts/check-phase8-ui.mjs --final-gate's real Playwright browser matrix (32 UI-SPEC scenarios) is unimplemented (deliberate fatal() at check-phase8-ui.mjs:458, never authored by 08-05); stage_ui_harness must keep failing closed until a Playwright spec is authored. User approved Phase 8 closure on 2026-09-24 with this gap carried forward -- see 08-10-SUMMARY.md and .planning/phases/08-oauth2-1-authorization-server/deferred-items.md. +### Quick Tasks Completed + +| # | Description | Date | Commit | Directory | +|---|-------------|------|--------|-----------| +| 260927-q23 | fix CR-01: /auth/refresh must enforce tokens_valid_after and is_activated | 2026-09-27 | be4a923 | [260927-q23-fix-cr-01-auth-refresh-must-enforce-toke](./quick/260927-q23-fix-cr-01-auth-refresh-must-enforce-toke/) | + ## Deferred Items Items acknowledged and carried forward from previous milestone close: diff --git a/.planning/phases/10-admin-vue-spa/10-REVIEW-DISPOSITION.md b/.planning/phases/10-admin-vue-spa/10-REVIEW-DISPOSITION.md index 5767960..c35b8eb 100644 --- a/.planning/phases/10-admin-vue-spa/10-REVIEW-DISPOSITION.md +++ b/.planning/phases/10-admin-vue-spa/10-REVIEW-DISPOSITION.md @@ -8,7 +8,7 @@ created: 2026-09-27T16:36:06Z | Finding | Severity | Disposition | Note | |---|---|---|---| -| CR-01 | critical | open | Refresh ignores the tokens_valid_after cutoff, so the SPA undoes session revocation (confirmed in bouncer/refresh.go; flaw from 09-01, exposed by the Phase 10 cookie auto-refresh) | +| CR-01 | critical | fixed | Fixed in be4a923 (tests a13a121), quick 260927-q23. Admin refresh now applies the backend guard's subject checks (activated, not deleted, iat not before tokens_valid_after) via bouncer.RefreshAudienceFor before minting, and a refused cookie refresh expires summer_admin. | | WR-01 | warning | open | Logout does not expire the cookie when the token is rejected | | WR-02 | warning | open | A belongsTo foreign key exposed as a scalar field skips the relation scope check | | WR-03 | warning | open | Model rules run before relation values are assigned | diff --git a/.planning/phases/10-admin-vue-spa/10-SECURITY-REVIEW.md b/.planning/phases/10-admin-vue-spa/10-SECURITY-REVIEW.md index 94a1ddc..4bd7e83 100644 --- a/.planning/phases/10-admin-vue-spa/10-SECURITY-REVIEW.md +++ b/.planning/phases/10-admin-vue-spa/10-SECURITY-REVIEW.md @@ -17,7 +17,7 @@ Commands run from `summercms.go`. `../fonoteka.go` tests run inside that reposit | T-10-02 | Tampering | cookie-authenticated unsafe admin routes (CSRF) | high | mitigated | `cabana/csrf.go` `requireAjax` wraps every POST/PUT/DELETE except login and refuses a request with neither Bearer nor `X-Requested-With: XMLHttpRequest` before decoding. The cookie is SameSite=Strict | `TestPhase10CSRF` (walks every mounted handler with a body-read spy), `TestPhase10Coverage/every unsafe mounted route is CSRF-walked` (fails when an unsafe route is added without the walk), `TestPhase10TracerSPA` step 8; stage `--security` | pass; removal check fails TestPhase10CSRF and TestPhase10TracerSPA | Relies on browsers not sending custom headers cross-origin without a CORS preflight, and the admin API answers none | | T-10-03 | Information Disclosure | boardwalk static serving | medium | mitigated | `boardwalk/boardwalk.go` serves the embedded fs only, with `path.Clean`. It lists no directory, answers 404 for an extension miss and the JSON envelope for `api/` misses | `go test ./boardwalk` (TestTraversalIsCleaned, TestDirectoryIsNeverListed, TestMissingFileWithExtensionIs404, TestPhase10BoardwalkServing encoded traversal); stage `--security` | pass | None known | | T-10-04 | Tampering | admin HTML responses (clickjacking, sniffing, indexing) | medium | mitigated | `setSecurityHeaders`: X-Frame-Options DENY, CSP frame-ancestors none, script-src self, nosniff, Referrer-Policy same-origin, X-Robots-Tag noindex. The index has no inline script | `TestSecurityHeadersOnEveryResponse`, `TestNoInlineScript`, `TestPhase10BoardwalkServing/HEAD…`; stage `--security` | pass | None known | -| T-10-05 | Spoofing | admin session cookie attributes and logout | high | mitigated | `cabana/auth.go` `sessionCookie`: HttpOnly, Secure (opt-out refused in production), SameSite=Strict, Path=prefix, Max-Age=refresh TTL. `logout` blacklists the jti and expires the cookie | `TestPhase10CookieAuth`, `TestPhase10Coverage/cookie refresh…` (attributes on refresh), `TestPhase10AdminAuth` (fonoteka), `TestPhase10AssembledAcceptance` (old cookie is 401 after logout), `TestPhase10Prefix` (cookie_secure false refused in production); stages `--security`, `--postgres` | pass; removal checks (HttpOnly off, SameSite Lax, blacklist skipped) each fail | A stolen cookie is valid until logout or expiry, the same window as a Bearer token | +| T-10-05 | Spoofing | admin session cookie attributes and logout | high | mitigated | `cabana/auth.go` `sessionCookie`: HttpOnly, Secure (opt-out refused in production), SameSite=Strict, Path=prefix, Max-Age=refresh TTL. `logout` blacklists the jti and expires the cookie. `refresh` loads the admin through the guard's provider and refuses a missing, deactivated or pre-cutoff subject via `bouncer.RefreshAudienceFor`, expiring the cookie on that refusal | `TestPhase10CookieAuth`, `TestPhase10Coverage/cookie refresh…` (attributes on refresh), `TestPhase10AdminAuth` (fonoteka), `TestPhase10AssembledAcceptance` (old cookie is 401 after logout), `TestPhase10Prefix` (cookie_secure false refused in production), `TestAdminRefreshRevocation` (cabana, Postgres), `TestRefreshAudienceForSubject` (bouncer); stages `--security`, `--postgres` | pass; removal checks (HttpOnly off, SameSite Lax, blacklist skipped) each fail | A stolen cookie or Bearer token stays usable until logout, `summer admin:reset-password` (tokens_valid_after), deactivation or deletion of the admin, or the end of its refresh window. Before quick task 260927-q23 (CR-01), refresh skipped the tokens_valid_after and is_activated checks, so a reset did not end a session the SPA kept refreshing. The refresh window slides on every refresh, so a session refreshed at least once per refresh_ttl has no absolute expiry (WR-07, open) | | T-10-06 | Elevation of Privilege | prefix and controller ID collisions | medium | mitigated | `cabana.AdminPrefix` validation, `checkReservedSegments`, and `surf.checkAdminPrefix` rejecting non-cabana routes at or under the prefix | `TestPhase10Prefix`, `TestPhase10AdminPrefixCollision` (exact, deeper, raw, default `/backend`, sibling allowed); stage `--security` | pass | None known | | T-10-07 | Denial of Service | concurrent cookie refresh from two tabs | low | mitigated | fonoteka `blacklist_grace: 30`. `client.ts` single-flights one refresh and replays once | `tests/app/client.test.ts` (single-flight, one replay), `tests/smoke/tracer.smoke.test.ts`; stage `--spa` | pass | Two tabs refreshing more than 30 s apart with the same old cookie: the second tab re-logs in | | T-10-08 | Tampering | committed dist and generated types drift from source | medium | mitigated | `scripts/check-admin-dist.sh` rebuilds from the lockfile. `scripts/check-admin-openapi.sh --check` regenerates the document and types. Tailwind skips `admin/tests` and the generated files | stages `--dist`, `--openapi` | pass | None known | diff --git a/.planning/quick/260927-q23-fix-cr-01-auth-refresh-must-enforce-toke/260927-q23-PLAN.md b/.planning/quick/260927-q23-fix-cr-01-auth-refresh-must-enforce-toke/260927-q23-PLAN.md new file mode 100644 index 0000000..465ebb1 --- /dev/null +++ b/.planning/quick/260927-q23-fix-cr-01-auth-refresh-must-enforce-toke/260927-q23-PLAN.md @@ -0,0 +1,303 @@ +--- +phase: quick-260927-q23 +plan: 01 +type: execute +wave: 1 +depends_on: [] +files_modified: + - bouncer/jwt.go + - bouncer/refresh.go + - bouncer/refresh_test.go + - bouncer/jwt_guard_test.go + - cabana/http.go + - cabana/auth.go + - cabana/refresh_revocation_test.go + - cabana/phase10_coverage_test.go + - .planning/phases/10-admin-vue-spa/10-SECURITY-REVIEW.md + - .planning/phases/10-admin-vue-spa/10-REVIEW-DISPOSITION.md +autonomous: true +requirements: [ADMIN-06] + +estimate: + tokens: 70000 + raw_tokens: 70000 + tasks: 3 + confidence: low + +must_haves: + truths: + - "After `summer admin:reset-password`, POST {prefix}/api/v1/auth/refresh with a token issued before the reset answers 401 with error.code unauthenticated and mints no token, over both cookie and Bearer transport" + - "A cookie-transport refresh refused because the admin is missing, soft-deleted, deactivated or cut off by tokens_valid_after sends Set-Cookie summer_admin with an empty value, Max-Age below 0 and Path equal to the admin prefix" + - "A Bearer-transport refresh never sets or expires the summer_admin cookie, including when it is refused" + - "An active admin whose token postdates tokens_valid_after still refreshes over cookie and Bearer with the unchanged Phase 10 response shapes" + - "bouncer.Refresh (the golem15/user frontend refresh) and the JWT guard keep their exact behaviour, order of checks and error messages" + artifacts: + - path: "bouncer/refresh.go" + provides: "RefreshAudienceFor: RefreshAudience plus the guard's subject checks before minting" + contains: "func RefreshAudienceFor" + - path: "bouncer/jwt.go" + provides: "ErrSubjectRejected, subjectPrincipal and issuedBeforeCutoff shared by the JWT guard and RefreshAudienceFor" + contains: "ErrSubjectRejected" + - path: "cabana/auth.go" + provides: "admin refresh handler using RefreshAudienceFor and expiring the cookie on a rejected subject" + contains: "RefreshAudienceFor(r.Context(), s.users" + - path: "cabana/refresh_revocation_test.go" + provides: "Postgres regression test for CR-01 using the real admin:reset-password command" + contains: "TestAdminRefreshRevocation" + key_links: + - from: "cabana/http.go" + to: "bouncer.NewBackendJWTGuard and service.users" + via: "one lazyBackendUsers value passed to both" + pattern: "users:\\s+users" + - from: "cabana/auth.go (*service).refresh" + to: "bouncer.RefreshAudienceFor" + via: "s.users provider" + pattern: "RefreshAudienceFor\\(r\\.Context\\(\\), s\\.users" + - from: "bouncer/jwt.go jwtGuard.Authenticate" + to: "subjectPrincipal and issuedBeforeCutoff" + via: "shared helper, same order and messages as before" + pattern: "subjectPrincipal\\(r\\.Context\\(\\), g\\.users" + - from: "bouncer/refresh.go Refresh" + to: "refreshAudience" + via: "nil subject check, so the golem15/user refresh contract is unchanged" + pattern: "AudienceUser, true, refreshTTL, bl, grace, issuerURL, nil\\)" +--- + + +Fix review finding CR-01 (Phase 10, 10-REVIEW.md): `POST {prefix}/api/v1/auth/refresh` (`cabana/auth.go` `(*service).refresh`) calls `bouncer.RefreshAudience`, which checks only signature, audience, refresh window and blacklist, then mints a token with iat=now. It never loads the admin, so the backend guard's `tokens_valid_after` cutoff (`bouncer/jwt.go` line 144) and the `is_activated` check are skipped. With the SPA refreshing on every 401 (`admin/src/api/client.ts`), a copied `summer_admin` cookie survives `summer admin:reset-password` for up to refresh_ttl (14 days). That breaks Phase 9 truth T-09-04. + +After this plan, refresh loads the subject through the same provider and the same check helpers the guard uses, and refuses (same 401 `unauthenticated` body) a missing, soft-deleted, deactivated or pre-cutoff subject before minting. A refusal of that kind over cookie transport also expires the cookie. + +Purpose: restore session revocation for the admin SPA without touching the golem15/user frontend refresh (PHP-parity contract) or the guard's behaviour. +Output: bouncer helper extraction plus `RefreshAudienceFor`, the cabana wiring, Postgres and no-Docker regression tests, and corrected (uncommitted) Phase 10 planning docs. + +Scope notes (decisions made for this quick task): +- Callers checked: `bouncer.RefreshAudience` has one caller (cabana admin refresh). `bouncer.Refresh` is used by `../fonoteka.go/plugins/golem15/user/controllers/api_controller.go` (frontend user JWT refresh). Both share the unexported `refreshAudience`, so the new check is an optional hook that `Refresh` and `RefreshAudience` pass as nil. Both exported functions keep their signatures and behaviour (the framework is a shared dependency; no breaking change). +- Cookie expiry is limited to subject refusals (`ErrSubjectRejected`). Other refresh failures (no token, bad signature, wrong audience, outside the refresh window, blacklisted, provider error) keep today's no-Set-Cookie behaviour. `TestPhase10Coverage` pins "no cookies" for the out-of-window case, and the general stale-cookie cleanup is the separate open finding WR-01. A provider (DB) error answers 401 without expiring the cookie, so a transient lookup failure does not end the browser session. +- Out of scope: WR-01 (logout behind the guard) and WR-07 (sliding refresh window). The corrected residual-risk text names WR-07. +- fonoteka.go needs no change. Task 3 proves its tests stay green. + +Source coverage (orchestrator requirements -> task): +| Item | Task | +|---|---| +| Load the old token's `sub` before minting and apply the guard's checks (missing, not activated, old iat before tokens_valid_after), 401 with the same error shape | 1 | +| Expire summer_admin on refusal over cookie transport | 1 (behaviour), 2 (unit pins) | +| Reuse the guard's lookup and check logic, do not duplicate | 1 (shared `lazyBackendUsers` value, extracted `subjectPrincipal`/`issuedBeforeCutoff`) | +| Do not change the user plugin's refresh route or PHP-parity contract | 1 (nil hook), 2 (unchanged `TestRefresh*`), 3 (fonoteka user plugin tests) | +| Tests written red first: pre-reset token gives 401 and an expired cookie; deactivated user gives 401; the normal path still works | 1 | +| Correct the T-10-05 residual-risk line and set the CR-01 disposition row to fixed with a commit reference, uncommitted | 3 | +| fonoteka.go stays green | 3 | + + + +@~/.claude/gsd-core/workflows/execute-plan.md +@~/.claude/gsd-core/templates/summary.md + + + +@.planning/STATE.md +@CLAUDE.md +@.planning/phases/10-admin-vue-spa/10-REVIEW.md +@cabana/auth.go +@bouncer/refresh.go +@bouncer/jwt.go + + +Current code the executor works against (extracted, do not re-explore): + +bouncer/refresh.go +- Refresh(secret, tokenString string, refreshTTL time.Duration, bl BlacklistStore, grace time.Duration, issuerURL string) (string, error): calls refreshAudience(..., AudienceUser, true, ...). Used by fonoteka.go's golem15/user plugin. MUST stay behaviourally identical. +- RefreshAudience(secret, tokenString, audience string, refreshTTL, bl, grace, issuerURL) (string, error): calls refreshAudience(..., audience, false, ...). Keep exported and unchanged. +- refreshAudience order today: parse HS256 without claims validation, subject(claims), audience check, iat window (`iat + refreshTTL`), IsBlacklisted(old jti), exp/ttl claim check, MintAudience(secret, sub, issuerURL, ttl, audience), then bl.Add(old jti, later of iat+refreshTTL+1m and exp+1m, now+grace). + +bouncer/jwt.go +- type UserProvider interface { FindByID(ctx context.Context, id uint) (*Principal, error) } +- jwtGuard.Authenticate order today: extractToken, VerifyClaimsAudience or VerifyClaims, strconv.ParseUint(sub) (err or 0 gives errors.New(msgUserNotFound)), users nil gives msgUserNotFound, users.FindByID (err gives errors.New("Authentication error"), nil user gives msgUserNotFound), IsBlacklisted(jti) (err gives "Authentication error", blocked gives msgBadSignature), then `!user.TokensValidAfter.IsZero() && iat.Before(user.TokensValidAfter)` gives msgUserNotFound. +- msgUserNotFound = "User not found". The package-level func Middleware (legacy frontend middleware) is out of scope. Leave it as is. + +bouncer/context.go +- type Principal struct { ID uint; MustChangePassword bool; PreferredLocale string; TokensValidAfter time.Time; Backend, IsSuperuser bool; PermissionGrants map[string]bool } + +cabana/auth.go +- BackendUsers.FindByID returns (nil, nil) for id 0, a missing or soft-deleted row, or `!user.IsActivated`, and fills Principal.TokensValidAfter from the column. +- (*service).refresh (lines 217-241): sessionToken(r) returns (raw, fromCookie). Bearer wins over the cookie. On any error it calls logAuth(r, "failed", 0) and WriteError(w, 401, "unauthenticated", msgUnauthenticated). On success, cookie transport calls writeSessionCookie plus cookieLoginData(s.ttl), and Bearer returns {access_token, token_type: bearer}. +- (*service).expireSessionCookie(w) sets summer_admin with value "", MaxAge -1, Path adminPrefix(). + +cabana/http.go +- service struct (lines 32-49) has no user provider field. +- line 112: guard := bouncer.NewBackendJWTGuard(secret, lazyBackendUsers{app: app, reg: reg}, bl, writeUnauthenticated, AdminCookieName) +- lazyBackendUsers.FindByID (lines 171-180) resolves *gorm.DB from app and delegates to BackendUsers{DB, Registry}.FindByID. + +cabana/commands.go adminResetPassword: sets password and tokens_valid_after = time.Now().UTC().Add(time.Second). + +Test helpers (package cabana_test): adminGorm(t) (shared testcontainers Postgres, skips only under -short), adminHandler(t, gdb, nil), insertAdmin(t, gdb, login, email, password, activated, deleted) cabana.BackendUser, adminTestPassword, postJSON, postAuth(t, h, method, path, token, body), getAuth, accessToken, adminAPI(rel), phase10Send(t, h, method, path, body, cookie, ajax), phase10Cookie(t, rec, path), phase10AssertCookieBody, phase10AssertBearerBody, phase10ErrorCode, commandApp(t, gdb), commandByName(t, cabana.RuntimeCommands(app), "admin:reset-password"), flagInput{args, flags}, bonfire.NewOutput(nil, &buf, &buf). The logout cookie assertion pattern is in cabana/phase10_auth_test.go lines 84-96. +Test helpers (package bouncer): memUsers{byID, err} in jwt_test.go, sign(t, method, claims, key), decodeClaims, claimUnix, const secret, NewMemoryBlacklist(). +Test helpers (package cabana): the "cookie refresh of an expired access token inside the refresh window" subtest in cabana/phase10_coverage_test.go (lines 276-365) builds a bare &service{secret, ttl, refreshTTL, issuer, bl} with no app and signs tokens for sub "5". + + + + + + + Task 1: End-to-end: a pre-reset, deactivated or deleted admin token cannot be refreshed (cabana refresh, bouncer.RefreshAudienceFor, shared guard provider) + cabana/refresh_revocation_test.go, bouncer/jwt.go, bouncer/refresh.go, cabana/http.go, cabana/auth.go, cabana/phase10_coverage_test.go + + - cabana/auth.go lines 31-76 (BackendUsers.FindByID, principalFrom), 193-241 (cookie helpers, refresh) + - cabana/http.go lines 32-49 (service struct), 105-180 (guard wiring, service literal, lazyBackendUsers) + - bouncer/refresh.go (all), bouncer/jwt.go lines 1-160 (UserProvider, jwtGuard.Authenticate) + - cabana/commands_test.go lines 86-160 (TestAdminResetPasswordCommand, commandByName, commandApp, flagInput) + - cabana/phase10_auth_test.go lines 27-100 and 211-300 (cookie flow and helpers) + - cabana/phase10_coverage_test.go lines 276-365 + + + New file cabana/refresh_revocation_test.go, package cabana_test, one test TestAdminRefreshRevocation with one t.Run per case. Each case uses its own admin with a unique neutral login prefixed "rrev-" (adminGorm shares one database across the package, and the hygiene gate forbids app names in framework tests): + - "pre-reset cookie is refused and expired": log in over cookie transport (phase10Send with ajax true), run the real admin:reset-password command (commandByName over cabana.RuntimeCommands(commandApp(t, gdb)), flagInput args = the login, flags password = a new value). Then GET /auth/me with the old cookie gives 401. POST /auth/refresh with the old cookie and X-Requested-With gives 401 with phase10ErrorCode "unauthenticated", and the response carries a summer_admin cookie with Value "", MaxAge below 0 and Path cabana.DefaultAdminPrefix. + - "pre-reset bearer is refused without cookies": log in over Bearer before a reset, reset, then postAuth refresh with the Bearer token gives 401 unauthenticated and no Set-Cookie header at all. + - "deactivated admin is refused": log in over cookie, UPDATE backend_users SET is_activated = false for that id, then cookie refresh gives 401 plus the expiring cookie. + - "soft-deleted admin is refused": log in over cookie, gdb.Delete(&user), then cookie refresh gives 401 plus the expiring cookie. + - "active admin still refreshes": cookie refresh gives 200, phase10Cookie returns a rotated value, phase10AssertCookieBody passes, and GET /auth/me with the rotated cookie gives 200. Bearer refresh gives 200 and phase10AssertBearerBody passes. + Do NOT add a "log in again after the reset and refresh" step. adminResetPassword sets the cutoff to now+1s, so a token minted within that second is correctly refused, and the step would be flaky. + + + RED (per the orchestrator's "write them red first"): create only cabana/refresh_revocation_test.go as specified in behavior, then run `go test ./cabana -run '^TestAdminRefreshRevocation$' -count=1 -v`. Expect the four refusal subtests to fail with status 200 and "active admin still refreshes" to pass. Copy the failing lines into the SUMMARY. Do not commit the red state: the project rule is that `go vet` and `go test ./...` are green at every commit. + + GREEN, in this order: + + 1. bouncer/jwt.go: extract the guard's subject logic into shared helpers without changing the guard's behaviour. Add an exported sentinel `ErrSubjectRejected` whose message is exactly msgUserNotFound ("User not found"). Doc: the token's subject is not a loadable user (bad sub, nil provider, provider returned nil for a missing, deleted or not-activated user), or the token was issued before Principal.TokensValidAfter. Add an unexported `errAuthentication` with the message "Authentication error". Add unexported `subjectPrincipal(ctx context.Context, users UserProvider, sub string) (*Principal, error)`: strconv.ParseUint failure or 0 gives ErrSubjectRejected, nil users gives ErrSubjectRejected, a FindByID error gives errAuthentication, and a nil principal gives ErrSubjectRejected. Add unexported `issuedBeforeCutoff(user *Principal, iat time.Time) bool`, true when TokensValidAfter is non-zero and iat is before it. Rewrite jwtGuard.Authenticate to call subjectPrincipal(r.Context(), g.users, sub), then the unchanged blacklist block, then return ErrSubjectRejected when issuedBeforeCutoff(user, iat). The order and every error message stay identical. Leave the package-level Middleware func untouched. + + 2. bouncer/refresh.go: give the unexported refreshAudience a trailing parameter `check func(sub string, iat time.Time) error`. Call it, when non-nil, after the blacklist lookup and the exp/ttl claim check and immediately before MintAudience. A refused subject then neither mints a token nor blacklists the old jti, and all token-only checks run before any DB lookup. Refresh and RefreshAudience pass nil. Their signatures, docs and behaviour stay unchanged, which keeps the golem15/user frontend refresh (PHP-parity contract) as it is. Add exported `RefreshAudienceFor(ctx context.Context, users UserProvider, secret, tokenString, audience string, refreshTTL time.Duration, bl BlacklistStore, grace time.Duration, issuerURL string) (string, error)`. It rejects an empty audience like RefreshAudience, requires aud (allowMissing false), and passes a check that returns subjectPrincipal's error as is and returns ErrSubjectRejected when issuedBeforeCutoff. The doc comment says it applies the JWT guard's subject checks before minting, and that only subject refusals match errors.Is(err, ErrSubjectRejected). + + 3. cabana/http.go: add field `users bouncer.UserProvider` to service, commented as the same provider the backend guard uses. In the constructor build one `users := lazyBackendUsers{app: app, reg: reg}`, pass it to bouncer.NewBackendJWTGuard, and set `users: users` in the service literal. The guard and refresh then share one lookup (is_activated, soft delete, role grants, tokens_valid_after), with no duplicated query. + + 4. cabana/auth.go refresh: call bouncer.RefreshAudienceFor(r.Context(), s.users, s.secret, raw, bouncer.AudienceBackend, s.refreshTTL, s.bl, s.grace, s.issuer). On error: when fromCookie is true and errors.Is(err, bouncer.ErrSubjectRejected), call s.expireSessionCookie(w) before writing the body. Then keep the existing logAuth(r, "failed", 0) and the same WriteError 401 "unauthenticated" msgUnauthenticated. Bearer transport never touches the cookie. The success path and the empty-token path stay unchanged. Do not edit any swag annotation comment, so the admin OpenAPI document cannot drift. + + 5. cabana/phase10_coverage_test.go: the "cookie refresh of an expired access token inside the refresh window" subtest builds a bare service, which now needs a provider or refresh fails closed. Define a small map-backed test provider type in that file (for example `refreshSubjects`, implementing FindByID with an optional error field for Task 2), and set `users:` to it with principal ID 5 (Backend true). Change no assertion. + + Then run the verify command and commit code and tests together as `fix(cabana): enforce tokens_valid_after and is_activated on admin refresh`, with a body that names CR-01 and T-09-04. No co-author trailer (user and project rule). + + + cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && go vet ./bouncer ./cabana && go test ./bouncer ./cabana -count=1 -run 'TestAdminRefreshRevocation|TestPhase10Coverage|TestPhase10CookieAuth|TestAdminAuthLifecycle|TestAdminBlacklist|TestAdminInactive|TestAdminResetPasswordCommand|TestPhase10OpenAPIConformance|TestJWTGuard|TestRefresh|TestPhase09GuardIsolation|TestPhase10CookieGuard|TestMintRefreshBlacklistRoundTrip' + + + - The red run was observed and recorded: the four refusal subtests failed with status 200 before the fix. + - TestAdminRefreshRevocation passes on Postgres (not skipped). Every pre-existing test in the verify regex passes unchanged. + - `grep -n "RefreshAudienceFor(r.Context(), s.users" cabana/auth.go` and `grep -n "users:\s*users" cabana/http.go` both match. `grep -n "subjectPrincipal(r.Context(), g.users" bouncer/jwt.go` matches. `grep -n "issuerURL, nil)" bouncer/refresh.go` matches twice (Refresh and RefreshAudience). + - One commit `fix(cabana): ...` holds code and tests, with no co-author trailer. + + + + + Task 2: Unit coverage (no Docker) for the refresh subject checks and the shared guard helper + bouncer/refresh_test.go, bouncer/jwt_guard_test.go, cabana/phase10_coverage_test.go + + - bouncer/refresh_test.go (existing TestRefresh* style), bouncer/jwt_test.go lines 15-40 (memUsers, sign) + - bouncer/jwt_guard_test.go lines 96-130 (TestJWTGuardTokensValidAfter) + - cabana/phase10_coverage_test.go lines 276-365 (the refresh subtest and the provider type added in Task 1) + + + bouncer/refresh_test.go, new TestRefreshAudienceForSubject. Tokens are signed with aud backend, a jti, and iat 10 minutes ago. The providers are memUsers plus a small counting provider defined in the test: + - Active principal with no cutoff: returns a token with sub preserved and aud backend, and the old jti is blacklisted (grace 0). + - Cutoff 1 minute after the token's iat: errors.Is(err, ErrSubjectRejected), and the old jti is NOT blacklisted. + - Cutoff 1 second before iat: succeeds. + - Id absent from memUsers (models missing or not activated): ErrSubjectRejected, not blacklisted. + - nil users: ErrSubjectRejected. + - Non-numeric sub: ErrSubjectRejected. + - Provider returning an error: err non-nil, NOT errors.Is ErrSubjectRejected, and not blacklisted. + - Token outside the refresh window, frontend-audience token, wrong-secret token, or already-blacklisted jti: each errors and the counting provider records zero FindByID calls (token checks run before the lookup). + - Empty audience: errors. + bouncer/jwt_guard_test.go TestJWTGuardTokensValidAfter: add assertions that the cutoff refusal's err.Error() is still "User not found" and errors.Is(err, ErrSubjectRejected). This pins that the guard's message did not change through the helper extraction. + cabana/phase10_coverage_test.go: a new TestPhase10Coverage subtest "refresh enforces the guard's subject checks" with a bare service and the Task 1 provider, no Postgres, so it runs under -short: + - Cookie token with iat before the principal's TokensValidAfter: 401, phase10-style error code unauthenticated, and a summer_admin cookie with Value "", MaxAge below 0 and Path DefaultAdminPrefix. + - Cookie token for a sub with no principal: 401 plus the expiring cookie. + - Bearer token before the cutoff: 401 and zero cookies. + - Provider configured to return an error: 401 and zero cookies (a transient lookup failure does not end the browser session). + - Cookie token after the cutoff: 200 with a rotated cookie. + + + Write the tests in behavior against the Task 1 code. They are expected to pass on first run because Task 1 implemented the behaviour. Prove they would catch a regression with a removal check: temporarily edit RefreshAudienceFor in bouncer/refresh.go so it passes nil instead of its check to refreshAudience, then run `go test ./bouncer ./cabana -short -count=1 -run 'TestRefreshAudienceForSubject|TestPhase10Coverage'`. Confirm that the cutoff, missing, nil-users and non-numeric cases and the cabana cookie-expiry cases fail. Restore with `git checkout -- bouncer/refresh.go` (committed in Task 1, so this is safe) and re-run until green. Record the removal-check result in the SUMMARY. Use only the stdlib testing package, following the existing plain-`t.Fatalf` style in these files (no testify is imported there). Commit as `test(bouncer,cabana): cover admin refresh subject checks without a database`, with no co-author trailer. + + + cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && go vet ./bouncer ./cabana && go test ./bouncer ./cabana -short -count=1 -run 'TestRefreshAudienceForSubject|TestJWTGuardTokensValidAfter|TestPhase10Coverage|TestRefresh' -v + + + - TestRefreshAudienceForSubject and TestJWTGuardTokensValidAfter pass. The new TestPhase10Coverage subtest passes under -short. + - The removal check was run and recorded: with the check dropped, the new cases fail. After restore, `git diff --quiet -- bouncer/refresh.go` holds. + - One commit `test(bouncer,cabana): ...`, with no co-author trailer. + + + + + Task 3: Cross-repo gate and planning doc corrections (edited, not committed) + .planning/phases/10-admin-vue-spa/10-SECURITY-REVIEW.md, .planning/phases/10-admin-vue-spa/10-REVIEW-DISPOSITION.md + + - .planning/phases/10-admin-vue-spa/10-SECURITY-REVIEW.md lines 14-21 (table header and the T-10-05 row) + - .planning/phases/10-admin-vue-spa/10-REVIEW-DISPOSITION.md (the CR-01 row) + - scripts/check-phase10.sh lines 244-267 and 365-400 (the --go, --security, --postgres and --evidence stages) + + + 1. Confirm that no dependency changed: `git show --stat` of both task commits lists neither go.mod nor go.sum. Confirm that this task wrote nothing in ../fonoteka.go. + + 2. Run `bash scripts/check-phase10.sh --go` from summercms.go. It runs go vet and the full go test (including testcontainers) for summercms.go, and for fonoteka.go including ./plugins/golem15/user/... (the golem15/user refresh that shares bouncer.Refresh) and ./plugins/golem15/fonoteka/... (the assembled admin refresh tests). It accepts only the two documented parity failures. It must exit 0. Use a 600000 ms timeout or run it in the background. + + 3. Edit 10-SECURITY-REVIEW.md, T-10-05 row only. Keep exactly nine cells and use no pipe characters inside a cell (the --evidence parser reads the row). + - Residual-risk cell: replace "A stolen cookie is valid until logout or expiry, the same window as a Bearer token" with text stating these points. A stolen cookie or Bearer token stays usable until logout, `summer admin:reset-password` (tokens_valid_after), deactivation or deletion of the admin, or the end of its refresh window. Before quick task 260927-q23 (CR-01), refresh skipped the tokens_valid_after and is_activated checks, so a reset did not end a session the SPA kept refreshing. The refresh window slides on every refresh, so a session refreshed at least once per refresh_ttl has no absolute expiry (WR-07, open). + - Production-mitigation cell: append that `refresh` loads the admin through the guard's provider and refuses a missing, deactivated or pre-cutoff subject via `bouncer.RefreshAudienceFor`, expiring the cookie on that refusal. + - Test cell: append `TestAdminRefreshRevocation` (cabana, Postgres) and `TestRefreshAudienceForSubject` (bouncer). + + 4. Edit 10-REVIEW-DISPOSITION.md, CR-01 row only. Set Disposition to `fixed`. Set the Note to "Fixed in (tests ), quick 260927-q23", followed by one sentence: admin refresh now applies the backend guard's subject checks (activated, not deleted, iat not before tokens_valid_after) via bouncer.RefreshAudienceFor before minting, and a refused cookie refresh expires summer_admin. Take the hashes from `git log --format=%h -2`. + + 5. Do NOT commit either doc and do NOT touch STATE.md, ROADMAP.md, 10-VERIFICATION.md or any other planning file. The orchestrator commits planning docs separately (project rule: planning docs and code in separate commits). + + 6. Run `bash scripts/check-phase10.sh --evidence`. It parses the review table, then runs the --security, --postgres and --openapi stages. It must exit 0, which also proves that the admin OpenAPI document and types did not drift. + + + cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && bash scripts/check-phase10.sh --go && bash scripts/check-phase10.sh --evidence && grep -n '^| CR-01 | critical | fixed |' .planning/phases/10-admin-vue-spa/10-REVIEW-DISPOSITION.md && grep -n '^| T-10-05 ' .planning/phases/10-admin-vue-spa/10-SECURITY-REVIEW.md | grep -c 'RefreshAudienceFor' && git status --porcelain .planning/phases/10-admin-vue-spa + + + - `check-phase10.sh --go` and `check-phase10.sh --evidence` both exit 0. fonoteka.go's user and fonoteka plugin tests are green with no fonoteka.go change. + - The T-10-05 row keeps nine cells, names RefreshAudienceFor, TestAdminRefreshRevocation and WR-07, and no longer claims "valid until logout or expiry". + - The CR-01 row reads `fixed` and cites the Task 1 and Task 2 commit hashes. + - `git status --porcelain` shows both docs as modified and uncommitted. go.mod and go.sum are untouched. + + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| browser or Bearer client to POST {prefix}/api/v1/auth/refresh | Untrusted token (cookie or Authorization header) is exchanged for a new admin session | +| cabana refresh to backend_users (Postgres) | Subject state (is_activated, deleted_at, tokens_valid_after) decides whether a session may continue | +| framework bouncer to fonoteka.go golem15/user plugin | Shared refresh code; the frontend refresh contract must not change | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-q23-01 | Elevation of Privilege | cabana refresh after admin:reset-password | high | mitigate | RefreshAudienceFor refuses a token whose iat is before tokens_valid_after before minting; cookie refusal expires summer_admin (Task 1, TestAdminRefreshRevocation with the real reset command; Task 2 removal check) | +| T-q23-02 | Elevation of Privilege | cabana refresh for deactivated or deleted admin | high | mitigate | Refresh loads the subject through the guard's lazyBackendUsers provider, which returns nil for !is_activated and soft-deleted rows, mapped to ErrSubjectRejected (Task 1 deactivated and soft-deleted subtests) | +| T-q23-03 | Tampering | bouncer.Refresh used by golem15/user (PHP-parity) and the shared JWT guard | medium | mitigate | Subject check is an optional hook passed as nil by Refresh and RefreshAudience; guard helper extraction keeps order and messages; existing TestRefresh*, TestJWTGuard* and fonoteka user plugin tests run in Task 1 and Task 3 | +| T-q23-04 | Denial of Service | DB lookup added to refresh | low | mitigate | The lookup runs only after signature, audience, refresh-window and blacklist checks pass, so unsigned or garbage tokens never reach Postgres (Task 2 counting-provider cases) | +| T-q23-05 | Denial of Service | transient provider error during refresh | low | accept | Answers 401 like the guard but does not expire the cookie, so a DB blip does not end the browser session; the SPA retries after re-login at worst | +| T-q23-SC | Tampering | module and npm installs | low | accept | No new dependency; Task 3 confirms go.mod and go.sum are unchanged by both task commits | + + + +- Task 1 red run observed (refusal subtests returned 200 before the fix), then green on Postgres. +- `go vet ./...` and `go test ./...` are green in summercms.go at both task commits (the Task 3 `--go` stage re-proves this for the final state, plus fonoteka.go). +- Removal check recorded: dropping the subject check fails the new unit tests. +- `scripts/check-phase10.sh --evidence` exits 0 after the doc edits (table integrity, security, postgres, openapi drift). + + + +- A token issued before `summer admin:reset-password` can no longer be refreshed over cookie or Bearer. The SPA's refresh-on-401 now ends at the login screen, and the stale cookie is expired. +- A deactivated or deleted admin cannot mint new tokens through refresh. +- The normal refresh path, the golem15/user frontend refresh and the JWT guard behave exactly as before. +- CR-01 is recorded as fixed with commit references, and the T-10-05 residual risk is stated accurately. Both docs are left uncommitted for the orchestrator. + + + +Create `.planning/quick/260927-q23-fix-cr-01-auth-refresh-must-enforce-toke/260927-q23-SUMMARY.md` when done. Include the red-run failure lines from Task 1, the removal-check result from Task 2, both commit hashes, and the gate outputs from Task 3. + \ No newline at end of file diff --git a/.planning/quick/260927-q23-fix-cr-01-auth-refresh-must-enforce-toke/260927-q23-SUMMARY.md b/.planning/quick/260927-q23-fix-cr-01-auth-refresh-must-enforce-toke/260927-q23-SUMMARY.md new file mode 100644 index 0000000..6556a21 --- /dev/null +++ b/.planning/quick/260927-q23-fix-cr-01-auth-refresh-must-enforce-toke/260927-q23-SUMMARY.md @@ -0,0 +1,121 @@ +--- +phase: quick-260927-q23 +plan: 01 +subsystem: auth +tags: [bouncer, cabana, jwt, refresh, session-revocation, CR-01] +status: complete +requires: [] +provides: + - "bouncer.RefreshAudienceFor: RefreshAudience plus the JWT guard's subject checks before minting" + - "bouncer.ErrSubjectRejected sentinel shared by the guard and refresh" +affects: [cabana admin refresh, backend JWT guard] +tech-stack: + added: [] + patterns: + - "Optional check hook on the shared refreshAudience flow (nil for the golem15/user and RefreshAudience paths)" + - "One lazyBackendUsers provider shared by the backend guard and the refresh handler" +key-files: + created: + - cabana/refresh_revocation_test.go + modified: + - bouncer/jwt.go + - bouncer/refresh.go + - bouncer/refresh_test.go + - bouncer/jwt_guard_test.go + - cabana/http.go + - cabana/auth.go + - cabana/phase10_coverage_test.go + - .planning/phases/10-admin-vue-spa/10-SECURITY-REVIEW.md (uncommitted) + - .planning/phases/10-admin-vue-spa/10-REVIEW-DISPOSITION.md (uncommitted) +decisions: + - "Cookie expiry on refresh is limited to subject refusals (ErrSubjectRejected); a provider error answers 401 without expiring the cookie" + - "Subject lookup runs after all token-only checks, so unsigned, expired-window, wrong-audience or blacklisted tokens never reach Postgres" + - "bouncer.Refresh and bouncer.RefreshAudience keep their signatures and behaviour (nil hook); fonoteka.go unchanged" +metrics: + duration: 13min + completed: 2026-09-27 +actuals: + tokens: 7000 + tasks: 3 + commits: 2 +plan_head_before: 815cb903c684b25dca4eda81e61800c6351430f3 +plan_head_after: a13a1214cbbaffb2ff655d1072e4a5558e5ff88c +--- + +# Quick 260927-q23: Admin refresh enforces tokens_valid_after and is_activated (CR-01) Summary + +Admin `POST {prefix}/api/v1/auth/refresh` now loads the subject through the backend guard's own provider (`lazyBackendUsers`) and applies the guard's checks (`subjectPrincipal`, `issuedBeforeCutoff`) via the new `bouncer.RefreshAudienceFor` before it mints a token. A refresh refused for its subject over cookie transport expires `summer_admin`. So `summer admin:reset-password`, deactivation and soft-deletion now end a session the SPA keeps refreshing. + +## Commits + +| Task | Commit | Message | +|------|--------|---------| +| 1 | be4a923 | fix(cabana): enforce tokens_valid_after and is_activated on admin refresh | +| 2 | a13a121 | test(bouncer,cabana): cover admin refresh subject checks without a database | +| 3 | (none) | Doc edits left uncommitted for the orchestrator | + +## Task 1: RED run (before the fix, not committed) + +`go test ./cabana -run '^TestAdminRefreshRevocation$' -count=1 -v`: + +``` +refresh_revocation_test.go:88: pre-reset bearer refresh status=200 body={"data":{"access_token":"eyJ...","token_type":"bearer"},"meta":{}}, want 401 unauthenticated +refresh_revocation_test.go:103: refresh status=200 body={"data":{"token_type":"cookie","expires_in":3600},"meta":{}}, want 401 unauthenticated +refresh_revocation_test.go:114: refresh status=200 body={"data":{"token_type":"cookie","expires_in":3600},"meta":{}}, want 401 unauthenticated +--- FAIL: TestAdminRefreshRevocation (5.37s) + --- FAIL: TestAdminRefreshRevocation/pre-reset_cookie_is_refused_and_expired (0.20s) + --- FAIL: TestAdminRefreshRevocation/pre-reset_bearer_is_refused_without_cookies (0.21s) + --- FAIL: TestAdminRefreshRevocation/deactivated_admin_is_refused (0.15s) + --- FAIL: TestAdminRefreshRevocation/soft-deleted_admin_is_refused (0.14s) + --- PASS: TestAdminRefreshRevocation/active_admin_still_refreshes (0.21s) +``` + +The pre-reset cookie subtest failed the same way: the refresh returned status 200 with a cookie body. GREEN: all five subtests pass on Postgres. `go vet ./...` and `go test ./...` in summercms.go were green at both commits. + +## Task 2: removal check + +With `RefreshAudienceFor` temporarily passing `nil` instead of its check: + +``` +refresh_test.go:177: pre-cutoff refresh = "eyJ...", ; want ErrSubjectRejected +refresh_test.go:198: missing principal: +refresh_test.go:208: nil users: +refresh_test.go:215: non-numeric sub: +refresh_test.go:223: provider error: , want a non-subject error +--- FAIL: TestRefreshAudienceForSubject (5 subtests) +phase10_coverage_test.go:462: pre-cutoff cookie: status=200 body={"data":{"token_type":"cookie","expires_in":900},"meta":{}} +--- FAIL: TestPhase10Coverage/refresh_enforces_the_guard's_subject_checks +``` + +Restored with `git checkout -- bouncer/refresh.go`. `git diff --quiet -- bouncer/refresh.go` held, and the tests were green again. + +## Task 3: gates + +- `git show --stat` of be4a923 and a13a121 shows no go.mod or go.sum change. `../fonoteka.go` has no working-tree change. +- `bash scripts/check-phase10.sh --go`: the third run exited 0 (`phase10 go passed`, with only the two accepted parity failures TestMigrateSeedsCanonicalGenres and TestSchemaMatchesPHPSnapshot). The first two runs failed on one different fonoteka golem15/user test each: `user/classes TestCodes` the first time, `user TestForgotPassword` the second. Each passed when re-run on its own (TestForgotPassword three times). A full standalone fonoteka.go `go test -json ./... ./plugins/golem15/fonoteka/... ./plugins/golem15/user/...` showed only the two parity failures. Neither test calls refresh or the JWT guard: forgot-password and code issuance are unauthenticated. TestCodes asserts `time.Since(issuedAt) <= 2s` (codes_test.go:37). Load average was 10 to 14 on 12 cores, and another project's testcontainers Postgres was running at the same time. So these look like load flakes and are not caused by this change. See Deferred Issues. +- `bash scripts/check-phase10.sh --evidence`: exit 0 (table parse, security, postgres, openapi. The admin OpenAPI document and fonoteka docs/openapi.json did not drift). +- `10-REVIEW-DISPOSITION.md` CR-01 row: `| CR-01 | critical | fixed | Fixed in be4a923 (tests a13a121), quick 260927-q23. ...` +- `10-SECURITY-REVIEW.md` T-10-05 row: 9 cells. It names RefreshAudienceFor, TestAdminRefreshRevocation, TestRefreshAudienceForSubject and WR-07, and no longer says "valid until logout or expiry". +- Both docs are modified and uncommitted. + +## Deviations from Plan + +None in the code. Test-structure choice: each TestAdminRefreshRevocation subtest builds its own `adminHandler`, so the per-app login throttle (5 per minute) cannot trip across the six logins in the test. + +## Deferred Issues + +- fonoteka.go `plugins/golem15/user/classes TestCodes` and `plugins/golem15/user TestForgotPassword` each failed once, only under the full parallel `check-phase10.sh --go` run on a loaded machine. They pass alone and passed on the third gate run. Likely cause: timing sensitivity under load (for example the 2 s `time.Since` bound in codes_test.go:37). Out of scope here, and there is no code path shared with this change. + +## Known Stubs + +None. + +## Threat Flags + +None. No new endpoint or trust boundary. The refresh endpoint now does one extra indexed primary-key lookup, and only for tokens that passed every signature, audience, window and blacklist check (T-q23-04). + +## Self-Check: PASSED + +- FOUND: cabana/refresh_revocation_test.go +- FOUND: be4a923, a13a121 in git log +- FOUND: `RefreshAudienceFor(r.Context(), s.users` in cabana/auth.go, `users: users` in cabana/http.go, `subjectPrincipal(r.Context(), g.users` in bouncer/jwt.go, `issuerURL, nil)` twice in bouncer/refresh.go