From 2c902eaafba25c381b25e4778541c38a2d4de439 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 5 Sep 2025 00:22:13 +0100 Subject: [PATCH] fixup! fixup! Further pursue of perfection. --- cache.go | 8 +++ cache_optimized.go | 16 ++++- coverage_boost_minimal_test.go | 23 ++----- coverage_push_75_test.go | 4 +- error_recovery.go | 122 +++++++++++++++++++++++++++++++++ helpers.go | 4 +- http_client_factory.go | 7 +- jwk.go | 2 +- jwt.go | 2 +- main.go | 93 +++++++++++++++++-------- session.go | 20 +++--- 11 files changed, 238 insertions(+), 63 deletions(-) diff --git a/cache.go b/cache.go index 423af95..6273132 100644 --- a/cache.go +++ b/cache.go @@ -144,9 +144,17 @@ func (c *Cache) Delete(key string) { // This method is called automatically by the background cleanup task but can also // be called manually for immediate cleanup of expired entries. func (c *Cache) Cleanup() { + if c == nil { + return + } + c.mutex.Lock() defer c.mutex.Unlock() + if c.items == nil { + return + } + now := time.Now() for key, item := range c.items { if now.After(item.ExpiresAt) { diff --git a/cache_optimized.go b/cache_optimized.go index 2290f0b..3fd0201 100644 --- a/cache_optimized.go +++ b/cache_optimized.go @@ -172,20 +172,30 @@ func (c *OptimizedCache) Delete(key string) { // It scans the LRU list for expired entries and enforces memory limits // by evicting least recently used items if necessary. func (c *OptimizedCache) Cleanup() { + if c == nil { + return + } + c.mutex.Lock() defer c.mutex.Unlock() + if c.items == nil || c.head == nil || c.tail == nil { + return + } + now := time.Now() toRemove := make([]*OptimizedCacheEntry, 0, len(c.items)/10) - for entry := c.head.next; entry != c.tail; entry = entry.next { - if now.After(entry.ExpiresAt) { + for entry := c.head.next; entry != nil && entry != c.tail; entry = entry.next { + if entry != nil && now.After(entry.ExpiresAt) { toRemove = append(toRemove, entry) } } for _, entry := range toRemove { - c.removeEntry(entry) + if entry != nil { + c.removeEntry(entry) + } } for c.currentMemoryBytes > c.maxMemoryBytes && len(c.items) > 0 { diff --git a/coverage_boost_minimal_test.go b/coverage_boost_minimal_test.go index d89f371..17a6cb4 100644 --- a/coverage_boost_minimal_test.go +++ b/coverage_boost_minimal_test.go @@ -364,33 +364,24 @@ func TestReplayCacheBasic(t *testing.T) { // Test audience verification func TestAudienceVerify(t *testing.T) { // Exact match - claims1 := map[string]interface{}{ - "aud": "my-client", - } - if err := verifyAudience(claims1, "my-client"); err != nil { + if err := verifyAudience("my-client", "my-client"); err != nil { t.Errorf("Exact match should succeed: %v", err) } // Array contains - claims2 := map[string]interface{}{ - "aud": []interface{}{"client1", "my-client", "client2"}, - } - if err := verifyAudience(claims2, "my-client"); err != nil { + audArray := []interface{}{"client1", "my-client", "client2"} + if err := verifyAudience(audArray, "my-client"); err != nil { t.Errorf("Array contains should succeed: %v", err) } // Mismatch - claims3 := map[string]interface{}{ - "aud": "other-client", - } - if err := verifyAudience(claims3, "my-client"); err == nil { + if err := verifyAudience("other-client", "my-client"); err == nil { t.Error("Mismatch should fail") } - // Missing - claims4 := map[string]interface{}{} - if err := verifyAudience(claims4, "my-client"); err == nil { - t.Error("Missing aud should fail") + // Missing/nil audience + if err := verifyAudience(nil, "my-client"); err == nil { + t.Error("Nil aud should fail") } } diff --git a/coverage_push_75_test.go b/coverage_push_75_test.go index d2c5822..0464896 100644 --- a/coverage_push_75_test.go +++ b/coverage_push_75_test.go @@ -38,7 +38,9 @@ func TestInputValidatorExtended(t *testing.T) { func TestSessionManagerCreation(t *testing.T) { logger := NewLogger("debug") - sm, err := NewSessionManager("test-secret", false, "Lax", logger) + // Use a 32-byte encryption key as required + encryptionKey := "12345678901234567890123456789012" + sm, err := NewSessionManager(encryptionKey, false, "Lax", logger) if err != nil { t.Fatalf("NewSessionManager failed: %v", err) } diff --git a/error_recovery.go b/error_recovery.go index 5177d6f..ef3ad88 100644 --- a/error_recovery.go +++ b/error_recovery.go @@ -589,6 +589,128 @@ func (e *HTTPError) Error() string { return fmt.Sprintf("HTTP %d: %s", e.StatusCode, e.Message) } +// OIDCError represents OIDC-specific errors with context information. +// It provides structured error reporting for authentication and authorization failures. +type OIDCError struct { + // Code identifies the specific error type + Code string + // Message provides a human-readable description + Message string + // Context contains additional error context (e.g., provider, session details) + Context map[string]interface{} + // Cause is the underlying error that caused this error + Cause error +} + +// Error returns the string representation of the OIDC error. +// Implements the error interface. +func (e *OIDCError) Error() string { + if e.Cause != nil { + return fmt.Sprintf("OIDC error [%s]: %s - caused by: %v", e.Code, e.Message, e.Cause) + } + return fmt.Sprintf("OIDC error [%s]: %s", e.Code, e.Message) +} + +// Unwrap returns the underlying error for error chain unwrapping. +func (e *OIDCError) Unwrap() error { + return e.Cause +} + +// SessionError represents session-related errors with context. +// Used for session management, validation, and storage errors. +type SessionError struct { + // Operation describes what session operation failed + Operation string + // Message provides a human-readable description + Message string + // SessionID identifies the session (if available) + SessionID string + // Cause is the underlying error that caused this error + Cause error +} + +// Error returns the string representation of the session error. +// Implements the error interface. +func (e *SessionError) Error() string { + if e.Cause != nil { + return fmt.Sprintf("Session error in %s: %s - caused by: %v", e.Operation, e.Message, e.Cause) + } + return fmt.Sprintf("Session error in %s: %s", e.Operation, e.Message) +} + +// Unwrap returns the underlying error for error chain unwrapping. +func (e *SessionError) Unwrap() error { + return e.Cause +} + +// TokenError represents token-related errors with validation context. +// Used for JWT validation, token refresh, and token format errors. +type TokenError struct { + // TokenType identifies the type of token (id_token, access_token, refresh_token) + TokenType string + // Reason describes why the token is invalid + Reason string + // Message provides a human-readable description + Message string + // Cause is the underlying error that caused this error + Cause error +} + +// Error returns the string representation of the token error. +// Implements the error interface. +func (e *TokenError) Error() string { + if e.Cause != nil { + return fmt.Sprintf("Token error (%s) - %s: %s - caused by: %v", e.TokenType, e.Reason, e.Message, e.Cause) + } + return fmt.Sprintf("Token error (%s) - %s: %s", e.TokenType, e.Reason, e.Message) +} + +// Unwrap returns the underlying error for error chain unwrapping. +func (e *TokenError) Unwrap() error { + return e.Cause +} + +// NewOIDCError creates a new OIDC error with context. +func NewOIDCError(code, message string, cause error) *OIDCError { + return &OIDCError{ + Code: code, + Message: message, + Context: make(map[string]interface{}), + Cause: cause, + } +} + +// WithContext adds context information to the OIDC error. +func (e *OIDCError) WithContext(key string, value interface{}) *OIDCError { + e.Context[key] = value + return e +} + +// NewSessionError creates a new session error with operation context. +func NewSessionError(operation, message string, cause error) *SessionError { + return &SessionError{ + Operation: operation, + Message: message, + Cause: cause, + } +} + +// WithSessionID adds session ID to the session error. +func (e *SessionError) WithSessionID(sessionID string) *SessionError { + e.SessionID = sessionID + return e +} + +// NewTokenError creates a new token error with type and reason. +func NewTokenError(tokenType, reason, message string, cause error) *TokenError { + return &TokenError{ + TokenType: tokenType, + Reason: reason, + Message: message, + Cause: cause, + } +} + // GracefulDegradation implements graceful degradation patterns for service resilience. // It provides fallback mechanisms when primary services are unavailable and monitors // service health to automatically recover when services become available again. diff --git a/helpers.go b/helpers.go index 9b691d8..e02ac8a 100644 --- a/helpers.go +++ b/helpers.go @@ -263,7 +263,9 @@ func (tc *TokenCache) Delete(token string) { // This is typically called automatically but can be invoked manually for // removing expired token entries. func (tc *TokenCache) Cleanup() { - tc.cache.Cleanup() + if tc != nil && tc.cache != nil { + tc.cache.Cleanup() + } } // Close stops the cleanup goroutine and releases resources. diff --git a/http_client_factory.go b/http_client_factory.go index 6ea5cc3..2b78662 100644 --- a/http_client_factory.go +++ b/http_client_factory.go @@ -40,7 +40,7 @@ type HTTPClientConfig struct { // DefaultHTTPClientConfig returns the default configuration for general use func DefaultHTTPClientConfig() HTTPClientConfig { return HTTPClientConfig{ - Timeout: 5 * time.Second, + Timeout: 30 * time.Second, MaxRedirects: 10, UseCookieJar: false, DialTimeout: 5 * time.Second, @@ -63,8 +63,9 @@ func DefaultHTTPClientConfig() HTTPClientConfig { // TokenHTTPClientConfig returns configuration optimized for token operations func TokenHTTPClientConfig() HTTPClientConfig { config := DefaultHTTPClientConfig() - config.MaxRedirects = 50 // Token endpoints may redirect more - config.UseCookieJar = true // Enable cookie jar for token operations + config.Timeout = 10 * time.Second // Shorter timeout for token operations + config.MaxRedirects = 50 // Token endpoints may redirect more + config.UseCookieJar = true // Enable cookie jar for token operations return config } diff --git a/jwk.go b/jwk.go index 196105c..f407dab 100644 --- a/jwk.go +++ b/jwk.go @@ -133,7 +133,7 @@ func (c *JWKCache) GetJWKS(ctx context.Context, jwksURL string, httpClient *http // Cleanup removes expired entries from the JWK cache to free memory. // It delegates to the internal cache's cleanup method. func (c *JWKCache) Cleanup() { - if c.internalCache != nil { + if c != nil && c.internalCache != nil { c.internalCache.Cleanup() } } diff --git a/jwt.go b/jwt.go index da6e833..f257372 100644 --- a/jwt.go +++ b/jwt.go @@ -78,7 +78,7 @@ func getReplayCacheStats() (size int, maxSize int) { defer replayCacheMu.RUnlock() if replayCache == nil { - return 0, 0 + return 0, 10000 } return 0, 10000 diff --git a/main.go b/main.go index ae4ebee..7a2a729 100644 --- a/main.go +++ b/main.go @@ -751,16 +751,7 @@ func (t *TraefikOidc) updateMetadataEndpoints(metadata *ProviderMetadata) { func (t *TraefikOidc) startMetadataRefresh(providerURL string) { ticker := time.NewTicker(2 * time.Hour) - if t.goroutineWG != nil { - t.goroutineWG.Add(1) - } - go func() { - defer func() { - if t.goroutineWG != nil { - t.goroutineWG.Done() - } - }() defer ticker.Stop() consecutiveFailures := 0 @@ -925,7 +916,17 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { if !t.metadataRefreshStarted && t.providerURL != "" { t.metadataRefreshStarted = true - go t.startMetadataRefresh(t.providerURL) + if t.goroutineWG != nil { + t.goroutineWG.Add(1) + } + go func() { + defer func() { + if t.goroutineWG != nil { + t.goroutineWG.Done() + } + }() + t.startMetadataRefresh(t.providerURL) + }() } } t.firstRequestMutex.Unlock() @@ -1801,44 +1802,80 @@ func (t *TraefikOidc) validateHost(host string) error { // It runs periodic cleanup of token cache, JWK cache, and session chunks. // Includes panic recovery to ensure stability. func (t *TraefikOidc) startTokenCleanup() { + if t == nil { + return + } + + // Capture values to avoid race conditions + tokenCache := t.tokenCache + jwkCache := t.jwkCache + sessionManager := t.sessionManager + logger := t.logger + goroutineWG := t.goroutineWG + stopChan := t.tokenCleanupStopChan + ctx := t.ctx + ticker := time.NewTicker(1 * time.Minute) - if t.goroutineWG != nil { - t.goroutineWG.Add(1) + if goroutineWG != nil { + goroutineWG.Add(1) } go func() { defer func() { - if t.goroutineWG != nil { - t.goroutineWG.Done() + if goroutineWG != nil { + goroutineWG.Done() } ticker.Stop() if r := recover(); r != nil { - t.logger.Errorf("Token cleanup goroutine panic recovered: %v", r) + if logger != nil { + logger.Errorf("Token cleanup goroutine panic recovered: %v", r) + } } }() for { select { case <-ticker.C: - t.logger.Debug("Starting token cleanup cycle") - if t.tokenCache != nil { - t.tokenCache.Cleanup() + if logger != nil { + logger.Debug("Starting token cleanup cycle") } - if t.jwkCache != nil { - t.jwkCache.Cleanup() + if tokenCache != nil { + tokenCache.Cleanup() } - if t.sessionManager != nil { - t.sessionManager.PeriodicChunkCleanup() - - t.logger.Debug("Running session health monitoring") + if jwkCache != nil { + jwkCache.Cleanup() + } + if sessionManager != nil { + sessionManager.PeriodicChunkCleanup() + if logger != nil { + logger.Debug("Running session health monitoring") + } } - case <-t.tokenCleanupStopChan: - t.logger.Debug("Token cleanup goroutine stopped.") + case <-func() <-chan struct{} { + if stopChan != nil { + return stopChan + } + // Return a channel that never receives anything + c := make(chan struct{}) + return c + }(): + if logger != nil { + logger.Debug("Token cleanup goroutine stopped.") + } return - case <-t.ctx.Done(): - t.logger.Debug("Token cleanup goroutine stopped due to context cancellation.") + case <-func() <-chan struct{} { + if ctx != nil { + return ctx.Done() + } + // Return a channel that never receives anything + c := make(chan struct{}) + return c + }(): + if logger != nil { + logger.Debug("Token cleanup goroutine stopped due to context cancellation.") + } return } } diff --git a/session.go b/session.go index 7f01a68..88f6c8b 100644 --- a/session.go +++ b/session.go @@ -275,21 +275,27 @@ func NewSessionManager(encryptionKey string, forceHTTPS bool, cookieDomain strin // It cleans up orphaned token chunks, expired sessions, and unused pool objects. // This helps maintain performance and prevent cookie accumulation in client browsers. func (sm *SessionManager) PeriodicChunkCleanup() { + if sm == nil || sm.logger == nil { + return + } + sm.logger.Debug("Starting comprehensive session cleanup cycle") cleanupStart := time.Now() var orphanedChunks, expiredSessions, cleanupErrors int - if cookieStore, ok := sm.store.(*sessions.CookieStore); ok { - sm.logger.Debug("Running session store cleanup") - _ = cookieStore + if sm.store != nil { + if cookieStore, ok := sm.store.(*sessions.CookieStore); ok { + sm.logger.Debug("Running session store cleanup") + _ = cookieStore + } } poolCleaned := 0 for i := 0; i < 10; i++ { if poolSession := sm.sessionPool.Get(); poolSession != nil { - sessionData := poolSession.(*SessionData) - if sessionData != nil && !sessionData.inUse { + sessionData, ok := poolSession.(*SessionData) + if ok && sessionData != nil && !sessionData.inUse { sessionData.Reset() poolCleaned++ } @@ -861,10 +867,6 @@ func (sd *SessionData) clearAllSessionData(r *http.Request, expire bool) { // - An error if session saving fails during cleanup. func (sd *SessionData) Clear(r *http.Request, w http.ResponseWriter) error { defer func() { - if rec := recover(); rec != nil { - sd.returnToPoolSafely() - panic(rec) - } sd.returnToPoolSafely() }()