From be4a923f3666c9e334dc3365dd1ab1599f696065 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Sun, 27 Sep 2026 18:58:15 +0200 Subject: [PATCH] fix(cabana): enforce tokens_valid_after and is_activated on admin refresh Fixes review finding CR-01 (quick 260927-q23): POST {prefix}/api/v1/auth/refresh minted a new token without loading the admin, so a session kept alive by the SPA's refresh-on-401 survived admin:reset-password, deactivation and deletion. This broke Phase 9 truth T-09-04. - bouncer: extract the JWT guard's subject lookup into subjectPrincipal and issuedBeforeCutoff (same order and messages), add ErrSubjectRejected - bouncer: add RefreshAudienceFor, which runs the guard's subject checks after the token-only checks and before minting; Refresh and RefreshAudience are unchanged (nil hook) - cabana: share one lazyBackendUsers provider between the backend guard and refresh; a cookie refresh refused for its subject expires summer_admin - test: TestAdminRefreshRevocation (Postgres, real admin:reset-password) --- bouncer/jwt.go | 54 ++++++++--- bouncer/refresh.go | 38 +++++++- cabana/auth.go | 8 +- cabana/http.go | 5 +- cabana/phase10_coverage_test.go | 16 ++++ cabana/refresh_revocation_test.go | 144 ++++++++++++++++++++++++++++++ 6 files changed, 246 insertions(+), 19 deletions(-) create mode 100644 cabana/refresh_revocation_test.go diff --git a/bouncer/jwt.go b/bouncer/jwt.go index 3c38508..94e60f6 100644 --- a/bouncer/jwt.go +++ b/bouncer/jwt.go @@ -118,19 +118,9 @@ func (g *jwtGuard) Authenticate(r *http.Request) (*Principal, error) { if err != nil { return nil, err } - id, err := strconv.ParseUint(sub, 10, 64) - if err != nil || id == 0 { - return nil, errors.New(msgUserNotFound) - } - if g.users == nil { - return nil, errors.New(msgUserNotFound) - } - user, err := g.users.FindByID(r.Context(), uint(id)) + user, err := subjectPrincipal(r.Context(), g.users, sub) if err != nil { - return nil, errors.New("Authentication error") - } - if user == nil { - return nil, errors.New(msgUserNotFound) + return nil, err } if g.bl != nil { blocked, err := g.bl.IsBlacklisted(r.Context(), jti) @@ -141,12 +131,48 @@ func (g *jwtGuard) Authenticate(r *http.Request) (*Principal, error) { return nil, errors.New(msgBadSignature) } } - if !user.TokensValidAfter.IsZero() && iat.Before(user.TokensValidAfter) { - return nil, errors.New(msgUserNotFound) + if issuedBeforeCutoff(user, iat) { + return nil, ErrSubjectRejected } return user, nil } +// ErrSubjectRejected reports that a token's subject is not a loadable user +// (a non-numeric or zero sub, a nil provider, or a provider that returned no +// principal for a missing, deleted or not-activated user), or that the token +// was issued before Principal.TokensValidAfter. Its message is "User not found". +var ErrSubjectRejected = errors.New(msgUserNotFound) + +// errAuthentication reports a provider failure while loading the subject. +var errAuthentication = errors.New("Authentication error") + +// subjectPrincipal loads the token subject through users, shared by the JWT +// guard and RefreshAudienceFor. A provider error is errAuthentication; every +// other refusal is ErrSubjectRejected. +func subjectPrincipal(ctx context.Context, users UserProvider, sub string) (*Principal, error) { + id, err := strconv.ParseUint(sub, 10, 64) + if err != nil || id == 0 { + return nil, ErrSubjectRejected + } + if users == nil { + return nil, ErrSubjectRejected + } + user, err := users.FindByID(ctx, uint(id)) + if err != nil { + return nil, errAuthentication + } + if user == nil { + return nil, ErrSubjectRejected + } + return user, nil +} + +// issuedBeforeCutoff reports whether iat predates the user's +// tokens_valid_after cutoff (set by a password reset). A zero cutoff never cuts. +func issuedBeforeCutoff(user *Principal, iat time.Time) bool { + return !user.TokensValidAfter.IsZero() && iat.Before(user.TokensValidAfter) +} + func (g *jwtGuard) WriteUnauthorized(w http.ResponseWriter, err error) { if g.writeFn != nil { g.writeFn(w, err) diff --git a/bouncer/refresh.go b/bouncer/refresh.go index 929b518..7e21579 100644 --- a/bouncer/refresh.go +++ b/bouncer/refresh.go @@ -15,7 +15,7 @@ import ( // jwt-auth: the later of the old exp and iat+refreshTTL, plus one minute, so // a logged-out token cannot be refreshed again for the rest of its refresh window. func Refresh(secret, tokenString string, refreshTTL time.Duration, bl BlacklistStore, grace time.Duration, issuerURL string) (string, error) { - return refreshAudience(secret, tokenString, AudienceUser, true, refreshTTL, bl, grace, issuerURL) + return refreshAudience(secret, tokenString, AudienceUser, true, refreshTTL, bl, grace, issuerURL, nil) } // RefreshAudience reissues a token that already carries audience. Missing aud is rejected. @@ -23,10 +23,37 @@ func RefreshAudience(secret, tokenString, audience string, refreshTTL time.Durat if strings.TrimSpace(audience) == "" { return "", errors.New("bouncer: jwt audience is empty") } - return refreshAudience(secret, tokenString, audience, false, refreshTTL, bl, grace, issuerURL) + return refreshAudience(secret, tokenString, audience, false, refreshTTL, bl, grace, issuerURL, nil) } -func refreshAudience(secret, tokenString, audience string, allowMissing bool, refreshTTL time.Duration, bl BlacklistStore, grace time.Duration, issuerURL string) (string, error) { +// RefreshAudienceFor is RefreshAudience plus the JWT guard's subject checks +// before minting: the subject is loaded through users, and a missing, +// deleted or not-activated user, or a token issued before the user's +// TokensValidAfter cutoff, is refused. The lookup runs only after every +// token-only check (signature, audience, refresh window, blacklist, exp) +// passed, and a refused subject neither mints a token nor blacklists the old +// jti. Only subject refusals match errors.Is(err, ErrSubjectRejected); a +// provider failure returns a different error. +func RefreshAudienceFor(ctx context.Context, users UserProvider, secret, tokenString, audience string, refreshTTL time.Duration, bl BlacklistStore, grace time.Duration, issuerURL string) (string, error) { + if strings.TrimSpace(audience) == "" { + return "", errors.New("bouncer: jwt audience is empty") + } + check := func(sub string, iat time.Time) error { + user, err := subjectPrincipal(ctx, users, sub) + if err != nil { + return err + } + if issuedBeforeCutoff(user, iat) { + return ErrSubjectRejected + } + return nil + } + return refreshAudience(secret, tokenString, audience, false, refreshTTL, bl, grace, issuerURL, check) +} + +// 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) { if strings.TrimSpace(secret) == "" { return "", errors.New("bouncer: jwt secret is empty") } @@ -68,6 +95,11 @@ func refreshAudience(secret, tokenString, audience string, allowMissing bool, re if !expOK || ttl <= 0 { return "", errors.New(msgRequiredClaims) } + if check != nil { + if err := check(sub, iat); err != nil { + return "", err + } + } next, _, err := MintAudience(secret, sub, issuerURL, ttl, audience) if err != nil { return "", err diff --git a/cabana/auth.go b/cabana/auth.go index 6b07418..48f329a 100644 --- a/cabana/auth.go +++ b/cabana/auth.go @@ -221,8 +221,14 @@ func (s *service) refresh(w http.ResponseWriter, r *http.Request) { WriteError(w, http.StatusUnauthorized, "unauthenticated", msgUnauthenticated) return } - next, err := bouncer.RefreshAudience(s.secret, raw, bouncer.AudienceBackend, s.refreshTTL, s.bl, s.grace, s.issuer) + next, err := bouncer.RefreshAudienceFor(r.Context(), s.users, s.secret, raw, bouncer.AudienceBackend, s.refreshTTL, s.bl, s.grace, s.issuer) if err != nil { + // A subject the guard would refuse (deactivated, deleted, or cut off + // by tokens_valid_after) ends the browser session. Other failures, + // including a provider error, leave the cookie alone. + if fromCookie && errors.Is(err, bouncer.ErrSubjectRejected) { + s.expireSessionCookie(w) + } s.logAuth(r, "failed", 0) WriteError(w, http.StatusUnauthorized, "unauthenticated", msgUnauthenticated) return diff --git a/cabana/http.go b/cabana/http.go index 1295747..056996f 100644 --- a/cabana/http.go +++ b/cabana/http.go @@ -41,6 +41,7 @@ type service struct { loginDecay int issuer string bl bouncer.BlacklistStore + users bouncer.UserProvider // the backend guard's provider, reused by refresh prefix string spa http.Handler // insecureCookie drops Secure from the admin cookie (backend.cookie_secure @@ -109,7 +110,8 @@ func Activate(app *backpack.App, plugins []party.Plugin) (*Routes, error) { } } bl := adminBlacklist(app) - guard := bouncer.NewBackendJWTGuard(secret, lazyBackendUsers{app: app, reg: reg}, bl, writeUnauthenticated, AdminCookieName) + users := lazyBackendUsers{app: app, reg: reg} + guard := bouncer.NewBackendJWTGuard(secret, users, bl, writeUnauthenticated, AdminCookieName) if _, err := guards.Middleware("backend"); err != nil { if err := guards.Register("summercms.cabana", "backend", guard); err != nil { return nil, err @@ -132,6 +134,7 @@ func Activate(app *backpack.App, plugins []party.Plugin) (*Routes, error) { loginDecay: loginDecay, issuer: adminIssuer(app, prefix), bl: bl, + users: users, prefix: prefix, insecureCookie: !secureCookie, diff --git a/cabana/phase10_coverage_test.go b/cabana/phase10_coverage_test.go index 3c0e4f2..d0ebc96 100644 --- a/cabana/phase10_coverage_test.go +++ b/cabana/phase10_coverage_test.go @@ -2,6 +2,7 @@ package cabana import ( "bytes" + "context" "encoding/json" "fmt" "net/http" @@ -22,6 +23,20 @@ import ( "gorm.io/gorm" ) +// refreshSubjects is a map-backed bouncer.UserProvider for refresh tests +// that run without a database; err, when set, is returned for every lookup. +type refreshSubjects struct { + byID map[uint]*bouncer.Principal + err error +} + +func (p refreshSubjects) FindByID(_ context.Context, id uint) (*bouncer.Principal, error) { + if p.err != nil { + return nil, p.err + } + return p.byID[id], nil +} + // emptyOptionsRow is a filter source whose scope has no choices yet; // literalOptionsRow's choice label is a literal rather than a phrase key. type emptyOptionsRow struct { @@ -281,6 +296,7 @@ func TestPhase10Coverage(t *testing.T) { refreshTTL: 2 * time.Hour, issuer: "https://app.test" + DefaultAdminPrefix, bl: bouncer.NewMemoryBlacklist(), + users: refreshSubjects{byID: map[uint]*bouncer.Principal{5: {ID: 5, Backend: true}}}, } sign := func(iat, exp time.Time, jti string) string { t.Helper() diff --git a/cabana/refresh_revocation_test.go b/cabana/refresh_revocation_test.go new file mode 100644 index 0000000..6fd7b53 --- /dev/null +++ b/cabana/refresh_revocation_test.go @@ -0,0 +1,144 @@ +package cabana_test + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "testing" + + "git.golem15.com/golem15/summercms/bonfire" + "git.golem15.com/golem15/summercms/cabana" +) + +// TestAdminRefreshRevocation pins CR-01: POST {prefix}/api/v1/auth/refresh +// applies the backend guard's subject checks before minting. A token issued +// before `summer admin:reset-password` (tokens_valid_after), or held by a +// deactivated or soft-deleted admin, cannot be refreshed over either +// transport, and a refused cookie refresh expires summer_admin. +func TestAdminRefreshRevocation(t *testing.T) { + gdb := adminGorm(t) + + cookieLogin := func(t *testing.T, h http.Handler, login string) *http.Cookie { + t.Helper() + rec := phase10Send(t, h, http.MethodPost, adminAPI("/auth/login"), + map[string]string{"login": login, "password": adminTestPassword}, nil, true) + if rec.Code != http.StatusOK { + t.Fatalf("cookie login status=%d body=%s", rec.Code, rec.Body.String()) + } + return phase10Cookie(t, rec, cabana.DefaultAdminPrefix) + } + bearerLogin := func(t *testing.T, h http.Handler, login string) string { + t.Helper() + rec := phase10Send(t, h, http.MethodPost, adminAPI("/auth/login"), + map[string]string{"login": login, "password": adminTestPassword}, nil, false) + if rec.Code != http.StatusOK { + t.Fatalf("bearer login status=%d body=%s", rec.Code, rec.Body.String()) + } + return accessToken(t, rec.Body.Bytes()) + } + resetPassword := func(t *testing.T, login string) { + t.Helper() + reset := commandByName(t, cabana.RuntimeCommands(commandApp(t, gdb)), "admin:reset-password") + var buf bytes.Buffer + if err := reset.Run(context.Background(), flagInput{ + args: []string{login}, + flags: map[string]string{"password": "rrev-replacement-password"}, + }, bonfire.NewOutput(nil, &buf, &buf)); err != nil { + t.Fatalf("admin:reset-password: %v output=%s", err, buf.String()) + } + } + assertRefusedWithExpiredCookie := func(t *testing.T, rec *httptest.ResponseRecorder) { + t.Helper() + if rec.Code != http.StatusUnauthorized || phase10ErrorCode(t, rec) != "unauthenticated" { + t.Fatalf("refresh status=%d body=%s, want 401 unauthenticated", rec.Code, rec.Body.String()) + } + var expired *http.Cookie + for _, c := range rec.Result().Cookies() { + if c.Name == cabana.AdminCookieName { + expired = c + } + } + if expired == nil || expired.Value != "" || expired.MaxAge >= 0 || expired.Path != cabana.DefaultAdminPrefix { + t.Fatalf("refused refresh cookie = %+v, want an expiring %s with Path %s", expired, cabana.AdminCookieName, cabana.DefaultAdminPrefix) + } + } + + t.Run("pre-reset cookie is refused and expired", func(t *testing.T) { + h := adminHandler(t, gdb, nil) + insertAdmin(t, gdb, "rrev-cookie", "rrev-cookie@example.test", adminTestPassword, true, false) + old := cookieLogin(t, h, "rrev-cookie") + resetPassword(t, "rrev-cookie") + + if me := phase10Send(t, h, http.MethodGet, adminAPI("/auth/me"), nil, old, true); me.Code != http.StatusUnauthorized { + t.Fatalf("pre-reset cookie /auth/me status=%d body=%s", me.Code, me.Body.String()) + } + rec := phase10Send(t, h, http.MethodPost, adminAPI("/auth/refresh"), nil, old, true) + assertRefusedWithExpiredCookie(t, rec) + }) + + t.Run("pre-reset bearer is refused without cookies", func(t *testing.T) { + h := adminHandler(t, gdb, nil) + insertAdmin(t, gdb, "rrev-bearer", "rrev-bearer@example.test", adminTestPassword, true, false) + token := bearerLogin(t, h, "rrev-bearer") + resetPassword(t, "rrev-bearer") + + rec := postAuth(t, h, http.MethodPost, adminAPI("/auth/refresh"), token, nil) + if rec.Code != http.StatusUnauthorized || phase10ErrorCode(t, rec) != "unauthenticated" { + t.Fatalf("pre-reset bearer refresh status=%d body=%s, want 401 unauthenticated", rec.Code, rec.Body.String()) + } + if got := rec.Header().Values("Set-Cookie"); len(got) != 0 { + t.Fatalf("bearer refresh set cookies: %q", got) + } + }) + + t.Run("deactivated admin is refused", func(t *testing.T) { + h := adminHandler(t, gdb, nil) + user := insertAdmin(t, gdb, "rrev-deactivated", "rrev-deactivated@example.test", adminTestPassword, true, false) + old := cookieLogin(t, h, "rrev-deactivated") + if err := gdb.Exec(`UPDATE backend_users SET is_activated = false WHERE id = ?`, user.ID).Error; err != nil { + t.Fatal(err) + } + rec := phase10Send(t, h, http.MethodPost, adminAPI("/auth/refresh"), nil, old, true) + assertRefusedWithExpiredCookie(t, rec) + }) + + t.Run("soft-deleted admin is refused", func(t *testing.T) { + h := adminHandler(t, gdb, nil) + user := insertAdmin(t, gdb, "rrev-deleted", "rrev-deleted@example.test", adminTestPassword, true, false) + old := cookieLogin(t, h, "rrev-deleted") + if err := gdb.Delete(&user).Error; err != nil { + t.Fatal(err) + } + rec := phase10Send(t, h, http.MethodPost, adminAPI("/auth/refresh"), nil, old, true) + assertRefusedWithExpiredCookie(t, rec) + }) + + t.Run("active admin still refreshes", func(t *testing.T) { + h := adminHandler(t, gdb, nil) + insertAdmin(t, gdb, "rrev-active", "rrev-active@example.test", adminTestPassword, true, false) + first := cookieLogin(t, h, "rrev-active") + rec := phase10Send(t, h, http.MethodPost, adminAPI("/auth/refresh"), nil, first, true) + if rec.Code != http.StatusOK { + t.Fatalf("cookie refresh status=%d body=%s", rec.Code, rec.Body.String()) + } + second := phase10Cookie(t, rec, cabana.DefaultAdminPrefix) + if second.Value == first.Value { + t.Fatal("cookie refresh did not rotate the token") + } + phase10AssertCookieBody(t, rec, second.Value) + if me := phase10Send(t, h, http.MethodGet, adminAPI("/auth/me"), nil, second, true); me.Code != http.StatusOK { + t.Fatalf("rotated cookie /auth/me status=%d body=%s", me.Code, me.Body.String()) + } + + token := bearerLogin(t, h, "rrev-active") + bearer := postAuth(t, h, http.MethodPost, adminAPI("/auth/refresh"), token, nil) + if bearer.Code != http.StatusOK { + t.Fatalf("bearer refresh status=%d body=%s", bearer.Code, bearer.Body.String()) + } + phase10AssertBearerBody(t, bearer) + if got := bearer.Header().Values("Set-Cookie"); len(got) != 0 { + t.Fatalf("bearer refresh set cookies: %q", got) + } + }) +}