diff --git a/lib/bundler/compact_index_client/cache.rb b/lib/bundler/compact_index_client/cache.rb index 3bae6c9efdf8f5..c47535a0196195 100644 --- a/lib/bundler/compact_index_client/cache.rb +++ b/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 diff --git a/lib/bundler/fetcher.rb b/lib/bundler/fetcher.rb index 39e1856909a155..ce6f5fdcf402be 100644 --- a/lib/bundler/fetcher.rb +++ b/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.`) 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? diff --git a/lib/bundler/source_list.rb b/lib/bundler/source_list.rb index 23c73c23f1b27d..ea59ecdd5c5791 100644 --- a/lib/bundler/source_list.rb +++ b/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"] diff --git a/lib/rubygems/compact_index_client/cache.rb b/lib/rubygems/compact_index_client/cache.rb index 34e20dfea0b832..84c74fb86a1fb7 100644 --- a/lib/rubygems/compact_index_client/cache.rb +++ b/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 diff --git a/process.c b/process.c index 92c5502e3f5dab..9c2659af07d251 100644 --- a/process.c +++ b/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 $?: + * containing information on that process. + * Unlike Process.wait, this method does not set thread-local variable + * $?: * * Process.spawn('cat /nop') # => 1155880 * Process::Status.wait # => # - * $? # => # + * $? # => 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 $?: + * that does not represent an actual process: * * Process::Status.wait # => # - * $? # => # # Unchanged. * * May invoke the scheduler hook Fiber::Scheduler#process_wait. * diff --git a/spec/bundler/bundler/compact_index_client/cache_spec.rb b/spec/bundler/bundler/compact_index_client/cache_spec.rb new file mode 100644 index 00000000000000..85528c0cac32da --- /dev/null +++ b/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 diff --git a/spec/bundler/bundler/dsl_spec.rb b/spec/bundler/bundler/dsl_spec.rb index b6e67a312c0d41..902bd1ad6f2a45 100644 --- a/spec/bundler/bundler/dsl_spec.rb +++ b/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 diff --git a/spec/bundler/bundler/fetcher_spec.rb b/spec/bundler/bundler/fetcher_spec.rb index 3685d104c0eb42..6891a7991de636 100644 --- a/spec/bundler/bundler/fetcher_spec.rb +++ b/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 diff --git a/spec/bundler/bundler/source_list_spec.rb b/spec/bundler/bundler/source_list_spec.rb index 7a8e885b2e1bbb..c67edb2f201b73 100644 --- a/spec/bundler/bundler/source_list_spec.rb +++ b/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") diff --git a/spec/bundler/install/cooldown_spec.rb b/spec/bundler/install/cooldown_spec.rb index 2724132263cd84..5825f91b5789bd 100644 --- a/spec/bundler/install/cooldown_spec.rb +++ b/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 diff --git a/spec/bundler/support/shards.rb b/spec/bundler/support/shards.rb index 72b1a4a1b7f6dd..0de964017030b5 100644 --- a/spec/bundler/support/shards.rb +++ b/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", diff --git a/test/rubygems/test_gem_compact_index_client_cache.rb b/test/rubygems/test_gem_compact_index_client_cache.rb index e34ddce6f633be..2951f8e324191e 100644 --- a/test/rubygems/test_gem_compact_index_client_cache.rb +++ b/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)