From 6dbffcba40cd500727dc188a7a478d3ac08eb544 Mon Sep 17 00:00:00 2001
From: Lukasz Raczylo <lukasz@raczylo.com>
Date: Thu, 13 Aug 2026 17:11:17 +0100
Subject: [PATCH] R69: fix double pool-return on session-timeout path

---
 review_r69_session_pool_test.go | 51 +++++++++++++++++++++++++++++++++
 session.go                      |  9 ++++--
 2 files changed, 58 insertions(+), 2 deletions(-)
 create mode 100644 review_r69_session_pool_test.go

diff --git a/review_r69_session_pool_test.go b/review_r69_session_pool_test.go
new file mode 100644
index 0000000..76e328d
--- /dev/null
+++ b/review_r69_session_pool_test.go
@@ -0,0 +1,51 @@
+package traefikoidc
+
+import (
+	"net/http/httptest"
+	"sync/atomic"
+	"testing"
+	"time"
+)
+
+// TestSession_TimeoutNoDoublePool regresses a double return of the same
+// SessionData to the session pool on the session-timeout path: GetSession
+// called sessionData.Clear(r,nil) (whose defer returnToPoolSafely pooled
+// the object) and then handleError pooled it AGAIN. The identical pointer
+// therefore entered the pool twice, so two concurrent GetSession calls
+// could hand out one shared object (data race / cross-request session
+// bleed), and activeSessions was double-decremented (could go negative).
+func TestSession_TimeoutNoDoublePool(t *testing.T) {
+	sm := createTestSessionManager(t)
+
+	// Build an authenticated session whose creation time exceeds the max
+	// age (default 24h) and persist it so a later load sees it expired.
+	req0 := httptest.NewRequest("GET", "/", nil)
+	s, err := sm.GetSession(req0)
+	if err != nil {
+		t.Fatalf("create session: %v", err)
+	}
+	s.SetUserIdentifier("user@example.com")
+	if err := s.SetAuthenticated(true); err != nil {
+		t.Fatalf("authenticate: %v", err)
+	}
+	s.mainSession.Values["created_at"] = time.Now().Add(-48 * time.Hour).Unix()
+	rec := httptest.NewRecorder()
+	if err := s.Save(req0, rec); err != nil {
+		t.Fatalf("save session: %v", err)
+	}
+
+	mid := atomic.LoadInt64(&sm.activeSessions)
+
+	req := httptest.NewRequest("GET", "/", nil)
+	for _, c := range rec.Result().Cookies() {
+		req.AddCookie(c)
+	}
+	if _, err := sm.GetSession(req); err == nil {
+		t.Fatalf("expected session-timeout error for an expired session")
+	}
+
+	after := atomic.LoadInt64(&sm.activeSessions)
+	if after < mid {
+		t.Fatalf("activeSessions double-decremented (double pool return): mid=%d after=%d", mid, after)
+	}
+}
diff --git a/session.go b/session.go
index 7a9b36a..b1f3672 100644
--- a/session.go
+++ b/session.go
@@ -1072,7 +1072,11 @@ func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) {
 		// Check session timeout
 		if sessionData.getCreatedAtUnsafe() > 0 {
 			if time.Since(time.Unix(sessionData.getCreatedAtUnsafe(), 0)) > sm.sessionMaxAge {
-				_ = sessionData.Clear(r, nil) // Safe to ignore: session is being invalidated
+				// handleError returns the session to the pool and fully
+				// resets it; do NOT call Clear() here too, or the same
+				// pointer is pooled twice (two GetSession calls hand out
+				// one shared object -> data race / session bleed) and
+				// activeSessions is double-decremented.
 				return handleError(fmt.Errorf("session timeout"), "session expired")
 			}
 		}
@@ -1092,7 +1096,8 @@ func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) {
 
 	if createdAt, ok := sessionData.mainSession.Values["created_at"].(int64); ok {
 		if time.Since(time.Unix(createdAt, 0)) > sm.sessionMaxAge {
-			_ = sessionData.Clear(r, nil) // Safe to ignore: session is being invalidated
+			// handleError returns the session to the pool and resets it;
+			// do not also call Clear() (double pool-put, see above).
 			return handleError(fmt.Errorf("session timeout"), "session expired")
 		}
 	}