[pull] master from ruby:master

lib/bundler/compact_index_client/cache.rb

@@ -50,6 +50,7 @@ def versions_etag_path = directory.join("versions.etag")
 
       def info_path(name)
         name = name.to_s
+        validate_name!(name)
         # TODO: converge this into the info_root by hashing all filenames like info_etag_path
         if /[^a-z0-9_-]/.match?(name)
           name += "-#{SharedHelpers.digest(:MD5).hexdigest(name).downcase}"
@@ -61,9 +62,19 @@ def info_path(name)
 
       def info_etag_path(name)
         name = name.to_s
+        validate_name!(name)
         @info_etag_root.join("#{name}-#{SharedHelpers.digest(:MD5).hexdigest(name).downcase}")
       end
 
+      # Gem names come from the remote index and are not otherwise
+      # validated, so refuse anything that would escape the cache
+      # directory when used as a path component.
+      def validate_name!(name)
+        return if File.basename(name) == name
+
+        raise SecurityError, "malformed gem name: #{name.inspect}"
+      end
+
       def mkdir(name)
         directory.join(name).tap do |dir|
           SharedHelpers.filesystem_access(dir) do

lib/bundler/fetcher.rb

@@ -215,7 +215,13 @@ def user_agent
           agent << " #{ruby.engine}/#{ruby.versions_string(engine_version)}"
         end
 
-        agent << " options/#{Bundler.settings.all.join(",")}"
+        # Some settings keys embed a URI (e.g. `mirror.<uri>`) whose userinfo can
+        # carry credentials. Strip the userinfo before advertising the setting
+        # names so we don't leak secrets to every gem source we talk to.
+        options = Bundler.settings.all.map do |key|
+          key.include?("://") ? key.gsub(%r{//[^/@]+@}, "//") : key
+        end
+        agent << " options/#{options.join(",")}"
 
         agent << " ci/#{cis.join(",")}" if cis.any?
 

lib/bundler/source_list.rb

@@ -50,7 +50,13 @@ def add_git_source(options = {})
 
     def add_rubygems_source(options = {})
       new_source = Source::Rubygems.new(options)
-      return @global_rubygems_source if @global_rubygems_source == new_source
+      if @global_rubygems_source == new_source
+        warn_on_cooldown_conflict(new_source, @global_rubygems_source)
+        return @global_rubygems_source
+      end
+
+      existing_source = @rubygems_sources.find {|s| s == new_source }
+      warn_on_cooldown_conflict(new_source, existing_source) if existing_source
 
       add_source_to_list new_source, @rubygems_sources
     end
@@ -60,6 +66,13 @@ def add_plugin_source(source, options = {})
     end
 
     def add_global_rubygems_remote(uri, cooldown: nil)
+      unless cooldown.nil?
+        new_source = source_class.new("remotes" => uri, "cooldown" => cooldown)
+        [global_rubygems_source, *@rubygems_sources].find do |existing_source|
+          warn_on_cooldown_conflict(new_source, existing_source)
+        end
+      end
+
       global_rubygems_source.add_remote(uri, cooldown: cooldown)
       global_rubygems_source
     end
@@ -222,6 +235,22 @@ def source_list_for(source)
       end
     end
 
+    def warn_on_cooldown_conflict(new_source, existing_source)
+      new_source.remote_cooldowns.any? do |uri, cooldown|
+        next false unless existing_source.remotes.include?(uri)
+
+        existing_cooldown = existing_source.cooldown_for(uri)
+        next false if existing_cooldown == cooldown
+
+        previous = existing_cooldown ? "`cooldown: #{existing_cooldown}`" : "no cooldown"
+        Bundler.ui.warn "The source #{uri} is declared more than once with different cooldown " \
+          "values (`cooldown: #{cooldown}` here, #{previous} previously). All declarations of " \
+          "the same source URL share a single cooldown, so only one of these values will apply " \
+          "to all gems from this source."
+        true
+      end
+    end
+
     def warn_on_git_protocol(source)
       return if Bundler.settings["git.allow_insecure"]
 

lib/rubygems/compact_index_client/cache.rb

@@ -60,6 +60,7 @@ def versions_etag_path = directory.join("versions.etag")
 
     def info_path(name)
       name = name.to_s
+      validate_name!(name)
       if /[^a-z0-9_-]/.match?(name)
         name += "-#{Digest::MD5.hexdigest(name).downcase}"
         @special_characters_info_root.join(name)
@@ -70,9 +71,19 @@ def info_path(name)
 
     def info_etag_path(name)
       name = name.to_s
+      validate_name!(name)
       @info_etag_root.join("#{name}-#{Digest::MD5.hexdigest(name).downcase}")
     end
 
+    # Gem names come from the remote index and are not otherwise
+    # validated, so refuse anything that would escape the cache
+    # directory when used as a path component.
+    def validate_name!(name)
+      return if File.basename(name) == name
+
+      raise Gem::Exception, "malformed gem name: #{name.inspect}"
+    end
+
     def checksum_for_file(path)
       return unless path.file?
       Digest::MD5.file(path).hexdigest

process.c

@@ -1116,20 +1116,19 @@ rb_process_status_wait(rb_pid_t pid, int flags)
  *
  *  If there are child processes,
  *  waits for a child process to exit and returns a Process::Status object
- *  containing information on that process;
- *  sets thread-local variable <tt>$?</tt>:
+ *  containing information on that process.
+ *  Unlike Process.wait, this method does not set thread-local variable
+ *  <tt>$?</tt>:
  *
  *    Process.spawn('cat /nop') # => 1155880
  *    Process::Status.wait      # => #<Process::Status: pid 1155880 exit 1>
- *    $?                        # => #<Process::Status: pid 1155508 exit 1>
+ *    $?                        # => nil # Not set.
  *
  *  If there is no child process,
  *  returns an "empty" Process::Status object
- *  that does not represent an actual process;
- *  does not set thread-local variable <tt>$?</tt>:
+ *  that does not represent an actual process:
  *
  *    Process::Status.wait # => #<Process::Status: pid -1 exit 0>
- *    $?                   # => #<Process::Status: pid 1155508 exit 1> # Unchanged.
  *
  *  May invoke the scheduler hook Fiber::Scheduler#process_wait.
  *

spec/bundler/bundler/compact_index_client/cache_spec.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+require "bundler/compact_index_client"
+require "bundler/compact_index_client/cache"
+require "tmpdir"
+
+RSpec.describe Bundler::CompactIndexClient::Cache do
+  let(:directory) { Pathname.new(Dir.mktmpdir("compact_index")) }
+  let(:cache) { described_class.new(directory) }
+
+  it "rejects a gem name that would escape the cache directory" do
+    expect { cache.info("../../../../pwn") }.to raise_error(Bundler::SecurityError, /malformed gem name/)
+  end
+
+  it "accepts plain gem names, including special characters" do
+    expect { cache.info("rack") }.not_to raise_error
+    expect { cache.info("Rails.js") }.not_to raise_error
+  end
+end

spec/bundler/bundler/dsl_spec.rb

@@ -370,6 +370,7 @@
   describe "#source with cooldown" do
     before do
       allow(@rubygems).to receive(:add_remote)
+      allow(@rubygems).to receive(:remote_cooldowns).and_return({})
     end
 
     it "accepts a non-negative integer" do

spec/bundler/bundler/fetcher_spec.rb

@@ -141,6 +141,19 @@
       expect(fetcher.user_agent).to match(%r{options/foo,bar})
     end
 
+    it "strips userinfo from settings keys that embed a URI" do
+      allow(Bundler.settings).to receive(:all).and_return(
+        %w[foo mirror.http://user:token@example.org/ mirror.https://token@example.net/]
+      )
+      options = fetcher.user_agent.split(" ").find {|x| x.start_with?("options/") }
+
+      expect(options).not_to include("token")
+      expect(options).not_to include("user:")
+      expect(options).to include("mirror.http://example.org/")
+      expect(options).to include("mirror.https://example.net/")
+      expect(options).to include("foo")
+    end
+
     describe "include CI information" do
       it "from one CI" do
         with_env_vars("CI" => nil, "JENKINS_URL" => "foo") do

spec/bundler/bundler/source_list_spec.rb

@@ -161,6 +161,56 @@
     end
   end
 
+  describe "cooldown conflicts between duplicate source declarations" do
+    it "warns when a block source duplicates the global source with a different cooldown" do
+      source_list.add_global_rubygems_remote("https://rubygems.org", cooldown: 7)
+      expect(Bundler.ui).to receive(:warn).with(/declared more than once with different cooldown values \(`cooldown: 0` here, `cooldown: 7` previously\)/)
+      source_list.add_rubygems_source("remotes" => ["https://rubygems.org"], "cooldown" => 0)
+    end
+
+    it "warns when a global source duplicates a block source with a different cooldown" do
+      source_list.add_rubygems_source("remotes" => ["https://rubygems.org"], "cooldown" => 0)
+      expect(Bundler.ui).to receive(:warn).with(/declared more than once with different cooldown values \(`cooldown: 7` here, `cooldown: 0` previously\)/)
+      source_list.add_global_rubygems_remote("https://rubygems.org", cooldown: 7)
+    end
+
+    it "warns when a block source duplicates another block source with a different cooldown" do
+      source_list.add_rubygems_source("remotes" => ["https://rubygems.org"], "cooldown" => 7)
+      expect(Bundler.ui).to receive(:warn).with(/declared more than once with different cooldown values/)
+      source_list.add_rubygems_source("remotes" => ["https://rubygems.org"], "cooldown" => 0)
+    end
+
+    it "warns when a global remote is declared again with a different cooldown" do
+      source_list.add_global_rubygems_remote("https://rubygems.org", cooldown: 7)
+      expect(Bundler.ui).to receive(:warn).with(/declared more than once with different cooldown values/)
+      source_list.add_global_rubygems_remote("https://rubygems.org", cooldown: 0)
+    end
+
+    it "warns when a duplicate declaration adds a cooldown to a source declared without one" do
+      source_list.add_global_rubygems_remote("https://rubygems.org")
+      expect(Bundler.ui).to receive(:warn).with(/declared more than once with different cooldown values \(`cooldown: 7` here, no cooldown previously\)/)
+      source_list.add_rubygems_source("remotes" => ["https://rubygems.org"], "cooldown" => 7)
+    end
+
+    it "does not warn when the duplicate declaration uses the same cooldown" do
+      source_list.add_global_rubygems_remote("https://rubygems.org", cooldown: 7)
+      expect(Bundler.ui).not_to receive(:warn)
+      source_list.add_rubygems_source("remotes" => ["https://rubygems.org"], "cooldown" => 7)
+    end
+
+    it "does not warn when the duplicate declaration has no cooldown" do
+      source_list.add_global_rubygems_remote("https://rubygems.org", cooldown: 7)
+      expect(Bundler.ui).not_to receive(:warn)
+      source_list.add_rubygems_source("remotes" => ["https://rubygems.org"])
+    end
+
+    it "does not warn when different sources declare different cooldowns" do
+      source_list.add_global_rubygems_remote("https://rubygems.org", cooldown: 7)
+      expect(Bundler.ui).not_to receive(:warn)
+      source_list.add_rubygems_source("remotes" => ["https://other-rubygems.org"], "cooldown" => 0)
+    end
+  end
+
   describe "#all_sources" do
     it "includes the global rubygems source when rubygems sources have been added" do
       source_list.add_git_source("uri" => "git://host/path.git")

spec/bundler/install/cooldown_spec.rb

@@ -257,6 +257,45 @@
       expect(the_bundle).to include_gems("ripe_gem 1.0.0", "child 1.0.0")
     end
 
+    it "warns when the same source is declared again with a different cooldown and keeps the first value" do
+      # https://github.com/rubygems/rubygems/issues/9723: a second declaration
+      # of the same URL is deduped into the first one, so its cooldown cannot
+      # act as a per-gem exemption.
+      install_gemfile <<-G, artifice: "compact_index_cooldown"
+        source "https://gem.repo3", cooldown: 7
+        source "https://gem.repo3", cooldown: 0 do
+          gem "ripe_gem"
+        end
+      G
+
+      expect(err).to include("The source https://gem.repo3/ is declared more than once with different cooldown values (`cooldown: 0` here, `cooldown: 7` previously).")
+      expect(the_bundle).to include_gems("ripe_gem 1.0.0")
+    end
+
+    it "does not warn when the same source is declared again without a cooldown" do
+      install_gemfile <<-G, artifice: "compact_index_cooldown"
+        source "https://gem.repo3", cooldown: 7
+        source "https://gem.repo3" do
+          gem "ripe_gem"
+        end
+      G
+
+      expect(err).not_to include("cooldown")
+      expect(the_bundle).to include_gems("ripe_gem 1.0.0")
+    end
+
+    it "does not warn when the same source is declared again with the same cooldown" do
+      install_gemfile <<-G, artifice: "compact_index_cooldown"
+        source "https://gem.repo3", cooldown: 7
+        source "https://gem.repo3", cooldown: 7 do
+          gem "ripe_gem"
+        end
+      G
+
+      expect(err).not_to include("cooldown")
+      expect(the_bundle).to include_gems("ripe_gem 1.0.0")
+    end
+
     it "is overridden by CLI --cooldown when Gemfile sets a different per-source value" do
       gemfile <<-G
         source "https://gem.repo3", cooldown: 0

spec/bundler/support/shards.rb

@@ -148,6 +148,7 @@ module Shards
       ],
       shard_d: [
         "spec/bundler/compact_index_client/cache_file_spec.rb",
+        "spec/bundler/compact_index_client/cache_spec.rb",
         "spec/bundler/rubygems_ext_spec.rb",
         "spec/bundler/resolver/cooldown_spec.rb",
         "spec/install/cooldown_spec.rb",

test/rubygems/test_gem_compact_index_client_cache.rb

@@ -113,6 +113,29 @@ def test_fetch_info_fetches_without_checksum
     assert_equal "a 1.0.0\n", @dir.join("info", "a").read
   end
 
+  def test_info_rejects_name_escaping_cache_directory
+    fetcher = FakeFetcher.new("1.0.0\n")
+    cache = Gem::CompactIndexClient::Cache.new(@dir, fetcher)
+
+    e = assert_raise Gem::Exception do
+      cache.info("../../../../pwn", "no-match")
+    end
+
+    assert_includes e.message, "malformed gem name"
+    assert_empty fetcher.requests
+  end
+
+  def test_fetch_info_rejects_name_escaping_cache_directory
+    fetcher = FakeFetcher.new("1.0.0\n")
+    cache = Gem::CompactIndexClient::Cache.new(@dir, fetcher)
+
+    assert_raise Gem::Exception do
+      cache.fetch_info("../pwn")
+    end
+
+    assert_empty fetcher.requests
+  end
+
   def test_info_with_special_characters_uses_hashed_path
     fetcher = FakeFetcher.new("1.0.0\n")
     cache = Gem::CompactIndexClient::Cache.new(@dir, fetcher)