mirror of
https://github.com/lukaszraczylo/traefikoidc.git
synced 2026-10-13 04:07:37 +00:00
Issue #53: Fix CSRF token handling in reverse proxy
1. ✅ HTTPS Detection Fixed (session.go:723) - Now uses X-Forwarded-Proto header instead of r.URL.Scheme - Properly detects HTTPS in reverse proxy environments 2. ✅ SameSite Cookie Attribute Fixed - Removed automatic SameSiteStrictMode for HTTPS (would break OAuth) - Keeps SameSiteLaxMode to allow OAuth callbacks from external domains - Only uses Strict for AJAX requests which don't involve OAuth redirects 3. ✅ Cookie Domain Handling Fixed - Now respects X-Forwarded-Host header for cookie domain - Ensures cookies are set for the public domain, not internal proxy domain 4. ✅ EnhanceSessionSecurity Properly Integrated - Function is now actually called during session save - Applies security enhancements without breaking OAuth flow Why Issue #53 Failed Before: 1. Cookies were not marked Secure in HTTPS environments (browser wouldn't send them back) 2. If they had been Secure with SameSite=Strict, Azure callbacks would still fail 3. Cookie domain might have been wrong (internal vs public domain) Why It Works Now: 1. Cookies are properly marked Secure for HTTPS 2. Uses SameSite=Lax to allow OAuth provider callbacks 3. Cookie domain uses public domain from X-Forwarded-Host 4. CSRF token persists through the entire OAuth flow
This commit is contained in:
@@ -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("[email protected]")
|
||||
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("[email protected]")
|
||||
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
|
||||
}
|
||||
@@ -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("[email protected]")
|
||||
// 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, "[email protected]", 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
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
+22
-6
@@ -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
|
||||
|
||||
@@ -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("[email protected]")
|
||||
|
||||
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, "[email protected]", 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")
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user