Harden the global gem cache

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. <tt>gem fetch</tt> still writes to the working directory,
+  # and an unwritable cache directory falls back to the cache of the
+  # installation.
 
   attr_accessor :global_gem_cache
 

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}"
 
@@ -171,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
@@ -188,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
@@ -338,6 +346,40 @@ 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 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

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

test/rubygems/test_gem_remote_fetcher.rb

@@ -643,6 +643,234 @@ 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_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?
+
+    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
+
+    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
     fetcher = Gem::RemoteFetcher.new nil
     @fetcher = fetcher