FIX: FinalDestination#get forwarding Authorization header on redirects (#27043)

This commits updates `FinalDestination#get` to not forward
`Authorization` header on redirects since most HTTP clients I tested like
curl and wget does not it.

This also fixes a recent problem in `DiscourseIpInfo.mmdb_download`
where we will fail to download the databases when both `GlobalSetting.maxmind_account_id` and
`GlobalSetting.maxmind_license_key` has been set. The failure is due to
the bug above where the redirected URL given by MaxMind does not accept
an `Authorization` header.

lib/discourse_ip_info_spec.rb

@@ -9,7 +9,19 @@ RSpec.describe DiscourseIpInfo do
       stub_request(
         :get,
         "https://download.maxmind.com/geoip/databases/GeoLite2-City/download?suffix=tar.gz",
-      ).with(basic_auth: %w[account_id license_key]).to_return(status: 200, body: "", headers: {})
+      ).with(basic_auth: %w[account_id license_key]).to_return(
+        status: 302,
+        body: "",
+        headers: {
+          location:
+            "https://mm-prod-geoip-databases.a2649acb697e2c09b632799562c076f2.r2.cloudflarestorage.com/some-path",
+        },
+      )
+
+      stub_request(
+        :get,
+        "https://mm-prod-geoip-databases.a2649acb697e2c09b632799562c076f2.r2.cloudflarestorage.com/some-path",
+      ).with { |req| expect(req.headers.key?("Authorization")).to eq(false) }.to_return(status: 200)
 
       described_class.mmdb_download("GeoLite2-City")
     end

lib/final_destination.rb

@@ -156,7 +156,7 @@ class FinalDestination
 
   # this is a new interface for simply getting
   # N bytes accounting for all internal logic
-  def get(redirects = @limit, extra_headers: {}, &blk)
+  def get(redirects = @limit, extra_headers: {}, except_headers: [], &blk)
     raise "Must specify block" unless block_given?
 
     if @uri && @uri.port == 80 && FinalDestination.is_https_domain?(@uri.hostname)
@@ -167,7 +167,7 @@ class FinalDestination
     return if !validate_uri
     return if @stop_at_blocked_pages && blocked_domain?(@uri)
 
-    result, headers_subset = safe_get(@uri, &blk)
+    result, headers_subset = safe_get(@uri, except_headers:, &blk)
     return if !result
 
     cookie = headers_subset.set_cookie
@@ -198,7 +198,12 @@ class FinalDestination
       extra = nil
       extra = { "Cookie" => cookie } if cookie
 
-      get(redirects - 1, extra_headers: extra, &blk)
+      # Most HTTP Clients strip the Authorization header on redirects as the client could be redirecting to a untrusted
+      # party. Not stripping the Authorization header on redirect can also lead to problems where the
+      # redirected party does not expect a Authorization header and thus rejects the request.
+      except_headers = ["Authorization"]
+
+      get(redirects - 1, extra_headers: extra, except_headers:, &blk)
     elsif result == :ok
       @uri.to_s
     else
@@ -478,7 +483,7 @@ class FinalDestination
 
   protected
 
-  def safe_get(uri)
+  def safe_get(uri, except_headers: [])
     result = nil
     unsafe_close = false
     headers_subset = Struct.new(:location, :set_cookie).new
@@ -488,7 +493,7 @@ class FinalDestination
         request_headers.merge(
           "Accept-Encoding" => "gzip",
           "Host" => uri.hostname + (@include_port_in_host_header ? ":#{uri.port}" : ""),
-        )
+        ).except(*except_headers)
 
       req = FinalDestination::HTTP::Get.new(uri.request_uri, headers)
 

spec/lib/final_destination_spec.rb

@@ -460,7 +460,9 @@ RSpec.describe FinalDestination do
     before { described_class.clear_https_cache!("wikipedia.com") }
 
     context "when there is a redirect" do
-      before do
+      after { WebMock.reset! }
+
+      it "correctly streams" do
         stub_request(:get, "http://wikipedia.com/").to_return(
           status: 302,
           body: "",
@@ -468,6 +470,7 @@ RSpec.describe FinalDestination do
             "location" => "https://wikipedia.com/",
           },
         )
+
         # webmock does not do chunks
         stub_request(:get, "https://wikipedia.com/").to_return(
           status: 200,
@@ -475,11 +478,7 @@ RSpec.describe FinalDestination do
           headers: {
           },
         )
-      end
 
-      after { WebMock.reset! }
-
-      it "correctly streams" do
         chunk = nil
         result =
           fd.get do |resp, c|
@@ -490,6 +489,26 @@ RSpec.describe FinalDestination do
         expect(result).to eq("https://wikipedia.com/")
         expect(chunk).to eq("<html><head>")
       end
+
+      it "does not forward 'Authorization' header to subsequent hosts" do
+        fd =
+          FinalDestination.new(
+            "http://wikipedia.com",
+            headers: {
+              "Authorization" => "Basic #{Base64.strict_encode64("account_id:license_key")}",
+            },
+          )
+
+        stub_request(:get, "http://wikipedia.com").with(
+          basic_auth: %w[account_id license_key],
+        ).to_return(status: 302, body: "", headers: { "Location" => "http://some.host.com/" })
+
+        stub_request(:get, "http://some.host.com/")
+          .with { |req| expect(req.headers.key?("Authorization")).to eq(false) }
+          .to_return(status: 200, body: "")
+
+        fd.get {}
+      end
     end
 
     context "when there is a timeout" do