From 2d5650e620b9531be1c39b6d8d325909e23c4fdb Mon Sep 17 00:00:00 2001 From: Dan Milne Date: Mon, 20 Apr 2026 19:04:53 +1000 Subject: [PATCH] Bind forward-auth fa_token to its destination host An observed fa_token (via Referer leaks, access logs, JS monitors) could previously be redeemed against a different reverse-proxied app within the 60s TTL. The token now stores the destination host at creation and the verifier rejects mismatches without burning the cache entry, so legitimate destinations can still redeem. Co-Authored-By: Claude Opus 4 --- .../api/forward_auth_controller.rb | 19 ++- app/controllers/concerns/authentication.rb | 46 +++--- .../api/forward_auth_controller_test.rb | 131 ++++++++++++++++++ 3 files changed, 168 insertions(+), 28 deletions(-) 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)