Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor usage of Dir[] to eliminate unsafe glob construction #331

Closed
wants to merge 3 commits into from
Closed

Refactor usage of Dir[] to eliminate unsafe glob construction #331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32326ea
Refactor usage of Dir[] to eliminate unsafe glob construction
pixeltrix May 16, 2012
ce34bd0
Use File.directory? for 1.8.7 compatibility
pixeltrix May 16, 2012
a1d8313
Don't globally untaint globs
pixeltrix May 17, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions lib/rubygems.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ def self.all_load_paths
# Return all the partial paths in +gemdir+. # Return all the partial paths in +gemdir+.


def self.all_partials(gemdir) def self.all_partials(gemdir)
Dir[File.join(gemdir, "gems/*")] Gem.glob(gemdir, "gems/*")
end end


private_class_method :all_partials private_class_method :all_partials
Expand Down Expand Up @@ -469,7 +469,7 @@ def self.find_files(glob, check_load_path=true)


if check_load_path if check_load_path
files = $LOAD_PATH.map { |load_path| files = $LOAD_PATH.map { |load_path|
Dir["#{File.expand_path glob, load_path}#{Gem.suffix_pattern}"] Gem.glob(load_path, "#{glob}#{Gem.suffix_pattern}")
}.flatten.select { |file| File.file? file.untaint } }.flatten.select { |file| File.file? file.untaint }
end end


Expand Down Expand Up @@ -524,6 +524,21 @@ def self.find_home


private_class_method :find_home private_class_method :find_home


##
# Dir.glob wrapper that takes a base directory

def self.glob(dir, pattern)
# TODO: move to utils
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o_O

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming at some point a whole bunch of these methods would be moved to Gem::Utils - that doesn't exist at the moment does it? I didn't want to pollute the pull request with unrelated changes.

dir = File.expand_path(dir)
return [] unless File.directory?(dir)

Dir.chdir dir do
Dir.glob(pattern).map do |filename|
File.join(dir, filename)
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing directories feels wrong, for multithreaded programs it randomly disrupts Dir.pwd for other threads.

Checking if a directory exists and rebuilding paths feels like unnecessary work that glob can do for us.

Why not the simpler:

def self.glob dir, pattern
  dir = dir.gsub(/[*?{}\[\]]/, '\\\\\&')

  pattern = File.join dir, pattern

  Dir.glob pattern
end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what's I was originally going to do - but I discovered jruby/jruby#172 and @tenderlove suggested I use Dir.chdir in rack/rack#389

end

## ##
# Zlib::GzipReader wrapper that unzips +data+. # Zlib::GzipReader wrapper that unzips +data+.


Expand Down Expand Up @@ -1099,11 +1114,9 @@ def self.load_plugins
# Find all 'rubygems_plugin' files in $LOAD_PATH and load them # Find all 'rubygems_plugin' files in $LOAD_PATH and load them


def self.load_env_plugins def self.load_env_plugins
path = "rubygems_plugin"

files = [] files = []
$LOAD_PATH.each do |load_path| $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| globbed.each do |load_path_file|
files << load_path_file if File.file?(load_path_file.untaint) files << load_path_file if File.file?(load_path_file.untaint)
Expand Down
6 changes: 3 additions & 3 deletions lib/rubygems/commands/contents_command.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ def execute
end end


gem_path = spec.full_gem_path gem_path = spec.full_gem_path
extra = "/{#{spec.require_paths.join ','}}" if options[:lib_only] extra = "{#{spec.require_paths.join ','}}/" if options[:lib_only]
glob = "#{gem_path}#{extra}/**/*" glob = "#{extra}**/*"
files = Dir[glob] files = Gem.glob(gem_path, glob)


gem_path = File.join gem_path, '' # add trailing / if missing gem_path = File.join gem_path, '' # add trailing / if missing


Expand Down
2 changes: 1 addition & 1 deletion lib/rubygems/commands/setup_command.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ def install_rdoc
(not File.exist? rubygems_doc_dir or (not File.exist? rubygems_doc_dir or
File.writable? rubygems_doc_dir) then File.writable? rubygems_doc_dir) then
say "Removing old RubyGems RDoc and ri" if @verbose 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 rm_rf dir
end end


Expand Down
2 changes: 1 addition & 1 deletion lib/rubygems/commands/stale_command.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def execute
gem_to_atime = {} gem_to_atime = {}
Gem::Specification.each do |spec| Gem::Specification.each do |spec|
name = spec.full_name 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) next if File.directory?(file)
stat = File.stat(file) stat = File.stat(file)
gem_to_atime[name] ||= stat.atime gem_to_atime[name] ||= stat.atime
Expand Down
2 changes: 1 addition & 1 deletion lib/rubygems/indexer.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ def compress(filename, extension)
# List of gem file names to index. # List of gem file names to index.


def gem_file_list def gem_file_list
Dir[File.join(@dest_directory, "gems", '*.gem')] Gem.glob(File.join(@dest_directory, 'gems'), '*.gem')
end end


## ##
Expand Down
2 changes: 1 addition & 1 deletion lib/rubygems/installer.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ def installed_specs
@specs ||= begin @specs ||= begin
specs = [] specs = []


Dir[File.join(gem_home, "specifications", "*.gemspec")].each do |path| Gem.glob(File.join(gem_home, "specifications"), "*.gemspec").each do |path|
spec = Gem::Specification.load path.untaint spec = Gem::Specification.load path.untaint
specs << spec if spec specs << spec if spec
end end
Expand Down
2 changes: 1 addition & 1 deletion lib/rubygems/request_set.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def sorted_requests
end end


def specs_in(dir) def specs_in(dir)
Dir["#{dir}/specifications/*.gemspec"].map do |g| Gem.glob(dir, "specifications/*.gemspec").map do |g|
Gem::Specification.load g Gem::Specification.load g
end end
end end
Expand Down
4 changes: 1 addition & 3 deletions lib/rubygems/security/trust_dir.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ def cert_path certificate
def each_certificate def each_certificate
return enum_for __method__ unless block_given? return enum_for __method__ unless block_given?


glob = File.join @dir, '*.pem' Gem.glob(@dir, '*.pem').each do |certificate_file|

Dir[glob].each do |certificate_file|
begin begin
certificate = load_certificate certificate_file certificate = load_certificate certificate_file


Expand Down
8 changes: 3 additions & 5 deletions lib/rubygems/specification.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ def self._all # :nodoc:
specs = {} specs = {}


self.dirs.each { |dir| self.dirs.each { |dir|
Dir[File.join(dir, "*.gemspec")].each { |path| Gem.glob(dir, "*.gemspec").each { |path|
spec = Gem::Specification.load path.untaint spec = Gem::Specification.load path.untaint
# #load returns nil if the spec is bad, so we just ignore # #load returns nil if the spec is bad, so we just ignore
# it at this stage # it at this stage
Expand Down Expand Up @@ -1827,10 +1827,8 @@ def mark_version
# Return all files in this gem that match for +glob+. # Return all files in this gem that match for +glob+.


def matches_for_glob glob # TODO: rename? def matches_for_glob glob # TODO: rename?
# TODO: do we need these?? Kill it matches = Gem.glob(full_gem_path, "{#{require_paths.join ','}}/#{glob}")
glob = File.join(self.lib_dirs_glob, glob) matches.map{ |f| f.untaint } # FIX our tests are broken, run w/ SAFE=1

Dir[glob].map { |f| f.untaint } # FIX our tests are broken, run w/ SAFE=1
end end


## ##
Expand Down
8 changes: 4 additions & 4 deletions lib/rubygems/test_case.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def refute_path_exists path, msg = nil
# or <tt>i686-darwin8.10.1</tt> otherwise. # or <tt>i686-darwin8.10.1</tt> otherwise.
# #
# If the +KEEP_FILES+ environment variable is set the files will not be # 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 def setup
super super
Expand All @@ -122,9 +122,9 @@ def setup
tmpdir = File.expand_path("tmp/test") tmpdir = File.expand_path("tmp/test")


if ENV['KEEP_FILES'] then 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}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you've wrapped the value with {}. Could you explain?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test that glob characters don't cause a problem - writing a whole bunch of extra tests seemed excessive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not excessive when someone comes along and removes that because they don't know why it's there

else else
@tempdir = File.join(tmpdir, "test_rubygems_#{$$}") @tempdir = File.join(tmpdir, "test_rubygems_{#{$$}}")
end end
@tempdir.untaint @tempdir.untaint
@gemhome = File.join @tempdir, 'gemhome' @gemhome = File.join @tempdir, 'gemhome'
Expand Down Expand Up @@ -298,7 +298,7 @@ def uninstall_gem spec
def create_tmpdir def create_tmpdir
tmpdir = nil tmpdir = nil
Dir.chdir Dir.tmpdir do tmpdir = Dir.pwd end # HACK OSX /private/tmp 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 FileUtils.mkdir_p tmpdir
return tmpdir return tmpdir
end end
Expand Down
4 changes: 2 additions & 2 deletions test/rubygems/test_gem_commands_install_command.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ def test_execute_remote_ignores_files
FileUtils.rm_rf a1_gemspec FileUtils.rm_rf a1_gemspec
FileUtils.rm_rf a2_gemspec FileUtils.rm_rf a2_gemspec


start = Dir["#{gemdir}/*"] start = Gem.glob(gemdir, "*")


use_ui @ui do use_ui @ui do
Dir.chdir @tempdir do Dir.chdir @tempdir do
Expand All @@ -408,7 +408,7 @@ def test_execute_remote_ignores_files
assert_equal "1 gem installed", out.shift assert_equal "1 gem installed", out.shift
assert out.empty?, out.inspect assert out.empty?, out.inspect


fin = Dir["#{gemdir}/*"] fin = Gem.glob(gemdir, "*")


assert_equal [a1_gemspec], fin - start assert_equal [a1_gemspec], fin - start
end end
Expand Down
2 changes: 1 addition & 1 deletion test/rubygems/test_gem_indexer.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def setup


gems = File.join(@tempdir, 'gems') gems = File.join(@tempdir, 'gems')
FileUtils.mkdir_p 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, @indexer = Gem::Indexer.new(@tempdir,
:rss_title => 'ExampleForge gems', :rss_title => 'ExampleForge gems',
Expand Down
7 changes: 5 additions & 2 deletions test/rubygems/test_gem_remote_fetcher.rb
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -292,8 +292,11 @@ def test_download_local_read_only
inst = Gem::RemoteFetcher.fetcher inst = Gem::RemoteFetcher.fetcher
end end


assert_equal(File.join(@tempdir, @a1.file_name), uri = URI.const_defined?(:DEFAULT_PARSER) ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here? It seems unrelated to the rest of this change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The url returned by download is escaped, so the expected value needs to be escaped as well

inst.download(@a1, local_path)) 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 ensure
FileUtils.chmod 0755, @a1.cache_dir FileUtils.chmod 0755, @a1.cache_dir
end end
Expand Down