From 38dbd312f7c16f2a31260813364384a39397b3d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 22 Sep 2014 16:40:35 -0700 Subject: [PATCH 1/2] Tweak URI implementation for compatibility with stdlib When `url.scheme` is changed from "https" to "http" when someone configured their Enterprise instance to be used over insecure protocol, the "speedy" implementation changed the port as well, but the stdlib implementation doesn't. Things would break if someone used hub in an environment where stdlib URI was already loaded, like for instance from RubyGems. --- features/fork.feature | 1 + lib/hub/github_api.rb | 4 +++- lib/hub/speedy_stdlib.rb | 37 ++++++++++++++++++++++++++-------- test/speedy_stdlib_test.rb | 41 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 9 deletions(-) create mode 100644 test/speedy_stdlib_test.rb diff --git a/features/fork.feature b/features/fork.feature index 41f53d78..073bb1b2 100644 --- a/features/fork.feature +++ b/features/fork.feature @@ -141,6 +141,7 @@ Scenario: Related fork already exists """ before { halt 400 unless request.env['HTTP_X_ORIGINAL_SCHEME'] == 'http' + halt 400 unless request.env['HTTP_X_ORIGINAL_PORT'] == '80' halt 401 unless request.env['HTTP_AUTHORIZATION'] == 'token FITOKEN' } post('/api/v3/repos/evilchelu/dotfiles/forks', :host_name => 'git.my.org') { '' } diff --git a/lib/hub/github_api.rb b/lib/hub/github_api.rb index 2ec39620..6457753f 100644 --- a/lib/hub/github_api.rb +++ b/lib/hub/github_api.rb @@ -273,13 +273,15 @@ module Hub def configure_connection req, url url.scheme = config.protocol(url.host) + url.port = 80 if url.scheme == 'http' if ENV['HUB_TEST_HOST'] req['Host'] = url.host req['X-Original-Scheme'] = url.scheme + req['X-Original-Port'] = url.port url = url.dup url.scheme = 'http' url.host, test_port = ENV['HUB_TEST_HOST'].split(':') - url.port = test_port.to_i if test_port + url.port = test_port ? test_port.to_i : 80 end yield url end diff --git a/lib/hub/speedy_stdlib.rb b/lib/hub/speedy_stdlib.rb index 7d919a9a..ab9be03b 100644 --- a/lib/hub/speedy_stdlib.rb +++ b/lib/hub/speedy_stdlib.rb @@ -33,7 +33,9 @@ unless defined?(URI) end module URI - InvalidURIError = Class.new(StandardError) + Error = Class.new(StandardError) + InvalidURIError = Class.new(Error) + InvalidComponentError = Class.new(Error) def self.parse(str) URI::HTTP.new(str) @@ -56,23 +58,24 @@ unless defined?(URI) class HTTP attr_accessor :scheme, :user, :password, :host, :path, :query, :fragment - attr_writer :port + attr_reader :port alias hostname host def initialize(str) m = str.to_s.match(%r{^ ([\w-]+): // (?:([^/@]+)@)? ([^/?#]+) }x) raise InvalidURIError unless m _, self.scheme, self.userinfo, host = m.to_a - self.host, self.port = host.split(':', 2) + self.host, port = host.split(':', 2) + self.port = port ? port.to_i : default_port path, self.fragment = m.post_match.split('#', 2) - self.path, self.query = path.to_s.split('?', 2) + path, self.query = path.split('?', 2) if path + self.path = path.to_s end def to_s url = "#{scheme}://" url << "#{userinfo}@" if user || password - url << host - url << ":#{@port}" if @port + url << host << display_port url << path url << "?#{query}" if query url << "##{fragment}" if fragment @@ -85,8 +88,12 @@ unless defined?(URI) url end - def port - (@port || (scheme == 'https' ? 443 : 80)).to_i + def port=(number) + if number.is_a?(Fixnum) && number > 0 + @port = number + else + raise InvalidComponentError, "bad component(expected port component): %p" % number + end end def userinfo=(info) @@ -102,6 +109,20 @@ unless defined?(URI) def find_proxy end + + private + + def default_port + self.scheme == 'https' ? 443 : 80 + end + + def display_port + if port != default_port + ":#{port}" + else + "" + end + end end end end diff --git a/test/speedy_stdlib_test.rb b/test/speedy_stdlib_test.rb new file mode 100644 index 00000000..6b92b629 --- /dev/null +++ b/test/speedy_stdlib_test.rb @@ -0,0 +1,41 @@ +require 'helper' + +class URITest < Minitest::Test + def test_uri_display_port + assert_equal "https://example.com", URI.parse("https://example.com").to_s + assert_equal "https://example.com:80", URI.parse("https://example.com:80").to_s + assert_equal "http://example.com", URI.parse("http://example.com").to_s + assert_equal "http://example.com:443", URI.parse("http://example.com:443").to_s + end + + def test_uri_invalid_port + assert_raises URI::InvalidComponentError do + uri = URI.parse("https://example.com") + uri.port = "80" + end + end + + def test_uri_scheme_doesnt_affect_port + uri = URI.parse("https://example.com") + uri.scheme = "http" + assert_equal "http", uri.scheme + assert_equal 443, uri.port + uri.port = 80 + assert_equal 80, uri.port + end + + def test_blank_path + uri = URI.parse("https://example.com") + assert_equal "", uri.path + end + + def test_no_query + uri = URI.parse("https://example.com") + assert_nil uri.query + end + + def test_blank_query + uri = URI.parse("https://example.com?") + assert_equal "", uri.query + end +end From b311b7b343b1bc304af6693d7de798846d1a1bf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 22 Sep 2014 22:52:44 -0700 Subject: [PATCH 2/2] Ensure tests use URI from speedy_stdlib even if Bundler loaded URI When running through `bundle exec`, Bundler will have loaded URI already which will make our speedy_stdlib skip redefining it. Aggressively undefine URI if loaded through Bundler so that our tests perform against our implementation, not the stdlib one. --- test/helper.rb | 5 +++++ test/speedy_stdlib_test.rb | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/test/helper.rb b/test/helper.rb index 9f49e728..c5c91e88 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -1,3 +1,8 @@ +if defined?(Bundler) && defined?(URI) + Object.send(:remove_const, :URI) + $".reject! { |p| p =~ %r{(^|/)uri[/.]} } +end + require 'minitest/autorun' require 'hub' diff --git a/test/speedy_stdlib_test.rb b/test/speedy_stdlib_test.rb index 6b92b629..3b708901 100644 --- a/test/speedy_stdlib_test.rb +++ b/test/speedy_stdlib_test.rb @@ -1,6 +1,11 @@ require 'helper' class URITest < Minitest::Test + def test_uri_is_from_speedy_stdlib + # if it were stdlib, URI::HTTPS would be the result + assert_equal URI::HTTP, URI.parse("https://example.com").class + end + def test_uri_display_port assert_equal "https://example.com", URI.parse("https://example.com").to_s assert_equal "https://example.com:80", URI.parse("https://example.com:80").to_s