Refactor usage of Dir[] to eliminate unsafe glob construction

Contructing globs by concatenation through File.join or string interpolation is
susceptible to errors caused by paths inadvertently including glob characters.
This commit fixes this by using Dir.chdir to change the current directory to a
safe point from which to execute the Dir.glob and then reconstructs the path.

There are no specific tests that have been added but the temporary test
directory name has been altered to include the {} characters. This causes a
significant number of the existing tests to fail which should prevent any
regressions.

lib/rubygems.rb

@@ -261,7 +261,7 @@ def self.all_load_paths
   # Return all the partial paths in +gemdir+.
 
   def self.all_partials(gemdir)
-    Dir[File.join(gemdir, "gems/*")]
+    Gem.glob(gemdir, "gems/*")
   end
 
   private_class_method :all_partials
@@ -469,8 +469,8 @@ def self.find_files(glob, check_load_path=true)
 
     if check_load_path
       files = $LOAD_PATH.map { |load_path|
-        Dir["#{File.expand_path glob, load_path}#{Gem.suffix_pattern}"]
-      }.flatten.select { |file| File.file? file.untaint }
+        Gem.glob(load_path, "#{glob}#{Gem.suffix_pattern}")
+      }.flatten.select { |file| File.file? file }
     end
 
     files.concat Gem::Specification.map { |spec|
@@ -524,6 +524,21 @@ def self.find_home
 
   private_class_method :find_home
 
+  ##
+  # Dir.glob wrapper that takes a base directory
+
+  def self.glob(dir, pattern)
+    # TODO: move to utils
+    dir = File.expand_path(dir)
+    return [] unless Dir.exists?(dir)
+
+    Dir.chdir dir do
+      Dir.glob(pattern).map do |filename|
+        File.join(dir, filename).untaint
+      end
+    end
+  end
+
   ##
   # Zlib::GzipReader wrapper that unzips +data+.
 
@@ -1099,11 +1114,9 @@ def self.load_plugins
   # Find all 'rubygems_plugin' files in $LOAD_PATH and load them
 
   def self.load_env_plugins
-    path = "rubygems_plugin"
-
     files = []
     $LOAD_PATH.each do |load_path|
-      globbed = Dir["#{File.expand_path path, load_path}#{Gem.suffix_pattern}"]
+      globbed = Gem.glob(load_path, "rubygems_plugin#{Gem.suffix_pattern}")
 
       globbed.each do |load_path_file|
         files << load_path_file if File.file?(load_path_file.untaint)

lib/rubygems/commands/contents_command.rb

@@ -81,9 +81,9 @@ def execute
       end
 
       gem_path = spec.full_gem_path
-      extra    = "/{#{spec.require_paths.join ','}}" if options[:lib_only]
-      glob     = "#{gem_path}#{extra}/**/*"
-      files    = Dir[glob]
+      extra    = "{#{spec.require_paths.join ','}}/" if options[:lib_only]
+      glob     = "#{extra}**/*"
+      files    = Gem.glob(gem_path, glob)
 
       gem_path = File.join gem_path, '' # add trailing / if missing
 

lib/rubygems/commands/setup_command.rb

@@ -254,7 +254,7 @@ def install_rdoc
        (not File.exist? rubygems_doc_dir or
         File.writable? rubygems_doc_dir) then
       say "Removing old RubyGems RDoc and ri" if @verbose
-      Dir[File.join(Gem.dir, 'doc', 'rubygems-[0-9]*')].each do |dir|
+      Gem.glob(File.join(Gem.dir, 'doc'), 'rubygems-[0-9]*').each do |dir|
         rm_rf dir
       end
 

lib/rubygems/commands/stale_command.rb

@@ -13,7 +13,7 @@ def execute
     gem_to_atime = {}
     Gem::Specification.each do |spec|
       name = spec.full_name
-      Dir["#{spec.full_gem_path}/**/*.*"].each do |file|
+      Gem.glob(spec.full_gem_path, "**/*.*").each do |file|
         next if File.directory?(file)
         stat = File.stat(file)
         gem_to_atime[name] ||= stat.atime

lib/rubygems/indexer.rb

@@ -400,7 +400,7 @@ def compress(filename, extension)
   # List of gem file names to index.
 
   def gem_file_list
-    Dir[File.join(@dest_directory, "gems", '*.gem')]
+    Gem.glob(File.join(@dest_directory, 'gems'), '*.gem')
   end
 
   ##

lib/rubygems/installer.rb

@@ -299,8 +299,8 @@ def installed_specs
     @specs ||= begin
       specs = []
 
-      Dir[File.join(gem_home, "specifications", "*.gemspec")].each do |path|
-        spec = Gem::Specification.load path.untaint
+      Gem.glob(File.join(gem_home, "specifications"), "*.gemspec").each do |path|
+        spec = Gem::Specification.load path
         specs << spec if spec
       end
 

lib/rubygems/request_set.rb

@@ -83,7 +83,7 @@ def sorted_requests
     end
 
     def specs_in(dir)
-      Dir["#{dir}/specifications/*.gemspec"].map do |g|
+      Gem.glob(dir, "specifications/*.gemspec").map do |g|
         Gem::Specification.load g
       end
     end

lib/rubygems/security/trust_dir.rb

@@ -27,9 +27,7 @@ def cert_path certificate
   def each_certificate
     return enum_for __method__ unless block_given?
 
-    glob = File.join @dir, '*.pem'
-
-    Dir[glob].each do |certificate_file|
+    Gem.glob(@dir, '*.pem').each do |certificate_file|
       begin
         certificate = load_certificate certificate_file
 

lib/rubygems/specification.rb

@@ -632,8 +632,8 @@ def self._all # :nodoc:
       specs = {}
 
       self.dirs.each { |dir|
-        Dir[File.join(dir, "*.gemspec")].each { |path|
-          spec = Gem::Specification.load path.untaint
+        Gem.glob(dir, "*.gemspec").each { |path|
+          spec = Gem::Specification.load path
           # #load returns nil if the spec is bad, so we just ignore
           # it at this stage
           specs[spec.full_name] ||= spec if spec
@@ -1827,10 +1827,7 @@ def mark_version
   # Return all files in this gem that match for +glob+.
 
   def matches_for_glob glob # TODO: rename?
-    # TODO: do we need these?? Kill it
-    glob = File.join(self.lib_dirs_glob, glob)
-
-    Dir[glob].map { |f| f.untaint } # FIX our tests are broken, run w/ SAFE=1
+    Gem.glob(full_gem_path, "{#{require_paths.join ','}}/#{glob}")
   end
 
   ##

lib/rubygems/test_case.rb

@@ -107,7 +107,7 @@ def refute_path_exists path, msg = nil
   # or <tt>i686-darwin8.10.1</tt> otherwise.
   #
   # If the +KEEP_FILES+ environment variable is set the files will not be
-  # removed from <tt>/tmp/test_rubygems_#{$$}.#{Time.now.to_i}</tt>.
+  # removed from <tt>/tmp/test_rubygems_{#{$$}}.#{Time.now.to_i}</tt>.
 
   def setup
     super
@@ -122,9 +122,9 @@ def setup
     tmpdir = File.expand_path("tmp/test")
 
     if ENV['KEEP_FILES'] then
-      @tempdir = File.join(tmpdir, "test_rubygems_#{$$}.#{Time.now.to_i}")
+      @tempdir = File.join(tmpdir, "test_rubygems_{#{$$}}.#{Time.now.to_i}")
     else
-      @tempdir = File.join(tmpdir, "test_rubygems_#{$$}")
+      @tempdir = File.join(tmpdir, "test_rubygems_{#{$$}}")
     end
     @tempdir.untaint
     @gemhome  = File.join @tempdir, 'gemhome'
@@ -298,7 +298,7 @@ def uninstall_gem spec
   def create_tmpdir
     tmpdir = nil
     Dir.chdir Dir.tmpdir do tmpdir = Dir.pwd end # HACK OSX /private/tmp
-    tmpdir = File.join tmpdir, "test_rubygems_#{$$}"
+    tmpdir = File.join tmpdir, "test_rubygems_{#{$$}}"
     FileUtils.mkdir_p tmpdir
     return tmpdir
   end

test/rubygems/test_gem_commands_install_command.rb

@@ -391,7 +391,7 @@ def test_execute_remote_ignores_files
     FileUtils.rm_rf a1_gemspec
     FileUtils.rm_rf a2_gemspec
 
-    start = Dir["#{gemdir}/*"]
+    start = Gem.glob(gemdir, "*")
 
     use_ui @ui do
       Dir.chdir @tempdir do
@@ -408,7 +408,7 @@ def test_execute_remote_ignores_files
     assert_equal "1 gem installed", out.shift
     assert out.empty?, out.inspect
 
-    fin = Dir["#{gemdir}/*"]
+    fin = Gem.glob(gemdir, "*")
 
     assert_equal [a1_gemspec], fin - start
   end

test/rubygems/test_gem_indexer.rb

@@ -28,7 +28,7 @@ def setup
 
     gems = File.join(@tempdir, 'gems')
     FileUtils.mkdir_p gems
-    FileUtils.mv Dir[File.join(@gemhome, "cache", '*.gem')], gems
+    FileUtils.mv Gem.glob(File.join(@gemhome, "cache"), '*.gem'), gems
 
     @indexer = Gem::Indexer.new(@tempdir,
                                 :rss_title     => 'ExampleForge gems',

test/rubygems/test_gem_remote_fetcher.rb

@@ -292,8 +292,11 @@ def test_download_local_read_only
         inst = Gem::RemoteFetcher.fetcher
       end
 
-      assert_equal(File.join(@tempdir, @a1.file_name),
-                   inst.download(@a1, local_path))
+      uri = URI.const_defined?(:DEFAULT_PARSER) ?
+            URI::DEFAULT_PARSER.escape(File.join(@tempdir, @a1.file_name)) :
+            URI.escape(File.join(@tempdir, @a1.file_name))
+
+      assert_equal(uri, inst.download(@a1, local_path))
     ensure
       FileUtils.chmod 0755, @a1.cache_dir
     end