fix: invalidate session on fatal OAuth2 refresh errors (#3333)

* fix: invalidate session on fatal OAuth2 refresh errors

When a token refresh fails with a fatal OAuth2 error (invalid_grant,
invalid_client), the session is now cleared from the session store
and the cookie is removed, forcing re-authentication.

Previously, fatal refresh errors were logged but the stale session
continued to be served, leaving users logged in indefinitely after
their session was revoked at the provider level.

Transient errors (network timeouts, server errors) continue to
preserve the existing session as before.

Fixes #1945

Signed-off-by: Francesco Pasqualini <frapas@gmail.com>

* fix: apply review nits and add CHANGELOG entry

Signed-off-by: Francesco Pasqualini <frapas@gmail.com>
Signed-off-by: Jan Larwig <jan@larwig.com>

---------

Signed-off-by: Francesco Pasqualini <frapas@gmail.com>
Signed-off-by: Jan Larwig <jan@larwig.com>

CHANGELOG.md

@@ -8,9 +8,10 @@
 
 ## Changes since v7.15.1
 
-# V7.15.1
-
 - [#3411](https://github.com/oauth2-proxy/oauth2-proxy/pull/3411) chore(deps): update gomod dependencies (@tuunit)
+- [#3333](https://github.com/oauth2-proxy/oauth2-proxy/pull/3333) fix: invalidate session on fatal OAuth2 refresh errors (@frhack)
+
+# V7.15.1
 
 ## Release Highlights
 

pkg/middleware/stored_session.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"strings"
 	"time"
 
 	"github.com/justinas/alice"
@@ -31,6 +32,33 @@ const (
 	sessionRefreshRetryPeriod = 10 * time.Millisecond
 )
 
+// isFatalRefreshError checks if a refresh error indicates a revoked or
+// non-existent session that should be immediately invalidated.
+// Fatal errors indicate the session is no longer valid at the provider level.
+// Non-fatal errors (network issues, timeouts) should not invalidate the session.
+//
+// Only checks standard OAuth2 error codes (RFC 6749 Section 5.2).
+// Does NOT check error_description strings as they are optional and provider-specific.
+func isFatalRefreshError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// Only check standard OAuth2 error codes (RFC 6749 Section 5.2)
+	// Do NOT check error_description strings as they are optional and provider-specific
+	fatalErrors := []string{
+		"invalid_grant",  // refresh token revoked, expired, or session terminated
+		"invalid_client", // client credentials no longer valid
+	}
+
+	for _, fe := range fatalErrors {
+		if strings.Contains(err.Error(), fe) {
+			return true
+		}
+	}
+	return false
+}
+
 // StoredSessionLoaderOptions contains all of the requirements to construct
 // a stored session loader.
 // All options must be provided.
@@ -188,9 +216,24 @@ func (s *storedSessionLoader) refreshSessionIfNeeded(rw http.ResponseWriter, req
 	// We are holding the lock and the session needs a refresh
 	logger.Printf("Refreshing session - User: %s; SessionAge: %s", session.User, session.Age())
 	if err := s.refreshSession(rw, req, session); err != nil {
-		// If a preemptive refresh fails, we still keep the session
-		// if validateSession succeeds.
 		logger.Errorf("Unable to refresh session: %v", err)
+
+		// Check if this is a fatal error that indicates the session is revoked
+		// or no longer valid at the provider level
+		if isFatalRefreshError(err) {
+			logger.Printf("Fatal refresh error detected (session revoked or invalid), clearing session for user: %s", session.User)
+
+			// Clear the session from storage (Redis) and remove the cookie
+			if err := s.store.Clear(rw, req); err != nil {
+				logger.Errorf("failed clearing session: %v", err)
+			}
+
+			// Return error immediately to force re-authentication
+			return fmt.Errorf("session invalidated due to fatal refresh error: %w", err)
+		}
+
+		// For non-fatal errors (network issues, timeouts), keep the session
+		// and let validateSession determine if it's still usable
 	}
 
 	// Validate all sessions after any Redeem/Refresh operation (fail or success)

pkg/middleware/stored_session_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"sync"
+	"testing"
 	"time"
 
 	middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"
@@ -801,3 +802,53 @@ func (f *fakeSessionStore) Clear(rw http.ResponseWriter, req *http.Request) erro
 func (f *fakeSessionStore) VerifyConnection(_ context.Context) error {
 	return nil
 }
+
+// TestIsFatalRefreshError tests the isFatalRefreshError function to ensure
+// it correctly identifies fatal OAuth2 errors that should invalidate a session.
+func TestIsFatalRefreshError(t *testing.T) {
+	tests := []struct {
+		name     string
+		err      error
+		expected bool
+	}{
+		{
+			name:     "nil error",
+			err:      nil,
+			expected: false,
+		},
+		{
+			name:     "invalid_grant error",
+			err:      fmt.Errorf("failed to get token: oauth2: \"invalid_grant\" \"Session not active\""),
+			expected: true,
+		},
+		{
+			name:     "invalid_client error",
+			err:      fmt.Errorf("invalid_client: client not found"),
+			expected: true,
+		},
+		{
+			name:     "network timeout - not fatal",
+			err:      fmt.Errorf("Post \"https://keycloak/token\": dial tcp: connect: connection refused"),
+			expected: false,
+		},
+		{
+			name:     "server error - not fatal",
+			err:      fmt.Errorf("unexpected status code 500"),
+			expected: false,
+		},
+		{
+			name:     "generic refresh error - not fatal",
+			err:      fmt.Errorf("error refreshing tokens: context deadline exceeded"),
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := isFatalRefreshError(tt.err)
+			if result != tt.expected {
+				t.Errorf("isFatalRefreshError(%v) = %v, want %v", tt.err, result, tt.expected)
+			}
+		})
+	}
+}