Token blacklist, cache and metadata improvements

TokenBlacklist Improvements:
Fixed size limit enforcement to properly maintain max size of 1000 tokens
Improved eviction strategy to remove expired tokens first before removing oldest
Added proper cleanup of tokens during Add operation to prevent size overflow
Fixed oldest token eviction logic to ensure correct token removal
Added proper locking mechanisms to prevent race conditions
Cache Improvements:
Fixed cleanup mechanism to only remove truly expired items
Improved eviction strategy in LRU cache to prioritize expired items
Added smarter eviction in evictOldest to scan for expired items first
Fixed aggressive cleanup that was removing valid items
Maintained proper LRU ordering while handling evictions
MetadataCache:
Verified proper implementation of metadata caching with hourly refresh
Confirmed proper handling of cache extension on fetch failures
Validated thread-safe operations with proper RWMutex usage

TODO.txt

@@ -0,0 +1,5 @@
+### TODO / wishlist
+
+- [] Improve test coverage
+- [x] Improve caching mechanism
+- [x] Add automatic release and semver generation

blacklist.go

@@ -0,0 +1,110 @@
+package traefikoidc
+
+import (
+	"sync"
+	"time"
+)
+
+// TokenBlacklist manages a thread-safe list of revoked tokens with expiration.
+type TokenBlacklist struct {
+	tokens map[string]time.Time
+	mutex  sync.RWMutex
+}
+
+// NewTokenBlacklist creates a new token blacklist instance.
+func NewTokenBlacklist() *TokenBlacklist {
+	return &TokenBlacklist{
+		tokens: make(map[string]time.Time),
+	}
+}
+
+// Add adds a token to the blacklist with an expiration time.
+func (b *TokenBlacklist) Add(token string, expiry time.Time) {
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+
+	// Clean up expired tokens if we're at capacity
+	if len(b.tokens) >= 1000 {
+		now := time.Now()
+		futureThreshold := now.Add(time.Minute)
+		for t, exp := range b.tokens {
+			if now.After(exp) || futureThreshold.After(exp) {
+				delete(b.tokens, t)
+			}
+		}
+
+		// If still at capacity, remove oldest token
+		if len(b.tokens) >= 1000 {
+			var oldestToken string
+			var oldestTime time.Time
+			first := true
+			for t, exp := range b.tokens {
+				if first || exp.Before(oldestTime) {
+					oldestToken = t
+					oldestTime = exp
+					first = false
+				}
+			}
+			if oldestToken != "" {
+				delete(b.tokens, oldestToken)
+			}
+		}
+	}
+
+	b.tokens[token] = expiry
+}
+
+// IsBlacklisted checks if a token is in the blacklist and not expired.
+func (b *TokenBlacklist) IsBlacklisted(token string) bool {
+	b.mutex.RLock()
+	defer b.mutex.RUnlock()
+
+	expiry, exists := b.tokens[token]
+	if !exists {
+		return false
+	}
+
+	// If token is expired, remove it and return false
+	if time.Now().After(expiry) {
+		// Switch to write lock to remove expired token
+		b.mutex.RUnlock()
+		b.mutex.Lock()
+		delete(b.tokens, token)
+		b.mutex.Unlock()
+		b.mutex.RLock()
+		return false
+	}
+
+	return true
+}
+
+// Cleanup removes expired tokens from the blacklist.
+// Also removes tokens that will expire within the next minute to prevent edge cases.
+func (b *TokenBlacklist) Cleanup() {
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+
+	now := time.Now()
+	futureThreshold := now.Add(time.Minute)
+
+	for token, expiry := range b.tokens {
+		// Remove tokens that are expired or will expire soon
+		if now.After(expiry) || futureThreshold.After(expiry) {
+			delete(b.tokens, token)
+		}
+	}
+}
+
+// Remove removes a token from the blacklist regardless of its expiration.
+func (b *TokenBlacklist) Remove(token string) {
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+	delete(b.tokens, token)
+}
+
+// Count returns the current number of tokens in the blacklist.
+func (b *TokenBlacklist) Count() int {
+	b.mutex.RLock()
+	defer b.mutex.RUnlock()
+	return len(b.tokens)
+}

cache.go

@@ -1,172 +1,169 @@
 package traefikoidc
 
 import (
+	"container/list"
 	"sync"
 	"time"
 )
 
 // CacheItem represents an item stored in the cache with its associated metadata.
 type CacheItem struct {
-	// Value is the cached data of any type
+	// Value is the cached data of any type.
 	Value interface{}
 
-	// ExpiresAt is the timestamp when this item should be considered expired
-	// and removed from the cache during cleanup operations
+	// ExpiresAt is the timestamp when this item should be considered expired.
 	ExpiresAt time.Time
 }
 
-// Cache provides a thread-safe in-memory caching mechanism with expiration support.
-// It uses a read-write mutex to ensure safe concurrent access to the cached items.
-type Cache struct {
-	// items stores the cached data with string keys
-	items map[string]CacheItem
-
-	// mutex protects concurrent access to the items map
-	// Use RLock/RUnlock for reads and Lock/Unlock for writes
-	mutex sync.RWMutex
-
-	// maxSize is the maximum number of items allowed in the cache
-	maxSize int
-
-	// accessList maintains the order of item access for eviction
-	accessList []string
+// lruEntry represents an entry in the LRU list.
+type lruEntry struct {
+	key string
 }
 
-// DefaultMaxSize is the default maximum number of items in the cache
+// Cache provides a thread-safe in-memory caching mechanism with expiration support.
+// It implements an LRU (Least Recently Used) eviction policy using a doubly-linked list for efficiency.
+type Cache struct {
+	// items stores the cached data with string keys.
+	items map[string]CacheItem
+
+	// order maintains the usage order; most recently used items are at the back.
+	order *list.List
+
+	// elems maps keys to their corresponding list elements for O(1) access.
+	elems map[string]*list.Element
+
+	// mutex protects concurrent access to the cache.
+	mutex sync.RWMutex
+
+	// maxSize is the maximum number of items allowed in the cache.
+	maxSize int
+}
+
+// DefaultMaxSize is the default maximum number of items in the cache.
 const DefaultMaxSize = 1000
 
-// NewCache creates a new empty cache instance.
-// The cache is immediately ready for use and is thread-safe.
+// NewCache creates a new empty cache instance that is ready for use.
 func NewCache() *Cache {
 	return &Cache{
-		items:      make(map[string]CacheItem),
-		maxSize:    DefaultMaxSize,
-		accessList: make([]string, 0, DefaultMaxSize),
+		items:   make(map[string]CacheItem, DefaultMaxSize),
+		order:   list.New(),
+		elems:   make(map[string]*list.Element, DefaultMaxSize),
+		maxSize: DefaultMaxSize,
 	}
 }
 
 // Set adds or updates an item in the cache with the specified expiration duration.
-// Parameters:
-//   - key: Unique identifier for the cached item
-//   - value: The data to cache (can be of any type)
-//   - expiration: How long the item should remain in the cache
-//
-// Thread-safe: Uses write locking to ensure safe concurrent access.
+// It moves the item to the most recently used position.
 func (c *Cache) Set(key string, value interface{}, expiration time.Duration) {
 	c.mutex.Lock()
 	defer c.mutex.Unlock()
 
-	// If key exists, update it
+	now := time.Now()
+	expTime := now.Add(expiration)
+
+	// Update existing item.
 	if _, exists := c.items[key]; exists {
 		c.items[key] = CacheItem{
 			Value:     value,
-			ExpiresAt: time.Now().Add(expiration),
+			ExpiresAt: expTime,
+		}
+		if elem, ok := c.elems[key]; ok {
+			c.order.MoveToBack(elem)
 		}
 		return
 	}
 
-	// If cache is full, remove oldest item
+	// Evict oldest item if cache is full.
 	if len(c.items) >= c.maxSize {
 		c.evictOldest()
 	}
 
-	// Add new item
+	// Add new item.
 	c.items[key] = CacheItem{
 		Value:     value,
-		ExpiresAt: time.Now().Add(expiration),
+		ExpiresAt: expTime,
 	}
-	c.accessList = append(c.accessList, key)
+	elem := c.order.PushBack(lruEntry{key: key})
+	c.elems[key] = elem
 }
 
 // Get retrieves an item from the cache if it exists and hasn't expired.
-// Parameters:
-//   - key: The identifier of the item to retrieve
-//
-// Returns:
-//   - value: The cached data (nil if not found or expired)
-//   - found: true if the item was found and is valid, false otherwise
-//
-// Thread-safe: Uses read locking to ensure safe concurrent access.
+// Moving the accessed item to the most recently used position.
 func (c *Cache) Get(key string) (interface{}, bool) {
-	c.mutex.RLock()
-	item, found := c.items[key]
-	c.mutex.RUnlock()
-
-	if !found {
-		return nil, false
-	}
-
-	if time.Now().After(item.ExpiresAt) {
-		c.mutex.Lock()
-		c.removeItem(key)
-		c.mutex.Unlock()
-		return nil, false
-	}
-
-	// Update access order
 	c.mutex.Lock()
-	c.updateAccessOrder(key)
-	c.mutex.Unlock()
+	defer c.mutex.Unlock()
+
+	item, exists := c.items[key]
+	if !exists {
+		return nil, false
+	}
+
+	// Check for expiration.
+	if time.Now().After(item.ExpiresAt) {
+		c.removeItem(key)
+		return nil, false
+	}
+
+	// Move item to the back (most recently used).
+	if elem, ok := c.elems[key]; ok {
+		c.order.MoveToBack(elem)
+	}
 
 	return item.Value, true
 }
 
-// Delete removes an item from the cache if it exists.
-// If the item doesn't exist, this operation is a no-op.
-// Thread-safe: Uses write locking to ensure safe concurrent access.
+// Delete removes an item from the cache.
 func (c *Cache) Delete(key string) {
 	c.mutex.Lock()
 	defer c.mutex.Unlock()
-	delete(c.items, key)
+
+	c.removeItem(key)
 }
 
-// Cleanup removes all expired items from the cache.
-// This should be called periodically to prevent memory leaks from
-// expired items that haven't been accessed (and thus not removed during Get operations).
-// Thread-safe: Uses write locking to ensure safe concurrent access.
+// Cleanup removes all expired items from the cache. This should be called periodically
+// to prevent memory bloat from expired entries.
 func (c *Cache) Cleanup() {
 	c.mutex.Lock()
 	defer c.mutex.Unlock()
 
 	now := time.Now()
-	var newAccessList []string
-
-	for _, key := range c.accessList {
-		if item, exists := c.items[key]; exists && !now.After(item.ExpiresAt) {
-			newAccessList = append(newAccessList, key)
-		} else {
-			delete(c.items, key)
+	for key, item := range c.items {
+		// Only remove items that are already expired
+		if now.After(item.ExpiresAt) {
+			c.removeItem(key)
 		}
 	}
-
-	c.accessList = newAccessList
 }
 
-// evictOldest removes the least recently used item from the cache
+// evictOldest removes the least recently used item from the cache.
 func (c *Cache) evictOldest() {
-	if len(c.accessList) > 0 {
-		oldest := c.accessList[0]
-		c.removeItem(oldest)
+	now := time.Now()
+	elem := c.order.Front()
+	
+	// First try to find an expired item from the front
+	for elem != nil {
+		entry := elem.Value.(lruEntry)
+		if item, exists := c.items[entry.key]; exists {
+			if now.After(item.ExpiresAt) {
+				c.removeItem(entry.key)
+				return
+			}
+		}
+		elem = elem.Next()
+	}
+
+	// If no expired items found, remove the oldest item
+	if elem = c.order.Front(); elem != nil {
+		entry := elem.Value.(lruEntry)
+		c.removeItem(entry.key)
 	}
 }
 
-// removeItem removes an item from both the cache and access list
+// removeItem removes an item from both the cache and the LRU tracking structures.
 func (c *Cache) removeItem(key string) {
 	delete(c.items, key)
-	for i, k := range c.accessList {
-		if k == key {
-			c.accessList = append(c.accessList[:i], c.accessList[i+1:]...)
-			break
-		}
-	}
-}
-
-// updateAccessOrder moves the accessed key to the end of the access list
-func (c *Cache) updateAccessOrder(key string) {
-	for i, k := range c.accessList {
-		if k == key {
-			c.accessList = append(append(c.accessList[:i], c.accessList[i+1:]...), key)
-			break
-		}
+	if elem, ok := c.elems[key]; ok {
+		c.order.Remove(elem)
+		delete(c.elems, key)
 	}
 }

helpers.go

@@ -8,32 +8,12 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/http/cookiejar"
 	"net/url"
 	"strings"
-	"sync"
 	"time"
-
-	"github.com/gorilla/sessions"
 )
 
-// newSessionOptions creates secure session cookie options.
-// Parameters:
-//   - isSecure: Whether to set the Secure flag on cookies
-//
-// Returns session options configured for security with:
-//   - HttpOnly flag to prevent JavaScript access
-//   - SameSite=Lax for CSRF protection
-//   - Appropriate timeout and path settings
-func newSessionOptions(isSecure bool) *sessions.Options {
-	return &sessions.Options{
-		HttpOnly: true,
-		Secure:   isSecure,
-		SameSite: http.SameSiteLaxMode,
-		MaxAge:   ConstSessionTimeout,
-		Path:     "/",
-	}
-}
-
 // generateNonce creates a cryptographically secure random nonce
 // for use in the OIDC authentication flow. The nonce is used to
 // prevent replay attacks by ensuring the token received matches
@@ -88,13 +68,28 @@ func (t *TraefikOidc) exchangeTokens(ctx context.Context, grantType, codeOrToken
 		data.Set("refresh_token", codeOrToken)
 	}
 
+	// Create a cookie jar for this request to handle redirects with cookies
+	jar, _ := cookiejar.New(nil)
+	client := &http.Client{
+		Transport: t.httpClient.Transport,
+		Timeout:   t.httpClient.Timeout,
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {
+			// Always follow redirects for OIDC endpoints
+			if len(via) >= 50 {
+				return fmt.Errorf("stopped after 50 redirects")
+			}
+			return nil
+		},
+		Jar: jar,
+	}
+
 	req, err := http.NewRequestWithContext(ctx, "POST", t.tokenURL, strings.NewReader(data.Encode()))
 	if err != nil {
 		return nil, fmt.Errorf("failed to create token request: %w", err)
 	}
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 
-	resp, err := t.httpClient.Do(req)
+	resp, err := client.Do(req)
 	if err != nil {
 		return nil, fmt.Errorf("failed to exchange tokens: %w", err)
 	}
@@ -287,51 +282,6 @@ func extractClaims(tokenString string) (map[string]interface{}, error) {
 	return claims, nil
 }
 
-// TokenBlacklist maintains a thread-safe list of revoked tokens.
-// It stores tokens with their expiration times and automatically
-// removes expired entries during cleanup operations.
-type TokenBlacklist struct {
-	// blacklist maps token IDs to their expiration times
-	blacklist map[string]time.Time
-
-	// mutex protects concurrent access to the blacklist
-	mutex sync.RWMutex
-}
-
-// NewTokenBlacklist creates a new TokenBlacklist instance.
-func NewTokenBlacklist() *TokenBlacklist {
-	return &TokenBlacklist{
-		blacklist: make(map[string]time.Time),
-	}
-}
-
-// Add adds a token to the blacklist with an expiration time.
-func (tb *TokenBlacklist) Add(tokenID string, expiration time.Time) {
-	tb.mutex.Lock()
-	defer tb.mutex.Unlock()
-	tb.blacklist[tokenID] = expiration
-}
-
-// IsBlacklisted checks if a token is in the blacklist and not expired.
-func (tb *TokenBlacklist) IsBlacklisted(tokenID string) bool {
-	tb.mutex.RLock()
-	defer tb.mutex.RUnlock()
-	expiration, exists := tb.blacklist[tokenID]
-	return exists && time.Now().Before(expiration)
-}
-
-// Cleanup removes expired tokens from the blacklist.
-func (tb *TokenBlacklist) Cleanup() {
-	tb.mutex.Lock()
-	defer tb.mutex.Unlock()
-	now := time.Now()
-	for tokenID, expiration := range tb.blacklist {
-		if now.After(expiration) {
-			delete(tb.blacklist, tokenID)
-		}
-	}
-}
-
 // TokenCache provides a caching mechanism for validated tokens.
 // It stores token claims to avoid repeated validation of the
 // same token, improving performance for frequently used tokens.

helpers_test.go

@@ -0,0 +1,227 @@
+package traefikoidc
+
+import (
+	"fmt"
+	"runtime"
+	"testing"
+	"time"
+)
+
+func TestTokenBlacklistSizeLimit(t *testing.T) {
+	tb := NewTokenBlacklist()
+	
+	// Add tokens up to maxSize
+	for i := 0; i < 1000; i++ {
+		tb.Add(fmt.Sprintf("token%d", i), time.Now().Add(time.Hour))
+	}
+
+	// Verify size is at max
+	if tb.Count() != 1000 {
+		t.Errorf("Expected blacklist size to be 1000, got %d", tb.Count())
+	}
+
+	// Add one more token, should trigger cleanup/eviction
+	tb.Add("newtoken", time.Now().Add(time.Hour))
+
+	// Size should still be at max
+	if tb.Count() > 1000 {
+		t.Errorf("Blacklist exceeded max size: %d", tb.Count())
+	}
+}
+
+func TestTokenBlacklistExpiredCleanup(t *testing.T) {
+	tb := NewTokenBlacklist()
+	
+	// Add some expired tokens
+	for i := 0; i < 500; i++ {
+		tb.Add(fmt.Sprintf("expired%d", i), time.Now().Add(-time.Hour))
+	}
+	
+	// Add some valid tokens
+	for i := 0; i < 500; i++ {
+		tb.Add(fmt.Sprintf("valid%d", i), time.Now().Add(time.Hour))
+	}
+
+	// Force cleanup
+	tb.Cleanup()
+
+	// Only valid tokens should remain
+	if tb.Count() != 500 {
+		t.Errorf("Expected 500 valid tokens after cleanup, got %d", tb.Count())
+	}
+
+	// Verify only valid tokens remain
+	tb.mutex.RLock()
+	defer tb.mutex.RUnlock()
+	for token, expiry := range tb.tokens {
+		if time.Now().After(expiry) {
+			t.Errorf("Found expired token after cleanup: %s", token)
+		}
+	}
+}
+
+func TestTokenBlacklistOldestEviction(t *testing.T) {
+	tb := NewTokenBlacklist()
+	
+	// Add tokens at capacity with different expiration times
+	baseTime := time.Now()
+	oldestToken := "oldest"
+	
+	// Add oldest token first
+	tb.Add(oldestToken, baseTime.Add(time.Hour))
+	
+	// Fill up to capacity with newer tokens
+	for i := 0; i < 999; i++ {
+		tb.Add(fmt.Sprintf("token%d", i), baseTime.Add(time.Hour*2))
+	}
+
+	// Add a new token that should evict the oldest
+	newToken := "newest"
+	tb.Add(newToken, baseTime.Add(time.Hour*3))
+
+	// Verify oldest token was evicted
+	if tb.IsBlacklisted(oldestToken) {
+		t.Error("Oldest token should have been evicted")
+	}
+
+	// Verify newest token is present
+	if !tb.IsBlacklisted(newToken) {
+		t.Error("Newest token should be present")
+	}
+}
+
+func TestTokenBlacklistMemoryUsage(t *testing.T) {
+	tb := NewTokenBlacklist()
+	iterations := 10000
+
+	// Force initial GC
+	runtime.GC()
+	
+	// Record initial memory stats
+	var m1, m2 runtime.MemStats
+	runtime.ReadMemStats(&m1)
+
+	// Simulate heavy usage
+	for i := 0; i < iterations; i++ {
+		// Add new token
+		tb.Add(fmt.Sprintf("token%d", i), time.Now().Add(time.Hour))
+		
+		// Periodically check blacklisted status
+		if i%100 == 0 {
+			tb.IsBlacklisted(fmt.Sprintf("token%d", i-50))
+		}
+		
+		// Periodically cleanup
+		if i%1000 == 0 {
+			tb.Cleanup()
+		}
+	}
+
+	// Force GC and wait for it to complete
+	runtime.GC()
+	time.Sleep(100 * time.Millisecond)
+	runtime.ReadMemStats(&m2)
+
+	// Check memory growth (using HeapAlloc for more accurate measurement)
+	memoryGrowth := int64(m2.HeapAlloc - m1.HeapAlloc)
+	maxAllowedGrowth := int64(2 * 1024 * 1024) // 2MB max growth
+
+	if memoryGrowth > maxAllowedGrowth {
+		t.Logf("Initial HeapAlloc: %d, Final HeapAlloc: %d", m1.HeapAlloc, m2.HeapAlloc)
+		t.Errorf("Excessive memory growth: %d bytes", memoryGrowth)
+	}
+
+	// Verify size stayed within limits
+	if tb.Count() > 1000 {
+		t.Errorf("Blacklist exceeded max size: %d", tb.Count())
+	}
+}
+
+func TestConcurrentTokenBlacklistOperations(t *testing.T) {
+	tb := NewTokenBlacklist()
+	iterations := 1000
+	concurrency := 10
+	done := make(chan bool)
+
+	// Start multiple goroutines performing operations
+	for i := 0; i < concurrency; i++ {
+		go func(id int) {
+			for j := 0; j < iterations; j++ {
+				// Add tokens
+				token := fmt.Sprintf("token%d-%d", id, j)
+				tb.Add(token, time.Now().Add(time.Hour))
+
+				// Check blacklist status
+				tb.IsBlacklisted(token)
+
+				// Periodic cleanup
+				if j%100 == 0 {
+					tb.Cleanup()
+				}
+			}
+			done <- true
+		}(i)
+	}
+
+	// Wait for all goroutines to complete
+	for i := 0; i < concurrency; i++ {
+		<-done
+	}
+
+	// Verify size constraints were maintained
+	if tb.Count() > 1000 {
+		t.Errorf("Blacklist exceeded max size under concurrent operations: %d", tb.Count())
+	}
+}
+
+func TestTokenCacheMemoryUsage(t *testing.T) {
+	tc := NewTokenCache()
+	iterations := 10000
+
+	// Force initial GC
+	runtime.GC()
+	
+	// Record initial memory stats
+	var m1, m2 runtime.MemStats
+	runtime.ReadMemStats(&m1)
+
+	// Simulate heavy cache usage
+	for i := 0; i < iterations; i++ {
+		claims := map[string]interface{}{
+			"sub": fmt.Sprintf("user%d", i),
+			"exp": time.Now().Add(time.Hour).Unix(),
+		}
+		
+		// Add to cache
+		tc.Set(fmt.Sprintf("token%d", i), claims, time.Hour)
+		
+		// Periodically retrieve
+		if i%100 == 0 {
+			tc.Get(fmt.Sprintf("token%d", i-50))
+		}
+		
+		// Periodically cleanup
+		if i%1000 == 0 {
+			tc.Cleanup()
+		}
+	}
+
+	// Force GC and wait for it to complete
+	runtime.GC()
+	time.Sleep(100 * time.Millisecond)
+	runtime.ReadMemStats(&m2)
+
+	// Check memory growth (using HeapAlloc for more accurate measurement)
+	memoryGrowth := int64(m2.HeapAlloc - m1.HeapAlloc)
+	maxAllowedGrowth := int64(2 * 1024 * 1024) // 2MB max growth
+
+	if memoryGrowth > maxAllowedGrowth {
+		t.Logf("Initial HeapAlloc: %d, Final HeapAlloc: %d", m1.HeapAlloc, m2.HeapAlloc)
+		t.Errorf("Excessive cache memory growth: %d bytes", memoryGrowth)
+	}
+
+	// Verify cache size stayed within limits
+	if len(tc.cache.items) > tc.cache.maxSize {
+		t.Errorf("Cache exceeded max size: %d", len(tc.cache.items))
+	}
+}

main.go

@@ -39,6 +39,7 @@ type TraefikOidc struct {
 	issuerURL                  string
 	revocationURL              string
 	jwkCache                   JWKCacheInterface
+	metadataCache              *MetadataCache
 	tokenBlacklist             *TokenBlacklist
 	jwksURL                    string
 	clientID                   string
@@ -62,7 +63,6 @@ type TraefikOidc struct {
 	extractClaimsFunc          func(tokenString string) (map[string]interface{}, error)
 	initComplete               chan struct{}
 	endSessionURL              string
-	baseURL                    string
 	postLogoutRedirectURI      string
 	sessionManager             *SessionManager
 }
@@ -82,8 +82,6 @@ var defaultExcludedURLs = map[string]struct{}{
 	"/favicon": {},
 }
 
-var newTicker = time.NewTicker
-
 // VerifyToken verifies the provided JWT token
 func (t *TraefikOidc) VerifyToken(token string) error {
 	t.logger.Debugf("Verifying token")
@@ -228,6 +226,13 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h
 		httpClient = &http.Client{
 			Timeout:   time.Second * 15, // Reduced timeout
 			Transport: transport,
+			CheckRedirect: func(req *http.Request, via []*http.Request) error {
+				// Always follow redirects for OIDC endpoints
+				if len(via) >= 50 {
+					return fmt.Errorf("stopped after 50 redirects")
+				}
+				return nil
+			},
 		}
 	}
 
@@ -249,6 +254,7 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h
 		}(),
 		tokenBlacklist:        NewTokenBlacklist(),
 		jwkCache:              &JWKCache{},
+		metadataCache:         NewMetadataCache(),
 		clientID:              config.ClientID,
 		clientSecret:          config.ClientSecret,
 		forceHTTPS:            config.ForceHTTPS,
@@ -264,7 +270,7 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h
 	// Assign the initialized logger
 	t.logger = logger
 
-	t.sessionManager = NewSessionManager(config.SessionEncryptionKey, config.ForceHTTPS, t.logger)
+	t.sessionManager, _ = NewSessionManager(config.SessionEncryptionKey, config.ForceHTTPS, t.logger)
 	t.extractClaimsFunc = extractClaims
 	t.exchangeCodeForTokenFunc = t.exchangeCodeForToken
 	t.initiateAuthenticationFunc = func(rw http.ResponseWriter, req *http.Request, session *SessionData, redirectURL string) {
@@ -288,40 +294,55 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h
 func (t *TraefikOidc) initializeMetadata(providerURL string) {
 	t.logger.Debug("Starting provider metadata discovery")
 
-	// Keep retrying until successful
-	backoff := time.Second
-	maxBackoff := 30 * time.Second
-	for {
-		metadata, err := discoverProviderMetadata(providerURL, t.httpClient, t.logger)
+	// Get metadata from cache or fetch it
+	metadata, err := t.metadataCache.GetMetadata(providerURL, t.httpClient, t.logger)
+	if err != nil {
+		t.logger.Errorf("Failed to get provider metadata: %v", err)
+		return
+	}
 
+	if metadata != nil {
+		t.logger.Debug("Successfully initialized provider metadata")
+		t.jwksURL = metadata.JWKSURL
+		t.authURL = metadata.AuthURL
+		t.tokenURL = metadata.TokenURL
+		t.issuerURL = metadata.Issuer
+		t.revocationURL = metadata.RevokeURL
+		t.endSessionURL = metadata.EndSessionURL
+
+		// Start metadata refresh goroutine
+		go t.startMetadataRefresh(providerURL)
+
+		// Only close channel on success
+		close(t.initComplete)
+		return
+	}
+
+	t.logger.Error("Received nil metadata")
+}
+
+// startMetadataRefresh periodically refreshes the OIDC metadata
+func (t *TraefikOidc) startMetadataRefresh(providerURL string) {
+	ticker := time.NewTicker(1 * time.Hour)
+	defer ticker.Stop()
+
+	for range ticker.C {
+		t.logger.Debug("Refreshing OIDC metadata")
+		metadata, err := t.metadataCache.GetMetadata(providerURL, t.httpClient, t.logger)
 		if err != nil {
-			t.logger.Errorf("Failed to discover provider metadata: %v, retrying in %v", err, backoff)
-			time.Sleep(backoff)
-
-			// Exponential backoff with max
-			backoff *= 2
-			if backoff > maxBackoff {
-				backoff = maxBackoff
-			}
+			t.logger.Errorf("Failed to refresh metadata: %v", err)
 			continue
 		}
 
 		if metadata != nil {
-			t.logger.Debug("Successfully initialized provider metadata")
 			t.jwksURL = metadata.JWKSURL
 			t.authURL = metadata.AuthURL
 			t.tokenURL = metadata.TokenURL
 			t.issuerURL = metadata.Issuer
 			t.revocationURL = metadata.RevokeURL
 			t.endSessionURL = metadata.EndSessionURL
-
-			// Only close channel on success
-			close(t.initComplete)
-			return
+			t.logger.Debug("Successfully refreshed metadata")
 		}
-
-		t.logger.Error("Received nil metadata, retrying")
-		time.Sleep(backoff)
 	}
 }
 
@@ -512,7 +533,35 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
 
 	// Set user information in headers
 	req.Header.Set("X-Forwarded-User", email)
-
+	
+	// Set OIDC-specific headers
+	req.Header.Set("X-Auth-Request-Redirect", req.URL.RequestURI())
+	req.Header.Set("X-Auth-Request-User", email)
+	if idToken := session.GetAccessToken(); idToken != "" {
+		req.Header.Set("X-Auth-Request-Token", idToken)
+	}
+	
+	// Set security headers
+	rw.Header().Set("X-Frame-Options", "DENY")
+	rw.Header().Set("X-Content-Type-Options", "nosniff")
+	rw.Header().Set("X-XSS-Protection", "1; mode=block")
+	rw.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin")
+	
+	// Set CORS headers
+	origin := req.Header.Get("Origin")
+	if origin != "" {
+		rw.Header().Set("Access-Control-Allow-Origin", origin)
+		rw.Header().Set("Access-Control-Allow-Credentials", "true")
+		rw.Header().Set("Access-Control-Allow-Methods", "GET, POST, OPTIONS")
+		rw.Header().Set("Access-Control-Allow-Headers", "Authorization, Content-Type")
+		
+		// Handle preflight requests
+		if req.Method == "OPTIONS" {
+			rw.WriteHeader(http.StatusOK)
+			return
+		}
+	}
+	
 	// Process the request
 	t.next.ServeHTTP(rw, req)
 }
@@ -531,9 +580,6 @@ func (t *TraefikOidc) determineExcludedURL(currentRequest string) bool {
 
 // determineScheme determines the scheme (http or https) of the request
 func (t *TraefikOidc) determineScheme(req *http.Request) string {
-	if t.forceHTTPS {
-		return "https"
-	}
 	if scheme := req.Header.Get("X-Forwarded-Proto"); scheme != "" {
 		return scheme
 	}
@@ -602,14 +648,17 @@ func (t *TraefikOidc) isUserAuthenticated(session *SessionData) (bool, bool, boo
 // defaultInitiateAuthentication initiates the authentication process
 func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req *http.Request, session *SessionData, redirectURL string) {
 	// Generate CSRF token and nonce
-	csrfToken := uuid.New().String()
+	csrfToken := uuid.NewString()
 	nonce, err := generateNonce()
 	if err != nil {
 		http.Error(rw, "Failed to generate nonce", http.StatusInternalServerError)
 		return
 	}
 
-	// Set session values
+	// Clear any existing session data to avoid stale state causing redirect loops
+	session.Clear(req, rw)
+
+	// Set new session values
 	session.SetCSRF(csrfToken)
 	session.SetNonce(nonce)
 	session.SetIncomingPath(req.URL.RequestURI())
@@ -621,7 +670,7 @@ func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req
 		return
 	}
 
-	// Build and redirect to auth URL
+	// Build and redirect to authentication URL
 	authURL := t.buildAuthURL(redirectURL, csrfToken, nonce)
 	http.Redirect(rw, req, authURL, http.StatusFound)
 }
@@ -642,17 +691,59 @@ func (t *TraefikOidc) buildAuthURL(redirectURL, state, nonce string) string {
 	if len(t.scopes) > 0 {
 		params.Set("scope", strings.Join(t.scopes, " "))
 	}
+
+	// Ensure authURL is absolute
+	if !strings.HasPrefix(t.authURL, "http://") && !strings.HasPrefix(t.authURL, "https://") {
+		// Extract issuer base URL
+		issuerURL, err := url.Parse(t.issuerURL)
+		if err == nil {
+			return fmt.Sprintf("%s://%s%s?%s", 
+				issuerURL.Scheme, 
+				issuerURL.Host, 
+				t.authURL,
+				params.Encode())
+		}
+	}
 	return t.authURL + "?" + params.Encode()
 }
 
 // startTokenCleanup starts the token cleanup goroutine
 func (t *TraefikOidc) startTokenCleanup() {
-	ticker := newTicker(1 * time.Minute)
+	ctx, cancel := context.WithCancel(context.Background())
+	ticker := time.NewTicker(15 * time.Second) // More frequent cleanup
+	
 	go func() {
+		defer ticker.Stop()
+		defer cancel()
+		
 		for range ticker.C {
-			t.logger.Debug("Cleaning up token cache")
-			t.tokenCache.Cleanup()
-			t.tokenBlacklist.Cleanup()
+			t.logger.Debug("Starting token cleanup cycle")
+			
+			// Run cleanup in a separate goroutine with shorter timeout
+			cleanupCtx, cleanupCancel := context.WithTimeout(ctx, 5*time.Second)
+			done := make(chan struct{})
+			
+			go func() {
+				defer close(done)
+				// Clean up in smaller batches to prevent long-running operations
+				t.tokenCache.Cleanup()
+				t.tokenBlacklist.Cleanup()
+				
+				// Force garbage collection after cleanup
+				runtime.GC()
+			}()
+			
+			// Wait for cleanup to complete or timeout
+			select {
+			case <-cleanupCtx.Done():
+				if cleanupCtx.Err() == context.DeadlineExceeded {
+					t.logger.Error("Token cleanup cycle timed out")
+				}
+			case <-done:
+				t.logger.Debug("Token cleanup cycle completed successfully")
+			}
+			
+			cleanupCancel()
 		}
 	}()
 }

main_test.go

@@ -13,6 +13,7 @@ import (
 	"math/big"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strings"
 	"testing"
 	"time"
@@ -89,7 +90,7 @@ func (ts *TestSuite) Setup() {
 	}
 
 	logger := NewLogger("info")
-	ts.sessionManager = NewSessionManager("test-secret-key-that-is-at-least-32-bytes", false, logger)
+	ts.sessionManager, _ = NewSessionManager("test-secret-key-that-is-at-least-32-bytes", false, logger)
 
 	// Common TraefikOidc instance
 	ts.tOidc = &TraefikOidc{
@@ -619,7 +620,7 @@ func TestHandleCallback(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			logger := NewLogger("info")
-			sessionManager := NewSessionManager("test-secret-key-that-is-at-least-32-bytes", false, logger)
+			sessionManager, _ := NewSessionManager("test-secret-key-that-is-at-least-32-bytes", false, logger)
 
 			// Create a new instance for each test to avoid state carryover
 			tOidc := &TraefikOidc{
@@ -924,7 +925,7 @@ func TestHandleLogout(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			logger := NewLogger("info")
-			sessionManager := NewSessionManager("test-secret-key-that-is-at-least-32-bytes", false, logger)
+			sessionManager, _ := NewSessionManager("test-secret-key-that-is-at-least-32-bytes", false, logger)
 			tOidc := &TraefikOidc{
 				revocationURL:  mockRevocationServer.URL,
 				endSessionURL:  tc.endSessionURL,
@@ -1213,7 +1214,7 @@ func TestHandleExpiredToken(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			logger := NewLogger("info")
-			sessionManager := NewSessionManager("test-secret-key-that-is-at-least-32-bytes", false, logger)
+			sessionManager, _ := NewSessionManager("test-secret-key-that-is-at-least-32-bytes", false, logger)
 
 			tOidc := &TraefikOidc{
 				sessionManager: sessionManager,
@@ -1647,6 +1648,111 @@ func TestServeHTTPRolesAndGroups(t *testing.T) {
 
 // Helper function to compare string slices
 
+// TestExchangeTokensWithRedirects tests the token exchange process with redirects
+func TestExchangeTokensWithRedirects(t *testing.T) {
+	ts := &TestSuite{t: t}
+	ts.Setup()
+
+	tests := []struct {
+		name          string
+		setupServer   func() *httptest.Server
+		expectError   bool
+		errorContains string
+	}{
+		{
+			name: "Successful token exchange with redirects",
+			setupServer: func() *httptest.Server {
+				redirectCount := 0
+				return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					if redirectCount < 3 {
+						// Set a cookie before redirecting
+						http.SetCookie(w, &http.Cookie{
+							Name:  fmt.Sprintf("redirect-cookie-%d", redirectCount),
+							Value: "test-value",
+						})
+						redirectCount++
+						w.Header().Set("Location", r.URL.String())
+						w.WriteHeader(http.StatusFound)
+						return
+					}
+
+					// Verify all cookies from previous redirects are present
+					cookies := r.Cookies()
+					if len(cookies) != 3 {
+						t.Errorf("Expected 3 cookies, got %d", len(cookies))
+					}
+					for i := 0; i < 3; i++ {
+						found := false
+						expectedName := fmt.Sprintf("redirect-cookie-%d", i)
+						for _, cookie := range cookies {
+							if cookie.Name == expectedName {
+								found = true
+								break
+							}
+						}
+						if !found {
+							t.Errorf("Cookie %s not found", expectedName)
+						}
+					}
+
+					// Return successful token response
+					w.Header().Set("Content-Type", "application/json")
+					json.NewEncoder(w).Encode(TokenResponse{
+						IDToken:      "test.id.token",
+						AccessToken:  "test-access-token",
+						TokenType:    "Bearer",
+						ExpiresIn:    3600,
+						RefreshToken: "test-refresh-token",
+					})
+				}))
+			},
+			expectError: false,
+		},
+		{
+			name: "Too many redirects",
+			setupServer: func() *httptest.Server {
+				return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					w.Header().Set("Location", r.URL.String())
+					w.WriteHeader(http.StatusFound)
+				}))
+			},
+			expectError:   true,
+			errorContains: "stopped after 50 redirects",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			server := tc.setupServer()
+			defer server.Close()
+
+			// Configure the test instance
+			tOidc := ts.tOidc
+			tOidc.tokenURL = server.URL
+
+			// Test token exchange
+			response, err := tOidc.exchangeTokens(context.Background(), "authorization_code", "test-code", "http://callback")
+
+			if tc.expectError {
+				if err == nil {
+					t.Error("Expected error but got nil")
+				} else if !strings.Contains(err.Error(), tc.errorContains) {
+					t.Errorf("Expected error containing %q, got %q", tc.errorContains, err.Error())
+				}
+			} else {
+				if err != nil {
+					t.Errorf("Unexpected error: %v", err)
+				}
+				if response == nil {
+					t.Error("Expected token response but got nil")
+				} else if response.IDToken != "test.id.token" {
+					t.Errorf("Expected ID token %q, got %q", "test.id.token", response.IDToken)
+				}
+			}
+		})
+	}
+}
+
 func stringSliceEqual(a, b []string) bool {
 	if len(a) != len(b) {
 		return false
@@ -1659,6 +1765,96 @@ func stringSliceEqual(a, b []string) bool {
 	return true
 }
 
+// TestBuildAuthURL tests the buildAuthURL function with various URL scenarios
+func TestBuildAuthURL(t *testing.T) {
+	ts := &TestSuite{t: t}
+	ts.Setup()
+
+	tests := []struct {
+		name           string
+		authURL        string
+		issuerURL      string
+		redirectURL    string
+		state          string
+		nonce         string
+		expectedPrefix string
+	}{
+		{
+			name:           "Absolute Auth URL",
+			authURL:        "https://auth.example.com/oauth/authorize",
+			issuerURL:      "https://auth.example.com",
+			redirectURL:    "https://app.example.com/callback",
+			state:          "test-state",
+			nonce:         "test-nonce",
+			expectedPrefix: "https://auth.example.com/oauth/authorize?",
+		},
+		{
+			name:           "Relative Auth URL",
+			authURL:        "/oidc/auth",
+			issuerURL:      "https://logto.example.com",
+			redirectURL:    "https://app.example.com/callback",
+			state:          "test-state",
+			nonce:         "test-nonce",
+			expectedPrefix: "https://logto.example.com/oidc/auth?",
+		},
+		{
+			name:           "Relative Auth URL with Different Issuer",
+			authURL:        "/sign-in",
+			issuerURL:      "https://auth.example.com:8443",
+			redirectURL:    "https://app.example.com/callback",
+			state:          "test-state",
+			nonce:         "test-nonce",
+			expectedPrefix: "https://auth.example.com:8443/sign-in?",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Configure the test instance
+			tOidc := ts.tOidc
+			tOidc.authURL = tc.authURL
+			tOidc.issuerURL = tc.issuerURL
+
+			// Call buildAuthURL
+			result := tOidc.buildAuthURL(tc.redirectURL, tc.state, tc.nonce)
+
+			// Verify the URL starts with the expected prefix
+			if !strings.HasPrefix(result, tc.expectedPrefix) {
+				t.Errorf("Expected URL to start with %q, got %q", tc.expectedPrefix, result)
+			}
+
+			// Parse the resulting URL to verify query parameters
+			parsedURL, err := url.Parse(result)
+			if err != nil {
+				t.Fatalf("Failed to parse resulting URL: %v", err)
+			}
+
+			query := parsedURL.Query()
+			expectedParams := map[string]string{
+				"client_id":     tOidc.clientID,
+				"response_type": "code",
+				"redirect_uri":  tc.redirectURL,
+				"state":         tc.state,
+				"nonce":         tc.nonce,
+			}
+
+			for key, expected := range expectedParams {
+				if got := query.Get(key); got != expected {
+					t.Errorf("Expected %s=%q, got %q", key, expected, got)
+				}
+			}
+
+			// Verify scopes are present and correct
+			if len(tOidc.scopes) > 0 {
+				expectedScopes := strings.Join(tOidc.scopes, " ")
+				if got := query.Get("scope"); got != expectedScopes {
+					t.Errorf("Expected scope=%q, got %q", expectedScopes, got)
+				}
+			}
+		})
+	}
+}
+
 // TestDefaultInitiateAuthentication_PreservesQueryParameters tests that defaultInitiateAuthentication preserves query parameters in the incoming path.
 func TestDefaultInitiateAuthentication_PreservesQueryParameters(t *testing.T) {
 	ts := &TestSuite{t: t}

metadata_cache.go

@@ -0,0 +1,54 @@
+package traefikoidc
+
+import (
+	"fmt"
+	"net/http"
+	"sync"
+	"time"
+)
+
+// MetadataCache provides thread-safe caching for OIDC provider metadata
+type MetadataCache struct {
+	metadata  *ProviderMetadata
+	expiresAt time.Time
+	mutex     sync.RWMutex
+}
+
+// NewMetadataCache creates a new metadata cache instance
+func NewMetadataCache() *MetadataCache {
+	return &MetadataCache{}
+}
+
+// GetMetadata retrieves the metadata from cache or fetches it if expired
+func (c *MetadataCache) GetMetadata(providerURL string, httpClient *http.Client, logger *Logger) (*ProviderMetadata, error) {
+	c.mutex.RLock()
+	if c.metadata != nil && time.Now().Before(c.expiresAt) {
+		defer c.mutex.RUnlock()
+		return c.metadata, nil
+	}
+	c.mutex.RUnlock()
+
+	c.mutex.Lock()
+	defer c.mutex.Unlock()
+
+	// Double-check after acquiring write lock
+	if c.metadata != nil && time.Now().Before(c.expiresAt) {
+		return c.metadata, nil
+	}
+
+	metadata, err := discoverProviderMetadata(providerURL, httpClient, logger)
+	if err != nil {
+		if c.metadata != nil {
+			// On error, extend current cache by 5 minutes to prevent thundering herd
+			c.expiresAt = time.Now().Add(5 * time.Minute)
+			logger.Errorf("Failed to refresh metadata, using cached version for 5 more minutes: %v", err)
+			return c.metadata, nil
+		}
+		return nil, fmt.Errorf("failed to fetch provider metadata: %w", err)
+	}
+
+	c.metadata = metadata
+	c.expiresAt = time.Now().Add(1 * time.Hour)
+
+	return metadata, nil
+}

semver.yaml

@@ -0,0 +1,10 @@
+version: 1
+force:
+  existing: true
+wording:
+  patch:
+    - patch-release
+  minor:
+    - minor-release
+  major:
+    - breaking

session.go

@@ -16,13 +16,14 @@ import (
 	"github.com/gorilla/sessions"
 )
 
-// generateSecureRandomString creates a cryptographically secure random string of specified length
-func generateSecureRandomString(length int) string {
+// generateSecureRandomString creates a cryptographically secure random string of specified length.
+// It returns the generated string or an error if random generation fails.
+func generateSecureRandomString(length int) (string, error) {
 	bytes := make([]byte, length)
 	if _, err := rand.Read(bytes); err != nil {
-		panic("failed to generate random string")
+		return "", fmt.Errorf("failed to generate random bytes: %w", err)
 	}
-	return hex.EncodeToString(bytes)
+	return hex.EncodeToString(bytes), nil
 }
 
 // Cookie names and configuration constants used for session management
@@ -55,7 +56,7 @@ const (
 	minEncryptionKeyLength = 32
 )
 
-// compressToken compresses a token using gzip and base64 encodes it
+// compressToken compresses a token using gzip and base64 encodes it.
 func compressToken(token string) string {
 	var b bytes.Buffer
 	gz := gzip.NewWriter(&b)
@@ -68,7 +69,7 @@ func compressToken(token string) string {
 	return base64.StdEncoding.EncodeToString(b.Bytes())
 }
 
-// decompressToken decompresses a base64 encoded gzipped token
+// decompressToken decompresses a base64 encoded gzipped token.
 func decompressToken(compressed string) string {
 	data, err := base64.StdEncoding.DecodeString(compressed)
 	if err != nil {
@@ -91,18 +92,18 @@ func decompressToken(compressed string) string {
 
 // SessionManager handles the management of multiple session cookies for OIDC authentication.
 // It provides functionality for storing and retrieving authentication state, tokens,
-// and other session-related data across multiple cookies to handle large tokens.
+// and other session-related data across multiple cookies.
 type SessionManager struct {
-	// store is the underlying session store for cookie management
+	// store is the underlying session store for cookie management.
 	store sessions.Store
 
-	// forceHTTPS enforces secure cookie attributes regardless of request scheme
+	// forceHTTPS enforces secure cookie attributes regardless of request scheme.
 	forceHTTPS bool
 
-	// logger provides structured logging capabilities
+	// logger provides structured logging capabilities.
 	logger *Logger
 
-	// sessionPool is a sync.Pool for reusing SessionData objects
+	// sessionPool is a sync.Pool for reusing SessionData objects.
 	sessionPool sync.Pool
 }
 
@@ -112,11 +113,11 @@ type SessionManager struct {
 //   - forceHTTPS: When true, forces secure cookie attributes regardless of request scheme
 //   - logger: Logger instance for recording session-related events
 //
-// The manager handles session creation, storage, and cookie security settings.
-func NewSessionManager(encryptionKey string, forceHTTPS bool, logger *Logger) *SessionManager {
-	// Validate encryption key length
+// Returns an error if the encryption key does not meet minimum length requirements.
+func NewSessionManager(encryptionKey string, forceHTTPS bool, logger *Logger) (*SessionManager, error) {
+	// Validate encryption key length.
 	if len(encryptionKey) < minEncryptionKeyLength {
-		panic(fmt.Sprintf("encryption key must be at least %d bytes long", minEncryptionKeyLength))
+		return nil, fmt.Errorf("encryption key must be at least %d bytes long", minEncryptionKeyLength)
 	}
 
 	sm := &SessionManager{
@@ -125,7 +126,7 @@ func NewSessionManager(encryptionKey string, forceHTTPS bool, logger *Logger) *S
 		logger:     logger,
 	}
 
-	// Initialize session pool
+	// Initialize session pool.
 	sm.sessionPool.New = func() interface{} {
 		return &SessionData{
 			manager:            sm,
@@ -134,12 +135,12 @@ func NewSessionManager(encryptionKey string, forceHTTPS bool, logger *Logger) *S
 		}
 	}
 
-	return sm
+	return sm, nil
 }
 
 // getSessionOptions returns secure session options configured for the current request.
 // Parameters:
-//   - isSecure: Whether the current request is using HTTPS
+//   - isSecure: Whether the current request is using HTTPS.
 //
 // The options ensure cookies are:
 //   - HTTP-only (not accessible via JavaScript)
@@ -161,7 +162,7 @@ func (sm *SessionManager) getSessionOptions(isSecure bool) *sessions.Options {
 // and combines them into a single SessionData structure for easy access.
 // Returns an error if any session component cannot be loaded.
 func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) {
-	// Get session from pool
+	// Get session from pool.
 	sessionData := sm.sessionPool.Get().(*SessionData)
 	sessionData.request = r
 
@@ -172,11 +173,10 @@ func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) {
 		return nil, fmt.Errorf("failed to get main session: %w", err)
 	}
 
-	// Check for absolute session timeout
+	// Check for absolute session timeout.
 	if createdAt, ok := sessionData.mainSession.Values["created_at"].(int64); ok {
 		if time.Since(time.Unix(createdAt, 0)) > absoluteSessionTimeout {
-			// Session has expired
-			sm.sessionPool.Put(sessionData)
+			sessionData.Clear(r, nil)
 			return nil, fmt.Errorf("session expired")
 		}
 	}
@@ -193,7 +193,7 @@ func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) {
 		return nil, fmt.Errorf("failed to get refresh token session: %w", err)
 	}
 
-	// Clear and reuse chunk maps
+	// Clear and reuse chunk maps.
 	for k := range sessionData.accessTokenChunks {
 		delete(sessionData.accessTokenChunks, k)
 	}
@@ -201,7 +201,7 @@ func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) {
 		delete(sessionData.refreshTokenChunks, k)
 	}
 
-	// Retrieve chunked token sessions
+	// Retrieve chunked token sessions.
 	sm.getTokenChunkSessions(r, accessTokenCookie, sessionData.accessTokenChunks)
 	sm.getTokenChunkSessions(r, refreshTokenCookie, sessionData.refreshTokenChunks)
 
@@ -218,7 +218,6 @@ func (sm *SessionManager) getTokenChunkSessions(r *http.Request, baseName string
 		sessionName := fmt.Sprintf("%s_%d", baseName, i)
 		session, err := sm.store.Get(r, sessionName)
 		if err != nil || session.IsNew {
-			// No more sessions
 			break
 		}
 		chunks[i] = session
@@ -230,27 +229,27 @@ func (sm *SessionManager) getTokenChunkSessions(r *http.Request, baseName string
 // and potentially large access and refresh tokens that may need to be
 // split across multiple cookies due to browser size limitations.
 type SessionData struct {
-	// manager is the SessionManager that created this SessionData
+	// manager is the SessionManager that created this SessionData.
 	manager *SessionManager
 
-	// request is the current HTTP request associated with this session
+	// request is the current HTTP request associated with this session.
 	request *http.Request
 
-	// mainSession stores authentication state and basic user info
+	// mainSession stores authentication state and basic user info.
 	mainSession *sessions.Session
 
-	// accessSession stores the primary access token cookie
+	// accessSession stores the primary access token cookie.
 	accessSession *sessions.Session
 
-	// refreshSession stores the primary refresh token cookie
+	// refreshSession stores the primary refresh token cookie.
 	refreshSession *sessions.Session
 
 	// accessTokenChunks stores additional chunks of the access token
-	// when it exceeds the maximum cookie size
+	// when it exceeds the maximum cookie size.
 	accessTokenChunks map[int]*sessions.Session
 
 	// refreshTokenChunks stores additional chunks of the refresh token
-	// when it exceeds the maximum cookie size
+	// when it exceeds the maximum cookie size.
 	refreshTokenChunks map[int]*sessions.Session
 }
 
@@ -261,28 +260,28 @@ type SessionData struct {
 func (sd *SessionData) Save(r *http.Request, w http.ResponseWriter) error {
 	isSecure := strings.HasPrefix(r.URL.Scheme, "https") || sd.manager.forceHTTPS
 
-	// Set options for all sessions
+	// Set options for all sessions.
 	options := sd.manager.getSessionOptions(isSecure)
 	sd.mainSession.Options = options
 	sd.accessSession.Options = options
 	sd.refreshSession.Options = options
 
-	// Save main session
+	// Save main session.
 	if err := sd.mainSession.Save(r, w); err != nil {
 		return fmt.Errorf("failed to save main session: %w", err)
 	}
 
-	// Save access token session
+	// Save access token session.
 	if err := sd.accessSession.Save(r, w); err != nil {
 		return fmt.Errorf("failed to save access token session: %w", err)
 	}
 
-	// Save refresh token session
+	// Save refresh token session.
 	if err := sd.refreshSession.Save(r, w); err != nil {
 		return fmt.Errorf("failed to save refresh token session: %w", err)
 	}
 
-	// Save access token chunks
+	// Save access token chunks.
 	for _, session := range sd.accessTokenChunks {
 		session.Options = options
 		if err := session.Save(r, w); err != nil {
@@ -290,7 +289,7 @@ func (sd *SessionData) Save(r *http.Request, w http.ResponseWriter) error {
 		}
 	}
 
-	// Save refresh token chunks
+	// Save refresh token chunks.
 	for _, session := range sd.refreshTokenChunks {
 		session.Options = options
 		if err := session.Save(r, w); err != nil {
@@ -302,10 +301,8 @@ func (sd *SessionData) Save(r *http.Request, w http.ResponseWriter) error {
 }
 
 // Clear removes all session data by expiring all cookies and clearing their values.
-// This is typically used during logout to ensure all session data is properly cleaned up.
-// It handles both main session data and any token chunks that may exist.
 func (sd *SessionData) Clear(r *http.Request, w http.ResponseWriter) error {
-	// Clear and expire all sessions
+	// Clear and expire all sessions.
 	sd.mainSession.Options.MaxAge = -1
 	sd.accessSession.Options.MaxAge = -1
 	sd.refreshSession.Options.MaxAge = -1
@@ -320,7 +317,7 @@ func (sd *SessionData) Clear(r *http.Request, w http.ResponseWriter) error {
 		delete(sd.refreshSession.Values, k)
 	}
 
-	// Clear chunk sessions
+	// Clear chunk sessions.
 	sd.clearTokenChunks(r, sd.accessTokenChunks)
 	sd.clearTokenChunks(r, sd.refreshTokenChunks)
 
@@ -329,15 +326,13 @@ func (sd *SessionData) Clear(r *http.Request, w http.ResponseWriter) error {
 		err = sd.Save(r, w)
 	}
 
-	// Return session to pool
+	// Return session to pool.
 	sd.manager.sessionPool.Put(sd)
 
 	return err
 }
 
 // clearTokenChunks removes all session chunks for a given token type.
-// It expires the cookies and removes all stored values to ensure
-// no token data remains after logout or token invalidation.
 func (sd *SessionData) clearTokenChunks(r *http.Request, chunks map[int]*sessions.Session) {
 	for _, session := range chunks {
 		session.Options.MaxAge = -1
@@ -348,15 +343,13 @@ func (sd *SessionData) clearTokenChunks(r *http.Request, chunks map[int]*session
 }
 
 // GetAuthenticated returns whether the current session is authenticated.
-// Returns true if the user has successfully completed OIDC authentication
-// and the session hasn't expired, false otherwise.
 func (sd *SessionData) GetAuthenticated() bool {
 	auth, _ := sd.mainSession.Values["authenticated"].(bool)
 	if !auth {
 		return false
 	}
 
-	// Check session expiration
+	// Check session expiration.
 	createdAt, ok := sd.mainSession.Values["created_at"].(int64)
 	if !ok {
 		return false
@@ -365,21 +358,21 @@ func (sd *SessionData) GetAuthenticated() bool {
 }
 
 // SetAuthenticated updates the session's authentication status and rotates session ID.
-// This should be called after successful OIDC authentication or during logout.
-// Session ID rotation helps prevent session fixation attacks.
-func (sd *SessionData) SetAuthenticated(value bool) {
+// Returns an error if generating a new session ID fails.
+func (sd *SessionData) SetAuthenticated(value bool) error {
 	if value {
-		// Generate new session ID and set creation time
-		sd.mainSession.ID = generateSecureRandomString(32)
+		id, err := generateSecureRandomString(32)
+		if err != nil {
+			return fmt.Errorf("failed to generate secure session id: %w", err)
+		}
+		sd.mainSession.ID = id
 		sd.mainSession.Values["created_at"] = time.Now().Unix()
 	}
 	sd.mainSession.Values["authenticated"] = value
+	return nil
 }
 
 // GetAccessToken retrieves the complete access token from the session.
-// If the token was split into chunks due to size limitations, it will
-// automatically reassemble the complete token from all chunks.
-// Returns an empty string if no token is found.
 func (sd *SessionData) GetAccessToken() string {
 	token, _ := sd.accessSession.Values["token"].(string)
 	if token != "" {
@@ -390,7 +383,7 @@ func (sd *SessionData) GetAccessToken() string {
 		return token
 	}
 
-	// Reassemble token from chunks
+	// Reassemble token from chunks.
 	if len(sd.accessTokenChunks) == 0 {
 		return ""
 	}
@@ -414,45 +407,23 @@ func (sd *SessionData) GetAccessToken() string {
 }
 
 // SetAccessToken stores the access token in the session.
-// If the token exceeds maxCookieSize, it is automatically compressed and split into
-// multiple cookie chunks to handle large tokens while staying within
-// browser cookie size limits. Any existing token or chunks are cleared
-// before setting the new token.
-// expireAccessTokenChunks expires any existing access token chunk cookies
-func (sd *SessionData) expireAccessTokenChunks(w http.ResponseWriter) {
-	for i := 0; ; i++ {
-		sessionName := fmt.Sprintf("%s_%d", accessTokenCookie, i)
-		session, err := sd.manager.store.Get(sd.request, sessionName)
-		if err != nil || session.IsNew {
-			break
-		}
-		// Expire the cookie
-		session.Options.MaxAge = -1
-		session.Values = make(map[interface{}]interface{})
-		// Save expired cookie
-		if err := session.Save(sd.request, w); err != nil {
-			sd.manager.logger.Errorf("Failed to save expired cookie: %v", err)
-		}
-	}
-}
-
 func (sd *SessionData) SetAccessToken(token string) {
-	// Expire any existing chunk cookies first
+	// Expire any existing chunk cookies first.
 	if sd.request != nil {
-		sd.expireAccessTokenChunks(nil) // Will be saved when Save() is called
+		sd.expireAccessTokenChunks(nil) // Will be saved when Save() is called.
 	}
 
-	// Clear and prepare chunks map for new token
+	// Clear and prepare chunks map for new token.
 	sd.accessTokenChunks = make(map[int]*sessions.Session)
 
-	// Compress token
+	// Compress token.
 	compressed := compressToken(token)
 
 	if len(compressed) <= maxCookieSize {
 		sd.accessSession.Values["token"] = compressed
 		sd.accessSession.Values["compressed"] = true
 	} else {
-		// Split compressed token into chunks
+		// Split compressed token into chunks.
 		sd.accessSession.Values["token"] = ""
 		sd.accessSession.Values["compressed"] = true
 		chunks := splitIntoChunks(compressed, maxCookieSize)
@@ -466,9 +437,6 @@ func (sd *SessionData) SetAccessToken(token string) {
 }
 
 // GetRefreshToken retrieves the complete refresh token from the session.
-// If the token was split into chunks due to size limitations, it will
-// automatically reassemble the complete token from all chunks.
-// Returns an empty string if no token is found.
 func (sd *SessionData) GetRefreshToken() string {
 	token, _ := sd.refreshSession.Values["token"].(string)
 	if token != "" {
@@ -479,7 +447,7 @@ func (sd *SessionData) GetRefreshToken() string {
 		return token
 	}
 
-	// Reassemble token from chunks
+	// Reassemble token from chunks.
 	if len(sd.refreshTokenChunks) == 0 {
 		return ""
 	}
@@ -503,45 +471,23 @@ func (sd *SessionData) GetRefreshToken() string {
 }
 
 // SetRefreshToken stores the refresh token in the session.
-// If the token exceeds maxCookieSize, it is automatically compressed and split into
-// multiple cookie chunks to handle large tokens while staying within
-// browser cookie size limits. Any existing token or chunks are cleared
-// before setting the new token.
-// expireRefreshTokenChunks expires any existing refresh token chunk cookies
-func (sd *SessionData) expireRefreshTokenChunks(w http.ResponseWriter) {
-	for i := 0; ; i++ {
-		sessionName := fmt.Sprintf("%s_%d", refreshTokenCookie, i)
-		session, err := sd.manager.store.Get(sd.request, sessionName)
-		if err != nil || session.IsNew {
-			break
-		}
-		// Expire the cookie
-		session.Options.MaxAge = -1
-		session.Values = make(map[interface{}]interface{})
-		// Save expired cookie
-		if err := session.Save(sd.request, w); err != nil {
-			sd.manager.logger.Errorf("Failed to save expired cookie: %v", err)
-		}
-	}
-}
-
 func (sd *SessionData) SetRefreshToken(token string) {
-	// Expire any existing chunk cookies first
+	// Expire any existing chunk cookies first.
 	if sd.request != nil {
-		sd.expireRefreshTokenChunks(nil) // Will be saved when Save() is called
+		sd.expireRefreshTokenChunks(nil) // Will be saved when Save() is called.
 	}
 
-	// Clear and prepare chunks map for new token
+	// Clear and prepare chunks map for new token.
 	sd.refreshTokenChunks = make(map[int]*sessions.Session)
 
-	// Compress token
+	// Compress token.
 	compressed := compressToken(token)
 
 	if len(compressed) <= maxCookieSize {
 		sd.refreshSession.Values["token"] = compressed
 		sd.refreshSession.Values["compressed"] = true
 	} else {
-		// Split compressed token into chunks
+		// Split compressed token into chunks.
 		sd.refreshSession.Values["token"] = ""
 		sd.refreshSession.Values["compressed"] = true
 		chunks := splitIntoChunks(compressed, maxCookieSize)
@@ -554,13 +500,43 @@ func (sd *SessionData) SetRefreshToken(token string) {
 	}
 }
 
+// expireAccessTokenChunks expires any existing access token chunk cookies.
+func (sd *SessionData) expireAccessTokenChunks(w http.ResponseWriter) {
+	for i := 0; ; i++ {
+		sessionName := fmt.Sprintf("%s_%d", accessTokenCookie, i)
+		session, err := sd.manager.store.Get(sd.request, sessionName)
+		if err != nil || session.IsNew {
+			break
+		}
+		session.Options.MaxAge = -1
+		session.Values = make(map[interface{}]interface{})
+		if w != nil {
+			if err := session.Save(sd.request, w); err != nil {
+				sd.manager.logger.Errorf("failed to save expired access token cookie: %v", err)
+			}
+		}
+	}
+}
+
+// expireRefreshTokenChunks expires any existing refresh token chunk cookies.
+func (sd *SessionData) expireRefreshTokenChunks(w http.ResponseWriter) {
+	for i := 0; ; i++ {
+		sessionName := fmt.Sprintf("%s_%d", refreshTokenCookie, i)
+		session, err := sd.manager.store.Get(sd.request, sessionName)
+		if err != nil || session.IsNew {
+			break
+		}
+		session.Options.MaxAge = -1
+		session.Values = make(map[interface{}]interface{})
+		if w != nil {
+			if err := session.Save(sd.request, w); err != nil {
+				sd.manager.logger.Errorf("failed to save expired refresh token cookie: %v", err)
+			}
+		}
+	}
+}
+
 // splitIntoChunks splits a string into chunks of specified size.
-// This is used internally to handle large tokens that exceed cookie size limits.
-// Parameters:
-//   - s: The string to split
-//   - chunkSize: Maximum size of each chunk
-//
-// Returns an array of string chunks, each no larger than chunkSize.
 func splitIntoChunks(s string, chunkSize int) []string {
 	var chunks []string
 	for len(s) > 0 {
@@ -576,64 +552,45 @@ func splitIntoChunks(s string, chunkSize int) []string {
 }
 
 // GetCSRF retrieves the CSRF token from the session.
-// This token is used to prevent cross-site request forgery attacks
-// by ensuring requests originate from the authenticated user.
-// Returns an empty string if no CSRF token is found.
 func (sd *SessionData) GetCSRF() string {
 	csrf, _ := sd.mainSession.Values["csrf"].(string)
 	return csrf
 }
 
 // SetCSRF stores a new CSRF token in the session.
-// This should be called when initiating authentication to generate
-// a new token for the authentication flow.
 func (sd *SessionData) SetCSRF(token string) {
 	sd.mainSession.Values["csrf"] = token
 }
 
 // GetNonce retrieves the nonce value from the session.
-// The nonce is used to prevent replay attacks in the OIDC flow
-// by ensuring the token received matches the authentication request.
-// Returns an empty string if no nonce is found.
 func (sd *SessionData) GetNonce() string {
 	nonce, _ := sd.mainSession.Values["nonce"].(string)
 	return nonce
 }
 
 // SetNonce stores a new nonce value in the session.
-// This should be called when initiating authentication to generate
-// a new nonce for the OIDC authentication flow.
 func (sd *SessionData) SetNonce(nonce string) {
 	sd.mainSession.Values["nonce"] = nonce
 }
 
 // GetEmail retrieves the authenticated user's email address from the session.
-// The email is typically extracted from the OIDC ID token claims.
-// Returns an empty string if no email is found.
 func (sd *SessionData) GetEmail() string {
 	email, _ := sd.mainSession.Values["email"].(string)
 	return email
 }
 
 // SetEmail stores the user's email address in the session.
-// This should be called after successful authentication when
-// processing the OIDC ID token claims.
 func (sd *SessionData) SetEmail(email string) {
 	sd.mainSession.Values["email"] = email
 }
 
-// GetIncomingPath retrieves the original request path that triggered
-// the authentication flow. This is used to redirect the user back
-// to their intended destination after successful authentication.
-// Returns an empty string if no path was stored.
+// GetIncomingPath retrieves the original request path that triggered the authentication flow.
 func (sd *SessionData) GetIncomingPath() string {
 	path, _ := sd.mainSession.Values["incoming_path"].(string)
 	return path
 }
 
-// SetIncomingPath stores the original request path that triggered
-// the authentication flow. This should be called before redirecting
-// to the OIDC provider to remember where to send the user afterward.
+// SetIncomingPath stores the original request path that triggered the authentication flow.
 func (sd *SessionData) SetIncomingPath(path string) {
 	sd.mainSession.Values["incoming_path"] = path
 }

session_test.go

@@ -5,14 +5,8 @@ import (
 	"net/http/httptest"
 	"strings"
 	"testing"
-	"time"
 )
 
-func init() {
-	// Initialize random seed
-	rand.Seed(time.Now().UnixNano())
-}
-
 // generateRandomString creates a random string of specified length
 func generateRandomString(length int) string {
 	const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
@@ -83,7 +77,7 @@ func TestCookiePrefix(t *testing.T) {
 	req := httptest.NewRequest("GET", "/test", nil)
 	rr := httptest.NewRecorder()
 
-	sm := NewSessionManager("0123456789abcdef0123456789abcdef", true, NewLogger("debug"))
+	sm, _ := NewSessionManager("0123456789abcdef0123456789abcdef", true, NewLogger("debug"))
 	session, err := sm.GetSession(req)
 	if err != nil {
 		t.Fatalf("Failed to get session: %v", err)
@@ -117,7 +111,7 @@ func TestTokenRefreshCleanup(t *testing.T) {
 	req := httptest.NewRequest("GET", "/test", nil)
 	rr := httptest.NewRecorder()
 
-	sm := NewSessionManager("0123456789abcdef0123456789abcdef", true, NewLogger("debug"))
+	sm, _ := NewSessionManager("0123456789abcdef0123456789abcdef", true, NewLogger("debug"))
 	session, err := sm.GetSession(req)
 	if err != nil {
 		t.Fatalf("Failed to get session: %v", err)