fix(09): WR-14 let logout revoke an expired token that is still refreshable and always clear the cookie

modules/cabana/README.md

@@ -33,7 +33,7 @@ All paths are relative to `<prefix>/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. |

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{})
 }
 

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)
+		}
+	}
+}

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)

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"},