[rubygems/rubygems] User bundler UA when downloading gems

Gem::RemoteFetcher uses Gem::Request, which adds the RubyGems UA.
Gem::RemoteFetcher is used to download gems, as well as the full index.
We would like the bundler UA to be used whenever bundler is making
requests.

This PR also avoids unsafely mutating the headers hash on the shared
`Gem::RemoteFetcher.fetcher` instance, which could cause corruption or
incorrect headers when making parallel requests. Instead, we create one
remote fetcher per rubygems remote, which is similar to the connection
segregation bundler is already doing

https://github.com/rubygems/rubygems/commit/f0e8dacdec

lib/bundler/cli.rb

@@ -844,7 +844,7 @@ module Bundler
       return unless SharedHelpers.md5_available?
 
       latest = Fetcher::CompactIndex.
-               new(nil, Source::Rubygems::Remote.new(Bundler::URI("https://rubygems.org")), nil).
+               new(nil, Source::Rubygems::Remote.new(Bundler::URI("https://rubygems.org")), nil, nil).
                send(:compact_index_client).
                instance_variable_get(:@cache).
                dependencies("bundler").

lib/bundler/fetcher.rb

@@ -204,6 +204,16 @@ module Bundler
       fetchers.first.api_fetcher?
     end
 
+    def gem_remote_fetcher
+      @gem_remote_fetcher ||= begin
+        require_relative "fetcher/gem_remote_fetcher"
+        fetcher = GemRemoteFetcher.new Gem.configuration[:http_proxy]
+        fetcher.headers["User-Agent"] = user_agent
+        fetcher.headers["X-Gemfile-Source"] = @remote.original_uri.to_s if @remote.original_uri
+        fetcher
+      end
+    end
+
     private
 
     def available_fetchers
@@ -218,7 +228,7 @@ module Bundler
     end
 
     def fetchers
-      @fetchers ||= available_fetchers.map {|f| f.new(downloader, @remote, uri) }.drop_while {|f| !f.available? }
+      @fetchers ||= available_fetchers.map {|f| f.new(downloader, @remote, uri, gem_remote_fetcher) }.drop_while {|f| !f.available? }
     end
 
     def fetch_specs(gem_names)

lib/bundler/fetcher/base.rb

@@ -6,12 +6,14 @@ module Bundler
       attr_reader :downloader
       attr_reader :display_uri
       attr_reader :remote
+      attr_reader :gem_remote_fetcher
 
-      def initialize(downloader, remote, display_uri)
+      def initialize(downloader, remote, display_uri, gem_remote_fetcher)
         raise "Abstract class" if self.class == Base
         @downloader = downloader
         @remote = remote
         @display_uri = display_uri
+        @gem_remote_fetcher = gem_remote_fetcher
       end
 
       def remote_uri

lib/bundler/fetcher/gem_remote_fetcher.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+require "rubygems/remote_fetcher"
+
+module Bundler
+  class Fetcher
+    class GemRemoteFetcher < Gem::RemoteFetcher
+      def request(*args)
+        super do |req|
+          req.delete("User-Agent") if headers["User-Agent"]
+          yield req if block_given?
+        end
+      end
+    end
+  end
+end

lib/bundler/fetcher/index.rb

@@ -6,7 +6,7 @@ module Bundler
   class Fetcher
     class Index < Base
       def specs(_gem_names)
-        Bundler.rubygems.fetch_all_remote_specs(remote)
+        Bundler.rubygems.fetch_all_remote_specs(remote, gem_remote_fetcher)
       rescue Gem::RemoteFetcher::FetchError => e
         case e.message
         when /certificate verify failed/

lib/bundler/rubygems_integration.rb

@@ -467,11 +467,9 @@ module Bundler
       Gem::Specification.all = specs
     end
 
-    def fetch_specs(remote, name)
+    def fetch_specs(remote, name, fetcher)
       require "rubygems/remote_fetcher"
       path = remote.uri.to_s + "#{name}.#{Gem.marshal_version}.gz"
-      fetcher = gem_remote_fetcher
-      fetcher.headers = { "X-Gemfile-Source" => remote.original_uri.to_s } if remote.original_uri
       string = fetcher.fetch_path(path)
       specs = Bundler.safe_load_marshal(string)
       raise MarshalError, "Specs #{name} from #{remote} is expected to be an Array but was unexpected class #{specs.class}" unless specs.is_a?(Array)
@@ -481,18 +479,16 @@ module Bundler
       raise unless name == "prerelease_specs"
     end
 
-    def fetch_all_remote_specs(remote)
+    def fetch_all_remote_specs(remote, gem_remote_fetcher)
-      specs = fetch_specs(remote, "specs")
+      specs = fetch_specs(remote, "specs", gem_remote_fetcher)
-      pres = fetch_specs(remote, "prerelease_specs") || []
+      pres = fetch_specs(remote, "prerelease_specs", gem_remote_fetcher) || []
 
       specs.concat(pres)
     end
 
-    def download_gem(spec, uri, cache_dir)
+    def download_gem(spec, uri, cache_dir, fetcher)
       require "rubygems/remote_fetcher"
       uri = Bundler.settings.mirror_for(uri)
-      fetcher = gem_remote_fetcher
-      fetcher.headers = { "X-Gemfile-Source" => spec.remote.original_uri.to_s } if spec.remote.original_uri
       Bundler::Retry.new("download gem from #{uri}").attempts do
         gem_file_name = spec.file_name
         local_gem_path = File.join cache_dir, gem_file_name
@@ -519,11 +515,6 @@ module Bundler
       raise Bundler::HTTPError, "Could not download gem from #{uri} due to underlying error <#{e.message}>"
     end
 
-    def gem_remote_fetcher
-      require "rubygems/remote_fetcher"
-      Gem::RemoteFetcher.fetcher
-    end
-
     def build(spec, skip_validation = false)
       require "rubygems/package"
       Gem::Package.build(spec, skip_validation)

lib/bundler/source/rubygems.rb

@@ -255,11 +255,15 @@ module Bundler
         end
       end
 
-      def fetchers
+      def remote_fetchers
-        @fetchers ||= remotes.map do |uri|
+        @remote_fetchers ||= remotes.to_h do |uri|
           remote = Source::Rubygems::Remote.new(uri)
-          Bundler::Fetcher.new(remote)
+          [remote, Bundler::Fetcher.new(remote)]
+        end.freeze
       end
+
+      def fetchers
+        @fetchers ||= remote_fetchers.values.freeze
       end
 
       def double_check_for(unmet_dependency_names)
@@ -480,7 +484,8 @@ module Bundler
       def download_gem(spec, download_cache_path, previous_spec = nil)
         uri = spec.remote.uri
         Bundler.ui.confirm("Fetching #{version_message(spec, previous_spec)}")
-        Bundler.rubygems.download_gem(spec, uri, download_cache_path)
+        gem_remote_fetcher = remote_fetchers.fetch(spec.remote).gem_remote_fetcher
+        Bundler.rubygems.download_gem(spec, uri, download_cache_path, gem_remote_fetcher)
       end
 
       # Returns the global cache path of the calling Rubygems::Source object.

spec/bundler/bundler/fetcher/base_spec.rb

@@ -4,15 +4,16 @@ RSpec.describe Bundler::Fetcher::Base do
   let(:downloader)  { double(:downloader) }
   let(:remote)      { double(:remote) }
   let(:display_uri) { "http://sample_uri.com" }
+  let(:gem_remote_fetcher) { nil }
 
   class TestClass < described_class; end
 
-  subject { TestClass.new(downloader, remote, display_uri) }
+  subject { TestClass.new(downloader, remote, display_uri, gem_remote_fetcher) }
 
   describe "#initialize" do
     context "with the abstract Base class" do
       it "should raise an error" do
-        expect { described_class.new(downloader, remote, display_uri) }.to raise_error(RuntimeError, "Abstract class")
+        expect { described_class.new(downloader, remote, display_uri, gem_remote_fetcher) }.to raise_error(RuntimeError, "Abstract class")
       end
     end
 

spec/bundler/bundler/fetcher/compact_index_spec.rb

@@ -7,7 +7,8 @@ RSpec.describe Bundler::Fetcher::CompactIndex do
   let(:downloader)  { double(:downloader) }
   let(:display_uri) { Bundler::URI("http://sampleuri.com") }
   let(:remote)      { double(:remote, :cache_slug => "lsjdf", :uri => display_uri) }
-  let(:compact_index) { described_class.new(downloader, remote, display_uri) }
+  let(:gem_remote_fetcher) { nil }
+  let(:compact_index) { described_class.new(downloader, remote, display_uri, gem_remote_fetcher) }
 
   before do
     allow(compact_index).to receive(:log_specs) {}

spec/bundler/bundler/fetcher/dependency_spec.rb

@@ -4,8 +4,9 @@ RSpec.describe Bundler::Fetcher::Dependency do
   let(:downloader)  { double(:downloader) }
   let(:remote)      { double(:remote, :uri => Bundler::URI("http://localhost:5000")) }
   let(:display_uri) { "http://sample_uri.com" }
+  let(:gem_remote_fetcher) { nil }
 
-  subject { described_class.new(downloader, remote, display_uri) }
+  subject { described_class.new(downloader, remote, display_uri, gem_remote_fetcher) }
 
   describe "#available?" do
     let(:dependency_api_uri) { double(:dependency_api_uri) }

spec/bundler/bundler/fetcher/index_spec.rb

@@ -8,8 +8,9 @@ RSpec.describe Bundler::Fetcher::Index do
   let(:display_uri) { "http://sample_uri.com" }
   let(:rubygems)    { double(:rubygems) }
   let(:gem_names)   { %w[foo bar] }
+  let(:gem_remote_fetcher) { nil }
 
-  subject { described_class.new(downloader, remote, display_uri) }
+  subject { described_class.new(downloader, remote, display_uri, gem_remote_fetcher) }
 
   before { allow(Bundler).to receive(:rubygems).and_return(rubygems) }
 

spec/bundler/bundler/rubygems_integration_spec.rb

@@ -46,14 +46,12 @@ RSpec.describe Bundler::RubygemsIntegration do
     let(:fetcher) { double("gem_remote_fetcher") }
 
     it "successfully downloads gem with retries" do
-      expect(Bundler.rubygems).to receive(:gem_remote_fetcher).and_return(fetcher)
-      expect(fetcher).to receive(:headers=).with({ "X-Gemfile-Source" => "https://foo.bar" })
       expect(Bundler::Retry).to receive(:new).with("download gem from #{uri}/").
         and_return(bundler_retry)
       expect(bundler_retry).to receive(:attempts).and_yield
       expect(fetcher).to receive(:cache_update_path)
 
-      Bundler.rubygems.download_gem(spec, uri, cache_dir)
+      Bundler.rubygems.download_gem(spec, uri, cache_dir, fetcher)
     end
   end
 
@@ -68,11 +66,9 @@ RSpec.describe Bundler::RubygemsIntegration do
       let(:remote_with_mirror) { double("remote", :uri => uri, :original_uri => orig_uri) }
 
       it "sets the 'X-Gemfile-Source' header containing the original source" do
-        expect(Bundler.rubygems).to receive(:gem_remote_fetcher).twice.and_return(fetcher)
-        expect(fetcher).to receive(:headers=).with({ "X-Gemfile-Source" => "http://zombo.com" }).twice
         expect(fetcher).to receive(:fetch_path).with(uri + "specs.4.8.gz").and_return(specs_response)
         expect(fetcher).to receive(:fetch_path).with(uri + "prerelease_specs.4.8.gz").and_return(prerelease_specs_response)
-        result = Bundler.rubygems.fetch_all_remote_specs(remote_with_mirror)
+        result = Bundler.rubygems.fetch_all_remote_specs(remote_with_mirror, fetcher)
         expect(result).to eq(%w[specs prerelease_specs])
       end
     end
@@ -81,11 +77,9 @@ RSpec.describe Bundler::RubygemsIntegration do
       let(:remote_no_mirror) { double("remote", :uri => uri, :original_uri => nil) }
 
       it "does not set the 'X-Gemfile-Source' header" do
-        expect(Bundler.rubygems).to receive(:gem_remote_fetcher).twice.and_return(fetcher)
-        expect(fetcher).to_not receive(:headers=)
         expect(fetcher).to receive(:fetch_path).with(uri + "specs.4.8.gz").and_return(specs_response)
         expect(fetcher).to receive(:fetch_path).with(uri + "prerelease_specs.4.8.gz").and_return(prerelease_specs_response)
-        result = Bundler.rubygems.fetch_all_remote_specs(remote_no_mirror)
+        result = Bundler.rubygems.fetch_all_remote_specs(remote_no_mirror, fetcher)
         expect(result).to eq(%w[specs prerelease_specs])
       end
     end
@@ -95,9 +89,8 @@ RSpec.describe Bundler::RubygemsIntegration do
       let(:unexpected_specs_response) { Marshal.dump(3) }
 
       it "raises a MarshalError error" do
-        expect(Bundler.rubygems).to receive(:gem_remote_fetcher).once.and_return(fetcher)
         expect(fetcher).to receive(:fetch_path).with(uri + "specs.4.8.gz").and_return(unexpected_specs_response)
-        expect { Bundler.rubygems.fetch_all_remote_specs(remote_no_mirror) }.to raise_error(Bundler::MarshalError, /unexpected class/i)
+        expect { Bundler.rubygems.fetch_all_remote_specs(remote_no_mirror, fetcher) }.to raise_error(Bundler::MarshalError, /unexpected class/i)
       end
     end
   end

spec/bundler/realworld/gemfile_source_header_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe "fetching dependencies with a mirrored source", :realworld => tru
     @t.join
   end
 
-  it "sets the 'X-Gemfile-Source' header and bundles successfully" do
+  it "sets the 'X-Gemfile-Source' and 'User-Agent' headers and bundles successfully" do
     gemfile <<-G
       source "#{mirror}"
       gem 'weakling'

spec/bundler/support/artifice/endpoint_mirror_source.rb

@@ -4,7 +4,7 @@ require_relative "helpers/endpoint"
 
 class EndpointMirrorSource < Endpoint
   get "/gems/:id" do
-    if request.env["HTTP_X_GEMFILE_SOURCE"] == "https://server.example.org/"
+    if request.env["HTTP_X_GEMFILE_SOURCE"] == "https://server.example.org/" && request.env["HTTP_USER_AGENT"].start_with?("bundler")
       File.binread("#{gem_repo1}/gems/#{params[:id]}")
     else
       halt 500