diff --git a/controller/internal/web/auth.go b/controller/internal/web/auth.go index 980c8e6..69721c0 100644 --- a/controller/internal/web/auth.go +++ b/controller/internal/web/auth.go @@ -240,7 +240,7 @@ func (s *Server) handleLogout(w http.ResponseWriter, r *http.Request) { if cookie, err := r.Cookie(sessionCookieName); err == nil { s.sessionsMu.Lock() delete(s.sessions, sessionFingerprint(cookie.Value)) - _ = s.saveSessionsLocked() // R-35: a signed-out cookie stays signed out across a restart + s.saveSessionsRevokingLocked() // R-35: a signed-out cookie stays signed out across a restart s.sessionsMu.Unlock() } s.logger.Printf("[INFO] [web] User logged out from %s", r.RemoteAddr) @@ -299,7 +299,7 @@ func (s *Server) invalidateAllSessions() { s.sessionsMu.Lock() count := len(s.sessions) s.sessions = make(map[string]*session) - _ = s.saveSessionsLocked() // R-35: the password change ends every session on disk too + s.saveSessionsRevokingLocked() // R-35: the password change ends every session on disk too s.sessionsMu.Unlock() s.logger.Printf("[INFO] [web] All sessions invalidated (cleared %d)", count) } diff --git a/controller/internal/web/session_store.go b/controller/internal/web/session_store.go index 7bf6ec4..ad55557 100644 --- a/controller/internal/web/session_store.go +++ b/controller/internal/web/session_store.go @@ -26,14 +26,22 @@ import ( // invalidateAllSessions (password change, claim reset) write the file at once, so an ended session stays ended // across a restart (TestR35_LogoutEndsSessionAcrossRestart, TestR35_InvalidateAllEndsSessionAcrossRestart). // +// Two guards keep a REVOKED session from coming back if a write fails (security review 2026-10-08): +// - the file carries a fingerprint of the password hash in force when it was written (CredentialFP); at load, rows +// written under another password are dropped — a password change revokes on disk even if its own save failed +// (TestR35_PasswordChangeRevokesEvenIfSaveFailed); +// - a revoking save (logout, invalidateAllSessions) that fails removes the file instead, so the restart starts with +// no sessions rather than the stale ones (TestR35_FailedLogoutSaveRemovesFile). +// // A missing file is normal; an unreadable or corrupt one is logged and the server starts with no sessions — never // fatal, and the failure direction is "sign in again", never "signed in" (TestR35_CorruptFileStartsEmpty). const sessionsFileName = "dashboard-sessions.json" type sessionsFile struct { - Version int `json:"version"` - Sessions []sessionDisk `json:"sessions"` + Version int `json:"version"` + CredentialFP string `json:"credential_fp"` + Sessions []sessionDisk `json:"sessions"` } type sessionDisk struct { @@ -48,6 +56,13 @@ func sessionFingerprint(token string) string { return hex.EncodeToString(sum[:]) } +// credentialFingerprint is hex(sha256(the password hash in force)): it changes whenever the password does, from either +// source (settings.json or controller.yaml), and reveals nothing the bcrypt hash itself does not. +func (s *Server) credentialFingerprint() string { + sum := sha256.Sum256([]byte("felhom-dashboard-sessions\x00" + s.effectivePasswordHash())) + return hex.EncodeToString(sum[:]) +} + func (s *Server) sessionsPath() string { if s.cfg == nil || s.cfg.Paths.DataDir == "" { return "" @@ -75,6 +90,10 @@ func (s *Server) loadSessions() { logx.Warnf(s.logger, "[web] sessions: %s is not valid JSON, starting with none: %v", path, err) return } + if f.CredentialFP != s.credentialFingerprint() { + logx.Infof(s.logger, "[web] sessions: %d session(s) on disk were written under another password — dropped, sign in again", len(f.Sessions)) + return + } now := time.Now() loaded, expired, bad := 0, 0, 0 s.sessionsMu.Lock() @@ -102,7 +121,7 @@ func (s *Server) saveSessionsLocked() error { return nil } now := time.Now() - f := sessionsFile{Version: 1, Sessions: []sessionDisk{}} + f := sessionsFile{Version: 1, CredentialFP: s.credentialFingerprint(), Sessions: []sessionDisk{}} for fp, sess := range s.sessions { if !now.Before(sess.expiresAt) { continue @@ -122,6 +141,21 @@ func (s *Server) saveSessionsLocked() error { return nil } +// saveSessionsRevokingLocked is the save for a path that ENDS sessions (logout, invalidateAllSessions). If the write +// fails, the file is removed so a restart cannot bring an ended session back; a household then signs in again, which +// is the safe direction. Caller holds sessionsMu for writing. +func (s *Server) saveSessionsRevokingLocked() { + if err := s.saveSessionsLocked(); err == nil { + return + } + path := s.sessionsPath() + if err := os.Remove(path); err != nil && !errors.Is(err, os.ErrNotExist) { + logx.Errorf(s.logger, "[web] sessions: could not save NOR remove %s after ending a session — an ended session may return after a restart until the password changes: %v", path, err) + return + } + logx.Warnf(s.logger, "[web] sessions: save failed while ending a session — removed %s instead (every session ends at the next restart)", sessionsFileName) +} + // writeSessionsAtomic is tmp + fsync + rename at 0600 — the family.json shape (internal/family saveLocked). func writeSessionsAtomic(path string, b []byte) error { tmp := path + ".tmp" diff --git a/controller/internal/web/session_store_test.go b/controller/internal/web/session_store_test.go index 07115cd..a2c8c8f 100644 --- a/controller/internal/web/session_store_test.go +++ b/controller/internal/web/session_store_test.go @@ -195,3 +195,54 @@ func TestR35_CorruptFileStartsEmpty(t *testing.T) { t.Fatal("after a corrupt file, a new session must persist again") } } + +// Security review 2026-10-08: a password change revokes on disk even when its own save never happened. Here the +// password changes in settings.json WITHOUT invalidateAllSessions (the shape of a failed save that also could not +// remove the file); the restart drops every row written under the old password. +// RED-PROOF: skip the CredentialFP comparison in loadSessions → the old cookie signs in after the restart → FAILS. +func TestR35_PasswordChangeRevokesEvenIfSaveFailed(t *testing.T) { + dir := t.TempDir() + s1 := newR35Server(t, dir) + tok := s1.createSession() + s1.Close() + s2 := newR35Server(t, dir) + if !s2.isValidSession(tok) { + t.Fatal("setup: the session must survive one restart first (positive control)") + } + if err := s2.settings.SetPasswordHash("$2a$10$abcdefghijklmnopqrstuvABCDEFGHIJKLMNOPQRSTUVWXYZ01234"); err != nil { + t.Fatal(err) + } + s2.Close() + s3 := newR35Server(t, dir) + if s3.isValidSession(tok) { + t.Fatal("a session written under the old password signs in after the password changed") + } +} + +// Security review 2026-10-08: a logout whose save fails removes the file, so the signed-out cookie cannot return. +// The save is made to fail by a DIRECTORY at the temp path (OpenFile on a directory fails). +// RED-PROOF: make saveSessionsRevokingLocked only call saveSessionsLocked → the stale file keeps the row → FAILS. +func TestR35_FailedLogoutSaveRemovesFile(t *testing.T) { + dir := t.TempDir() + s1 := newR35Server(t, dir) + tok := s1.createSession() + s1.Close() + s2 := newR35Server(t, dir) + if !s2.isValidSession(tok) { + t.Fatal("setup: the session must survive one restart first (positive control)") + } + if err := os.Mkdir(filepath.Join(dir, sessionsFileName+".tmp"), 0o700); err != nil { + t.Fatal(err) + } + req := httptest.NewRequest(http.MethodPost, "/logout", nil) + req.AddCookie(&http.Cookie{Name: sessionCookieName, Value: tok}) + s2.handleLogout(httptest.NewRecorder(), req) + s2.Close() + if err := os.Remove(filepath.Join(dir, sessionsFileName+".tmp")); err != nil { + t.Fatal(err) + } + s3 := newR35Server(t, dir) + if s3.isValidSession(tok) { + t.Fatal("logout with a failed save: the signed-out cookie signs in again after a restart") + } +}