R70: pooled-client cookie jar, refresh Save-failure cleanup

http_client_pool.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net"
 	"net/http"
+	"net/http/cookiejar"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -289,6 +290,16 @@ func CreatePooledHTTPClient(config HTTPClientConfig) *http.Client {
 		Transport: transport,
 	}
 
+	// Honor the cookie-jar option, matching CreateHTTPClient
+	// (http_client_factory.go). Without this, token/OIDC clients built
+	// through the pool would silently drop UseCookieJar (config no-effect),
+	// so cookies set by the auth server on token/refresh responses were
+	// never stored or re-sent.
+	if config.UseCookieJar {
+		jar, _ := cookiejar.New(nil) // Safe to ignore: nil options rarely fail
+		client.Jar = jar
+	}
+
 	// Configure redirect policy
 	maxRedirects := config.MaxRedirects
 	if maxRedirects == 0 {

http_client_pool_test.go

@@ -724,3 +724,23 @@ func TestSharedTransportPoolAppliesConfigLimits(t *testing.T) {
 	assert.Equal(t, 41, transport.MaxIdleConns, "MaxIdleConns must come from config")
 	assert.Equal(t, 7*time.Second, transport.IdleConnTimeout, "IdleConnTimeout must come from config")
 }
+
+// TestCreatePooledHTTPClient_CookieJarOption regresses a config no-effect:
+// CreatePooledHTTPClient ignored config.UseCookieJar, so pooled token/OIDC
+// clients dropped the cookie jar that the non-pooled CreateHTTPClient
+// honors (queue cookies from auth-server token/refresh responses were
+// never stored or re-sent).
+func TestCreatePooledHTTPClient_CookieJarOption(t *testing.T) {
+	t.Run("enabled", func(t *testing.T) {
+		client := CreatePooledHTTPClient(HTTPClientConfig{UseCookieJar: true})
+		if client.Jar == nil {
+			t.Fatal("expected a cookie jar when UseCookieJar is true")
+		}
+	})
+	t.Run("disabled", func(t *testing.T) {
+		client := CreatePooledHTTPClient(HTTPClientConfig{UseCookieJar: false})
+		if client.Jar != nil {
+			t.Fatal("expected no cookie jar when UseCookieJar is false")
+		}
+	})
+}

review_r70_refresh_save_fail_test.go

@@ -0,0 +1,80 @@
+package traefikoidc
+
+import (
+	"crypto/rand"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+// TestRefresh_SaveFailureClearsRotatedTokens regresses the refresh path
+// leaving a half-rotated session when the post-refresh Save fails: the
+// IdP had already consumed the old refresh token (rotation), but the old
+// code only flipped authenticated to false and kept the new rotated
+// tokens in memory — so the in-memory session carried a fresh refresh
+// token while flagged unauthenticated. The Save-failure branch must clear
+// the rotated tokens (mirroring the SetAuthenticated-error branch).
+func TestRefresh_SaveFailureClearsRotatedTokens(t *testing.T) {
+	sessionManager, err := NewSessionManager(
+		"test-encryption-key-32-bytes-long!!", false, "", "", 0, NewLogger("error"))
+	if err != nil {
+		t.Fatalf("session manager: %v", err)
+	}
+	defer sessionManager.Shutdown()
+
+	claims := map[string]any{"email": "[email protected]", "exp": float64(9999999999)}
+	tOidc := &TraefikOidc{
+		logger:              NewLogger("error"),
+		userIdentifierClaim: "email",
+		sessionManager:      sessionManager,
+		tokenExchanger: &EnhancedMockTokenExchanger{
+			RefreshResponse: &TokenResponse{
+				AccessToken:  "new-access-token",
+				RefreshToken: "new-refresh-token",
+				IDToken:      "new-id-token",
+				ExpiresIn:    3600,
+			},
+		},
+		tokenVerifier:     &EnhancedMockTokenVerifier{Err: nil},
+		extractClaimsFunc: func(token string) (map[string]any, error) { return claims, nil },
+	}
+
+	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
+	rw := httptest.NewRecorder()
+
+	session, err := sessionManager.GetSession(req)
+	if err != nil {
+		t.Fatalf("get session: %v", err)
+	}
+	defer session.returnToPoolSafely()
+
+	session.SetRefreshToken("initial-refresh-token")
+
+	// Bloat the session with incompressible data so the combined-cookie
+	// Save overflows maxCombinedChunks (10) and fails deterministically.
+	bloat := make([]byte, 4000)
+	_, _ = rand.Read(bloat)
+	for i := 0; i < 25; i++ {
+		session.mainSession.Values[fmt.Sprintf("bulk%d", i)] = string(bloat)
+	}
+
+	refreshed := tOidc.refreshToken(rw, req, session)
+	if refreshed {
+		t.Fatal("expected refresh to fail when the session Save fails")
+	}
+
+	if session.GetAuthenticated() {
+		t.Error("session must not be authenticated after a failed refresh Save")
+	}
+	// Rotated tokens must not linger in memory (half-rotated state).
+	if got := session.GetRefreshToken(); got != "" {
+		t.Errorf("refresh token not cleared after failed Save, got %q", got)
+	}
+	if got := session.GetAccessToken(); got != "" {
+		t.Errorf("access token not cleared after failed Save, got %q", got)
+	}
+	if got := session.GetIDToken(); got != "" {
+		t.Errorf("id token not cleared after failed Save, got %q", got)
+	}
+}

token_manager.go

@@ -564,10 +564,18 @@ func (t *TraefikOidc) refreshToken(rw http.ResponseWriter, req *http.Request, se
 
 	if err := session.Save(req, rw); err != nil {
 		t.logger.Errorf("refreshToken failed: Failed to save session after successful token refresh: %v", err)
-		// Reset authentication state since we couldn't persist it
+		// Reset authentication state since we couldn't persist it. Also
+		// clear the just-rotated tokens: the old refresh token was already
+		// consumed by the IdP (rotation), so leaving the new ones in
+		// memory would be a half-rotated session that is flagged
+		// unauthenticated yet still carries a fresh refresh token.
 		if err := session.SetAuthenticated(false); err != nil {
 			t.logger.Errorf("Failed to set authenticated to false: %v", err)
 		}
+		session.SetRefreshToken("")
+		session.SetAccessToken("")
+		session.SetIDToken("")
+		session.SetUserIdentifier("")
 		return false
 	}