Skip to content

OCPBUGS-65967: Clean up old session cookies to prevent accumulation #15837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 3 commits into from
Feb 3, 2026
Merged

OCPBUGS-65967: Clean up old session cookies to prevent accumulation #15837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
96646c9
OCPBUGS-65967: Clean up old session cookies to prevent accumulation
TheRealJon Dec 12, 2025
ec0c7f3
OCPBUGS-65967: Fix session cookie deletion by setting proper attributes
TheRealJon Dec 12, 2025
c2dd500
OCPBUGS-65967: Refactor session cookie cleanup and expand coverage
TheRealJon Jan 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 43 additions & 3 deletions pkg/auth/sessions/combined_sessions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,35 @@ func NewSessionStore(authnKey, encryptKey []byte, secureCookies bool, cookiePath
}
}

// 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,
Secure: cs.clientStore.Options.Secure,
HttpOnly: cs.clientStore.Options.HttpOnly,
SameSite: cs.clientStore.Options.SameSite,
})
Comment on lines +54 to +62
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, find Domain configuration in cookie options
rg -n "\.Options\.Domain" --type=go

Repository: openshift/console

Length of output: 43

🏁 Script executed:

# Also check the Options struct definition
rg -n "type.*Options struct" --type=go -A 10

Repository: openshift/console

Length of output: 50374

🏁 Script executed:

# Examine the specific file and lines mentioned
cat -n pkg/auth/sessions/combined_sessions.go | head -250 | tail -200

Repository: openshift/console

Length of output: 9045

🏁 Script executed:

# Find where clientStore is defined and what type it is
rg -n "clientStore.*Options" pkg/auth/sessions/combined_sessions.go -B 5 -A 5

Repository: openshift/console

Length of output: 1662

🏁 Script executed:

# Search for the Options struct definition related to cookies/sessions
rg -n "type.*Options struct" pkg/auth/sessions/ -A 15

Repository: openshift/console

Length of output: 43

🏁 Script executed:

# Look for imports to understand what cookie store is being used
head -30 pkg/auth/sessions/combined_sessions.go

Repository: openshift/console

Length of output: 648

🏁 Script executed:

# Check the gorilla/sessions Options struct definition
rg -n "type Options struct" vendor/github.com/gorilla/sessions/ -A 20

Repository: openshift/console

Length of output: 1079

🏁 Script executed:

# Also verify if Domain is ever set on clientStore anywhere in the codebase
rg -n "\.Options\.Domain\s*=" --type=go

Repository: openshift/console

Length of output: 43

🏁 Script executed:

# Verify that Domain is never configured in clientStore.Options throughout the codebase
rg -n "clientStore\.Options" pkg/auth/sessions/ --type=go

Repository: openshift/console

Length of output: 1108

🏁 Script executed:

# Also check if Domain might be set through some other mechanism
rg -n "Domain\s*:" pkg/auth/sessions/ --type=go -B 2 -A 2

Repository: openshift/console

Length of output: 43

Include Domain in cookie expiration for consistency, even though it's not currently configured.

The comment on line 53 states "Must match all attributes of the original cookie," but the Domain attribute from cs.clientStore.Options is omitted. While Domain is not currently set in your cookie store initialization, including it makes the code more complete and defensive. If Domain configuration is ever added in the future, this ensures cookies will be properly deleted.

Proposed fix 
 			http.SetCookie(w, &http.Cookie{
 				Name:     cookie.Name,
 				Value:    "",
 				Path:     cs.clientStore.Options.Path,
+				Domain:   cs.clientStore.Options.Domain,
 				MaxAge:   -1,
 				Secure:   cs.clientStore.Options.Secure,
 				HttpOnly: cs.clientStore.Options.HttpOnly,
 				SameSite: cs.clientStore.Options.SameSite,
 			})

Also applies to: 231-239

🤖 Prompt for AI Agents 
In `@pkg/auth/sessions/combined_sessions.go` around lines 54 - 62, The
cookie-clearing code that calls http.SetCookie currently omits the Domain
attribute, which violates the "Must match all attributes of the original cookie"
rule; update both occurrences where http.SetCookie is used (the block that sets
Name: cookie.Name, Value: "", MaxAge: -1) to include Domain:
cs.clientStore.Options.Domain so the expired cookie matches the original cookie
attributes from cs.clientStore.Options and will be properly deleted if Domain is
later configured.

}
}
}

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 {
return nil, fmt.Errorf("failed to add session to server store: %w", err)
Expand Down Expand Up @@ -87,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)

Expand Down Expand Up @@ -136,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
Expand Down Expand Up @@ -193,10 +227,16 @@ 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: "",
Path: cs.clientStore.Options.Path,
MaxAge: -1,
Secure: cs.clientStore.Options.Secure,
HttpOnly: cs.clientStore.Options.HttpOnly,
SameSite: cs.clientStore.Options.SameSite,
})
}
}

Expand Down
138 changes: 135 additions & 3 deletions pkg/auth/sessions/combined_sessions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -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,
},
Expand Down Expand Up @@ -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,
},
}
Expand Down Expand Up @@ -695,3 +695,135 @@ 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")
}

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")
}