diff --git a/app/controllers/api/forward_auth_controller.rb b/app/controllers/api/forward_auth_controller.rb index 1165358..e5f218c 100644 --- a/app/controllers/api/forward_auth_controller.rb +++ b/app/controllers/api/forward_auth_controller.rb @@ -196,11 +196,14 @@ module Api original_host = request.headers["X-Forwarded-Host"] original_uri = request.headers["X-Forwarded-Uri"] || request.headers["X-Forwarded-Path"] || "/" - original_url = if original_host - "https://#{original_host}#{original_uri}" - else - redirect_url || base_url - end + # X-Forwarded-Host is attacker-influenceable, so only honour the forwarded + # URL as a post-login redirect target if it resolves to a known, active + # forward-auth application. Otherwise this is an open redirect: a spoofed + # host would be stored and reflected into the signin `rd`, then followed + # (with allow_other_host) after the user authenticates. Fall back to a + # validated `rd` or, failing that, the IdP's own base URL. + forwarded_url = "https://#{original_host}#{original_uri}" if original_host.present? + original_url = validate_redirect_url(forwarded_url) || redirect_url || base_url session[:return_to_after_authenticating] = original_url diff --git a/test/integration/forward_auth_integration_test.rb b/test/integration/forward_auth_integration_test.rb index 6193bf1..e23aa9b 100644 --- a/test/integration/forward_auth_integration_test.rb +++ b/test/integration/forward_auth_integration_test.rb @@ -153,6 +153,10 @@ class ForwardAuthIntegrationTest < ActionDispatch::IntegrationTest # Redirect URL Integration Tests test "unauthenticated request redirects to signin with parameters" do + # grafana.example.com must be a registered forward-auth app for its URL to be + # honoured as a redirect target (otherwise it would be an open-redirect vector). + grant_everyone_access Application.create!(name: "Grafana", slug: "grafana", app_type: "forward_auth", domain_pattern: "grafana.example.com", active: true) + # Test that unauthenticated requests redirect to signin with rd and rm parameters get "/api/verify", headers: { "X-Forwarded-Host" => "grafana.example.com" @@ -172,7 +176,32 @@ class ForwardAuthIntegrationTest < ActionDispatch::IntegrationTest assert_includes location, "grafana.example.com" end + test "spoofed X-Forwarded-Host is not reflected as a redirect target" do + # CLINCH_HOST pins the IdP origin (as in production) so base_url cannot be + # influenced by the request; this isolates the return_to/redirect behaviour. + original_clinch_host = ENV["CLINCH_HOST"] + ENV["CLINCH_HOST"] = "https://auth.example.com" + + # No forward-auth app exists for evil.com, and no valid rd is supplied. The + # attacker-controlled host must NOT be stored or reflected into the signin URL. + get "/api/verify", headers: { + "X-Forwarded-Host" => "evil.com", + "X-Forwarded-Uri" => "/steal" + } + + assert_response 302 + assert_match %r{/signin}, response.location + refute_includes response.location, "evil.com" + refute_match(/evil\.com/, session[:return_to_after_authenticating].to_s) + ensure + ENV["CLINCH_HOST"] = original_clinch_host + end + test "return URL functionality after authentication" do + # app.example.com must be a registered forward-auth app for its URL to be + # honoured as a redirect target. + grant_everyone_access Application.create!(name: "App FA", slug: "app-fa", app_type: "forward_auth", domain_pattern: "app.example.com", active: true) + # Initial request should set return URL get "/api/verify", headers: { "X-Forwarded-Host" => "app.example.com",