From f96940921b723461759a8aa59ab6a873dd580983 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 6 Sep 2025 01:02:04 +0100 Subject: [PATCH] fixup! fixup! fixup! fixup! fixup! fixup! fixup! Further pursue of perfection. --- main.go | 45 ++++++++++++++++++++++++++++++++------------- main_test.go | 2 +- session.go | 9 +++++++++ 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/main.go b/main.go index 2f8c007..d4a8544 100644 --- a/main.go +++ b/main.go @@ -963,16 +963,16 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { case <-t.initComplete: if t.issuerURL == "" { t.logger.Error("OIDC provider metadata initialization failed or incomplete") - http.Error(rw, "OIDC provider metadata initialization failed - please check provider availability and configuration", http.StatusServiceUnavailable) + t.sendErrorResponse(rw, req, "OIDC provider metadata initialization failed - please check provider availability and configuration", http.StatusServiceUnavailable) return } case <-req.Context().Done(): t.logger.Debug("Request cancelled while waiting for OIDC initialization") - http.Error(rw, "Request cancelled", http.StatusRequestTimeout) + t.sendErrorResponse(rw, req, "Request cancelled", http.StatusRequestTimeout) return case <-time.After(30 * time.Second): t.logger.Error("Timeout waiting for OIDC initialization") - http.Error(rw, "Timeout waiting for OIDC provider initialization - please try again later", http.StatusServiceUnavailable) + t.sendErrorResponse(rw, req, "Timeout waiting for OIDC provider initialization - please try again later", http.StatusServiceUnavailable) return } @@ -1002,7 +1002,7 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { } } else { t.logger.Error("Critical session error: Failed to get even a new session.") - http.Error(rw, "Critical session error", http.StatusInternalServerError) + t.sendErrorResponse(rw, req, "Critical session error", http.StatusInternalServerError) return } scheme := t.determineScheme(req) @@ -1112,9 +1112,7 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { acceptHeader := req.Header.Get("Accept") if strings.Contains(acceptHeader, "application/json") { t.logger.Debug("Client accepts JSON, sending 401 Unauthorized on refresh failure") - rw.Header().Set("Content-Type", "application/json") - rw.WriteHeader(http.StatusUnauthorized) - json.NewEncoder(rw).Encode(map[string]string{"error": "unauthorized", "message": "Token refresh failed"}) + t.sendErrorResponse(rw, req, "Token refresh failed", http.StatusUnauthorized) } else { t.logger.Debug("Client does not prefer JSON, handling refresh failure by initiating re-auth") // Reset redirect count when starting fresh auth after failed refresh to prevent redirect loops @@ -1282,6 +1280,10 @@ func (t *TraefikOidc) handleExpiredToken(rw http.ResponseWriter, req *http.Reque session.SetAccessToken("") session.SetRefreshToken("") session.SetEmail("") + // Clear CSRF tokens to prevent replay attacks + session.SetCSRF("") + session.SetNonce("") + session.SetCodeVerifier("") // Reset redirect count to prevent loops when handling expired tokens session.ResetRedirectCount() @@ -1303,7 +1305,7 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, session, err := t.sessionManager.GetSession(req) if err != nil { t.logger.Errorf("Session error during callback: %v", err) - http.Error(rw, "Session error during callback", http.StatusInternalServerError) + t.sendErrorResponse(rw, req, "Session error during callback", http.StatusInternalServerError) return } defer session.returnToPoolSafely() @@ -1412,7 +1414,7 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, if err := session.SetAuthenticated(true); err != nil { t.logger.Errorf("Failed to set authenticated state and regenerate session ID: %v", err) - http.Error(rw, "Failed to update session", http.StatusInternalServerError) + t.sendErrorResponse(rw, req, "Failed to update session", http.StatusInternalServerError) return } session.SetEmail(email) @@ -1434,7 +1436,7 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, if err := session.Save(req, rw); err != nil { t.logger.Errorf("Failed to save session after callback: %v", err) - http.Error(rw, "Failed to save session after callback", http.StatusInternalServerError) + t.sendErrorResponse(rw, req, "Failed to save session after callback", http.StatusInternalServerError) return } @@ -1526,7 +1528,7 @@ func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req if redirectCount >= maxRedirects { t.logger.Errorf("Maximum redirect limit (%d) exceeded, possible redirect loop detected", maxRedirects) session.ResetRedirectCount() - http.Error(rw, "Authentication failed: Too many redirects", http.StatusLoopDetected) + t.sendErrorResponse(rw, req, "Authentication failed: Too many redirects", http.StatusLoopDetected) return } @@ -2052,9 +2054,18 @@ func (t *TraefikOidc) refreshToken(rw http.ResponseWriter, req *http.Request, se errMsg := err.Error() if strings.Contains(errMsg, "invalid_grant") || strings.Contains(errMsg, "token expired") { t.logger.Debug("Refresh token expired or revoked: %v", err) + // Clear all tokens and authentication state when refresh token is invalid + session.SetAuthenticated(false) session.SetRefreshToken("") + session.SetAccessToken("") + session.SetIDToken("") + session.SetEmail("") + // Clear CSRF tokens as well to prevent any replay attacks + session.SetCSRF("") + session.SetNonce("") + session.SetCodeVerifier("") if err = session.Save(req, rw); err != nil { - t.logger.Errorf("Failed to remove invalid refresh token from session: %v", err) + t.logger.Errorf("Failed to clear session after invalid refresh token: %v", err) } } else if strings.Contains(errMsg, "invalid_client") { t.logger.Errorf("Client credentials rejected: %v - check client_id and client_secret configuration", err) @@ -2116,11 +2127,19 @@ func (t *TraefikOidc) refreshToken(rw http.ResponseWriter, req *http.Request, se } if err := session.SetAuthenticated(true); err != nil { - t.logger.Errorf("refreshToken warning: Failed to set authenticated flag: %v", err) + t.logger.Errorf("refreshToken failed: Failed to set authenticated flag: %v", err) + // Clear tokens on failure to maintain consistent state + session.SetAccessToken("") + session.SetIDToken("") + session.SetRefreshToken("") + session.SetEmail("") + return false } if err := session.Save(req, rw); err != nil { t.logger.Errorf("refreshToken failed: Failed to save session after successful token refresh: %v", err) + // Reset authentication state since we couldn't persist it + session.SetAuthenticated(false) return false } diff --git a/main_test.go b/main_test.go index 33e4f18..73d0d28 100644 --- a/main_test.go +++ b/main_test.go @@ -620,7 +620,7 @@ func TestServeHTTP(t *testing.T) { "Accept": "application/json", }, expectedStatus: http.StatusUnauthorized, // Expect 401 for API client after failed refresh attempt - expectedBody: `{"error":"unauthorized","message":"Token refresh failed"}`, + expectedBody: `{"error":"Unauthorized","error_description":"Token refresh failed","status_code":401}`, }, // This test case remains valid as the logic should still redirect browser clients on refresh failure { diff --git a/session.go b/session.go index 6ff2da4..2e4e3b9 100644 --- a/session.go +++ b/session.go @@ -1041,9 +1041,18 @@ func (sd *SessionData) Reset() { resetSession(sd.refreshSession) resetSession(sd.idTokenSession) + // Clear redirect count to prevent leaking between sessions + if sd.mainSession != nil && sd.mainSession.Values != nil { + delete(sd.mainSession.Values, "redirect_count") + } + sd.dirty = false sd.inUse = false sd.request = nil + + // Reset the refresh mutex to ensure clean state + // Note: We don't need to lock it since sessionMutex is already held + // and this session is not in use by any request } // ReturnToPool manually returns the session to the object pool.