diff --git a/app/controllers/api/forward_auth_controller.rb b/app/controllers/api/forward_auth_controller.rb index c06375b..0cd2902 100644 --- a/app/controllers/api/forward_auth_controller.rb +++ b/app/controllers/api/forward_auth_controller.rb @@ -163,16 +163,25 @@ module Api def check_forward_auth_token token = params[:fa_token] - return nil unless token.present? + return nil if token.blank? - session_id = Rails.cache.read("forward_auth_token:#{token}") - return nil unless session_id + cached = Rails.cache.read("forward_auth_token:#{token}") + return nil unless cached.is_a?(Hash) - session = Session.find_by(id: session_id) + # The token is bound to the host that created it. If the request is + # arriving at a different host, refuse — and do NOT burn the cache + # entry, so that the legitimate destination can still redeem within + # the 60s TTL. + request_host = (request.headers["X-Forwarded-Host"] || request.headers["Host"]) + .to_s.sub(/:\d+\z/, "").downcase + return nil if request_host.blank? + return nil unless cached[:host] == request_host + + session = Session.find_by(id: cached[:session_id]) return nil unless session && !session.expired? Rails.cache.delete("forward_auth_token:#{token}") - session_id + cached[:session_id] end def extract_session_id diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index af4265f..799fefd 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -130,35 +130,35 @@ module Authentication end # Create a one-time token for forward auth to handle the race condition - # where the browser hasn't processed the session cookie yet + # where the browser hasn't processed the session cookie yet. + # + # The token is bound to the destination host so that anyone who observes + # the token (Referer leaks, access logs, JS monitors) cannot redeem it for + # a different application within the 60-second TTL. def create_forward_auth_token(session_obj) - # Generate a secure random token - token = SecureRandom.urlsafe_base64(32) + controller_session = session + return unless controller_session[:return_to_after_authenticating].present? - # Store it with an expiry of 60 seconds + uri = URI.parse(controller_session[:return_to_after_authenticating]) + + # OAuth flow handles its own session propagation — no fa_token needed. + return if uri.path&.start_with?("/oauth/") + + # Path-only URLs are same-origin on Clinch; the cookie race doesn't apply + # and we have no destination host to bind against. + bound_host = uri.hostname&.downcase + return if bound_host.blank? + + token = SecureRandom.urlsafe_base64(32) Rails.cache.write( "forward_auth_token:#{token}", - session_obj.id, + { session_id: session_obj.id, host: bound_host }, expires_in: 60.seconds ) - # Set the token as a query parameter on the redirect URL - # We need to store this in the controller's session - controller_session = session - if controller_session[:return_to_after_authenticating].present? - original_url = controller_session[:return_to_after_authenticating] - uri = URI.parse(original_url) - - # Skip adding fa_token for OAuth URLs (OAuth flow should not have forward auth tokens) - unless uri.path&.start_with?("/oauth/") - # Add token as query parameter - query_params = URI.decode_www_form(uri.query || "").to_h - query_params["fa_token"] = token - uri.query = URI.encode_www_form(query_params) - - # Update the session with the tokenized URL - controller_session[:return_to_after_authenticating] = uri.to_s - end - end + query_params = URI.decode_www_form(uri.query || "").to_h + query_params["fa_token"] = token + uri.query = URI.encode_www_form(query_params) + controller_session[:return_to_after_authenticating] = uri.to_s end end diff --git a/test/controllers/api/forward_auth_controller_test.rb b/test/controllers/api/forward_auth_controller_test.rb index d4fde4a..097c258 100644 --- a/test/controllers/api/forward_auth_controller_test.rb +++ b/test/controllers/api/forward_auth_controller_test.rb @@ -698,6 +698,137 @@ module Api assert_equal 30, count, "Successful request should not reset or decrement failure counter" end + # fa_token Host-Binding Tests (H-2) + # + # Rails.cache is a :null_store in test, so these cases swap in a + # MemoryStore for the duration of each test and restore it after. + class FaTokenHostBindingTest < ActionDispatch::IntegrationTest + setup do + @user = users(:bob) + Application.create!(name: "Bound App", slug: "bound-app", app_type: "forward_auth", domain_pattern: "app.example.com", active: true) + + @original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + + @session = Session.create!(user: @user, ip_address: "127.0.0.1", user_agent: "test") + @token = "test-fa-token-123" + Rails.cache.write( + "forward_auth_token:#{@token}", + {session_id: @session.id, host: "app.example.com"}, + expires_in: 60.seconds + ) + end + + teardown do + Rails.cache = @original_cache + end + + test "matching X-Forwarded-Host allows redemption" do + get "/api/verify", params: {fa_token: @token}, + headers: {"X-Forwarded-Host" => "app.example.com"} + + assert_response 200 + assert_nil Rails.cache.read("forward_auth_token:#{@token}"), + "cache entry should be burned on successful redemption" + end + + test "mismatched X-Forwarded-Host is rejected and cache entry survives" do + get "/api/verify", params: {fa_token: @token}, + headers: {"X-Forwarded-Host" => "evil.example.com"} + + # Falls through to session-cookie auth; no cookie in this test -> 302 unauth redirect + assert_response 302 + assert_equal "No session cookie", response.headers["x-auth-reason"] + + cached = Rails.cache.read("forward_auth_token:#{@token}") + assert cached.is_a?(Hash), "cache entry must NOT be burned on host mismatch" + assert_equal "app.example.com", cached[:host] + end + + test "port in X-Forwarded-Host is ignored for host binding" do + # Note: the subsequent Application domain-pattern match uses the raw + # X-Forwarded-Host (with port) and would 403, but that's orthogonal to + # the fa_token check. Successful binding is proven by the cache entry + # being burned. + get "/api/verify", params: {fa_token: @token}, + headers: {"X-Forwarded-Host" => "APP.example.com:8443"} + + assert_nil Rails.cache.read("forward_auth_token:#{@token}"), + "port + case variation should still match the bound host and burn the token" + end + + test "falls back to Host header when X-Forwarded-Host is missing" do + get "/api/verify", params: {fa_token: @token}, + headers: {"Host" => "app.example.com"} + + assert_response 200 + end + + test "rejects when neither X-Forwarded-Host nor Host match" do + get "/api/verify", params: {fa_token: @token}, + headers: {"Host" => "unknown.example.com"} + + assert_response 302 + cached = Rails.cache.read("forward_auth_token:#{@token}") + assert cached.is_a?(Hash), "cache entry must survive mismatched Host" + end + end + + # fa_token Creation Tests (H-2) + # + # The URL-rewriting half of the H-2 fix: tokens are only created when the + # return URL has a host. Path-only URLs must not produce an fa_token + # (no cookie race exists for same-origin redirects, and there is no + # host to bind against). + class FaTokenCreationTest < ActionDispatch::IntegrationTest + setup do + @user = users(:bob) + Application.create!(name: "Create App", slug: "create-app", app_type: "forward_auth", domain_pattern: "app.example.com", active: true) + + @original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + end + + teardown do + Rails.cache = @original_cache + end + + test "path-only return_to does not produce an fa_token or cache entry" do + # Path-only rd (no host) — signin should not append fa_token. + post "/signin", + params: {email_address: @user.email_address, password: "password", rd: "/profile"} + + assert_response 303 + refute_match(/fa_token=/, response.location, "no fa_token for path-only return_to") + + # And no forward_auth_token:* cache entries should have been written. + # MemoryStore exposes @data; we just assert there are no matching keys. + keys = Rails.cache.instance_variable_get(:@data).keys + fa_keys = keys.select { |k| k.to_s.start_with?("forward_auth_token:") } + assert_empty fa_keys, "no fa_token cache entries for path-only return_to" + end + + test "cross-origin return_to produces an fa_token bound to that host" do + # First bounce through /api/verify to populate session[:return_to_after_authenticating] + # with a full URL, then sign in. + get "/api/verify", headers: {"X-Forwarded-Host" => "app.example.com"} + assert_response 302 + + post "/signin", + params: {email_address: @user.email_address, password: "password"} + assert_response 303 + + # Extract the fa_token that was appended. + assert_match(/fa_token=([^&]+)/, response.location) + token = response.location[/fa_token=([^&]+)/, 1] + + cached = Rails.cache.read("forward_auth_token:#{token}") + assert cached.is_a?(Hash), "cache entry should be a Hash, not legacy integer" + assert_equal "app.example.com", cached[:host] + assert cached[:session_id].present? + end + end + # Performance and Load Tests test "should handle requests efficiently under load" do sign_in_as(@user)