From 08de9b79aad089c0c5f5170b1d96496f46d757f5 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 13 Aug 2026 17:15:42 +0100 Subject: [PATCH] R70: pooled-client cookie jar, refresh Save-failure cleanup --- http_client_pool.go | 11 ++++ http_client_pool_test.go | 20 +++++++ review_r70_refresh_save_fail_test.go | 80 ++++++++++++++++++++++++++++ token_manager.go | 10 +++- 4 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 review_r70_refresh_save_fail_test.go diff --git a/http_client_pool.go b/http_client_pool.go index 8dc2932..80d1eb0 100644 --- a/http_client_pool.go +++ b/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 { diff --git a/http_client_pool_test.go b/http_client_pool_test.go index f5ec70e..287cb87 100644 --- a/http_client_pool_test.go +++ b/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") + } + }) +} diff --git a/review_r70_refresh_save_fail_test.go b/review_r70_refresh_save_fail_test.go new file mode 100644 index 0000000..a4b62b8 --- /dev/null +++ b/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": "user@example.com", "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) + } +} diff --git a/token_manager.go b/token_manager.go index 3d3eb01..ce16e0b 100644 --- a/token_manager.go +++ b/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 }