From 96646c9ff39ee0391d853cec519fa7235acb95bd Mon Sep 17 00:00:00 2001 From: Jon Jackson Date: Fri, 12 Dec 2025 09:34:54 -0500 Subject: [PATCH 1/2] 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 ec0c7f322e80ec6db636d35546bbb452031dc018 Mon Sep 17 00:00:00 2001 From: Jon Jackson Date: Fri, 12 Dec 2025 10:12:50 -0500 Subject: [PATCH 2/2] 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, }) }