From 387f3d1f253e4d2c1a470d444ad35c9004976718 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 6730453406e..4605589fb39 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) @@ -160,10 +175,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 b0092185c01..c706aebc06a 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, }, } @@ -681,3 +681,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 1937bdc26ef95b7c36b89f2693d8a639d2b1b232 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 4605589fb39..b5720e734cc 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, }) } @@ -178,8 +178,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 662a13b5bd4a5b5295c56b3de8f7f93776cf5365 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 b5720e734cc..7f9578d3786 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 { @@ -101,6 +111,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) @@ -136,6 +151,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 oldRefreshToken string if oldToken, ok := clientSession.refreshToken.Values["refresh-token"]; ok { @@ -177,10 +196,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 c706aebc06a..53bbd38c0c4 100644 --- a/pkg/auth/sessions/combined_sessions_test.go +++ b/pkg/auth/sessions/combined_sessions_test.go @@ -729,3 +729,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") +}