ruby-changes:67481
From: Daniel <ko1@a...>
Date: Tue, 31 Aug 2021 19:08:07 +0900 (JST)
Subject: [ruby-changes:67481] 3adc141a79 (master): [rubygems/rubygems] Refactor `Gem::RemoteFetcher::FetchError` initializer to `build` method
https://git.ruby-lang.org/ruby.git/commit/?id=3adc141a79 From 3adc141a79cdab83671b7f68301901dd7726e7c4 Mon Sep 17 00:00:00 2001 From: Daniel Niknam <mhmd.niknam@g...> Date: Sun, 22 Aug 2021 01:40:21 +1000 Subject: [rubygems/rubygems] Refactor `Gem::RemoteFetcher::FetchError` initializer to `build` method The `initialize` method is already doing a lot and by adding the `Gem::PrintableUri` to redact sensitive information, things are getting complicated and hard to read here. For the start, I have refactored the `initialize` method into a class method called `build`. https://github.com/rubygems/rubygems/commit/4312e8fdf5 --- lib/rubygems/remote_fetcher.rb | 36 +++++++++++++--------- lib/rubygems/request.rb | 8 ++--- test/rubygems/test_gem_commands_sources_command.rb | 2 +- test/rubygems/test_gem_remote_fetcher.rb | 32 ++++++++++++++++++- test/rubygems/test_gem_resolver_best_set.rb | 6 ++-- test/rubygems/test_gem_spec_fetcher.rb | 2 +- test/rubygems/test_remote_fetch_error.rb | 6 ++-- test/rubygems/utilities.rb | 4 +-- 8 files changed, 66 insertions(+), 30 deletions(-) diff --git a/lib/rubygems/remote_fetcher.rb b/lib/rubygems/remote_fetcher.rb index 8c9c9e6..60c5da0 100644 --- a/lib/rubygems/remote_fetcher.rb +++ b/lib/rubygems/remote_fetcher.rb @@ -5,6 +5,7 @@ require_relative 'request/connection_pools' https://github.com/ruby/ruby/blob/trunk/lib/rubygems/remote_fetcher.rb#L5 require_relative 's3_uri_signer' require_relative 'uri_formatter' require_relative 'uri_parser' +require_relative 'printable_uri' require_relative 'user_interaction' ## @@ -21,19 +22,24 @@ class Gem::RemoteFetcher https://github.com/ruby/ruby/blob/trunk/lib/rubygems/remote_fetcher.rb#L22 class FetchError < Gem::Exception ## # The URI which was being accessed when the exception happened. + def self.build(message, uri) + original_uri = uri.dup + uri = Gem::PrintableUri.parse_uri(uri) - attr_accessor :uri, :original_uri - - def initialize(message, uri) - super message + if uri.respond_to?(:original_password) && uri.original_password + message = message.sub(uri.original_password, 'REDACTED') + end - uri = Gem::UriParser.parse_uri(uri) + new(message, uri.to_s, original_uri) + end - @original_uri = uri.dup + attr_accessor :uri, :original_uri - uri.password = 'REDACTED' if uri.respond_to?(:password) && uri.password + def initialize(message, uri, original_uri = nil) + super message - @uri = uri.to_s + @uri = uri + @original_uri = original_uri ? original_uri : uri end def to_s # :nodoc: @@ -219,20 +225,20 @@ class Gem::RemoteFetcher https://github.com/ruby/ruby/blob/trunk/lib/rubygems/remote_fetcher.rb#L225 head ? response : response.body when Net::HTTPMovedPermanently, Net::HTTPFound, Net::HTTPSeeOther, Net::HTTPTemporaryRedirect then - raise FetchError.new('too many redirects', uri) if depth > 10 + raise FetchError.build('too many redirects', uri) if depth > 10 unless location = response['Location'] - raise FetchError.new("redirecting but no redirect location was given", uri) + raise FetchError.build("redirecting but no redirect location was given", uri) end location = Gem::UriParser.parse_uri location if https?(uri) && !https?(location) - raise FetchError.new("redirecting to non-https resource: #{location}", uri) + raise FetchError.build("redirecting to non-https resource: #{location}", uri) end fetch_http(location, last_modified, head, depth + 1) else - raise FetchError.new("bad response #{response.message} #{response.code}", uri) + raise FetchError.build("bad response #{response.message} #{response.code}", uri) end end @@ -254,21 +260,21 @@ class Gem::RemoteFetcher https://github.com/ruby/ruby/blob/trunk/lib/rubygems/remote_fetcher.rb#L260 begin data = Gem::Util.gunzip data rescue Zlib::GzipFile::Error - raise FetchError.new("server did not return a valid file", uri) + raise FetchError.build("server did not return a valid file", uri) end end data rescue Timeout::Error, IOError, SocketError, SystemCallError, *(OpenSSL::SSL::SSLError if Gem::HAVE_OPENSSL) => e - raise FetchError.new("#{e.class}: #{e}", uri) + raise FetchError.build("#{e.class}: #{e}", uri) end def fetch_s3(uri, mtime = nil, head = false) begin public_uri = s3_uri_signer(uri).sign rescue Gem::S3URISigner::ConfigurationError, Gem::S3URISigner::InstanceProfileError => e - raise FetchError.new(e.message, "s3://#{uri.host}") + raise FetchError.build(e.message, "s3://#{uri.host}") end fetch_https public_uri, mtime, head end diff --git a/lib/rubygems/request.rb b/lib/rubygems/request.rb index fdc4c55..d0d9b58 100644 --- a/lib/rubygems/request.rb +++ b/lib/rubygems/request.rb @@ -127,7 +127,7 @@ class Gem::Request https://github.com/ruby/ruby/blob/trunk/lib/rubygems/request.rb#L127 @connection_pool.checkout rescue Gem::HAVE_OPENSSL ? OpenSSL::SSL::SSLError : Errno::EHOSTDOWN, Errno::EHOSTDOWN => e - raise Gem::RemoteFetcher::FetchError.new(e.message, uri) + raise Gem::RemoteFetcher::FetchError.build(e.message, uri) end def fetch @@ -228,14 +228,14 @@ class Gem::Request https://github.com/ruby/ruby/blob/trunk/lib/rubygems/request.rb#L228 reset connection - raise Gem::RemoteFetcher::FetchError.new('too many bad responses', @uri) if bad_response + raise Gem::RemoteFetcher::FetchError.build('too many bad responses', @uri) if bad_response bad_response = true retry rescue Net::HTTPFatalError verbose "fatal error" - raise Gem::RemoteFetcher::FetchError.new('fatal error', @uri) + raise Gem::RemoteFetcher::FetchError.build('fatal error', @uri) # HACK work around EOFError bug in Net::HTTP # NOTE Errno::ECONNABORTED raised a lot on Windows, and make impossible # to install gems. @@ -245,7 +245,7 @@ class Gem::Request https://github.com/ruby/ruby/blob/trunk/lib/rubygems/request.rb#L245 requests = @requests[connection.object_id] verbose "connection reset after #{requests} requests, retrying" - raise Gem::RemoteFetcher::FetchError.new('too many connection resets', @uri) if retried + raise Gem::RemoteFetcher::FetchError.build('too many connection resets', @uri) if retried reset connection diff --git a/test/rubygems/test_gem_commands_sources_command.rb b/test/rubygems/test_gem_commands_sources_command.rb index 7bca0f3..6fcbbf6 100644 --- a/test/rubygems/test_gem_commands_sources_command.rb +++ b/test/rubygems/test_gem_commands_sources_command.rb @@ -182,7 +182,7 @@ class TestGemCommandsSourcesCommand < Gem::TestCase https://github.com/ruby/ruby/blob/trunk/test/rubygems/test_gem_commands_sources_command.rb#L182 uri = "http://beta-gems.example.com/specs.#{@marshal_version}.gz" @fetcher.data[uri] = proc do - raise Gem::RemoteFetcher::FetchError.new('it died', uri) + raise Gem::RemoteFetcher::FetchError.build('it died', uri) end @cmd.handle_options %w[--add http://beta-gems.example.com] diff --git a/test/rubygems/test_gem_remote_fetcher.rb b/test/rubygems/test_gem_remote_fetcher.rb index 1bae924..b4c82ce 100644 --- a/test/rubygems/test_gem_remote_fetcher.rb +++ b/test/rubygems/test_gem_remote_fetcher.rb @@ -204,7 +204,7 @@ PeIQQkFng2VVot/WAQbv3ePqWq07g1BBcwIBAg== https://github.com/ruby/ruby/blob/trunk/test/rubygems/test_gem_remote_fetcher.rb#L204 @test_data end - raise Gem::RemoteFetcher::FetchError.new("haha!", '') + raise Gem::RemoteFetcher::FetchError.build("haha!", '') end end @@ -241,6 +241,36 @@ PeIQQkFng2VVot/WAQbv3ePqWq07g1BBcwIBAg== https://github.com/ruby/ruby/blob/trunk/test/rubygems/test_gem_remote_fetcher.rb#L241 assert File.exist?(a1_cache_gem) end + def test_download_with_token + a1_data = nil + File.open @a1_gem, 'rb' do |fp| + a1_data = fp.read + end + + fetcher = util_fuck_with_fetcher a1_data + + a1_cache_gem = @a1.cache_file + assert_equal a1_cache_gem, fetcher.download(@a1, 'http://token@g...') + assert_equal("http://token@g.../gems/a-1.gem", + fetcher.instance_variable_get(:@test_arg).to_s) + assert File.exist?(a1_cache_gem) + end + + def test_download_with_x_oauth_basic + a1_data = nil + File.open @a1_gem, 'rb' do |fp| + a1_data = fp.read + end + + fetcher = util_fuck_with_fetcher a1_data + + a1_cache_gem = @a1.cache_file + assert_equal a1_cache_gem, fetcher.download(@a1, 'http://token:x-oauth-basic@g...') + assert_equal("http://token:x-oauth-basic@g.../gems/a-1.gem", + fetcher.instance_variable_get(:@test_arg).to_s) + assert File.exist?(a1_cache_gem) + end + def test_download_with_encoded_auth a1_data = nil File.open @a1_gem, 'rb' do |fp| diff --git a/test/rubygems/test_gem_resolver_best_set.rb b/test/rubygems/test_gem_resolver_best_set.rb index 0e279d1..a448ec0 100644 --- a/test/rubygems/test_gem_resolver_best_set.rb +++ b/test/rubygems/test_gem_resolver_best_set.rb @@ -106,7 +106,7 @@ class TestGemResolverBestSet < Gem::TestCase https://github.com/ruby/ruby/blob/trunk/test/rubygems/test_gem_resolver_best_set.rb#L106 error_uri = api_uri + 'a' - error = Gem::RemoteFetcher::FetchError.new 'bogus', error_uri + error = Gem::RemoteFetcher::FetchError.build 'bogus', error_uri set.replace_failed_api_set error @@ -124,7 +124,7 @@ class TestGemResolverBestSet < Gem::TestCase https://github.com/ruby/ruby/blob/trunk/test/rubygems/test_gem_resolver_best_set.rb#L124 set.sets << index_set - error = Gem::RemoteFetcher (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/