Do not normalize URL before fetching it (#26219)
This commit is contained in:
		
				
					committed by
					
						
						GitHub
					
				
			
			
				
	
			
			
			
						parent
						
							51768de16e
						
					
				
				
					commit
					fd284311e7
				
			@@ -68,13 +68,26 @@ class Request
 | 
			
		||||
  # about 15s in total
 | 
			
		||||
  TIMEOUT = { connect_timeout: 5, read_timeout: 10, write_timeout: 10, read_deadline: 30 }.freeze
 | 
			
		||||
 | 
			
		||||
  # Workaround for overly-eager decoding of percent-encoded characters in Addressable::URI#normalized_path
 | 
			
		||||
  # https://github.com/sporkmonger/addressable/issues/366
 | 
			
		||||
  URI_NORMALIZER = lambda do |uri|
 | 
			
		||||
    uri = HTTP::URI.parse(uri)
 | 
			
		||||
 | 
			
		||||
    HTTP::URI.new(
 | 
			
		||||
      scheme: uri.normalized_scheme,
 | 
			
		||||
      authority: uri.normalized_authority,
 | 
			
		||||
      path: Addressable::URI.normalize_path(uri.path),
 | 
			
		||||
      query: uri.query
 | 
			
		||||
    )
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  include RoutingHelper
 | 
			
		||||
 | 
			
		||||
  def initialize(verb, url, **options)
 | 
			
		||||
    raise ArgumentError if url.blank?
 | 
			
		||||
 | 
			
		||||
    @verb        = verb
 | 
			
		||||
    @url         = Addressable::URI.parse(url).normalize
 | 
			
		||||
    @url         = URI_NORMALIZER.call(url)
 | 
			
		||||
    @http_client = options.delete(:http_client)
 | 
			
		||||
    @allow_local = options.delete(:allow_local)
 | 
			
		||||
    @options     = options.merge(socket_class: use_proxy? || @allow_local ? ProxySocket : Socket)
 | 
			
		||||
@@ -139,7 +152,7 @@ class Request
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    def http_client
 | 
			
		||||
      HTTP.use(:auto_inflate).follow(max_hops: 3)
 | 
			
		||||
      HTTP.use(:auto_inflate).use(normalize_uri: { normalizer: URI_NORMALIZER }).follow(max_hops: 3)
 | 
			
		||||
    end
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
 
 | 
			
		||||
@@ -129,6 +129,37 @@ describe SignatureVerification do
 | 
			
		||||
      end
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    context 'with non-normalized URL' do
 | 
			
		||||
      before do
 | 
			
		||||
        get :success
 | 
			
		||||
 | 
			
		||||
        fake_request = Request.new(:get, 'http://test.host/subdir/../success')
 | 
			
		||||
        fake_request.on_behalf_of(author)
 | 
			
		||||
 | 
			
		||||
        request.headers.merge!(fake_request.headers)
 | 
			
		||||
 | 
			
		||||
        allow(controller).to receive(:actor_refresh_key!).and_return(author)
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      describe '#build_signed_string' do
 | 
			
		||||
        it 'includes the normalized request path' do
 | 
			
		||||
          expect(controller.send(:build_signed_string)).to start_with "(request-target): get /success\n"
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      describe '#signed_request?' do
 | 
			
		||||
        it 'returns true' do
 | 
			
		||||
          expect(controller.signed_request?).to be true
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      describe '#signed_request_actor' do
 | 
			
		||||
        it 'returns an account' do
 | 
			
		||||
          expect(controller.signed_request_account).to eq author
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    context 'with request with unparsable Date header' do
 | 
			
		||||
      before do
 | 
			
		||||
        get :success
 | 
			
		||||
@@ -202,7 +233,7 @@ describe SignatureVerification do
 | 
			
		||||
 | 
			
		||||
        request.headers.merge!(fake_request.headers)
 | 
			
		||||
 | 
			
		||||
        stub_request(:get, 'http://localhost:5000/actor#main-key').to_raise(Mastodon::HostValidationError)
 | 
			
		||||
        stub_request(:get, 'http://localhost:5000/actor').to_raise(Mastodon::HostValidationError)
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      describe '#signed_request?' do
 | 
			
		||||
 
 | 
			
		||||
@@ -4,7 +4,9 @@ require 'rails_helper'
 | 
			
		||||
require 'securerandom'
 | 
			
		||||
 | 
			
		||||
describe Request do
 | 
			
		||||
  subject { described_class.new(:get, 'http://example.com') }
 | 
			
		||||
  subject { described_class.new(:get, url) }
 | 
			
		||||
 | 
			
		||||
  let(:url) { 'http://example.com' }
 | 
			
		||||
 | 
			
		||||
  describe '#headers' do
 | 
			
		||||
    it 'returns user agent' do
 | 
			
		||||
@@ -92,6 +94,99 @@ describe Request do
 | 
			
		||||
        expect { subject.perform }.to raise_error Mastodon::ValidationError
 | 
			
		||||
      end
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    context 'with unnormalized URL' do
 | 
			
		||||
      let(:url) { 'HTTP://EXAMPLE.com:80/foo%41%3A?bar=%41%3A#baz' }
 | 
			
		||||
 | 
			
		||||
      before do
 | 
			
		||||
        stub_request(:get, 'http://example.com/foo%41%3A?bar=%41%3A')
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'normalizes scheme' do
 | 
			
		||||
        subject.perform do |response|
 | 
			
		||||
          expect(response.request.uri.scheme).to eq 'http'
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'normalizes host' do
 | 
			
		||||
        subject.perform do |response|
 | 
			
		||||
          expect(response.request.uri.authority).to eq 'example.com'
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'does modify path' do
 | 
			
		||||
        subject.perform do |response|
 | 
			
		||||
          expect(response.request.uri.path).to eq '/foo%41%3A'
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'does modify query string' do
 | 
			
		||||
        subject.perform do |response|
 | 
			
		||||
          expect(response.request.uri.query).to eq 'bar=%41%3A'
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'strips fragment' do
 | 
			
		||||
        subject.perform do |response|
 | 
			
		||||
          expect(response.request.uri.fragment).to be_nil
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    context 'with non-ASCII URL' do
 | 
			
		||||
      let(:url) { 'http://éxample.com/föo?bär=1' }
 | 
			
		||||
 | 
			
		||||
      before do
 | 
			
		||||
        stub_request(:get, 'http://xn--xample-9ua.com/f%C3%B6o?b%C3%A4r=1')
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'IDN-encodes host' do
 | 
			
		||||
        subject.perform do |response|
 | 
			
		||||
          expect(response.request.uri.authority).to eq 'xn--xample-9ua.com'
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'percent-escapes path and query string' do
 | 
			
		||||
        subject.perform
 | 
			
		||||
 | 
			
		||||
        expect(a_request(:get, 'http://xn--xample-9ua.com/f%C3%B6o?b%C3%A4r=1')).to have_been_made
 | 
			
		||||
      end
 | 
			
		||||
    end
 | 
			
		||||
 | 
			
		||||
    context 'with redirecting URL' do
 | 
			
		||||
      let(:url) { 'http://example.com/foo' }
 | 
			
		||||
 | 
			
		||||
      before do
 | 
			
		||||
        stub_request(:get, 'http://example.com/foo').to_return(status: 302, headers: { 'Location' => 'HTTPS://EXAMPLE.net/Bar' })
 | 
			
		||||
        stub_request(:get, 'https://example.net/Bar').to_return(body: 'Lorem ipsum')
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'resolves redirect' do
 | 
			
		||||
        subject.perform do |response|
 | 
			
		||||
          expect(response.body.to_s).to eq 'Lorem ipsum'
 | 
			
		||||
        end
 | 
			
		||||
 | 
			
		||||
        expect(a_request(:get, 'https://example.net/Bar')).to have_been_made
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'normalizes destination scheme' do
 | 
			
		||||
        subject.perform do |response|
 | 
			
		||||
          expect(response.request.uri.scheme).to eq 'https'
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'normalizes destination host' do
 | 
			
		||||
        subject.perform do |response|
 | 
			
		||||
          expect(response.request.uri.authority).to eq 'example.net'
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
 | 
			
		||||
      it 'does modify path' do
 | 
			
		||||
        subject.perform do |response|
 | 
			
		||||
          expect(response.request.uri.path).to eq '/Bar'
 | 
			
		||||
        end
 | 
			
		||||
      end
 | 
			
		||||
    end
 | 
			
		||||
  end
 | 
			
		||||
 | 
			
		||||
  describe "response's body_with_limit method" do
 | 
			
		||||
 
 | 
			
		||||
		Reference in New Issue
	
	Block a user