diff --git a/ChangeLog b/ChangeLog index eb9f689151..4fd14736c2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,33 @@ +Tue Feb 5 11:35:35 2013 Eric Hodel + + * lib/rubygems/commands/push_command.rb: Fixed credential download for + `gem push --host` + * lib/rubygems/gemcutter_utilities.rb: ditto. + * test/rubygems/test_gem_commands_push_command.rb: Test for the above. + * test/rubygems/test_gem_gemcutter_utilities.rb: ditto. + + * lib/rubygems/config_file.rb: Abort if the `gem push` credentials + file has insecure permissions. + * test/rubygems/test_gem_config_file.rb: Test for the above. + + * lib/rubygems/ext/builder.rb: Do not look for Gemfile, Isolate, etc. + while building gem extensions. + + * lib/rubygems/package.rb: Unset spec and files list if a gem's + signatures cannot be verified. + * test/rubygems/test_gem_package.rb: Test for the above. + + * lib/rubygems/specification.rb: Reduce use of eval. + * lib/rubygems/test_case.rb: ditto. + + * test/rubygems/test_gem_specification.rb: Test setting + specification_version for legacy gems. Dup Gem.ruby before + untainting in case it's frozen. + + * lib/rubygems.rb: Reduce use of eval. Only read files when looking + for Gemfile, Isolate, etc. + * test/rubygems/test_gem.rb: Test for the above. + Tue Feb 5 10:15:00 2013 Zachary Scott * doc/security.rdoc: Wrap security guide at 80 columns diff --git a/lib/rubygems.rb b/lib/rubygems.rb index dcfe74039c..0fb718aedc 100644 --- a/lib/rubygems.rb +++ b/lib/rubygems.rb @@ -207,7 +207,7 @@ module Gem begin while true - path = GEM_DEP_FILES.find { |f| File.exists?(f) } + path = GEM_DEP_FILES.find { |f| File.file?(f) } if path path = File.join here, path @@ -226,7 +226,9 @@ module Gem end end - return unless File.exists? path + path.untaint + + return unless File.file? path rs = Gem::RequestSet.new rs.load_gemdeps path @@ -369,29 +371,6 @@ module Gem paths.path end - ## - # Expand each partial gem path with each of the required paths specified - # in the Gem spec. Each expanded path is yielded. - - def self.each_load_path(partials) - partials.each do |gp| - base = File.basename gp - specfn = File.join(dir, "specifications", "#{base}.gemspec") - if File.exists? specfn - spec = eval(File.read(specfn)) - spec.require_paths.each do |rp| - yield File.join(gp,rp) - end - else - filename = File.join(gp, 'lib') - yield(filename) if File.exists? filename - end - end - end - - private_class_method :each_load_path - - ## # Quietly ensure the named Gem directory contains all the proper # subdirectories. If we can't create a directory due to a permission diff --git a/lib/rubygems/commands/push_command.rb b/lib/rubygems/commands/push_command.rb index ce70836823..2df00b0457 100644 --- a/lib/rubygems/commands/push_command.rb +++ b/lib/rubygems/commands/push_command.rb @@ -24,16 +24,19 @@ class Gem::Commands::PushCommand < Gem::Command add_proxy_option add_key_option - add_option( - '--host HOST', - 'Push to another gemcutter-compatible host' - ) do |value, options| + add_option('--host HOST', + 'Push to another gemcutter-compatible host') do |value, options| options[:host] = value end + + @host = nil end def execute - sign_in + @host = options[:host] + + sign_in @host + send_gem get_one_gem_name end @@ -44,26 +47,30 @@ class Gem::Commands::PushCommand < Gem::Command if latest_rubygems_version < Gem.rubygems_version and Gem.rubygems_version.prerelease? and - Gem::Version.new('2.0.0.preview3') != Gem.rubygems_version then + Gem::Version.new('2.0.0.rc.2') != Gem.rubygems_version then alert_error <<-ERROR You are using a beta release of RubyGems (#{Gem::VERSION}) which is not allowed to push gems. Please downgrade or upgrade to a release version. The latest released RubyGems version is #{latest_rubygems_version} + +You can upgrade or downgrade to the latest release version with: + + gem update --system=#{latest_rubygems_version} + ERROR terminate_interaction 1 end - host = options[:host] - unless host + unless @host then if gem_data = Gem::Package.new(name) then - host = gem_data.spec.metadata['default_gem_server'] + @host = gem_data.spec.metadata['default_gem_server'] end end - args << host if host + args << @host if @host - say "Pushing gem to #{host || Gem.host}..." + say "Pushing gem to #{@host || Gem.host}..." response = rubygems_api_request(*args) do |request| request.body = Gem.read_binary name diff --git a/lib/rubygems/config_file.rb b/lib/rubygems/config_file.rb index 81ee32a1d6..7e1432b349 100644 --- a/lib/rubygems/config_file.rb +++ b/lib/rubygems/config_file.rb @@ -33,6 +33,8 @@ class Gem::ConfigFile + include Gem::UserInteraction + DEFAULT_BACKTRACE = false DEFAULT_BULK_THRESHOLD = 1000 DEFAULT_VERBOSITY = true @@ -223,6 +225,34 @@ class Gem::ConfigFile handle_arguments arg_list end + ## + # Checks the permissions of the credentials file. If they are not 0600 an + # error message is displayed and RubyGems aborts. + + def check_credentials_permissions + return unless File.exist? credentials_path + + existing_permissions = File.stat(credentials_path).mode & 0777 + + return if existing_permissions == 0600 + + alert_error <<-ERROR +Your gem push credentials file located at: + +\t#{credentials_path} + +has file permissions of 0#{existing_permissions.to_s 8} but 0600 is required. + +You should reset your credentials at: + +\thttps://rubygems.org/profile/edit + +if you believe they were disclosed to a third party. + ERROR + + terminate_interaction 1 + end + ## # Location of RubyGems.org credentials @@ -231,6 +261,8 @@ class Gem::ConfigFile end def load_api_keys + check_credentials_permissions + @api_keys = if File.exist? credentials_path then load_file(credentials_path) else @@ -243,7 +275,9 @@ class Gem::ConfigFile end end - def rubygems_api_key=(api_key) + def rubygems_api_key= api_key + check_credentials_permissions + config = load_file(credentials_path).merge(:rubygems_api_key => api_key) dirname = File.dirname credentials_path diff --git a/lib/rubygems/ext/builder.rb b/lib/rubygems/ext/builder.rb index 81b96e84a9..d7d953fec3 100644 --- a/lib/rubygems/ext/builder.rb +++ b/lib/rubygems/ext/builder.rb @@ -43,12 +43,18 @@ class Gem::Ext::Builder def self.run(command, results, command_name = nil) verbose = Gem.configuration.really_verbose - if verbose - puts(command) - system(command) - else - results << command - results << `#{command} #{redirector}` + begin + # TODO use Process.spawn when ruby 1.8 support is dropped. + rubygems_gemdeps, ENV['RUBYGEMS_GEMDEPS'] = ENV['RUBYGEMS_GEMDEPS'], nil + if verbose + puts(command) + system(command) + else + results << command + results << `#{command} #{redirector}` + end + ensure + ENV['RUBYGEMS_GEMDEPS'] = rubygems_gemdeps end unless $?.success? then diff --git a/lib/rubygems/gemcutter_utilities.rb b/lib/rubygems/gemcutter_utilities.rb index 3042092125..474d07f775 100644 --- a/lib/rubygems/gemcutter_utilities.rb +++ b/lib/rubygems/gemcutter_utilities.rb @@ -27,17 +27,25 @@ module Gem::GemcutterUtilities end end - def sign_in + def sign_in sign_in_host = self.host return if Gem.configuration.rubygems_api_key - say "Enter your RubyGems.org credentials." - say "Don't have an account yet? Create one at http://rubygems.org/sign_up" + pretty_host = if Gem::DEFAULT_HOST == sign_in_host then + 'RubyGems.org' + else + sign_in_host + end + + say "Enter your #{pretty_host} credentials." + say "Don't have an account yet? " + + "Create one at https://#{sign_in_host}/sign_up" email = ask " Email: " password = ask_for_password "Password: " say "\n" - response = rubygems_api_request :get, "api/v1/api_key" do |request| + response = rubygems_api_request(:get, "api/v1/api_key", + sign_in_host) do |request| request.basic_auth email, password end diff --git a/lib/rubygems/package.rb b/lib/rubygems/package.rb index e33dea06e9..c662da2a55 100644 --- a/lib/rubygems/package.rb +++ b/lib/rubygems/package.rb @@ -473,6 +473,10 @@ EOM @security_policy true + rescue Gem::Security::Exception + @spec = nil + @files = [] + raise rescue Errno::ENOENT => e raise Gem::Package::FormatError.new e.message rescue Gem::Package::TarInvalidError => e diff --git a/lib/rubygems/specification.rb b/lib/rubygems/specification.rb index 841bfbf842..cabdf8df7f 100644 --- a/lib/rubygems/specification.rb +++ b/lib/rubygems/specification.rb @@ -904,7 +904,7 @@ class Gem::Specification raise Gem::Exception, "YAML data doesn't evaluate to gem specification" end - spec.instance_eval { @specification_version ||= NONEXISTENT_SPECIFICATION_VERSION } + spec.specification_version ||= NONEXISTENT_SPECIFICATION_VERSION spec.reset_nil_attributes_to_default spec diff --git a/lib/rubygems/test_case.rb b/lib/rubygems/test_case.rb index 47fb9d4962..3da3173285 100644 --- a/lib/rubygems/test_case.rb +++ b/lib/rubygems/test_case.rb @@ -143,8 +143,9 @@ class Gem::TestCase < MiniTest::Unit::TestCase @gemhome = File.join @tempdir, 'gemhome' @userhome = File.join @tempdir, 'userhome' - @orig_ruby = if ruby = ENV['RUBY'] then - Gem.class_eval { ruby, @ruby = @ruby, ruby.dup } + @orig_ruby = if ENV['RUBY'] then + ruby = Gem.instance_variable_get :@ruby + Gem.instance_variable_set :@ruby, ENV['RUBY'] ruby end @@ -264,7 +265,7 @@ class Gem::TestCase < MiniTest::Unit::TestCase ENV['GEM_PATH'] = @orig_gem_path _ = @orig_ruby - Gem.class_eval { @ruby = _ } if _ + Gem.instance_variable_set :@ruby, @orig_ruby if @orig_ruby if @orig_ENV_HOME then ENV['HOME'] = @orig_ENV_HOME diff --git a/test/rubygems/test_gem.rb b/test/rubygems/test_gem.rb index 8fbae7f669..fdeef699d4 100644 --- a/test/rubygems/test_gem.rb +++ b/test/rubygems/test_gem.rb @@ -667,6 +667,25 @@ class TestGem < Gem::TestCase assert_equal %w[http://rubygems.org/], Gem.default_sources end + def test_self_detect_gemdeps + rubygems_gemdeps, ENV['RUBYGEMS_GEMDEPS'] = ENV['RUBYGEMS_GEMDEPS'], '-' + + FileUtils.mkdir_p 'detect/a/b' + FileUtils.mkdir_p 'detect/a/Isolate' + + FileUtils.touch 'detect/Isolate' + + begin + Dir.chdir 'detect/a/b' + + assert_empty Gem.detect_gemdeps + ensure + Dir.chdir @tempdir + end + ensure + ENV['RUBYGEMS_GEMDEPS'] = rubygems_gemdeps + end + def test_self_dir assert_equal @gemhome, Gem.dir end @@ -1457,7 +1476,7 @@ class TestGem < Gem::TestCase ENV['GEM_PATH'] = path ENV['RUBYGEMS_GEMDEPS'] = "-" - out = `#{Gem.ruby.untaint} -I #{LIB_PATH.untaint} -rubygems -e "p Gem.loaded_specs.values.map(&:full_name).sort"` + out = `#{Gem.ruby.dup.untaint} -I #{LIB_PATH.untaint} -rubygems -e "p Gem.loaded_specs.values.map(&:full_name).sort"` assert_equal '["a-1", "b-1", "c-1"]', out.strip end @@ -1489,7 +1508,7 @@ class TestGem < Gem::TestCase Dir.mkdir "sub1" out = Dir.chdir "sub1" do - `#{Gem.ruby.untaint} -I #{LIB_PATH.untaint} -rubygems -e "p Gem.loaded_specs.values.map(&:full_name).sort"` + `#{Gem.ruby.dup.untaint} -I #{LIB_PATH.untaint} -rubygems -e "p Gem.loaded_specs.values.map(&:full_name).sort"` end Dir.rmdir "sub1" diff --git a/test/rubygems/test_gem_commands_push_command.rb b/test/rubygems/test_gem_commands_push_command.rb index 41324b524e..5245e864d2 100644 --- a/test/rubygems/test_gem_commands_push_command.rb +++ b/test/rubygems/test_gem_commands_push_command.rb @@ -46,6 +46,7 @@ class TestGemCommandsPushCommand < Gem::TestCase def send_battery use_ui @ui do + @cmd.instance_variable_set :@host, @host @cmd.send_gem(@path) end @@ -133,7 +134,7 @@ class TestGemCommandsPushCommand < Gem::TestCase end def test_raises_error_with_no_arguments - def @cmd.sign_in; end + def @cmd.sign_in(*); end assert_raises Gem::CommandLineError do @cmd.execute end diff --git a/test/rubygems/test_gem_config_file.rb b/test/rubygems/test_gem_config_file.rb index 0781e16540..89e16d3a34 100644 --- a/test/rubygems/test_gem_config_file.rb +++ b/test/rubygems/test_gem_config_file.rb @@ -164,6 +164,36 @@ class TestGemConfigFile < Gem::TestCase assert_equal 2048, @cfg.bulk_threshold end + def test_check_credentials_permissions + @cfg.rubygems_api_key = 'x' + + File.chmod 0644, @cfg.credentials_path + + use_ui @ui do + assert_raises Gem::MockGemUi::TermError do + @cfg.load_api_keys + end + end + + assert_empty @ui.output + + expected = <<-EXPECTED +ERROR: Your gem push credentials file located at: + +\t#{@cfg.credentials_path} + +has file permissions of 0644 but 0600 is required. + +You should reset your credentials at: + +\thttps://rubygems.org/profile/edit + +if you believe they were disclosed to a third party. + EXPECTED + + assert_equal expected, @ui.error + end + def test_handle_arguments args = %w[--backtrace --bunch --of --args here] @@ -215,6 +245,32 @@ class TestGemConfigFile < Gem::TestCase assert_equal true, @cfg.backtrace end + def test_load_api_keys + temp_cred = File.join Gem.user_home, '.gem', 'credentials' + FileUtils.mkdir File.dirname(temp_cred) + File.open temp_cred, 'w', 0600 do |fp| + fp.puts ":rubygems_api_key: 701229f217cdf23b1344c7b4b54ca97" + fp.puts ":other: a5fdbb6ba150cbb83aad2bb2fede64c" + end + + util_config_file + + assert_equal({:rubygems => '701229f217cdf23b1344c7b4b54ca97', + :other => 'a5fdbb6ba150cbb83aad2bb2fede64c'}, @cfg.api_keys) + end + + def test_load_api_keys_bad_permission + skip 'chmod not supported' if win_platform? + + @cfg.rubygems_api_key = 'x' + + File.chmod 0644, @cfg.credentials_path + + assert_raises Gem::MockGemUi::TermError do + @cfg.load_api_keys + end + end + def test_really_verbose assert_equal false, @cfg.really_verbose @@ -227,6 +283,46 @@ class TestGemConfigFile < Gem::TestCase assert_equal true, @cfg.really_verbose end + def test_rubygems_api_key_equals + @cfg.rubygems_api_key = 'x' + + assert_equal 'x', @cfg.rubygems_api_key + + expected = { + :rubygems_api_key => 'x', + } + + assert_equal expected, YAML.load_file(@cfg.credentials_path) + + unless win_platform? then + stat = File.stat @cfg.credentials_path + + assert_equal 0600, stat.mode & 0600 + end + end + + def test_rubygems_api_key_equals_bad_permission + skip 'chmod not supported' if win_platform? + + @cfg.rubygems_api_key = 'x' + + File.chmod 0644, @cfg.credentials_path + + assert_raises Gem::MockGemUi::TermError do + @cfg.rubygems_api_key = 'y' + end + + expected = { + :rubygems_api_key => 'x', + } + + assert_equal expected, YAML.load_file(@cfg.credentials_path) + + stat = File.stat @cfg.credentials_path + + assert_equal 0644, stat.mode & 0644 + end + def test_write @cfg.backtrace = true @cfg.update_sources = false @@ -287,40 +383,6 @@ class TestGemConfigFile < Gem::TestCase assert_equal %w[http://even-more-gems.example.com], Gem.sources end - def test_load_rubygems_api_key_from_credentials - temp_cred = File.join Gem.user_home, '.gem', 'credentials' - FileUtils.mkdir File.dirname(temp_cred) - File.open temp_cred, 'w' do |fp| - fp.puts ":rubygems_api_key: 701229f217cdf23b1344c7b4b54ca97" - end - - util_config_file - - assert_equal "701229f217cdf23b1344c7b4b54ca97", @cfg.rubygems_api_key - end - - def test_load_api_keys_from_config - temp_cred = File.join Gem.user_home, '.gem', 'credentials' - FileUtils.mkdir File.dirname(temp_cred) - File.open temp_cred, 'w' do |fp| - fp.puts ":rubygems_api_key: 701229f217cdf23b1344c7b4b54ca97" - fp.puts ":other: a5fdbb6ba150cbb83aad2bb2fede64c" - end - - util_config_file - - assert_equal({:rubygems => '701229f217cdf23b1344c7b4b54ca97', - :other => 'a5fdbb6ba150cbb83aad2bb2fede64c'}, @cfg.api_keys) - end - - def test_save_credentials_file_with_strict_permissions - util_config_file - FileUtils.mkdir File.dirname(@cfg.credentials_path) - @cfg.rubygems_api_key = '701229f217cdf23b1344c7b4b54ca97' - mode = 0100600 & (~File.umask) - assert_equal mode, File.stat(@cfg.credentials_path).mode unless win_platform? - end - def test_ignore_invalid_config_file File.open @temp_conf, 'w' do |fp| fp.puts "some-non-yaml-hash-string" diff --git a/test/rubygems/test_gem_gemcutter_utilities.rb b/test/rubygems/test_gem_gemcutter_utilities.rb index 8de40f2037..38979ac960 100644 --- a/test/rubygems/test_gem_gemcutter_utilities.rb +++ b/test/rubygems/test_gem_gemcutter_utilities.rb @@ -77,9 +77,24 @@ class TestGemGemcutterUtilities < Gem::TestCase def test_sign_in_with_host api_key = 'a5fdbb6ba150cbb83aad2bb2fede64cf040453903' + + util_sign_in [api_key, 200, 'OK'], 'http://example.com', :param + + assert_match "Enter your http://example.com credentials.", + @sign_in_ui.output + assert @fetcher.last_request["authorization"] + assert_match %r{Signed in.}, @sign_in_ui.output + + credentials = YAML.load_file Gem.configuration.credentials_path + assert_equal api_key, credentials[:rubygems_api_key] + end + + def test_sign_in_with_host_ENV + api_key = 'a5fdbb6ba150cbb83aad2bb2fede64cf040453903' util_sign_in [api_key, 200, 'OK'], 'http://example.com' - assert_match %r{Enter your RubyGems.org credentials.}, @sign_in_ui.output + assert_match "Enter your http://example.com credentials.", + @sign_in_ui.output assert @fetcher.last_request["authorization"] assert_match %r{Signed in.}, @sign_in_ui.output @@ -125,14 +140,14 @@ class TestGemGemcutterUtilities < Gem::TestCase assert_match %r{Access Denied.}, @sign_in_ui.output end - def util_sign_in response, host = nil + def util_sign_in response, host = nil, style = :ENV skip 'Always uses $stdin on windows' if Gem.win_platform? email = 'you@example.com' password = 'secret' if host - ENV['RUBYGEMS_HOST'] = host + ENV['RUBYGEMS_HOST'] = host if style == :ENV else host = Gem.host end @@ -144,7 +159,11 @@ class TestGemGemcutterUtilities < Gem::TestCase @sign_in_ui = Gem::MockGemUi.new "#{email}\n#{password}\n" use_ui @sign_in_ui do - @cmd.sign_in + if style == :param then + @cmd.sign_in host + else + @cmd.sign_in + end end end diff --git a/test/rubygems/test_gem_package.rb b/test/rubygems/test_gem_package.rb index afca143ea3..3051147948 100644 --- a/test/rubygems/test_gem_package.rb +++ b/test/rubygems/test_gem_package.rb @@ -499,6 +499,9 @@ class TestGemPackage < Gem::Package::TarTestCase assert_equal 'unsigned gems are not allowed by the High Security policy', e.message + + refute package.instance_variable_get(:@spec), '@spec must not be loaded' + assert_empty package.instance_variable_get(:@files), '@files must empty' end def test_verify_truncate diff --git a/test/rubygems/test_gem_specification.rb b/test/rubygems/test_gem_specification.rb index 2ba2d5e20d..bdac866bca 100644 --- a/test/rubygems/test_gem_specification.rb +++ b/test/rubygems/test_gem_specification.rb @@ -118,6 +118,15 @@ end assert_equal @current_version, new_spec.specification_version end + def test_self_from_yaml + @a1.instance_variable_set :@specification_version, nil + + spec = Gem::Specification.from_yaml @a1.to_yaml + + assert_equal Gem::Specification::NONEXISTENT_SPECIFICATION_VERSION, + spec.specification_version + end + def test_self_from_yaml_syck_date_bug # This is equivalent to (and totally valid) psych 1.0 output and # causes parse errors on syck.