security: GHSA-fxmj-cjvj-x372 harden oauth session issuance

Co-authored-by: Cursor <cursoragent@cursor.com>

service/internal/auth/otoauth2/restapi_auth_oauth2.go

@@ -213,6 +213,14 @@ func (h *OAuth2Handler) validateStateMatch(queryState, cookieState string) bool
 	return queryState != "" && queryState == cookieState
 }
 
+func pendingStateExpired(pending *oauth2PendingState, now time.Time) bool {
+	return pending.createdAt.Before(now.Add(-oauthStateMaxAge * time.Second))
+}
+
+func sessionExpired(session *oauth2Session, now time.Time) bool {
+	return session.createdAt.Before(now.Add(-oauthSessionMaxAge * time.Second))
+}
+
 func (h *OAuth2Handler) consumePendingOAuthState(w http.ResponseWriter, r *http.Request) (*oauth2PendingState, string, bool) {
 	cookie, err := r.Cookie(oauthStateCookieName)
 	if err != nil {
@@ -233,15 +241,30 @@ func (h *OAuth2Handler) consumePendingOAuthState(w http.ResponseWriter, r *http.
 		return nil, state, false
 	}
 
+	return h.loadValidPendingOAuthState(w, state)
+}
+
+func (h *OAuth2Handler) loadValidPendingOAuthState(w http.ResponseWriter, state string) (*oauth2PendingState, string, bool) {
 	h.mu.RLock()
 	pending, ok := h.pendingStates[state]
+	expired := ok && pendingStateExpired(pending, time.Now())
 	h.mu.RUnlock()
+
 	if !ok {
 		log.Errorf("OAuth2 pending state not found on server")
 		http.Error(w, "State not found in server", http.StatusBadRequest)
 		return nil, state, false
 	}
 
+	if expired {
+		h.mu.Lock()
+		h.deletePendingStateLocked(state)
+		h.mu.Unlock()
+		log.Errorf("OAuth2 pending state expired")
+		http.Error(w, "State expired", http.StatusBadRequest)
+		return nil, state, false
+	}
+
 	return pending, state, true
 }
 
@@ -329,13 +352,27 @@ func (h *OAuth2Handler) issueOAuthSession(pending *oauth2PendingState, state, us
 	h.mu.Lock()
 	defer h.mu.Unlock()
 
+	pendingEntry, ok := h.pendingStates[state]
+	if !ok {
+		return "", fmt.Errorf("pending OAuth state no longer exists")
+	}
+	if pendingStateExpired(pendingEntry, time.Now()) {
+		h.deletePendingStateLocked(state)
+		return "", fmt.Errorf("pending OAuth state expired")
+	}
+
+	providerName := pending.providerName
+	if pendingEntry.providerName != "" {
+		providerName = pendingEntry.providerName
+	}
+
 	h.deletePendingStateLocked(state)
 	h.sweepExpiredOAuthEntriesLocked(time.Now())
 	h.sessions[sessionID] = &oauth2Session{
 		createdAt:    time.Now(),
 		Username:     username,
 		Usergroup:    usergroup,
-		providerName: pending.providerName,
+		providerName: providerName,
 	}
 
 	return sessionID, nil
@@ -527,13 +564,15 @@ func joinGroupArray(arrayVal []any, field string, sep string) string {
 }
 
 func (h *OAuth2Handler) lookupOAuth2UserBySession(sessionID string) (*authTypes.AuthenticatedUser, bool) {
-	h.mu.Lock()
-	defer h.mu.Unlock()
-
-	h.sweepExpiredOAuthEntriesLocked(time.Now())
-
+	h.mu.RLock()
 	session, found := h.sessions[sessionID]
 	if !found || session.Username == "" {
+		h.mu.RUnlock()
+		return nil, false
+	}
+	if sessionExpired(session, time.Now()) {
+		h.mu.RUnlock()
+		h.deleteSessionIfExpired(sessionID)
 		return nil, false
 	}
 
@@ -543,9 +582,20 @@ func (h *OAuth2Handler) lookupOAuth2UserBySession(sessionID string) (*authTypes.
 		Provider:      "oauth2",
 		SID:           sessionID,
 	}
+	h.mu.RUnlock()
 	return user, true
 }
 
+func (h *OAuth2Handler) deleteSessionIfExpired(sessionID string) {
+	h.mu.Lock()
+	defer h.mu.Unlock()
+
+	session, found := h.sessions[sessionID]
+	if found && sessionExpired(session, time.Now()) {
+		delete(h.sessions, sessionID)
+	}
+}
+
 func (h *OAuth2Handler) RevokeSession(sid string) {
 	h.mu.Lock()
 	defer h.mu.Unlock()

service/internal/auth/otoauth2/restapi_auth_oauth2_test.go

@@ -277,3 +277,74 @@ func TestCheckUserFromOAuth2CookieUsesSessionCookieOnly(t *testing.T) {
 	reqLegacyStateCookie.AddCookie(&http.Cookie{Name: oauthStateCookieName, Value: "csrf-state"})
 	assert.Nil(t, h.CheckUserFromOAuth2Cookie(&authTypes.AuthCheckingContext{Request: reqLegacyStateCookie}))
 }
+
+func TestIssueOAuthSessionRejectsMissingPendingState(t *testing.T) {
+	h := &OAuth2Handler{
+		pendingStates: make(map[string]*oauth2PendingState),
+		sessions:      make(map[string]*oauth2Session),
+	}
+
+	pending := &oauth2PendingState{providerName: "test", createdAt: time.Now()}
+	sessionID, err := h.issueOAuthSession(pending, "already-consumed", "alice", "admins")
+	require.Error(t, err)
+	assert.Empty(t, sessionID)
+	assert.Empty(t, h.sessions)
+}
+
+func TestIssueOAuthSessionRejectsExpiredPendingState(t *testing.T) {
+	h := &OAuth2Handler{
+		pendingStates: make(map[string]*oauth2PendingState),
+		sessions:      make(map[string]*oauth2Session),
+	}
+
+	state := "expired-state"
+	pending := &oauth2PendingState{
+		providerName: "test",
+		createdAt:    time.Now().Add(-2 * oauthStateMaxAge * time.Second),
+	}
+	h.pendingStates[state] = pending
+
+	sessionID, err := h.issueOAuthSession(pending, state, "alice", "admins")
+	require.Error(t, err)
+	assert.Empty(t, sessionID)
+	assert.NotContains(t, h.pendingStates, state)
+	assert.Empty(t, h.sessions)
+}
+
+func TestLookupOAuth2UserBySessionRejectsExpiredSession(t *testing.T) {
+	h := &OAuth2Handler{
+		pendingStates: make(map[string]*oauth2PendingState),
+		sessions:      make(map[string]*oauth2Session),
+	}
+
+	h.sessions["stale"] = &oauth2Session{
+		Username:  "alice",
+		createdAt: time.Now().Add(-2 * oauthSessionMaxAge * time.Second),
+	}
+
+	user, found := h.lookupOAuth2UserBySession("stale")
+	assert.False(t, found)
+	assert.Nil(t, user)
+	assert.NotContains(t, h.sessions, "stale")
+}
+
+func TestLoadValidPendingOAuthStateRejectsExpired(t *testing.T) {
+	h := &OAuth2Handler{
+		pendingStates: make(map[string]*oauth2PendingState),
+		sessions:      make(map[string]*oauth2Session),
+	}
+
+	state := "expired-pending"
+	h.pendingStates[state] = &oauth2PendingState{
+		providerName: "test",
+		createdAt:    time.Now().Add(-2 * oauthStateMaxAge * time.Second),
+	}
+
+	rec := httptest.NewRecorder()
+	pending, gotState, ok := h.loadValidPendingOAuthState(rec, state)
+	assert.False(t, ok)
+	assert.Nil(t, pending)
+	assert.Equal(t, state, gotState)
+	assert.Equal(t, http.StatusBadRequest, rec.Code)
+	assert.NotContains(t, h.pendingStates, state)
+}