From 645fe42ef9c9650a0e5502c59bbc9e51b242a9c4 Mon Sep 17 00:00:00 2001 From: Hiroshi SHIBATA Date: Fri, 28 Aug 2026 17:39:11 +0900 Subject: [PATCH 1/3] Restrict the shared global gem cache to remote sources A gem installed from a local path was copied into the global cache under its canonical file name, where later remote installs of the same name and version would reuse it without re-verification, so one local install could poison every other project on the machine. Two smaller problems in the same expression go with it. The global cache branch was evaluated before the one gem fetch relies on, so gem fetch wrote into the cache instead of the working directory, and an unwritable cache directory aborted the install rather than falling back the way it does with the cache disabled. The working directory is compared by identity because the paths can differ while naming the same place, and because Dir.pwd raises once that directory is gone. Co-Authored-By: Claude Fable 5 --- lib/rubygems/config_file.rb | 7 +- lib/rubygems/remote_fetcher.rb | 50 +++++--- test/rubygems/test_gem_remote_fetcher.rb | 150 +++++++++++++++++++++++ 3 files changed, 192 insertions(+), 15 deletions(-) diff --git a/lib/rubygems/config_file.rb b/lib/rubygems/config_file.rb index e9cd2c6c3554..853de9e8ecd1 100644 --- a/lib/rubygems/config_file.rb +++ b/lib/rubygems/config_file.rb @@ -191,7 +191,12 @@ class Gem::ConfigFile ## # Use a global cache for .gem files shared across all Ruby installations. - # When enabled, gems are cached to ~/.cache/gem/gems (or XDG_CACHE_HOME/gem/gems). + # When enabled, gems fetched from a remote source are cached to + # ~/.cache/gem/gems (or XDG_CACHE_HOME/gem/gems). Gems installed from a + # local path are not, since a cached copy is later reused without being + # verified again. gem fetch still writes to the working directory, + # and an unwritable cache directory falls back to the cache of the + # installation. attr_accessor :global_gem_cache diff --git a/lib/rubygems/remote_fetcher.rb b/lib/rubygems/remote_fetcher.rb index 5b83dc6f6f20..3aa55f7963fb 100644 --- a/lib/rubygems/remote_fetcher.rb +++ b/lib/rubygems/remote_fetcher.rb @@ -47,6 +47,10 @@ class UnknownHostError < FetchError end deprecate_constant(:UnknownHostError) + # Schemes fetched over the network, as opposed to copied from a local path. + REMOTE_SCHEMES = %w[http https s3].freeze + private_constant :REMOTE_SCHEMES + @fetcher = nil ## @@ -113,12 +117,21 @@ def download_to_cache(dependency) def download(spec, source_uri, install_dir = Gem.dir) gem_file_name = File.basename spec.cache_file + source_uri = Gem::Uri.new(source_uri) + + scheme = source_uri.scheme + + # Gem::URI.parse gets confused by MS Windows paths with forward slashes. + scheme = nil if /^[a-z]$/i.match?(scheme) + + remote_source = REMOTE_SCHEMES.include?(scheme) + install_cache_dir = File.join install_dir, "cache" cache_dir = - if Gem.configuration.global_gem_cache - Gem.global_gem_cache_path - elsif Dir.pwd == install_dir # see fetch_command + if File.identical?(".", install_dir) # gem fetch asks for it this way install_dir + elsif Gem.configuration.global_gem_cache && remote_source && ensure_writable_cache_dir(Gem.global_gem_cache_path) + Gem.global_gem_cache_path elsif File.writable?(install_cache_dir) || (File.writable?(install_dir) && !File.exist?(install_cache_dir)) install_cache_dir else @@ -134,20 +147,15 @@ def download(spec, source_uri, install_dir = Gem.dir) nil end unless File.exist? cache_dir - source_uri = Gem::Uri.new(source_uri) - - scheme = source_uri.scheme - - # Gem::URI.parse gets confused by MS Windows paths with forward slashes. - scheme = nil if /^[a-z]$/i.match?(scheme) - # REFACTOR: split this up and dispatch on scheme (eg download_http) # REFACTOR: be sure to clean up fake fetcher when you do this... cleaner case scheme - when "http", "https", "s3" then - unless File.exist? local_gem_path + when *REMOTE_SCHEMES then + if File.exist? local_gem_path + verbose "Using local gem #{local_gem_path}" + else begin - verbose "Downloading gem #{gem_file_name}" + verbose "Downloading gem #{gem_file_name} to #{cache_dir}" remote_gem_path = source_uri + "gems/#{gem_file_name}" @@ -157,7 +165,7 @@ def download(spec, source_uri, install_dir = Gem.dir) alternate_name = "#{spec.original_name}.gem" - verbose "Failed, downloading gem #{alternate_name}" + verbose "Failed, downloading gem #{alternate_name} to #{cache_dir}" remote_gem_path = source_uri + "gems/#{alternate_name}" @@ -338,6 +346,20 @@ def close_all private + # Creates +cache_dir+ so its writability can be probed, since File.writable? + # is false for a path that does not exist yet. + + def ensure_writable_cache_dir(cache_dir) + require "fileutils" + begin + FileUtils.mkdir_p cache_dir + rescue SystemCallError + return false + end + + File.writable?(cache_dir) + end + def proxy_for(proxy, uri) Gem::Request.proxy_uri(proxy || Gem::Request.get_proxy_from_env(uri.scheme)) end diff --git a/test/rubygems/test_gem_remote_fetcher.rb b/test/rubygems/test_gem_remote_fetcher.rb index c35da2fc5ae2..d754a35ab0ff 100644 --- a/test/rubygems/test_gem_remote_fetcher.rb +++ b/test/rubygems/test_gem_remote_fetcher.rb @@ -643,6 +643,156 @@ def fetcher.fetch_path(uri, *rest) end end + def test_download_with_global_gem_cache_fetches_to_current_directory + test_cache_dir = File.join(@tempdir, "global_gem_cache_test") + + Gem.stub :global_gem_cache_path, test_cache_dir do + Gem.configuration.global_gem_cache = true + + fetcher = Gem::RemoteFetcher.fetcher + def fetcher.fetch_path(uri, *rest) + File.binread File.join(@test_gem_dir, "a-1.gem") + end + fetcher.instance_variable_set(:@test_gem_dir, File.dirname(@a1_gem)) + + fetch_dir = File.join @tempdir, "fetch_dir" + FileUtils.mkdir_p fetch_dir + + # gem fetch downloads into the current directory, see fetch_command + fetched_gem = Dir.chdir fetch_dir do + fetcher.download(@a1, "http://gems.example.com", fetch_dir) + end + + assert_equal File.join(fetch_dir, @a1.file_name), fetched_gem + assert File.exist?(fetched_gem) + refute File.exist?(test_cache_dir), + "gem fetch output should not be diverted to the global cache" + end + ensure + Gem.configuration.global_gem_cache = false + end + + def test_download_to_current_directory_reached_through_a_symlink + omit "symlinks are not usable on Windows" if Gem.win_platform? + + fetch_dir = File.join @tempdir, "fetch_dir" + FileUtils.mkdir_p fetch_dir + linked_dir = File.join @tempdir, "linked_dir" + File.symlink fetch_dir, linked_dir + + fetcher = Gem::RemoteFetcher.fetcher + def fetcher.fetch_path(uri, *rest) + File.binread File.join(@test_gem_dir, "a-1.gem") + end + fetcher.instance_variable_set(:@test_gem_dir, File.dirname(@a1_gem)) + + # gem fetch passes the working directory as install_dir, and the two can + # name the same directory through different paths + fetched_gem = Dir.chdir fetch_dir do + fetcher.download(@a1, "http://gems.example.com", linked_dir) + end + + assert_equal File.join(linked_dir, @a1.file_name), fetched_gem + assert File.exist?(fetched_gem) + end + + def test_download_local_with_global_gem_cache + omit "doesn't work if tempdir has +" if @tempdir.include?("+") + test_cache_dir = File.join(@tempdir, "global_gem_cache_test") + + Gem.stub :global_gem_cache_path, test_cache_dir do + Gem.configuration.global_gem_cache = true + + FileUtils.mv @a1_gem, @tempdir + local_path = File.join @tempdir, @a1.file_name + inst = nil + + Dir.chdir @tempdir do + inst = Gem::RemoteFetcher.fetcher + end + + assert_equal @a1.cache_file, inst.download(@a1, local_path) + refute File.exist?(test_cache_dir), + "local gems should not be copied to the global cache" + end + ensure + Gem.configuration.global_gem_cache = false + end + + def test_download_file_scheme_with_global_gem_cache + test_cache_dir = File.join(@tempdir, "global_gem_cache_test") + + Gem.stub :global_gem_cache_path, test_cache_dir do + Gem.configuration.global_gem_cache = true + + repo_dir = File.join @tempdir, "repo" + FileUtils.mkdir_p File.join(repo_dir, "gems") + FileUtils.cp @a1_gem, File.join(repo_dir, "gems", @a1.file_name) + + uri_path = repo_dir.start_with?("/") ? repo_dir : "/#{repo_dir}" + inst = Gem::RemoteFetcher.fetcher + + assert_equal @a1.cache_file, inst.download(@a1, "file://#{uri_path}") + assert File.exist?(@a1.cache_file) + refute File.exist?(test_cache_dir), + "local gems should not be copied to the global cache" + end + ensure + Gem.configuration.global_gem_cache = false + end + + unless Gem.win_platform? || Process.uid.zero? # File.chmod doesn't work + def test_download_with_global_gem_cache_not_writable + test_cache_dir = File.join(@tempdir, "global_gem_cache_test") + FileUtils.mkdir_p test_cache_dir + FileUtils.chmod 0o555, test_cache_dir + + Gem.stub :global_gem_cache_path, test_cache_dir do + Gem.configuration.global_gem_cache = true + + fetcher = Gem::RemoteFetcher.fetcher + def fetcher.fetch_path(uri, *rest) + File.binread File.join(@test_gem_dir, "a-1.gem") + end + fetcher.instance_variable_set(:@test_gem_dir, File.dirname(@a1_gem)) + + a1_cache_gem = @a1.cache_file + assert_equal a1_cache_gem, fetcher.download(@a1, "http://gems.example.com") + assert File.exist?(a1_cache_gem) + assert_empty Dir.children(test_cache_dir) + end + ensure + FileUtils.chmod 0o755, test_cache_dir if File.exist?(test_cache_dir) + Gem.configuration.global_gem_cache = false + end + + def test_download_with_global_gem_cache_not_creatable + parent_dir = File.join(@tempdir, "global_gem_cache_parent") + FileUtils.mkdir_p parent_dir + FileUtils.chmod 0o555, parent_dir + test_cache_dir = File.join(parent_dir, "gems") + + Gem.stub :global_gem_cache_path, test_cache_dir do + Gem.configuration.global_gem_cache = true + + fetcher = Gem::RemoteFetcher.fetcher + def fetcher.fetch_path(uri, *rest) + File.binread File.join(@test_gem_dir, "a-1.gem") + end + fetcher.instance_variable_set(:@test_gem_dir, File.dirname(@a1_gem)) + + a1_cache_gem = @a1.cache_file + assert_equal a1_cache_gem, fetcher.download(@a1, "http://gems.example.com") + assert File.exist?(a1_cache_gem) + refute File.exist?(test_cache_dir) + end + ensure + FileUtils.chmod 0o755, parent_dir if File.exist?(parent_dir) + Gem.configuration.global_gem_cache = false + end + + end + def test_fetch_http_with_custom_error_header fetcher = Gem::RemoteFetcher.new nil @fetcher = fetcher From 9507af2bf3b16107cdddf3b7254ec6518f1ee6ad Mon Sep 17 00:00:00 2001 From: Hiroshi SHIBATA Date: Fri, 28 Aug 2026 17:39:33 +0900 Subject: [PATCH 2/3] Copy local gems into the cache dir atomically The file and path schemes wrote into the shared cache directory with a plain FileUtils.cp, so a concurrent install could read a half-written .gem file. Route them through Gem::AtomicFileWriter like the http scheme, which also makes replacing a read-only cache copy work. cp passed the source mode to File.open, so it reached only a file being created and the umask still applied to it. Reproduce that rather than chmodding unconditionally, which would skip the umask, carry setuid across, and rewrite the mode of a file cp would have left alone. Co-Authored-By: Claude Fable 5 --- lib/rubygems/remote_fetcher.rb | 24 +++++++- test/rubygems/test_gem_remote_fetcher.rb | 78 ++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/lib/rubygems/remote_fetcher.rb b/lib/rubygems/remote_fetcher.rb index 3aa55f7963fb..d3ab256029e8 100644 --- a/lib/rubygems/remote_fetcher.rb +++ b/lib/rubygems/remote_fetcher.rb @@ -179,7 +179,7 @@ def download(spec, source_uri, install_dir = Gem.dir) remote_gem_path = Gem::Util.correct_for_windows_path(File.join(path, "gems", gem_file_name)) - FileUtils.cp(remote_gem_path, local_gem_path) + atomic_copy(remote_gem_path, local_gem_path) rescue Errno::EACCES local_gem_path = source_uri.to_s end @@ -196,7 +196,7 @@ def download(spec, source_uri, install_dir = Gem.dir) source_path = Gem::UriFormatter.new(source_path).unescape begin - FileUtils.cp source_path, local_gem_path unless + atomic_copy(source_path, local_gem_path) unless File.identical?(source_path, local_gem_path) rescue Errno::EACCES local_gem_path = source_uri.to_s @@ -360,6 +360,26 @@ def ensure_writable_cache_dir(cache_dir) File.writable?(cache_dir) end + def atomic_copy(source_path, destination_path) + File.open(source_path, "rb") do |source| + # FileUtils.cp passed the source mode to File.open, so it only reached a + # file being created and the umask still applied to it. The writer + # already carries over the mode of a file it replaces. + mode = source.stat.mode & 0o777 & ~File.umask + replacing = File.exist?(destination_path) + + Gem::AtomicFileWriter.open(destination_path) do |io| + IO.copy_stream(source, io) + + begin + io.chmod(mode) unless replacing + rescue Errno::EPERM, Errno::EACCES + # the filesystem does not carry permissions + end + end + end + end + def proxy_for(proxy, uri) Gem::Request.proxy_uri(proxy || Gem::Request.get_proxy_from_env(uri.scheme)) end diff --git a/test/rubygems/test_gem_remote_fetcher.rb b/test/rubygems/test_gem_remote_fetcher.rb index d754a35ab0ff..2c599e00986b 100644 --- a/test/rubygems/test_gem_remote_fetcher.rb +++ b/test/rubygems/test_gem_remote_fetcher.rb @@ -672,6 +672,63 @@ def fetcher.fetch_path(uri, *rest) Gem.configuration.global_gem_cache = false end + def test_download_local_takes_the_source_permissions_through_the_umask + omit "File.chmod doesn't work on Windows" if Gem.win_platform? + omit "doesn't work if tempdir has +" if @tempdir.include?("+") + + FileUtils.mv @a1_gem, @tempdir + local_path = File.join @tempdir, @a1.file_name + FileUtils.chmod 0o666, local_path + inst = nil + + Dir.chdir @tempdir do + inst = Gem::RemoteFetcher.fetcher + end + + assert_equal @a1.cache_file, inst.download(@a1, local_path) + assert_equal 0o666 & ~File.umask, File.stat(@a1.cache_file).mode & 0o777 + end + + def test_download_local_keeps_a_restrictive_source_permission + omit "File.chmod doesn't work on Windows" if Gem.win_platform? + omit "doesn't work if tempdir has +" if @tempdir.include?("+") + + FileUtils.mv @a1_gem, @tempdir + local_path = File.join @tempdir, @a1.file_name + FileUtils.chmod 0o600, local_path + inst = nil + + Dir.chdir @tempdir do + inst = Gem::RemoteFetcher.fetcher + end + + # a mode the writer would not produce on its own, so dropping the chmod + # would show up here + assert_equal @a1.cache_file, inst.download(@a1, local_path) + assert_equal 0o600, File.stat(@a1.cache_file).mode & 0o777 + end + + def test_download_local_keeps_the_replaced_cache_file_permissions + omit "File.chmod doesn't work on Windows" if Gem.win_platform? + omit "doesn't work if tempdir has +" if @tempdir.include?("+") + + FileUtils.mv @a1_gem, @tempdir + local_path = File.join @tempdir, @a1.file_name + FileUtils.chmod 0o666, local_path + inst = nil + + FileUtils.mkdir_p File.dirname(@a1.cache_file) + FileUtils.touch @a1.cache_file + FileUtils.chmod 0o640, @a1.cache_file + + Dir.chdir @tempdir do + inst = Gem::RemoteFetcher.fetcher + end + + assert_equal @a1.cache_file, inst.download(@a1, local_path) + assert_equal 0o640, File.stat(@a1.cache_file).mode & 0o777 + end + def test_download_to_current_directory_reached_through_a_symlink omit "symlinks are not usable on Windows" if Gem.win_platform? @@ -791,6 +848,27 @@ def fetcher.fetch_path(uri, *rest) Gem.configuration.global_gem_cache = false end + def test_download_local_replaces_read_only_cache_file + omit "doesn't work if tempdir has +" if @tempdir.include?("+") + FileUtils.mv @a1_gem, @tempdir + local_path = File.join @tempdir, @a1.file_name + inst = nil + + FileUtils.mkdir_p File.dirname(@a1.cache_file) + FileUtils.touch @a1.cache_file + FileUtils.chmod 0o444, @a1.cache_file + + Dir.chdir @tempdir do + inst = Gem::RemoteFetcher.fetcher + end + + # the atomic replacement of the cache copy must not depend on the + # permissions of the previous file + assert_equal @a1.cache_file, inst.download(@a1, local_path) + assert_equal File.binread(local_path), File.binread(@a1.cache_file) + ensure + FileUtils.chmod 0o644, @a1.cache_file if File.exist?(@a1.cache_file) + end end def test_fetch_http_with_custom_error_header From ca3c7c08b7c0b4f0338cf02ba9b9d7bbc301b20e Mon Sep 17 00:00:00 2001 From: Hiroshi SHIBATA Date: Fri, 28 Aug 2026 17:39:46 +0900 Subject: [PATCH 3/3] Gate the global cache specs on the version that ships the API The expected cache base was computed from Gem.global_gem_cache_path, the same expression the code under test uses, so the assertion could never catch an unintended path change. That method was added after 4.0 was cut, so no released 4.0.x has it and the old gate would make the specs expect the RubyGems layout from a Bundler that falls back to its own. Co-Authored-By: Claude Fable 5 --- spec/install/global_cache_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/install/global_cache_spec.rb b/spec/install/global_cache_spec.rb index 5e5494dbe9d3..ce0564ee7851 100644 --- a/spec/install/global_cache_spec.rb +++ b/spec/install/global_cache_spec.rb @@ -9,11 +9,11 @@ let(:source2) { "http://gemserver.example.org" } def cache_base - # Use the unified global gem cache path if the RubyGems under test - # provides it, otherwise fall back to the Bundler-specific cache - # location that Bundler uses on RubyGems older than 4.0 - if exercised_rubygems_version >= Gem::Version.new("4.0.0.a") - Pathname.new(Gem.global_gem_cache_path) + # Gem.global_gem_cache_path first ships in RubyGems 4.1. Older RubyGems + # fall back to the Bundler-specific cache location. The suite clears + # XDG_CACHE_HOME, so the path resolves to the ~/.cache default. + if exercised_rubygems_version >= Gem::Version.new("4.1.0.a") + home(".cache", "gem", "gems") else home(".bundle", "cache", "gems") end