From e31cf66f1160fa6c3964e3b711eb23cf8c6b6b4e Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 16 May 2024 08:37:34 +0800 Subject: [PATCH] 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 | 14 +++++++++++++- lib/final_destination.rb | 15 ++++++++++----- spec/lib/final_destination_spec.rb | 29 ++++++++++++++++++++++++----- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/lib/discourse_ip_info_spec.rb b/lib/discourse_ip_info_spec.rb index 9152de09e31..613a4dd07e9 100644 --- a/lib/discourse_ip_info_spec.rb +++ b/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 diff --git a/lib/final_destination.rb b/lib/final_destination.rb index a67a0810774..7fffb608ce8 100644 --- a/lib/final_destination.rb +++ b/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) diff --git a/spec/lib/final_destination_spec.rb b/spec/lib/final_destination_spec.rb index aa316794ce5..7dd1c0f5755 100644 --- a/spec/lib/final_destination_spec.rb +++ b/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("") 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