From 11ef3cecf098979bf152ff7a3633721969fc233d Mon Sep 17 00:00:00 2001
From: Hartley McGuire <skipkayhil@gmail.com>
Date: Tue, 15 Aug 2023 03:16:15 -0400
Subject: [PATCH] Fix host display when X_FORWARDED_HOST authorized

Previously, when a Request had a non-authorized HTTP_HOST but an
authorized HTTP_X_FORWARDED_HOST, the HTTP_X_FORWARDED_HOST value would
be displayed as the one being blocked. However, this could be confusing
for users since that value would already be added to `config.hosts`.

This commit addresses the issue by tweaking how the blocked host is
displayed. Instead of always displaying Request#host (which will return
X_FORWARDED_HOST when present whether or not that's the host being
blocked), each host being blocked will be displayed on its own.

Co-authored-by: Daniel Schlosser <Eusebius1920@users.noreply.github.com>
---
 actionpack/CHANGELOG.md                       |  5 +++
 .../middleware/host_authorization.rb          | 17 +++++++---
 .../templates/rescues/blocked_host.html.erb   | 10 ++++--
 .../templates/rescues/blocked_host.text.erb   |  8 +++--
 .../test/dispatch/host_authorization_test.rb  | 32 +++++++++----------
 5 files changed, 45 insertions(+), 27 deletions(-)

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 385a5350f1882..beb7d1d462bb2 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   Fix `HostAuthorization` potentially displaying the value of the
+    X_FORWARDED_HOST header when the HTTP_HOST header is being blocked.
+
+    *Hartley McGuire*, *Daniel Schlosser*
+
 *   Rename `fixture_file_upload` method to `file_fixture_upload`
 
     Declare an alias to preserve the backwards compatibility of `fixture_file_upload`
diff --git a/actionpack/lib/action_dispatch/middleware/host_authorization.rb b/actionpack/lib/action_dispatch/middleware/host_authorization.rb
index f49a191f7e8c9..8004e2018c3f5 100644
--- a/actionpack/lib/action_dispatch/middleware/host_authorization.rb
+++ b/actionpack/lib/action_dispatch/middleware/host_authorization.rb
@@ -98,7 +98,7 @@ def call(env)
         def response_body(request)
           return "" unless request.get_header("action_dispatch.show_detailed_exceptions")
 
-          template = DebugView.new(host: request.host)
+          template = DebugView.new(hosts: request.env["action_dispatch.blocked_hosts"])
           template.render(template: "rescues/blocked_host", layout: "rescues/layout")
         end
 
@@ -114,7 +114,7 @@ def log_error(request)
 
           return unless logger
 
-          logger.error("[#{self.class.name}] Blocked host: #{request.host}")
+          logger.error("[#{self.class.name}] Blocked hosts: #{request.env["action_dispatch.blocked_hosts"].join(", ")}")
         end
 
         def available_logger(request)
@@ -134,21 +134,28 @@ def call(env)
       return @app.call(env) if @permissions.empty?
 
       request = Request.new(env)
+      hosts = blocked_hosts(request)
 
-      if authorized?(request) || excluded?(request)
+      if hosts.empty? || excluded?(request)
         mark_as_authorized(request)
         @app.call(env)
       else
+        env["action_dispatch.blocked_hosts"] = hosts
         @response_app.call(env)
       end
     end
 
     private
-      def authorized?(request)
+      def blocked_hosts(request)
+        hosts = []
+
         origin_host = request.get_header("HTTP_HOST")
+        hosts << origin_host unless @permissions.allows?(origin_host)
+
         forwarded_host = request.x_forwarded_host&.split(/,\s?/)&.last
+        hosts << forwarded_host unless forwarded_host.blank? || @permissions.allows?(forwarded_host)
 
-        @permissions.allows?(origin_host) && (forwarded_host.blank? || @permissions.allows?(forwarded_host))
+        hosts
       end
 
       def excluded?(request)
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/blocked_host.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/blocked_host.html.erb
index 137dfe2236805..e452947306145 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/blocked_host.html.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/blocked_host.html.erb
@@ -1,8 +1,12 @@
 <header>
-  <h1>Blocked host: <%= @host %></h1>
+  <h1>Blocked hosts: <%= @hosts.join(", ") %></h1>
 </header>
 <main role="main" id="container">
-  <h2>To allow requests to <%= @host %> make sure it is a valid hostname (containing only numbers, letters, dashes and dots), then add the following to your environment configuration:</h2>
-  <pre>config.hosts &lt;&lt; "<%= @host %>"</pre>
+  <h2>To allow requests to these hosts, make sure they are valid hostnames (containing only numbers, letters, dashes and dots), then add the following to your environment configuration:</h2>
+  <pre>
+  <% @hosts.each do |host| %>
+    config.hosts &lt;&lt; "<%= host %>"
+  <% end %>
+  </pre>
   <p>For more details view: <a href="https://guides.rubyonrails.org/configuring.html#actiondispatch-hostauthorization">the Host Authorization guide</a></p>
 </main>
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/blocked_host.text.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/blocked_host.text.erb
index a6e59661d0e19..bc5b5fe8ee8df 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/blocked_host.text.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/blocked_host.text.erb
@@ -1,7 +1,9 @@
-Blocked host: <%= @host %>
+Blocked hosts: <%= @hosts.join(", ") %>
 
-To allow requests to <%= @host %> make sure it is a valid hostname (containing only numbers, letters, dashes and dots), then add the following to your environment configuration:
+To allow requests to these hosts, make sure they are valid hostnames (containing only numbers, letters, dashes and dots), then add the following to your environment configuration:
 
-  config.hosts << "<%= @host %>"
+<% @hosts.each do |host| %>
+  config.hosts << "<%= host %>"
+<% end %>
 
 For more details on host authorization view: https://guides.rubyonrails.org/configuring.html#actiondispatch-hostauthorization
diff --git a/actionpack/test/dispatch/host_authorization_test.rb b/actionpack/test/dispatch/host_authorization_test.rb
index dceaa721d6aa5..3469d94538937 100644
--- a/actionpack/test/dispatch/host_authorization_test.rb
+++ b/actionpack/test/dispatch/host_authorization_test.rb
@@ -21,7 +21,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     get "/", env: { "action_dispatch.show_detailed_exceptions" => true }
 
     assert_response :forbidden
-    assert_match "Blocked host: www.example.com", response.body
+    assert_match "Blocked hosts: www.example.com", response.body
   end
 
   test "allows all requests if hosts is empty" do
@@ -93,7 +93,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     }
 
     assert_response :forbidden
-    assert_match "Blocked host: www.example.local", response.body
+    assert_match "Blocked hosts: www.example.local", response.body
   end
 
   test "passes requests to allowed hosts with domain name notation" do
@@ -114,7 +114,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     }
 
     assert_response :forbidden
-    assert_match "Blocked host: .example.com", response.body
+    assert_match "Blocked hosts: .example.com", response.body
   end
 
   test "checks for requests with #=== to support wider range of host checks" do
@@ -140,7 +140,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     get "/", env: { "action_dispatch.show_detailed_exceptions" => true }
 
     assert_response :forbidden
-    assert_match "Blocked host: www.example.com", response.body
+    assert_match "Blocked hosts: www.example.com", response.body
   end
 
   test "blocks requests to unallowed host supporting custom responses" do
@@ -284,7 +284,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     }
 
     assert_response :forbidden
-    assert_match "Blocked host: 127.0.0.1", response.body
+    assert_match "Blocked hosts: www.example.com", response.body
   end
 
   test "blocks requests with spoofed relative X-FORWARDED-HOST" do
@@ -297,7 +297,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     }
 
     assert_response :forbidden
-    assert_match "Blocked host: //randomhost.com", response.body
+    assert_match "Blocked hosts: //randomhost.com", response.body
   end
 
   test "forwarded secondary hosts are allowed when permitted" do
@@ -322,7 +322,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     }
 
     assert_response :forbidden
-    assert_match "Blocked host: evil.com", response.body
+    assert_match "Blocked hosts: evil.com", response.body
   end
 
   test "does not consider IP addresses in X-FORWARDED-HOST spoofed when disabled" do
@@ -347,7 +347,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     }
 
     assert_response :forbidden
-    assert_match "Blocked host: localhost", response.body
+    assert_match "Blocked hosts: www.example.com", response.body
   end
 
   test "forwarded hosts should be permitted" do
@@ -360,7 +360,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     }
 
     assert_response :forbidden
-    assert_match "Blocked host: sub.domain.com", response.body
+    assert_match "Blocked hosts: sub.domain.com", response.body
   end
 
   test "sub-sub domains should not be permitted" do
@@ -372,7 +372,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     }
 
     assert_response :forbidden
-    assert_match "Blocked host: secondary.sub.domain.com", response.body
+    assert_match "Blocked hosts: secondary.sub.domain.com", response.body
   end
 
   test "forwarded hosts are allowed when permitted" do
@@ -420,7 +420,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
       }
 
       assert_response :forbidden
-      assert_match "Blocked host: #{host}", response.body
+      assert_match "Blocked hosts: #{host}", response.body
     end
   end
 
@@ -439,7 +439,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     get "/foo", env: { "action_dispatch.show_detailed_exceptions" => true }
 
     assert_response :forbidden
-    assert_match "Blocked host: www.example.com", response.body
+    assert_match "Blocked hosts: www.example.com", response.body
   end
 
   test "blocks requests with invalid hostnames" do
@@ -451,7 +451,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     }
 
     assert_response :forbidden
-    assert_match "Blocked host: attacker.com#x.example.com", response.body
+    assert_match "Blocked hosts: attacker.com#x.example.com", response.body
   end
 
   test "blocks requests to similar host" do
@@ -463,7 +463,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     }
 
     assert_response :forbidden
-    assert_match "Blocked host: sub-example.com", response.body
+    assert_match "Blocked hosts: sub-example.com", response.body
   end
 
   test "uses logger from the env" do
@@ -473,7 +473,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     get "/", env: { "action_dispatch.logger" => Logger.new(output) }
 
     assert_response :forbidden
-    assert_match "Blocked host: www.example.com", output.rewind && output.read
+    assert_match "Blocked hosts: www.example.com", output.rewind && output.read
   end
 
   test "uses ActionView::Base logger when no logger in the env" do
@@ -489,7 +489,7 @@ class HostAuthorizationTest < ActionDispatch::IntegrationTest
     end
 
     assert_response :forbidden
-    assert_match "Blocked host: www.example.com", output.rewind && output.read
+    assert_match "Blocked hosts: www.example.com", output.rewind && output.read
   end
 
   private