From 94893cf24fc95b32cc7a756262acbe008c20a9d2 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 19 Sep 2023 16:52:52 +0200 Subject: [PATCH] Merge pull request from GHSA-hcqf-fw2r-52g4 * Revert "Fix request URL normalisation for bare domain and 8-bit characters (#26285)" This reverts commit 8891d8945d837f0da16a3a5aa2dc9783e39b0acd. * Revert "Do not normalize URL before fetching it (#26219)" This reverts commit fd284311e79854d6bc2901a9d9363ba9e7e00513. --- app/lib/request.rb | 23 +-- .../concerns/signature_verification_spec.rb | 33 +--- spec/lib/request_spec.rb | 150 +----------------- 3 files changed, 4 insertions(+), 202 deletions(-) diff --git a/app/lib/request.rb b/app/lib/request.rb index fa0e3472f6..5f128af734 100644 --- a/app/lib/request.rb +++ b/app/lib/request.rb @@ -68,26 +68,13 @@ 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(encode_non_ascii(uri.path)).presence || '/', - query: encode_non_ascii(uri.query) - ) - end - include RoutingHelper def initialize(verb, url, **options) raise ArgumentError if url.blank? @verb = verb - @url = URI_NORMALIZER.call(url) + @url = Addressable::URI.parse(url).normalize @http_client = options.delete(:http_client) @allow_local = options.delete(:allow_local) @options = options.merge(socket_class: use_proxy? || @allow_local ? ProxySocket : Socket) @@ -151,14 +138,8 @@ class Request %w(http https).include?(parsed_url.scheme) && parsed_url.host.present? end - NON_ASCII_PATTERN = /[^\x00-\x7F]+/ - - def encode_non_ascii(str) - str&.gsub(NON_ASCII_PATTERN) { |substr| CGI.escape(substr.encode(Encoding::UTF_8)) } - end - def http_client - HTTP.use(:auto_inflate).use(normalize_uri: { normalizer: URI_NORMALIZER }).follow(max_hops: 3) + HTTP.use(:auto_inflate).follow(max_hops: 3) end end diff --git a/spec/controllers/concerns/signature_verification_spec.rb b/spec/controllers/concerns/signature_verification_spec.rb index f8e44845d7..650cd21eaf 100644 --- a/spec/controllers/concerns/signature_verification_spec.rb +++ b/spec/controllers/concerns/signature_verification_spec.rb @@ -129,37 +129,6 @@ 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 @@ -233,7 +202,7 @@ describe SignatureVerification do request.headers.merge!(fake_request.headers) - stub_request(:get, 'http://localhost:5000/actor').to_raise(Mastodon::HostValidationError) + stub_request(:get, 'http://localhost:5000/actor#main-key').to_raise(Mastodon::HostValidationError) end describe '#signed_request?' do diff --git a/spec/lib/request_spec.rb b/spec/lib/request_spec.rb index 8ccfcacef2..f0861376b9 100644 --- a/spec/lib/request_spec.rb +++ b/spec/lib/request_spec.rb @@ -4,9 +4,7 @@ require 'rails_helper' require 'securerandom' describe Request do - subject { described_class.new(:get, url) } - - let(:url) { 'http://example.com' } + subject { described_class.new(:get, 'http://example.com') } describe '#headers' do it 'returns user agent' do @@ -94,152 +92,6 @@ describe Request do expect { subject.perform }.to raise_error Mastodon::ValidationError end end - - context 'with bare domain URL' do - let(:url) { 'http://example.com' } - - before do - stub_request(:get, 'http://example.com') - end - - it 'normalizes path' do - subject.perform do |response| - expect(response.request.uri.path).to eq '/' - end - end - - it 'normalizes path used for request signing' do - subject.perform - - headers = subject.instance_variable_get(:@headers) - expect(headers[Request::REQUEST_TARGET]).to eq 'get /' - end - - it 'normalizes path used in request line' do - subject.perform do |response| - expect(response.request.headline).to eq 'GET / HTTP/1.1' - end - 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 not modify path' do - subject.perform do |response| - expect(response.request.uri.path).to eq '/foo%41%3A' - end - end - - it 'does not modify query string' do - subject.perform do |response| - expect(response.request.uri.query).to eq 'bar=%41%3A' - end - end - - it 'does not modify path used for request signing' do - subject.perform - - headers = subject.instance_variable_get(:@headers) - expect(headers[Request::REQUEST_TARGET]).to eq 'get /foo%41%3A' - end - - it 'does not modify path used in request line' do - subject.perform do |response| - expect(response.request.headline).to eq 'GET /foo%41%3A?bar=%41%3A HTTP/1.1' - 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:81/föo?bär=1' } - - before do - stub_request(:get, 'http://xn--xample-9ua.com:81/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:81' - end - end - - it 'IDN-encodes host in Host header' do - subject.perform do |response| - expect(response.request.headers['Host']).to eq 'xn--xample-9ua.com' - end - end - - it 'percent-escapes path used for request signing' do - subject.perform - - headers = subject.instance_variable_get(:@headers) - expect(headers[Request::REQUEST_TARGET]).to eq 'get /f%C3%B6o' - end - - it 'normalizes path used in request line' do - subject.perform do |response| - expect(response.request.headline).to eq 'GET /f%C3%B6o?b%C3%A4r=1 HTTP/1.1' - end - 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