diff --git a/csrf_session_test.go b/csrf_session_test.go new file mode 100644 index 0000000..e924ea5 --- /dev/null +++ b/csrf_session_test.go @@ -0,0 +1,476 @@ +package traefikoidc + +import ( + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCSRFTokenSessionManagement tests the session management changes that fix the login loop +func TestCSRFTokenSessionManagement(t *testing.T) { + // Test that CSRF tokens persist through the authentication flow + t.Run("CSRF_Token_Persists_After_Selective_Clear", func(t *testing.T) { + // Create a session manager + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Create initial request + req := httptest.NewRequest("GET", "http://example.com/test", nil) + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Set initial values + csrfToken := "critical-csrf-token" + session.SetCSRF(csrfToken) + session.SetNonce("test-nonce") + session.SetAuthenticated(true) + session.SetEmail("user@example.com") + session.SetAccessToken("old-access-token") + session.SetRefreshToken("old-refresh-token") + session.SetIDToken("old-id-token") + + // Save session + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Get cookies + cookies := rec.Result().Cookies() + + // Create new request with cookies (simulating redirect back) + req2 := httptest.NewRequest("GET", "http://example.com/test2", nil) + for _, cookie := range cookies { + req2.AddCookie(cookie) + } + + // Get session again + session2, err := sessionManager.GetSession(req2) + require.NoError(t, err) + + // Verify all values are there + assert.Equal(t, csrfToken, session2.GetCSRF()) + assert.Equal(t, "test-nonce", session2.GetNonce()) + assert.True(t, session2.GetAuthenticated()) + + // Now perform selective clearing (as done in the fix) + session2.SetAuthenticated(false) + session2.SetEmail("") + session2.SetAccessToken("") + session2.SetRefreshToken("") + session2.SetIDToken("") + // Clear OIDC flow values from previous attempts + session2.SetNonce("") + session2.SetCodeVerifier("") + + // CRITICAL: CSRF token should still be there + assert.Equal(t, csrfToken, session2.GetCSRF(), "CSRF token must persist after selective clearing") + + // Save again + rec2 := httptest.NewRecorder() + err = session2.Save(req2, rec2) + require.NoError(t, err) + + // Verify CSRF token persists in new session + req3 := httptest.NewRequest("GET", "http://example.com/callback", nil) + for _, cookie := range rec2.Result().Cookies() { + req3.AddCookie(cookie) + } + + session3, err := sessionManager.GetSession(req3) + require.NoError(t, err) + assert.Equal(t, csrfToken, session3.GetCSRF(), "CSRF token must persist across saves") + }) + + // Test that marking session as dirty forces save + t.Run("Mark_Dirty_Forces_Session_Save", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + req := httptest.NewRequest("GET", "http://example.com/test", nil) + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Set CSRF token + csrfToken := "test-csrf-token" + session.SetCSRF(csrfToken) + + // Mark as dirty explicitly + session.MarkDirty() + + // Save should work even if no apparent changes + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Verify cookie was set + cookies := rec.Result().Cookies() + assert.NotEmpty(t, cookies, "Cookies should be set after save") + + // Find main session cookie + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie, "Main session cookie should be set") + }) + + // Test Azure-specific session handling + t.Run("Azure_Session_Cookie_Configuration", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Simulate Azure callback scenario + req := httptest.NewRequest("GET", "http://example.com/oidc/callback?code=test&state=test-csrf", nil) + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Set values as would happen in auth flow + session.SetCSRF("test-csrf") + session.SetNonce("test-nonce") + + // Save with proper cookie settings + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Check cookie attributes + cookies := rec.Result().Cookies() + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + // Azure requires SameSite=Lax for cross-site redirects + assert.Equal(t, http.SameSiteLaxMode, cookie.SameSite, "SameSite should be Lax for Azure compatibility") + assert.Equal(t, "/", cookie.Path, "Path should be root") + assert.True(t, cookie.HttpOnly, "Cookie should be HttpOnly") + // In production, Secure would be true, but false in test + } + } + }) + + // Test session continuity through auth flow + t.Run("Session_Continuity_Through_Auth_Flow", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Step 1: Initial request + req1 := httptest.NewRequest("GET", "http://example.com/protected", nil) + session1, err := sessionManager.GetSession(req1) + require.NoError(t, err) + + // Simulate auth initiation + csrfToken := "auth-flow-csrf-token" + nonce := "auth-flow-nonce" + session1.SetCSRF(csrfToken) + session1.SetNonce(nonce) + session1.SetIncomingPath("/protected") + + // Force save + session1.MarkDirty() + rec1 := httptest.NewRecorder() + err = session1.Save(req1, rec1) + require.NoError(t, err) + + cookies := rec1.Result().Cookies() + require.NotEmpty(t, cookies) + + // Step 2: Callback request with same cookies + req2 := httptest.NewRequest("GET", "http://example.com/oidc/callback?code=test&state="+csrfToken, nil) + for _, cookie := range cookies { + req2.AddCookie(cookie) + } + + session2, err := sessionManager.GetSession(req2) + require.NoError(t, err) + + // Verify session continuity + assert.Equal(t, csrfToken, session2.GetCSRF(), "CSRF token should be maintained") + assert.Equal(t, nonce, session2.GetNonce(), "Nonce should be maintained") + assert.Equal(t, "/protected", session2.GetIncomingPath(), "Incoming path should be maintained") + }) + + // Test large token handling doesn't affect CSRF + t.Run("Large_Tokens_Dont_Affect_CSRF", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + req := httptest.NewRequest("GET", "http://example.com/test", nil) + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Set CSRF first + csrfToken := "important-csrf" + session.SetCSRF(csrfToken) + + // Add large tokens that might cause chunking + largeToken := generateMockJWT(5000) + session.SetIDToken(largeToken) + session.SetAccessToken(largeToken) + + // Save + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Count cookies + cookies := rec.Result().Cookies() + mainFound := false + chunkCount := 0 + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainFound = true + } + if strings.Contains(cookie.Name, "_oidc_raczylo_") && strings.Contains(cookie.Name, "_") { + chunkCount++ + } + } + + assert.True(t, mainFound, "Main session cookie must exist") + t.Logf("Total chunks created: %d", chunkCount) + + // Verify CSRF is still accessible + req2 := httptest.NewRequest("GET", "http://example.com/test2", nil) + for _, cookie := range cookies { + req2.AddCookie(cookie) + } + + session2, err := sessionManager.GetSession(req2) + require.NoError(t, err) + assert.Equal(t, csrfToken, session2.GetCSRF(), "CSRF must be preserved with large tokens") + }) +} + +// TestAuthFlowWithoutExternalDependencies tests the auth flow without external dependencies +func TestAuthFlowWithoutExternalDependencies(t *testing.T) { + plugin := CreateConfig() + plugin.ProviderURL = "https://login.microsoftonline.com/test-tenant/v2.0" + plugin.ClientID = "test-client-id" + plugin.ClientSecret = "test-client-secret" + plugin.CallbackURL = "http://example.com/oidc/callback" + plugin.SessionEncryptionKey = "test-encryption-key-32-characters" + plugin.LogLevel = "debug" + + // Variables removed as they're not used in this test + + // We can't fully initialize TraefikOidc without network access, + // but we can test the session management directly + sessionManager, err := NewSessionManager(plugin.SessionEncryptionKey, plugin.ForceHTTPS, NewLogger(plugin.LogLevel)) + require.NoError(t, err) + + t.Run("Session_Created_On_Protected_Request", func(t *testing.T) { + req := httptest.NewRequest("GET", "http://example.com/protected", nil) + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Session should be new + assert.False(t, session.GetAuthenticated()) + + // Set auth flow values + session.SetCSRF("test-csrf-token") + session.SetNonce("test-nonce") + session.SetIncomingPath("/protected") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Should have set cookies + cookies := rec.Result().Cookies() + assert.NotEmpty(t, cookies) + }) +} + +// TestRegressionLoginLoop specifically tests the fix for issue #53 +func TestRegressionLoginLoop(t *testing.T) { + // This test verifies that the specific changes made to fix the login loop work correctly + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Simulate the exact flow that was causing the login loop + t.Run("Fix_Session_Clear_Timing", func(t *testing.T) { + // Initial request + req := httptest.NewRequest("GET", "http://example.com/protected", nil) + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Set initial session data + session.SetAuthenticated(true) + session.SetEmail("old@example.com") + session.SetAccessToken("old-token") + session.SetCSRF("existing-csrf") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + cookies := rec.Result().Cookies() + + // New request with existing session (user hits protected resource again) + req2 := httptest.NewRequest("GET", "http://example.com/protected", nil) + for _, cookie := range cookies { + req2.AddCookie(cookie) + } + + session2, err := sessionManager.GetSession(req2) + require.NoError(t, err) + + // OLD BEHAVIOR: session.Clear() would have been called here, losing CSRF + // NEW BEHAVIOR: Selective clearing + session2.SetAuthenticated(false) + session2.SetEmail("") + session2.SetAccessToken("") + session2.SetRefreshToken("") + session2.SetIDToken("") + session2.SetNonce("") + session2.SetCodeVerifier("") + + // CSRF should still exist + existingCSRF := session2.GetCSRF() + assert.Equal(t, "existing-csrf", existingCSRF, "CSRF should persist through selective clear") + + // Set new auth flow values + newCSRF := "new-csrf-for-auth" + session2.SetCSRF(newCSRF) + session2.SetNonce("new-nonce") + + // Force save + session2.MarkDirty() + rec2 := httptest.NewRecorder() + err = session2.Save(req2, rec2) + require.NoError(t, err) + + // Simulate callback + cookies2 := rec2.Result().Cookies() + req3 := httptest.NewRequest("GET", "http://example.com/oidc/callback?code=test&state="+newCSRF, nil) + for _, cookie := range cookies2 { + req3.AddCookie(cookie) + } + + session3, err := sessionManager.GetSession(req3) + require.NoError(t, err) + + // CSRF should match + assert.Equal(t, newCSRF, session3.GetCSRF(), "CSRF token should be available in callback") + }) + + t.Run("Fix_Force_Session_Save", func(t *testing.T) { + req := httptest.NewRequest("GET", "http://example.com/test", nil) + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Set CSRF but don't change authenticated status + session.SetCSRF("important-csrf") + + // Without MarkDirty(), the session might not save if the session manager + // doesn't detect the change. The fix ensures we call MarkDirty() + session.MarkDirty() + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Verify cookie was actually set + cookies := rec.Result().Cookies() + found := false + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + found = true + assert.NotEmpty(t, cookie.Value, "Cookie should have value") + } + } + assert.True(t, found, "Main session cookie must be set after MarkDirty") + }) +} + +// TestCSRFValidationTiming tests timing-sensitive CSRF validation scenarios +func TestCSRFValidationTiming(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + t.Run("Rapid_Redirect_Maintains_CSRF", func(t *testing.T) { + // Simulate rapid redirect (no delay between auth init and callback) + req1 := httptest.NewRequest("GET", "http://example.com/auth", nil) + session1, err := sessionManager.GetSession(req1) + require.NoError(t, err) + + csrfToken := "rapid-redirect-csrf" + session1.SetCSRF(csrfToken) + session1.MarkDirty() + + rec1 := httptest.NewRecorder() + err = session1.Save(req1, rec1) + require.NoError(t, err) + + // Immediate callback (no delay) + cookies := rec1.Result().Cookies() + req2 := httptest.NewRequest("GET", "http://example.com/callback", nil) + for _, cookie := range cookies { + req2.AddCookie(cookie) + } + + session2, err := sessionManager.GetSession(req2) + require.NoError(t, err) + assert.Equal(t, csrfToken, session2.GetCSRF()) + }) + + t.Run("Delayed_Redirect_Maintains_CSRF", func(t *testing.T) { + // Simulate delayed redirect (user takes time at provider) + req1 := httptest.NewRequest("GET", "http://example.com/auth", nil) + session1, err := sessionManager.GetSession(req1) + require.NoError(t, err) + + csrfToken := "delayed-redirect-csrf" + session1.SetCSRF(csrfToken) + session1.MarkDirty() + + rec1 := httptest.NewRecorder() + err = session1.Save(req1, rec1) + require.NoError(t, err) + + // Simulate delay + time.Sleep(500 * time.Millisecond) + + // Callback after delay + cookies := rec1.Result().Cookies() + req2 := httptest.NewRequest("GET", "http://example.com/callback", nil) + for _, cookie := range cookies { + req2.AddCookie(cookie) + } + + session2, err := sessionManager.GetSession(req2) + require.NoError(t, err) + assert.Equal(t, csrfToken, session2.GetCSRF(), "CSRF should persist even with delay") + }) +} + +// Helper function to generate a mock JWT of specified size +func generateMockJWT(targetSize int) string { + header := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9" + signature := "signature" + + // Calculate payload size needed + overhead := len(header) + len(signature) + 2 // 2 dots + payloadSize := targetSize - overhead + + // Create payload with padding + payload := map[string]interface{}{ + "sub": "1234567890", + "name": "Test User", + "iat": time.Now().Unix(), + "exp": time.Now().Add(time.Hour).Unix(), + "padding": strings.Repeat("x", payloadSize-100), // Leave room for JSON structure + } + + payloadJSON, _ := json.Marshal(payload) + payloadB64 := base64.RawURLEncoding.EncodeToString(payloadJSON) + + return header + "." + payloadB64 + "." + signature +} diff --git a/issue53_regression_test.go b/issue53_regression_test.go new file mode 100644 index 0000000..2469712 --- /dev/null +++ b/issue53_regression_test.go @@ -0,0 +1,239 @@ +package traefikoidc + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestIssue53Regression tests the specific issue reported in GitHub issue #53 +// where Azure OIDC authentication fails with "CSRF token missing in session" +// This was caused by incorrect HTTPS detection in reverse proxy environments +func TestIssue53Regression(t *testing.T) { + t.Run("Issue53_CSRF_Missing_In_Session_Fix", func(t *testing.T) { + // This test reproduces the exact scenario from issue #53: + // 1. User accesses app via HTTPS through Traefik + // 2. Traefik terminates SSL and forwards HTTP internally + // 3. Session cookies must be properly configured for HTTPS + // 4. CSRF token must persist through the OAuth flow + + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Step 1: Initial request to protected resource + // User accesses https://app.example.com/protected + // Traefik forwards as http://internal/protected with X-Forwarded-Proto: https + initReq := httptest.NewRequest("GET", "http://internal/protected", nil) + initReq.Header.Set("X-Forwarded-Proto", "https") + initReq.Header.Set("X-Forwarded-Host", "app.example.com") + initReq.Header.Set("User-Agent", "Mozilla/5.0") // Real browser + + // Get session and set OAuth flow data + session, err := sessionManager.GetSession(initReq) + require.NoError(t, err) + + // Set CSRF and other OAuth data + csrfToken := "csrf-token-for-azure" + nonce := "nonce-for-azure" + session.SetCSRF(csrfToken) + session.SetNonce(nonce) + session.SetCodeVerifier("pkce-verifier") + session.SetIncomingPath("/protected") + session.MarkDirty() + + // Save session - this is where the bug was + // Previously: used r.URL.Scheme which is always "http" behind proxy + // Now: uses X-Forwarded-Proto header + rec := httptest.NewRecorder() + err = session.Save(initReq, rec) + require.NoError(t, err) + + // Verify cookies are secure + cookies := rec.Result().Cookies() + require.NotEmpty(t, cookies, "Cookies must be set") + + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie, "Main session cookie must be set") + + // Critical assertions for issue #53 + assert.True(t, mainCookie.Secure, "Cookie MUST have Secure flag for HTTPS (was the bug)") + assert.Equal(t, http.SameSiteLaxMode, mainCookie.SameSite, "MUST use Lax for OAuth callbacks to work") + assert.Equal(t, "/", mainCookie.Path, "Cookie path must be root") + assert.True(t, mainCookie.HttpOnly, "Cookie must be HttpOnly") + assert.Equal(t, "app.example.com", mainCookie.Domain, "Domain should use X-Forwarded-Host") + + // Step 2: OAuth provider redirects back to callback + // Azure redirects to https://app.example.com/oidc/callback?code=...&state=... + // Traefik forwards as http://internal/oidc/callback with headers + callbackReq := httptest.NewRequest("GET", + "http://internal/oidc/callback?code=azure-auth-code&state="+csrfToken, nil) + callbackReq.Header.Set("X-Forwarded-Proto", "https") + callbackReq.Header.Set("X-Forwarded-Host", "app.example.com") + callbackReq.Header.Set("User-Agent", "Mozilla/5.0") + + // Add cookies from initial request + // Browser sends secure cookies because request is HTTPS + for _, cookie := range cookies { + callbackReq.AddCookie(cookie) + } + + // Get session in callback + callbackSession, err := sessionManager.GetSession(callbackReq) + require.NoError(t, err) + + // Verify CSRF token is present (was missing in issue #53) + retrievedCSRF := callbackSession.GetCSRF() + assert.Equal(t, csrfToken, retrievedCSRF, + "CSRF token MUST persist (was missing in issue #53)") + + // Verify other session data also persists + assert.Equal(t, nonce, callbackSession.GetNonce(), + "Nonce must persist for security") + assert.Equal(t, "pkce-verifier", callbackSession.GetCodeVerifier(), + "PKCE verifier must persist") + assert.Equal(t, "/protected", callbackSession.GetIncomingPath(), + "Original path must persist for redirect after auth") + }) + + t.Run("Issue53_Signature_Verification_With_Secure_Session", func(t *testing.T) { + // This test ensures that once the session is properly maintained, + // token signature verification works correctly for Azure tokens + + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Create authenticated session with Azure tokens + req := httptest.NewRequest("GET", "http://internal/api/data", nil) + req.Header.Set("X-Forwarded-Proto", "https") + req.Header.Set("X-Forwarded-Host", "app.example.com") + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Simulate successful Azure authentication + session.SetAuthenticated(true) + session.SetEmail("user@example.com") + // Azure may use opaque access tokens + session.SetAccessToken("opaque-azure-access-token") + session.SetIDToken("eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWUsImlhdCI6MTUxNjIzOTAyMn0.NHVaYe26MbtOYhSKkoKYdFVomg4i8ZJd8_-RU8VNbftc4TSMb4bXP3l3YlNWACwyXPGffz5aXHc6lty1Y2t4SWRqGteragsVdZufDn5BlnJl9pdR_kdVFUsra2rWKEofkZeIC4yWytE58sMIihvo9H1ScmmVwBcQP6XETqYd0aSHp1gOa9RdUPDvoXQ5oqygTqVtxaDr6wUFKrKItgBMzWIdNZ6y7O9E0DhEPTbE9rfBo6KTFsHAZnMg4k68CDp2woYIaXbmYTWcvbzIuHO7_37GT79XdIwkm95QJ7hYC9RiwrV7mesbY4PAahERJawntho0my942XheVLmGwLMBkQ") + session.SetRefreshToken("azure-refresh-token") + + // Save with proper security + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Verify session can be retrieved and tokens are intact + cookies := rec.Result().Cookies() + req2 := httptest.NewRequest("GET", "http://internal/api/data", nil) + req2.Header.Set("X-Forwarded-Proto", "https") + for _, cookie := range cookies { + req2.AddCookie(cookie) + } + + session2, err := sessionManager.GetSession(req2) + require.NoError(t, err) + + assert.True(t, session2.GetAuthenticated(), "User should remain authenticated") + assert.Equal(t, "user@example.com", session2.GetEmail()) + assert.NotEmpty(t, session2.GetAccessToken(), "Access token should persist") + assert.NotEmpty(t, session2.GetIDToken(), "ID token should persist") + assert.NotEmpty(t, session2.GetRefreshToken(), "Refresh token should persist") + }) + + t.Run("Issue53_Redirect_Loop_Prevention", func(t *testing.T) { + // This test verifies the redirect loop prevention mechanism + // that was added to handle authentication failures gracefully + + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + req := httptest.NewRequest("GET", "http://internal/protected", nil) + req.Header.Set("X-Forwarded-Proto", "https") + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Simulate multiple redirect attempts + for i := 0; i < 3; i++ { + session.IncrementRedirectCount() + } + + // Verify redirect count is tracked + count := session.GetRedirectCount() + assert.Equal(t, 3, count, "Redirect count should be tracked") + + // After successful auth, count should be reset + session.SetAuthenticated(true) + session.ResetRedirectCount() + assert.Equal(t, 0, session.GetRedirectCount(), "Count should reset after auth") + }) +} + +// TestReverseProxySameSiteHandling tests SameSite cookie attribute handling +// in different reverse proxy scenarios +func TestReverseProxySameSiteHandling(t *testing.T) { + t.Run("SameSite_Lax_For_HTTPS_OAuth", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // HTTPS request via proxy + req := httptest.NewRequest("GET", "http://internal/test", nil) + req.Header.Set("X-Forwarded-Proto", "https") + req.Header.Set("User-Agent", "Mozilla/5.0") + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + session.SetCSRF("test") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // HTTPS should use Lax mode for OAuth compatibility + cookies := rec.Result().Cookies() + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + assert.Equal(t, http.SameSiteLaxMode, cookie.SameSite, + "HTTPS should use Lax SameSite for OAuth callbacks") + break + } + } + }) + + t.Run("SameSite_Lax_For_HTTP", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Plain HTTP request (no proxy headers) + req := httptest.NewRequest("GET", "http://localhost/test", nil) + req.Header.Set("User-Agent", "Mozilla/5.0") + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + session.SetCSRF("test") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // HTTP should use Lax mode for compatibility + cookies := rec.Result().Cookies() + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + assert.Equal(t, http.SameSiteLaxMode, cookie.SameSite, + "HTTP should use Lax SameSite for compatibility") + break + } + } + }) +} diff --git a/reverse_proxy_test.go b/reverse_proxy_test.go new file mode 100644 index 0000000..d1ece3c --- /dev/null +++ b/reverse_proxy_test.go @@ -0,0 +1,357 @@ +package traefikoidc + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestReverseProxyHTTPSDetection tests that HTTPS is properly detected in reverse proxy environments +func TestReverseProxyHTTPSDetection(t *testing.T) { + t.Run("HTTPS_Detection_With_X_Forwarded_Proto", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Simulate request from reverse proxy (Traefik/nginx) + // The reverse proxy terminates SSL and forwards HTTP internally + req := httptest.NewRequest("GET", "http://example.com/test", nil) + req.Header.Set("X-Forwarded-Proto", "https") + req.Header.Set("X-Forwarded-Host", "example.com") + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Set critical session data + session.SetCSRF("important-csrf-token") + session.SetNonce("test-nonce") + + // Save session + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Verify cookies have Secure flag when X-Forwarded-Proto is https + cookies := rec.Result().Cookies() + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie, "Main session cookie should be set") + assert.True(t, mainCookie.Secure, "Cookie should have Secure flag when X-Forwarded-Proto is https") + assert.Equal(t, http.SameSiteLaxMode, mainCookie.SameSite, "Should use Lax SameSite for OAuth compatibility") + }) + + t.Run("HTTPS_Detection_Without_Headers", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Request without reverse proxy headers (direct HTTP) + req := httptest.NewRequest("GET", "http://example.com/test", nil) + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + session.SetCSRF("test-csrf") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Verify cookies don't have Secure flag for plain HTTP + cookies := rec.Result().Cookies() + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie) + assert.False(t, mainCookie.Secure, "Cookie should not have Secure flag for HTTP") + assert.Equal(t, http.SameSiteLaxMode, mainCookie.SameSite, "Should use Lax SameSite in HTTP") + }) + + t.Run("HTTPS_Detection_With_ForceHTTPS", func(t *testing.T) { + // Test with forceHTTPS enabled + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", true, NewLogger("debug")) + require.NoError(t, err) + + // Even without headers, forceHTTPS should make cookies secure + req := httptest.NewRequest("GET", "http://example.com/test", nil) + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + session.SetCSRF("forced-secure-csrf") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + cookies := rec.Result().Cookies() + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie) + assert.True(t, mainCookie.Secure, "Cookie should have Secure flag with forceHTTPS") + }) +} + +// TestCSRFPersistenceInReverseProxy tests CSRF token persistence in reverse proxy setups +func TestCSRFPersistenceInReverseProxy(t *testing.T) { + t.Run("CSRF_Persists_Through_OAuth_Flow_With_Proxy", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Step 1: Initial request to protected resource (HTTPS via proxy) + req1 := httptest.NewRequest("GET", "http://example.com/protected", nil) + req1.Header.Set("X-Forwarded-Proto", "https") + req1.Header.Set("X-Forwarded-Host", "example.com") + + session1, err := sessionManager.GetSession(req1) + require.NoError(t, err) + + // Set CSRF and other auth flow data + csrfToken := "proxy-csrf-token-12345" + session1.SetCSRF(csrfToken) + session1.SetNonce("proxy-nonce") + session1.SetIncomingPath("/protected") + + // Save session (should set Secure cookie) + rec1 := httptest.NewRecorder() + err = session1.Save(req1, rec1) + require.NoError(t, err) + + cookies := rec1.Result().Cookies() + + // Step 2: Simulate OAuth callback (also HTTPS via proxy) + req2 := httptest.NewRequest("GET", "http://example.com/oidc/callback?code=auth-code&state="+csrfToken, nil) + req2.Header.Set("X-Forwarded-Proto", "https") + req2.Header.Set("X-Forwarded-Host", "example.com") + + // Add cookies from step 1 + for _, cookie := range cookies { + req2.AddCookie(cookie) + } + + // Get session in callback + session2, err := sessionManager.GetSession(req2) + require.NoError(t, err) + + // CSRF token should persist + retrievedCSRF := session2.GetCSRF() + assert.Equal(t, csrfToken, retrievedCSRF, "CSRF token must persist through OAuth flow in reverse proxy") + assert.Equal(t, "proxy-nonce", session2.GetNonce(), "Nonce should also persist") + assert.Equal(t, "/protected", session2.GetIncomingPath(), "Incoming path should persist") + }) + + t.Run("Session_Cookie_Domain_With_Proxy_Headers", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Test with X-Forwarded-Host header + req := httptest.NewRequest("GET", "http://internal.local/test", nil) + req.Header.Set("X-Forwarded-Proto", "https") + req.Header.Set("X-Forwarded-Host", "public.example.com") + req.Host = "internal.local" // Internal host + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + session.SetCSRF("domain-test-csrf") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + cookies := rec.Result().Cookies() + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie) + + // Domain should be set based on X-Forwarded-Host when present + // This ensures cookies work correctly with the public domain + assert.Equal(t, "public.example.com", mainCookie.Domain, "Cookie domain should use forwarded host") + }) +} + +// TestAzureOIDCWithReverseProxy simulates Azure OIDC flow behind a reverse proxy +func TestAzureOIDCWithReverseProxy(t *testing.T) { + t.Run("Azure_Provider_Detection_And_Configuration", func(t *testing.T) { + // This test verifies Azure-specific provider detection and configuration + // without making actual network calls + + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Step 1: Test session setup for Azure OAuth flow + req := httptest.NewRequest("GET", "http://internal/protected", nil) + req.Header.Set("X-Forwarded-Proto", "https") + req.Header.Set("X-Forwarded-Host", "app.example.com") + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + // Simulate OAuth flow initialization + csrfToken := "azure-csrf-token" + nonce := "azure-nonce" + session.SetCSRF(csrfToken) + session.SetNonce(nonce) + session.SetIncomingPath("/protected") + session.MarkDirty() + + // Save session with proper HTTPS detection + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Verify cookies have correct security attributes for Azure + cookies := rec.Result().Cookies() + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie, "Main session cookie must be set") + assert.True(t, mainCookie.Secure, "Cookie must be secure for HTTPS reverse proxy") + assert.Equal(t, http.SameSiteLaxMode, mainCookie.SameSite, "Should use Lax SameSite for OAuth compatibility") + + // Step 2: Simulate callback and verify session persistence + callbackReq := httptest.NewRequest("GET", "http://internal/oidc/callback?code=azure-code&state="+csrfToken, nil) + callbackReq.Header.Set("X-Forwarded-Proto", "https") + callbackReq.Header.Set("X-Forwarded-Host", "app.example.com") + + // Add cookies from initial request + for _, cookie := range cookies { + callbackReq.AddCookie(cookie) + } + + // Get session in callback + callbackSession, err := sessionManager.GetSession(callbackReq) + require.NoError(t, err) + + // Verify session data persisted correctly + assert.Equal(t, csrfToken, callbackSession.GetCSRF(), "CSRF token must persist in Azure flow") + assert.Equal(t, nonce, callbackSession.GetNonce(), "Nonce must persist") + assert.Equal(t, "/protected", callbackSession.GetIncomingPath(), "Original path must persist") + }) + + t.Run("Mixed_HTTP_HTTPS_Requests", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Simulate a scenario where some requests come via HTTPS proxy and some don't + // This can happen in development or misconfigured environments + + // Request 1: HTTPS via proxy + req1 := httptest.NewRequest("GET", "http://localhost:8080/test", nil) + req1.Header.Set("X-Forwarded-Proto", "https") + + session1, err := sessionManager.GetSession(req1) + require.NoError(t, err) + session1.SetCSRF("mixed-csrf") + + rec1 := httptest.NewRecorder() + err = session1.Save(req1, rec1) + require.NoError(t, err) + + cookies1 := rec1.Result().Cookies() + + // Request 2: Direct HTTP (no proxy headers) + req2 := httptest.NewRequest("GET", "http://localhost:8080/test", nil) + // No X-Forwarded-Proto header + + // Try to use cookies from HTTPS request + for _, cookie := range cookies1 { + // Remove Secure flag to simulate browser behavior + // (browser wouldn't send secure cookie over HTTP) + if !cookie.Secure { + req2.AddCookie(cookie) + } + } + + session2, err := sessionManager.GetSession(req2) + require.NoError(t, err) + + // Session should be empty because secure cookies weren't sent + csrf2 := session2.GetCSRF() + assert.Empty(t, csrf2, "CSRF should be empty when secure cookies can't be sent over HTTP") + }) +} + +// TestEnhanceSessionSecurity verifies the security enhancement function +func TestEnhanceSessionSecurity(t *testing.T) { + t.Run("Security_Enhancement_For_AJAX_Requests", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // AJAX request via HTTPS proxy + req := httptest.NewRequest("GET", "http://internal/api/data", nil) + req.Header.Set("X-Forwarded-Proto", "https") + req.Header.Set("X-Requested-With", "XMLHttpRequest") + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + session.SetCSRF("ajax-csrf") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Check that AJAX requests get strict same-site + cookies := rec.Result().Cookies() + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + assert.Equal(t, http.SameSiteStrictMode, cookie.SameSite, "AJAX requests should use Strict SameSite") + break + } + } + }) + + t.Run("Security_Enhancement_Missing_User_Agent", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Request without User-Agent (potential bot/attack) + req := httptest.NewRequest("GET", "http://example.com/test", nil) + req.Header.Set("X-Forwarded-Proto", "https") + // No User-Agent header + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + session.SetCSRF("no-ua-csrf") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Verify reduced session timeout for suspicious requests + cookies := rec.Result().Cookies() + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + // Should have reduced MaxAge (half of normal) + assert.Less(t, cookie.MaxAge, int(absoluteSessionTimeout.Seconds()), "Suspicious requests should have reduced timeout") + break + } + } + }) +} diff --git a/session.go b/session.go index 51627b4..9f65ff4 100644 --- a/session.go +++ b/session.go @@ -497,13 +497,14 @@ func (sm *SessionManager) EnhanceSessionSecurity(options *sessions.Options, r *h // Apply stricter security settings for production if r.Header.Get("X-Forwarded-Proto") == "https" || r.TLS != nil || sm.forceHTTPS { options.Secure = true - // Enable strict same-site policy for secure connections - options.SameSite = http.SameSiteStrictMode + // IMPORTANT: Keep SameSite as Lax for OAuth flows to work + // Strict mode would block cookies from OAuth provider callbacks + // Only use Strict for AJAX requests which don't involve redirects } // Additional security headers analysis if r.Header.Get("X-Requested-With") == "XMLHttpRequest" { - // AJAX requests get stricter same-site policy + // AJAX requests get stricter same-site policy since they don't involve OAuth redirects options.SameSite = http.SameSiteStrictMode } } @@ -513,13 +514,24 @@ func (sm *SessionManager) EnhanceSessionSecurity(options *sessions.Options, r *h // Set secure Domain attribute for production if options.Domain == "" && r != nil { - // Extract domain from Host header for proper cookie scoping + // In reverse proxy setups, use the original host if available + // This ensures cookies work correctly with the public domain host := r.Host + + // Check for X-Forwarded-Host which indicates reverse proxy + if forwardedHost := r.Header.Get("X-Forwarded-Host"); forwardedHost != "" { + // Use the forwarded host for cookie domain + host = forwardedHost + } + if host != "" && !strings.Contains(host, "localhost") && !strings.Contains(host, "127.0.0.1") { // Only set domain for non-local development if colonIndex := strings.Index(host, ":"); colonIndex != -1 { host = host[:colonIndex] // Remove port } + // For cookie domain, we might want to use the base domain + // to allow cookies to work across subdomains + // But for now, use the exact host for security options.Domain = host } } @@ -719,10 +731,14 @@ func (sd *SessionData) MarkDirty() { // Returns: // - An error if saving any of the session components fails. func (sd *SessionData) Save(r *http.Request, w http.ResponseWriter) error { - isSecure := strings.HasPrefix(r.URL.Scheme, "https") || sd.manager.forceHTTPS + // Properly detect HTTPS in reverse proxy environments + isSecure := r.Header.Get("X-Forwarded-Proto") == "https" || r.TLS != nil || sd.manager.forceHTTPS - // Set options for all sessions. + // Get base options and enhance with security settings options := sd.manager.getSessionOptions(isSecure) + options = sd.manager.EnhanceSessionSecurity(options, r) + + // Apply enhanced options to all sessions sd.mainSession.Options = options sd.accessSession.Options = options sd.refreshSession.Options = options diff --git a/session_compatibility_test.go b/session_compatibility_test.go new file mode 100644 index 0000000..6c274e2 --- /dev/null +++ b/session_compatibility_test.go @@ -0,0 +1,199 @@ +package traefikoidc + +import ( + "crypto/tls" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSessionCompatibilityAfterChanges ensures our session changes maintain backward compatibility +func TestSessionCompatibilityAfterChanges(t *testing.T) { + t.Run("Plain_HTTP_Without_Proxy_Headers", func(t *testing.T) { + // Test that plain HTTP requests without proxy headers still work + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + req := httptest.NewRequest("GET", "http://localhost:8080/test", nil) + req.Header.Set("User-Agent", "Mozilla/5.0") + // No X-Forwarded-Proto header + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + session.SetCSRF("plain-http-csrf") + session.SetAuthenticated(true) + session.SetEmail("user@example.com") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + // Verify cookies work for plain HTTP + cookies := rec.Result().Cookies() + require.NotEmpty(t, cookies) + + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie) + + // Plain HTTP should NOT have Secure flag + assert.False(t, mainCookie.Secure, "Plain HTTP should not have Secure flag") + // Should use Lax for compatibility + assert.Equal(t, http.SameSiteLaxMode, mainCookie.SameSite, "Plain HTTP should use Lax SameSite") + + // Verify session can be retrieved + req2 := httptest.NewRequest("GET", "http://localhost:8080/test2", nil) + req2.Header.Set("User-Agent", "Mozilla/5.0") + for _, cookie := range cookies { + req2.AddCookie(cookie) + } + + session2, err := sessionManager.GetSession(req2) + require.NoError(t, err) + + assert.Equal(t, "plain-http-csrf", session2.GetCSRF()) + assert.True(t, session2.GetAuthenticated()) + assert.Equal(t, "user@example.com", session2.GetEmail()) + }) + + t.Run("HTTPS_With_TLS_Field", func(t *testing.T) { + // Test direct HTTPS connection (not proxied) + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + req := httptest.NewRequest("GET", "https://example.com/test", nil) + req.Header.Set("User-Agent", "Mozilla/5.0") + // Simulate TLS connection + req.TLS = &tls.ConnectionState{} + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + session.SetCSRF("direct-https-csrf") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + cookies := rec.Result().Cookies() + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie) + + // Direct HTTPS should have Secure flag + assert.True(t, mainCookie.Secure, "Direct HTTPS should have Secure flag") + assert.Equal(t, http.SameSiteLaxMode, mainCookie.SameSite, "HTTPS should use Lax for OAuth compatibility") + }) + + t.Run("ForceHTTPS_Setting", func(t *testing.T) { + // Test forceHTTPS setting works regardless of request + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", true, NewLogger("debug")) + require.NoError(t, err) + + // Plain HTTP request + req := httptest.NewRequest("GET", "http://localhost/test", nil) + req.Header.Set("User-Agent", "Mozilla/5.0") + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + session.SetCSRF("forced-https-csrf") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + cookies := rec.Result().Cookies() + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie) + + // With forceHTTPS, even HTTP requests get Secure cookies + assert.True(t, mainCookie.Secure, "ForceHTTPS should always set Secure flag") + }) + + t.Run("AJAX_Request_Gets_Strict_SameSite", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // AJAX request + req := httptest.NewRequest("GET", "http://example.com/api/data", nil) + req.Header.Set("X-Requested-With", "XMLHttpRequest") + req.Header.Set("X-Forwarded-Proto", "https") + req.Header.Set("User-Agent", "Mozilla/5.0") + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + session.SetCSRF("ajax-csrf") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + cookies := rec.Result().Cookies() + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie) + + // AJAX requests always get Strict SameSite + assert.Equal(t, http.SameSiteStrictMode, mainCookie.SameSite, "AJAX requests should use Strict SameSite") + }) + + t.Run("Missing_UserAgent_Gets_Reduced_Timeout", func(t *testing.T) { + sessionManager, err := NewSessionManager("test-encryption-key-32-characters", false, NewLogger("debug")) + require.NoError(t, err) + + // Request without User-Agent (suspicious) + req := httptest.NewRequest("GET", "http://example.com/test", nil) + req.Header.Set("X-Forwarded-Proto", "https") + // No User-Agent + + session, err := sessionManager.GetSession(req) + require.NoError(t, err) + + session.SetCSRF("no-ua-csrf") + + rec := httptest.NewRecorder() + err = session.Save(req, rec) + require.NoError(t, err) + + cookies := rec.Result().Cookies() + var mainCookie *http.Cookie + for _, cookie := range cookies { + if cookie.Name == "_oidc_raczylo_m" { + mainCookie = cookie + break + } + } + require.NotNil(t, mainCookie) + + // Should have reduced MaxAge for suspicious requests + expectedMaxAge := int((absoluteSessionTimeout / 2).Seconds()) + assert.Equal(t, expectedMaxAge, mainCookie.MaxAge, "Missing User-Agent should get reduced timeout") + }) +}