From 299d220b514188399df92af1e21f05a7c0bee868 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 1 Oct 2026 21:23:42 +0200 Subject: [PATCH] fix(09): WR-14 let logout revoke an expired token that is still refreshable and always clear the cookie --- docs/backend/users-and-permissions.md | 2 +- modules/bouncer/README.md | 1 + modules/bouncer/refresh.go | 36 ++++++++++++++++ modules/bouncer/refresh_test.go | 45 ++++++++++++++++++++ modules/cabana/README.md | 2 +- modules/cabana/auth.go | 17 +++++--- modules/cabana/auth_test.go | 54 ++++++++++++++++++++++++ modules/cabana/http.go | 5 ++- modules/cabana/security_coverage_test.go | 4 +- 9 files changed, 157 insertions(+), 9 deletions(-) diff --git a/docs/backend/users-and-permissions.md b/docs/backend/users-and-permissions.md index 94da4e0..a08d9e4 100644 --- a/docs/backend/users-and-permissions.md +++ b/docs/backend/users-and-permissions.md @@ -10,7 +10,7 @@ The admin keeps WinterCMS's backend user model: the `backend_users` and `backend ## Signing in -`POST /api/v1/auth/login` checks the login and password against `backend_users` and issues a JWT for the admin audience, signed with `admin.jwt.secret`. Login attempts are throttled per `admin.login.max_attempts` and `admin.login.decay_minutes`. `POST .../auth/refresh` reissues a token inside the refresh window, and `POST .../auth/logout` revokes the current token by blacklisting its ID in `backend_jwt_blacklist`. +`POST /api/v1/auth/login` checks the login and password against `backend_users` and issues a JWT for the admin audience, signed with `admin.jwt.secret`. Login attempts are throttled per `admin.login.max_attempts` and `admin.login.decay_minutes`. `POST .../auth/refresh` reissues a token inside the refresh window, and `POST .../auth/logout` revokes the current token by blacklisting its ID in `backend_jwt_blacklist`. Logout accepts a token whose access lifetime has run out as long as its refresh window is open, because `.../auth/refresh` would still accept it, and it always clears the session cookie. The admin API accepts the token two ways: diff --git a/modules/bouncer/README.md b/modules/bouncer/README.md index 8a68673..01ebe36 100644 --- a/modules/bouncer/README.md +++ b/modules/bouncer/README.md @@ -88,6 +88,7 @@ func Login(secret string) (string, error) { | `bouncer.VerifyClaimsAudience` | `bouncer.VerifyClaims` with a required audience. | | `bouncer.Refresh` | Reissues a frontend token inside the refresh window and blacklists the old jti. | | `bouncer.RefreshAudience` | Refresh for a token that carries the given audience. | +| `bouncer.VerifyRefreshableClaimsAudience` | Verifies signature and audience without checking `exp`, while the refresh window is open; for revoking a refreshable token on logout. | | `bouncer.RefreshAudienceFor` | `bouncer.RefreshAudience` plus the guard's user checks. | | `bouncer.ErrSubjectRejected` | The token subject is not a loadable user, or the token predates the user's cutoff. | | `bouncer.AudienceUser`, `bouncer.AudienceBackend` | The frontend and admin audience values. | diff --git a/modules/bouncer/refresh.go b/modules/bouncer/refresh.go index 7e21579..cdb4efd 100644 --- a/modules/bouncer/refresh.go +++ b/modules/bouncer/refresh.go @@ -51,6 +51,42 @@ func RefreshAudienceFor(ctx context.Context, users UserProvider, secret, tokenSt return refreshAudience(secret, tokenString, audience, false, refreshTTL, bl, grace, issuerURL, check) } +// VerifyRefreshableClaimsAudience verifies the signature and the required +// audience of a token without checking exp, and accepts it while its refresh +// window (iat plus refreshTTL) is open: exactly the tokens RefreshAudience still +// reissues. It lets a logout revoke a token whose access lifetime has passed but +// which could still be refreshed. It returns the subject, iat, exp and jti; a +// token with no jti, no iat or no exp is refused. +func VerifyRefreshableClaimsAudience(tokenString, secret, audience string, refreshTTL time.Duration) (sub string, iat, exp time.Time, jti string, err error) { + if strings.TrimSpace(audience) == "" { + return "", time.Time{}, time.Time{}, "", errors.New("bouncer: jwt audience is empty") + } + if strings.TrimSpace(secret) == "" { + return "", time.Time{}, time.Time{}, "", errors.New("bouncer: jwt secret is empty") + } + parser := jwt.NewParser(jwt.WithValidMethods([]string{"HS256"}), jwt.WithoutClaimsValidation()) + claims := jwt.MapClaims{} + if _, err := parser.ParseWithClaims(tokenString, claims, func(*jwt.Token) (any, error) { + return []byte(secret), nil + }); err != nil { + return "", time.Time{}, time.Time{}, "", mapJWTError(err) + } + if !audienceMatches(claims, audience) { + return "", time.Time{}, time.Time{}, "", errors.New(msgBadSignature) + } + sub = subject(claims) + jti, _ = claims["jti"].(string) + iat, iatOK := claimTime(claims, "iat") + exp, expOK := claimTime(claims, "exp") + if sub == "" || jti == "" || !iatOK || !expOK { + return "", time.Time{}, time.Time{}, "", errors.New(msgRequiredClaims) + } + if time.Now().After(iat.Add(refreshTTL)) { + return "", time.Time{}, time.Time{}, "", errors.New("Token has expired and can no longer be refreshed") + } + return sub, iat, exp, jti, nil +} + // refreshAudience holds the shared refresh flow. check, when non-nil, runs // after every token-only check and immediately before minting. func refreshAudience(secret, tokenString, audience string, allowMissing bool, refreshTTL time.Duration, bl BlacklistStore, grace time.Duration, issuerURL string, check func(sub string, iat time.Time) error) (string, error) { diff --git a/modules/bouncer/refresh_test.go b/modules/bouncer/refresh_test.go index 04b3b8e..6f0f2e7 100644 --- a/modules/bouncer/refresh_test.go +++ b/modules/bouncer/refresh_test.go @@ -259,3 +259,48 @@ func TestRefreshAudienceForSubject(t *testing.T) { } }) } + +func TestVerifyRefreshableClaimsAudience(t *testing.T) { + now := time.Now() + token := func(method jwt.SigningMethod, key []byte, claims jwt.MapClaims) string { + return sign(t, method, claims, key) + } + base := func() jwt.MapClaims { + return jwt.MapClaims{"sub": "7", "jti": "rj", "aud": AudienceBackend, "iat": now.Add(-10 * time.Minute).Unix(), "exp": now.Add(-time.Minute).Unix()} + } + + sub, iat, exp, jti, err := VerifyRefreshableClaimsAudience(token(jwt.SigningMethodHS256, []byte(secret), base()), secret, AudienceBackend, time.Hour) + if err != nil || sub != "7" || jti != "rj" || !exp.Before(now) || !iat.Before(exp) { + t.Fatalf("expired token inside the refresh window: %q %v %v %q %v", sub, iat, exp, jti, err) + } + // VerifyClaimsAudience rejects the same token: the difference is the point. + if _, _, _, _, err := VerifyClaimsAudience(token(jwt.SigningMethodHS256, []byte(secret), base()), secret, AudienceBackend); err == nil { + t.Fatal("VerifyClaimsAudience accepted an expired token") + } + + tests := map[string]string{ + "wrong secret": token(jwt.SigningMethodHS256, []byte("another-secret-value-for-the-test!"), base()), + "empty token": "", + "window closed": token(jwt.SigningMethodHS256, []byte(secret), func() jwt.MapClaims { + c := base() + c["iat"] = now.Add(-2 * time.Hour).Unix() + return c + }()), + "wrong audience": token(jwt.SigningMethodHS256, []byte(secret), func() jwt.MapClaims { c := base(); c["aud"] = AudienceUser; return c }()), + "missing audience": token(jwt.SigningMethodHS256, []byte(secret), func() jwt.MapClaims { + c := base() + delete(c, "aud") + return c + }()), + "missing jti": token(jwt.SigningMethodHS256, []byte(secret), func() jwt.MapClaims { c := base(); delete(c, "jti"); return c }()), + "missing iat": token(jwt.SigningMethodHS256, []byte(secret), func() jwt.MapClaims { c := base(); delete(c, "iat"); return c }()), + } + for name, raw := range tests { + if _, _, _, _, err := VerifyRefreshableClaimsAudience(raw, secret, AudienceBackend, time.Hour); err == nil { + t.Fatalf("%s: token was accepted", name) + } + } + if _, _, _, _, err := VerifyRefreshableClaimsAudience("x", secret, "", time.Hour); err == nil { + t.Fatal("empty audience was accepted") + } +} diff --git a/modules/cabana/README.md b/modules/cabana/README.md index 62e351e..01fe594 100644 --- a/modules/cabana/README.md +++ b/modules/cabana/README.md @@ -33,7 +33,7 @@ All paths are relative to `/api/v1`. A controller ID `vendor.plugin.cont |-----------------|---------| | POST `/auth/login`, POST `/auth/refresh` | Sign in (throttled) and refresh a token. Public. | | GET `/lang` | The `backend::lang` string bundle for the request locale. Public, so the login screen can load it. | -| POST `/auth/logout`, GET `/auth/me` | Revoke the current token; return the signed-in administrator. | +| POST `/auth/logout`, GET `/auth/me` | Revoke the current token, also when its access lifetime has expired but its refresh window is open, and clear the session cookie; return the signed-in administrator. | | GET `/navigation`, GET `/settings` | Navigation and settings entries the administrator may open. | | GET `/settings/{code}/schema`, GET and PUT `/settings/{code}` | Settings form schema, values and update. | | GET `/{vendor}/{plugin}/{controller}/schema/list`, `.../schema/form`, `.../schema/relation/{name}` | Localized list, form and relation schemas. | diff --git a/modules/cabana/auth.go b/modules/cabana/auth.go index c7cef63..38d5028 100644 --- a/modules/cabana/auth.go +++ b/modules/cabana/auth.go @@ -8,6 +8,7 @@ import ( "log/slog" "net" "net/http" + "strconv" "strings" "sync" "time" @@ -249,10 +250,17 @@ func (s *service) refresh(w http.ResponseWriter, r *http.Request) { // logout blacklists the presented token's jti and always expires the admin // cookie, so a browser session ends even when only the Bearer was revoked. +// +// Logout is mounted outside the backend guard, which rejects an expired access +// token before any handler runs. The token is verified here instead, with +// exp unchecked, so a token whose access lifetime has passed but whose refresh +// window is still open (and which /auth/refresh would still accept) can be +// revoked. The cookie is expired on every outcome, including a refusal. func (s *service) logout(w http.ResponseWriter, r *http.Request) { + s.expireSessionCookie(w) raw, _ := sessionToken(r) - _, iat, exp, jti, err := bouncer.VerifyClaimsAudience(raw, s.secret, bouncer.AudienceBackend) - if err != nil || jti == "" { + sub, iat, exp, jti, err := bouncer.VerifyRefreshableClaimsAudience(raw, s.secret, bouncer.AudienceBackend, s.refreshTTL) + if err != nil { WriteError(w, http.StatusUnauthorized, "unauthenticated", msgUnauthenticated) return } @@ -267,11 +275,10 @@ func (s *service) logout(w http.ResponseWriter, r *http.Request) { } } id := uint(0) - if principal, ok := bouncer.User(r.Context()); ok { - id = principal.ID + if n, err := strconv.ParseUint(sub, 10, 64); err == nil { + id = uint(n) } s.logAuth(r, "success", id) - s.expireSessionCookie(w) WriteData(w, http.StatusOK, AdminLogoutData{Status: "logged_out"}, map[string]any{}) } diff --git a/modules/cabana/auth_test.go b/modules/cabana/auth_test.go index 725645f..ce443f2 100644 --- a/modules/cabana/auth_test.go +++ b/modules/cabana/auth_test.go @@ -576,3 +576,57 @@ func TestLoginAmbiguousIdentifier(t *testing.T) { t.Fatalf("admin B by login = %d %s", rec.Code, rec.Body.String()) } } + +// TestAdminLogoutRevokesExpiredRefreshableToken pins WR-14: a token whose +// access lifetime has passed but whose refresh window is open can still be +// revoked through logout (the guard would reject it with 401 before any +// handler), so a leaked copy cannot be refreshed afterwards. An unusable token +// is a 401 that still clears the session cookie. +func TestAdminLogoutRevokesExpiredRefreshableToken(t *testing.T) { + gdb := adminGorm(t) + h := adminHandler(t, gdb, nil) + user := insertAdmin(t, gdb, "stale", "stale@example.test", adminTestPassword, true, false) + token, jti, err := bouncer.MintAudience(adminTestSecret, strconv.FormatUint(uint64(user.ID), 10), "https://app.test/backend/api/v1/auth/login", -time.Minute, bouncer.AudienceBackend) + if err != nil { + t.Fatal(err) + } + if _, _, _, _, err := bouncer.VerifyClaimsAudience(token, adminTestSecret, bouncer.AudienceBackend); err == nil { + t.Fatal("fixture token is not expired") + } + if me := postAuth(t, h, http.MethodGet, adminAPI("/auth/me"), token, nil); me.Code != http.StatusUnauthorized { + t.Fatalf("expired token on a guarded route = %d, want 401", me.Code) + } + + out := postAuth(t, h, http.MethodPost, adminAPI("/auth/logout"), token, nil) + if out.Code != http.StatusOK { + t.Fatalf("logout of an expired refreshable token = %d %s", out.Code, out.Body.String()) + } + var n int + if err := gdb.Raw(`SELECT COUNT(*) FROM backend_jwt_blacklist WHERE jti = ?`, jti).Scan(&n).Error; err != nil || n != 1 { + t.Fatalf("blacklist rows for the expired token = %d, %v; want 1", n, err) + } + if again := postAuth(t, h, http.MethodPost, adminAPI("/auth/refresh"), token, nil); again.Code != http.StatusUnauthorized { + t.Fatalf("refresh after logout = %d %s, want 401", again.Code, again.Body.String()) + } + + // Not a token at all is refused, and the cookie is cleared either way. + cookie := &http.Cookie{Name: cabana.AdminCookieName, Value: "not-a-token"} + for name, rec := range map[string]*httptest.ResponseRecorder{ + "garbage bearer": postAuth(t, h, http.MethodPost, adminAPI("/auth/logout"), "not-a-token", nil), + "garbage cookie": phase10Send(t, h, http.MethodPost, adminAPI("/auth/logout"), nil, cookie, true), + "no token": phase10Send(t, h, http.MethodPost, adminAPI("/auth/logout"), nil, nil, true), + } { + if rec.Code != http.StatusUnauthorized { + t.Fatalf("%s logout = %d, want 401", name, rec.Code) + } + cleared := false + for _, c := range rec.Result().Cookies() { + if c.Name == cabana.AdminCookieName && c.MaxAge < 0 { + cleared = true + } + } + if !cleared { + t.Fatalf("%s logout did not clear the session cookie", name) + } + } +} diff --git a/modules/cabana/http.go b/modules/cabana/http.go index 158a6c7..255c7c2 100644 --- a/modules/cabana/http.go +++ b/modules/cabana/http.go @@ -193,13 +193,16 @@ func (s *service) mount(r pact.Router) { // Bearer body and no cookie is set, so a cross-site post gains nothing. g.Post("/login", s.login, throttle) g.Post("/refresh", requireAjax(s.refresh)) + // Logout reads and verifies the token itself (see service.logout), so + // it is mounted outside the backend guard, which rejects an expired + // access token that is still refreshable. + g.Post("/logout", requireAjax(s.logout)) }) // The string bundle is public: the login screen needs it before auth. r.GroupRaw(api, nil, func(g pact.Router) { g.Get("/lang", s.langBundle) }) r.GroupRaw(api, []string{"backend"}, func(g pact.Router) { - g.Post("/auth/logout", requireAjax(s.logout)) g.Get("/auth/me", s.me) g.Get("/navigation", s.navigation) g.Get("/settings", s.settingsList) diff --git a/modules/cabana/security_coverage_test.go b/modules/cabana/security_coverage_test.go index b19657c..ca28164 100644 --- a/modules/cabana/security_coverage_test.go +++ b/modules/cabana/security_coverage_test.go @@ -35,7 +35,9 @@ var phase09Routes = []adminRoute{ {key: "POST /auth/login", public: true}, {key: "POST /auth/refresh", public: true}, {key: "GET /lang", public: true}, - {key: "POST /auth/logout"}, + // Logout verifies the token itself (exp unchecked, so an expired but + // refreshable token can be revoked) and is therefore not behind the guard. + {key: "POST /auth/logout", public: true}, {key: "GET /auth/me"}, {key: "GET /navigation"}, {key: "GET /settings"},