Harden the compact index HTTP fetcher's redirect and error handling

lib/rubygems/compact_index_client/http_fetcher.rb

@@ -26,9 +26,7 @@ def call(path, headers = {})
     private
 
     def fetch(uri, headers, redirects_remaining)
-      response = @remote_fetcher.request(uri, Gem::Net::HTTP::Get) do |req|
-        headers.each {|name, value| req[name] = value }
-      end
+      response = request(uri, headers)
 
       case response
       when Gem::Net::HTTPSuccess, Gem::Net::HTTPNotModified
@@ -40,7 +38,12 @@ def fetch(uri, headers, redirects_remaining)
         location = response["Location"]
         raise Gem::RemoteFetcher::FetchError.new("redirecting but no redirect location was given", uri) unless location
 
-        fetch(uri + location, headers, redirects_remaining - 1)
+        redirect = uri + location
+        if https?(uri) && !https?(redirect)
+          raise Gem::RemoteFetcher::FetchError.new("redirecting to non-https resource: #{Gem::Uri.redact(redirect)}", uri)
+        end
+
+        fetch(redirect, headers, redirects_remaining - 1)
       when Gem::Net::HTTPRangeNotSatisfiable
         raise Gem::RemoteFetcher::FetchError.new("bad response #{response.message} #{response.code}", uri) unless headers.key?("Range")
 
@@ -50,5 +53,20 @@ def fetch(uri, headers, redirects_remaining)
         raise Gem::RemoteFetcher::FetchError.new("bad response #{response.message} #{response.code}", uri)
       end
     end
+
+    # The callers fall back to the Marshal index when a fetch fails, and they
+    # only recognize a failure that arrives as a FetchError.
+    def request(uri, headers)
+      @remote_fetcher.request(uri, Gem::Net::HTTP::Get) do |req|
+        headers.each {|name, value| req[name] = value }
+      end
+    rescue Gem::Timeout::Error, IOError, SocketError, SystemCallError,
+           *(OpenSSL::SSL::SSLError if Gem::HAVE_OPENSSL) => e
+      raise Gem::RemoteFetcher::FetchError.new("#{e.class}: #{e}", uri)
+    end
+
+    def https?(uri)
+      uri.scheme == "https"
+    end
   end
 end

test/rubygems/test_gem_compact_index_client_http_fetcher.rb

@@ -14,6 +14,16 @@ def initialize(body)
     alias_method :body, :fake_body
   end
 
+  class FakePartialContent < Gem::Net::HTTPPartialContent
+    def initialize(body)
+      super("1.1", "206", "Partial Content")
+      @fake_body = body
+    end
+
+    attr_reader :fake_body
+    alias_method :body, :fake_body
+  end
+
   class FakeRedirect < Gem::Net::HTTPFound
     def initialize(location)
       super("1.1", "302", "Found")
@@ -52,7 +62,10 @@ def request(uri, request_class)
       request = request_class.new(uri)
       yield request if block_given?
       @requests << [uri, request]
-      @responses.fetch(uri.to_s)
+      response = @responses.fetch(uri.to_s)
+      # A mapped exception stands in for a connection that never produced a response.
+      raise response if response.is_a?(Exception)
+      response
     end
   end
 
@@ -70,6 +83,19 @@ def test_call_joins_path_with_base_uri
     assert_equal Gem::URI("https://index.example/info/a"), remote.requests.first.first
   end
 
+  def test_call_returns_not_modified_responses
+    response = Gem::Net::HTTPNotModified.new("1.1", "304", "Not Modified")
+    fetcher, _remote = fetcher_for("https://index.example/versions" => response)
+
+    assert_same response, fetcher.call("versions")
+  end
+
+  def test_call_returns_partial_content_responses
+    fetcher, _remote = fetcher_for("https://index.example/versions" => FakePartialContent.new("tail"))
+
+    assert_equal "tail", fetcher.call("versions", "Range" => "bytes=10-").body
+  end
+
   def test_call_applies_request_headers
     fetcher, remote = fetcher_for("https://index.example/versions" => FakeResponse.new("data"))
 
@@ -110,6 +136,104 @@ def test_call_resolves_relative_redirect_location
     assert_equal "data", fetcher.call("versions").body
   end
 
+  def test_call_rejects_https_to_http_redirect
+    fetcher, remote = fetcher_for(
+      "https://index.example/versions" => FakeRedirect.new("http://mirror.example/versions")
+    )
+
+    error = assert_raise Gem::RemoteFetcher::FetchError do
+      fetcher.call("versions")
+    end
+
+    assert_match(%r{redirecting to non-https resource: http://mirror\.example/versions}, error.message)
+    assert_equal 1, remote.requests.size
+  end
+
+  def test_call_wraps_connection_refused_in_fetch_error
+    fetcher, remote = fetcher_for(
+      "https://index.example/versions" => Errno::ECONNREFUSED.new("Connection refused")
+    )
+
+    error = assert_raise Gem::RemoteFetcher::FetchError do
+      fetcher.call("versions")
+    end
+
+    assert_match(/Errno::ECONNREFUSED/, error.message)
+    assert_equal 1, remote.requests.size
+  end
+
+  def test_call_wraps_ssl_error_in_fetch_error
+    pend "OpenSSL is unavailable" unless Gem::HAVE_OPENSSL
+
+    fetcher, _remote = fetcher_for(
+      "https://index.example/versions" => OpenSSL::SSL::SSLError.new("certificate verify failed")
+    )
+
+    error = assert_raise Gem::RemoteFetcher::FetchError do
+      fetcher.call("versions")
+    end
+
+    assert_match(/OpenSSL::SSL::SSLError/, error.message)
+  end
+
+  def test_call_wraps_socket_error_in_fetch_error
+    fetcher, _remote = fetcher_for(
+      "https://index.example/versions" => SocketError.new("getaddrinfo: Name or service not known")
+    )
+
+    error = assert_raise Gem::RemoteFetcher::FetchError do
+      fetcher.call("versions")
+    end
+
+    assert_match(/SocketError/, error.message)
+  end
+
+  def test_call_follows_redirects_from_an_http_source
+    remote = FakeRemoteFetcher.new(
+      "http://index.example/versions" => FakeRedirect.new("http://mirror.example/versions"),
+      "http://mirror.example/versions" => FakeResponse.new("data")
+    )
+    fetcher = Gem::CompactIndexClient::HTTPFetcher.new("http://index.example", remote)
+
+    assert_equal "data", fetcher.call("versions").body
+    assert_equal 2, remote.requests.size
+  end
+
+  def test_call_redacts_credentials_in_rejected_redirect
+    fetcher, _remote = fetcher_for(
+      "https://index.example/versions" => FakeRedirect.new("http://user:s3cr3t@mirror.example/versions")
+    )
+
+    error = assert_raise Gem::RemoteFetcher::FetchError do
+      fetcher.call("versions")
+    end
+
+    refute_match(/s3cr3t/, error.message)
+    assert_match(%r{redirecting to non-https resource: http://user:REDACTED@mirror\.example/versions}, error.message)
+  end
+
+  def test_call_keeps_credentials_on_an_accepted_redirect
+    remote = FakeRemoteFetcher.new(
+      "https://user:s3cr3t@index.example/versions" => FakeRedirect.new("/v2/versions"),
+      "https://user:s3cr3t@index.example/v2/versions" => FakeResponse.new("data")
+    )
+    fetcher = Gem::CompactIndexClient::HTTPFetcher.new("https://user:s3cr3t@index.example", remote)
+
+    assert_equal "data", fetcher.call("versions").body
+    assert_equal "s3cr3t", remote.requests.last.first.password
+  end
+
+  def test_call_drops_credentials_on_a_cross_host_redirect
+    remote = FakeRemoteFetcher.new(
+      "https://user:s3cr3t@index.example/versions" => FakeRedirect.new("https://mirror.example/versions"),
+      "https://mirror.example/versions" => FakeResponse.new("data")
+    )
+    fetcher = Gem::CompactIndexClient::HTTPFetcher.new("https://user:s3cr3t@index.example", remote)
+
+    assert_equal "data", fetcher.call("versions").body
+    assert_nil remote.requests.last.first.userinfo
+  end
+
   def test_call_raises_after_too_many_redirects
     fetcher, _remote = fetcher_for(
       "https://index.example/versions" => FakeRedirect.new("https://index.example/versions")