From 1b59380b12884ece561f453b325b5df365e745c6 Mon Sep 17 00:00:00 2001 From: Jon Jackson Date: Fri, 12 Dec 2025 09:34:54 -0500 Subject: [PATCH 1/3] OCPBUGS-65967: Clean up old session cookies to prevent accumulation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When users are load-balanced across multiple console pods, each pod creates a session cookie with a unique name based on POD_NAME: openshift-session-token-. With a 1-month cookie expiration, users accumulate cookies from different pods without old ones being removed, eventually causing the cookie header to exceed 4096 bytes. This fix cleans up session cookies from other pods when creating a new session, ensuring only one active session cookie exists at a time. Changes: - Modified AddSession() to expire old pod cookies before creating new session - Updated DeleteSession() to use modern cookie expiration pattern - Added test to verify old pod cookies are properly expired Fixes: OCPBUGS-65967 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- pkg/auth/sessions/combined_sessions.go | 24 +++++++-- pkg/auth/sessions/combined_sessions_test.go | 54 +++++++++++++++++++-- 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/pkg/auth/sessions/combined_sessions.go b/pkg/auth/sessions/combined_sessions.go index 8a1aaf03827..0ad6a00841a 100644 --- a/pkg/auth/sessions/combined_sessions.go +++ b/pkg/auth/sessions/combined_sessions.go @@ -47,6 +47,21 @@ func (cs *CombinedSessionStore) AddSession(w http.ResponseWriter, r *http.Reques cs.sessionLock.Lock() defer cs.sessionLock.Unlock() + // Clean up old session cookies from previous pods before creating new session + // This prevents cookie accumulation when users are load-balanced across multiple pods + currentCookieName := SessionCookieName() + for _, cookie := range r.Cookies() { + // Expire any session cookies that are not for the current pod + if strings.HasPrefix(cookie.Name, OpenshiftAccessTokenCookieName) && cookie.Name != currentCookieName { + http.SetCookie(w, &http.Cookie{ + Name: cookie.Name, + Value: cookie.Value, + Path: cookie.Path, + MaxAge: -1, + }) + } + } + ls, err := cs.serverStore.AddSession(tokenVerifier, token) if err != nil { return nil, fmt.Errorf("failed to add session to server store: %w", err) @@ -193,10 +208,13 @@ func (cs *CombinedSessionStore) DeleteSession(w http.ResponseWriter, r *http.Req defer cs.sessionLock.Unlock() for _, cookie := range r.Cookies() { - cookie := cookie if strings.HasPrefix(cookie.Name, OpenshiftAccessTokenCookieName) { - cookie.MaxAge = -1 - http.SetCookie(w, cookie) + http.SetCookie(w, &http.Cookie{ + Name: cookie.Name, + Value: cookie.Value, + Path: cookie.Path, + MaxAge: -1, + }) } } diff --git a/pkg/auth/sessions/combined_sessions_test.go b/pkg/auth/sessions/combined_sessions_test.go index 9c1ec5f4563..b6e57d06d87 100644 --- a/pkg/auth/sessions/combined_sessions_test.go +++ b/pkg/auth/sessions/combined_sessions_test.go @@ -52,7 +52,7 @@ func TestCombinedSessionStore_AddSession(t *testing.T) { }, testIDToken, ), - origRefreshToken: utilptr.To[string]("orig-refresh-token"), + origRefreshToken: utilptr.To("orig-refresh-token"), wantRefreshToken: "random-token-string", wantRawToken: testIDToken, }, @@ -64,7 +64,7 @@ func TestCombinedSessionStore_AddSession(t *testing.T) { }, testIDToken, ), - origSessionToken: utilptr.To[string]("orig-session-token"), + origSessionToken: utilptr.To("orig-session-token"), wantRefreshToken: "random-token-string", wantRawToken: testIDToken, }, @@ -93,7 +93,7 @@ func TestCombinedSessionStore_AddSession(t *testing.T) { &oauth2.Token{}, testIDToken, ), - origRefreshToken: utilptr.To[string]("random-token-string"), + origRefreshToken: utilptr.To("random-token-string"), wantRawToken: testIDToken, }, } @@ -695,3 +695,51 @@ func indexSessionByRefreshToken(serverStore *SessionStore, refreshToken string, serverStore.byRefreshToken[refreshToken] = session return serverStore } + +func TestCombinedSessionStore_AddSession_CleansUpOldPodCookies(t *testing.T) { + testIDToken := createTestIDToken(`{"sub":"user-id-0"}`) + testVerifier := newTestVerifier(`{"sub":"user-id-0"}`) + + encryptionKey := []byte(randomString(32)) + authnKey := []byte(randomString(64)) + cs := NewSessionStore(authnKey, encryptionKey, true, "/") + + token := addIDToken( + &oauth2.Token{ + RefreshToken: "new-refresh-token", + }, + testIDToken, + ) + + // Simulate request with old session cookies from different pods + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-1", Value: "old-value-1"}) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-2", Value: "old-value-2"}) + req.AddCookie(&http.Cookie{Name: "other-cookie", Value: "other-value"}) + + testWriter := httptest.NewRecorder() + _, err := cs.AddSession(testWriter, req, testVerifier, token) + require.NoError(t, err) + + // Verify old pod cookies were expired + cookies := testWriter.Result().Cookies() + expiredCookies := 0 + newSessionCookie := false + for _, c := range cookies { + if c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-1" || + c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-2" { + require.Equal(t, -1, c.MaxAge, "Old pod cookie %s should be expired", c.Name) + expiredCookies++ + } + if c.Name == SessionCookieName() { + require.NotEqual(t, -1, c.MaxAge, "New session cookie should not be expired") + newSessionCookie = true + } + if c.Name == "other-cookie" { + t.Errorf("Non-session cookie 'other-cookie' should not be modified") + } + } + + require.Equal(t, 2, expiredCookies, "Both old pod cookies should be expired") + require.True(t, newSessionCookie, "New session cookie should be created") +} From 05eb69059edb3fdbd2cff1a30251f7c12bf53cdf Mon Sep 17 00:00:00 2001 From: Jon Jackson Date: Fri, 12 Dec 2025 10:12:50 -0500 Subject: [PATCH 2/3] OCPBUGS-65967: Fix session cookie deletion by setting proper attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When deleting cookies via HTTP response headers, browsers need the deletion cookie to match the Name, Path, and Domain of the original cookie. The previous implementation only set MaxAge=-1 on the existing cookie object without explicitly setting the Path, which could prevent proper cookie deletion. This change creates a new cookie with the minimal required attributes (Name, Path, Value="", MaxAge=-1) using the path from the session store options, ensuring the browser properly recognizes and deletes the cookie. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- pkg/auth/sessions/combined_sessions.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/auth/sessions/combined_sessions.go b/pkg/auth/sessions/combined_sessions.go index 0ad6a00841a..48819a44091 100644 --- a/pkg/auth/sessions/combined_sessions.go +++ b/pkg/auth/sessions/combined_sessions.go @@ -55,8 +55,8 @@ func (cs *CombinedSessionStore) AddSession(w http.ResponseWriter, r *http.Reques if strings.HasPrefix(cookie.Name, OpenshiftAccessTokenCookieName) && cookie.Name != currentCookieName { http.SetCookie(w, &http.Cookie{ Name: cookie.Name, - Value: cookie.Value, - Path: cookie.Path, + Value: "", + Path: cs.clientStore.Options.Path, MaxAge: -1, }) } @@ -211,8 +211,8 @@ func (cs *CombinedSessionStore) DeleteSession(w http.ResponseWriter, r *http.Req if strings.HasPrefix(cookie.Name, OpenshiftAccessTokenCookieName) { http.SetCookie(w, &http.Cookie{ Name: cookie.Name, - Value: cookie.Value, - Path: cookie.Path, + Value: "", + Path: cs.clientStore.Options.Path, MaxAge: -1, }) } From 55a1dda5242af70871b6570b705f706c22780d1c Mon Sep 17 00:00:00 2001 From: Jon Jackson Date: Fri, 30 Jan 2026 10:08:27 -0500 Subject: [PATCH 3/3] OCPBUGS-65967: Refactor session cookie cleanup and expand coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract cookie cleanup logic into expireOldPodCookies helper method and add proper cookie attributes (Secure, HttpOnly, SameSite) required for browsers to properly delete cookies. Expand cleanup to GetSession and UpdateTokens to handle all load balancing scenarios. Add comprehensive test coverage for all cleanup paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Leo Li Co-Authored-By: Claude Sonnet 4.5 --- pkg/auth/sessions/combined_sessions.go | 50 ++++++++---- pkg/auth/sessions/combined_sessions_test.go | 84 +++++++++++++++++++++ 2 files changed, 120 insertions(+), 14 deletions(-) diff --git a/pkg/auth/sessions/combined_sessions.go b/pkg/auth/sessions/combined_sessions.go index 48819a44091..18c9f6ac3b1 100644 --- a/pkg/auth/sessions/combined_sessions.go +++ b/pkg/auth/sessions/combined_sessions.go @@ -43,24 +43,34 @@ func NewSessionStore(authnKey, encryptKey []byte, secureCookies bool, cookiePath } } -func (cs *CombinedSessionStore) AddSession(w http.ResponseWriter, r *http.Request, tokenVerifier IDTokenVerifier, token *oauth2.Token) (*LoginState, error) { - cs.sessionLock.Lock() - defer cs.sessionLock.Unlock() - - // Clean up old session cookies from previous pods before creating new session - // This prevents cookie accumulation when users are load-balanced across multiple pods +// expireOldPodCookies expires session cookies from other pods to prevent cookie accumulation +// when users are load-balanced across multiple pods. +func (cs *CombinedSessionStore) expireOldPodCookies(w http.ResponseWriter, r *http.Request) { currentCookieName := SessionCookieName() for _, cookie := range r.Cookies() { // Expire any session cookies that are not for the current pod if strings.HasPrefix(cookie.Name, OpenshiftAccessTokenCookieName) && cookie.Name != currentCookieName { + // Must match all attributes of the original cookie for browsers to properly delete it http.SetCookie(w, &http.Cookie{ - Name: cookie.Name, - Value: "", - Path: cs.clientStore.Options.Path, - MaxAge: -1, + Name: cookie.Name, + Value: "", + Path: cs.clientStore.Options.Path, + MaxAge: -1, + Secure: cs.clientStore.Options.Secure, + HttpOnly: cs.clientStore.Options.HttpOnly, + SameSite: cs.clientStore.Options.SameSite, }) } } +} + +func (cs *CombinedSessionStore) AddSession(w http.ResponseWriter, r *http.Request, tokenVerifier IDTokenVerifier, token *oauth2.Token) (*LoginState, error) { + cs.sessionLock.Lock() + defer cs.sessionLock.Unlock() + + // Clean up old session cookies from previous pods before creating new session + // This prevents cookie accumulation when users are load-balanced across multiple pods + cs.expireOldPodCookies(w, r) ls, err := cs.serverStore.AddSession(tokenVerifier, token) if err != nil { @@ -102,6 +112,11 @@ func (cs *CombinedSessionStore) GetSession(w http.ResponseWriter, r *http.Reques cs.sessionLock.Lock() defer cs.sessionLock.Unlock() + // Clean up old session cookies from previous pods + // This is done here because GetSession is called on /api/* requests where + // session cookies (with Path=/api) are actually sent by the browser + cs.expireOldPodCookies(w, r) + // Get always returns a session, even if empty. clientSession := cs.getCookieSession(r) @@ -151,6 +166,10 @@ func (cs *CombinedSessionStore) UpdateTokens(w http.ResponseWriter, r *http.Requ cs.sessionLock.Lock() defer cs.sessionLock.Unlock() + // Clean up old session cookies from previous pods when refreshing tokens + // This handles the case where a user is load-balanced to a different pod + cs.expireOldPodCookies(w, r) + clientSession := cs.getCookieSession(r) var oldRefreshTokenID string var oldRefreshToken string @@ -210,10 +229,13 @@ func (cs *CombinedSessionStore) DeleteSession(w http.ResponseWriter, r *http.Req for _, cookie := range r.Cookies() { if strings.HasPrefix(cookie.Name, OpenshiftAccessTokenCookieName) { http.SetCookie(w, &http.Cookie{ - Name: cookie.Name, - Value: "", - Path: cs.clientStore.Options.Path, - MaxAge: -1, + Name: cookie.Name, + Value: "", + Path: cs.clientStore.Options.Path, + MaxAge: -1, + Secure: cs.clientStore.Options.Secure, + HttpOnly: cs.clientStore.Options.HttpOnly, + SameSite: cs.clientStore.Options.SameSite, }) } } diff --git a/pkg/auth/sessions/combined_sessions_test.go b/pkg/auth/sessions/combined_sessions_test.go index b6e57d06d87..268d210e03e 100644 --- a/pkg/auth/sessions/combined_sessions_test.go +++ b/pkg/auth/sessions/combined_sessions_test.go @@ -743,3 +743,87 @@ func TestCombinedSessionStore_AddSession_CleansUpOldPodCookies(t *testing.T) { require.Equal(t, 2, expiredCookies, "Both old pod cookies should be expired") require.True(t, newSessionCookie, "New session cookie should be created") } + +func TestCombinedSessionStore_GetSession_CleansUpOldPodCookies(t *testing.T) { + encryptionKey := []byte(randomString(32)) + authnKey := []byte(randomString(64)) + cs := NewSessionStore(authnKey, encryptionKey, true, "/") + + // Simulate request with old session cookies from different pods + // This is the primary cleanup path since GetSession is called on /api/* requests + // where session cookies (with Path=/api) are actually sent by the browser + req := httptest.NewRequest(http.MethodGet, "/api/some-endpoint", nil) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-1", Value: "old-value-1"}) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-2", Value: "old-value-2"}) + req.AddCookie(&http.Cookie{Name: "other-cookie", Value: "other-value"}) + + testWriter := httptest.NewRecorder() + _, _ = cs.GetSession(testWriter, req) + + // Verify old pod cookies were expired + cookies := testWriter.Result().Cookies() + expiredCookies := 0 + for _, c := range cookies { + if c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-1" || + c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-2" { + require.Equal(t, -1, c.MaxAge, "Old pod cookie %s should be expired", c.Name) + expiredCookies++ + } + if c.Name == "other-cookie" { + t.Errorf("Non-session cookie 'other-cookie' should not be modified") + } + } + + require.Equal(t, 2, expiredCookies, "Both old pod cookies should be expired") +} + +func TestCombinedSessionStore_UpdateTokens_CleansUpOldPodCookies(t *testing.T) { + currentTime := strconv.FormatInt(time.Now().Add(5*time.Minute).Unix(), 10) + testIDToken := createTestIDToken(`{"sub":"user-id-0","exp":` + currentTime + `}`) + testVerifier := newTestVerifier(`{"sub":"user-id-0","exp":` + currentTime + `}`) + + encryptionKey := []byte(randomString(32)) + authnKey := []byte(randomString(64)) + cs := NewSessionStore(authnKey, encryptionKey, true, "/") + + token := addIDToken( + &oauth2.Token{ + RefreshToken: "new-refresh-token", + }, + testIDToken, + ) + + // Simulate request with old session cookies from different pods + // This simulates the scenario where a user is load-balanced to a new pod + // and their token is being refreshed + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-1", Value: "old-value-1"}) + req.AddCookie(&http.Cookie{Name: OpenshiftAccessTokenCookieName + "-console-old-pod-2", Value: "old-value-2"}) + req.AddCookie(&http.Cookie{Name: "other-cookie", Value: "other-value"}) + + testWriter := httptest.NewRecorder() + _, err := cs.UpdateTokens(testWriter, req, testVerifier, token) + require.NoError(t, err) + + // Verify old pod cookies were expired + cookies := testWriter.Result().Cookies() + expiredCookies := 0 + newSessionCookie := false + for _, c := range cookies { + if c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-1" || + c.Name == OpenshiftAccessTokenCookieName+"-console-old-pod-2" { + require.Equal(t, -1, c.MaxAge, "Old pod cookie %s should be expired", c.Name) + expiredCookies++ + } + if c.Name == SessionCookieName() { + require.NotEqual(t, -1, c.MaxAge, "New session cookie should not be expired") + newSessionCookie = true + } + if c.Name == "other-cookie" { + t.Errorf("Non-session cookie 'other-cookie' should not be modified") + } + } + + require.Equal(t, 2, expiredCookies, "Both old pod cookies should be expired") + require.True(t, newSessionCookie, "New session cookie should be created") +}