From 76195216ffe346d88139675701a6f4353fb15b99 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Mon, 14 Feb 2022 12:11:09 +1100 Subject: [PATCH] SECURITY: Onebox response timeout and size limit (#15927) Validation to ensure that Onebox request is no longer than 10 seconds and response size is not bigger than 1 MB --- lib/final_destination.rb | 17 ++++++++++++++--- spec/components/final_destination_spec.rb | 21 +++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/lib/final_destination.rb b/lib/final_destination.rb index 2fbc5711b18..078fd4fd781 100644 --- a/lib/final_destination.rb +++ b/lib/final_destination.rb @@ -8,6 +8,8 @@ require 'url_helper' # Determine the final endpoint for a Web URI, following redirects class FinalDestination + MAX_REQUEST_TIME_SECONDS = 10 + MAX_REQUEST_SIZE_BYTES = 1_048_576 # 1024 * 1024 def self.clear_https_cache!(domain) key = redis_https_key(domain) @@ -203,12 +205,21 @@ class FinalDestination middlewares = Excon.defaults[:middlewares] middlewares << Excon::Middleware::Decompress if @http_verb == :get + request_start_time = Time.now + response_body = +"" + request_validator = lambda do |chunk, _remaining_bytes, _total_bytes| + response_body << chunk + raise Excon::Errors::ExpectationFailed.new("response size too big: #{@uri.to_s}") if response_body.bytesize > MAX_REQUEST_SIZE_BYTES + raise Excon::Errors::ExpectationFailed.new("connect timeout reached: #{@uri.to_s}") if Time.now - request_start_time > MAX_REQUEST_TIME_SECONDS + end + response = Excon.public_send(@http_verb, @uri.to_s, read_timeout: timeout, connect_timeout: timeout, headers: headers, - middlewares: middlewares + middlewares: middlewares, + response_block: request_validator ) location = nil @@ -220,12 +231,12 @@ class FinalDestination # Cache body of successful `get` requests if @http_verb == :get if Oneboxer.cache_response_body?(@uri) - Oneboxer.cache_response_body(@uri.to_s, response.body) + Oneboxer.cache_response_body(@uri.to_s, response_body) end end if @follow_canonical - next_url = fetch_canonical_url(response.body) + next_url = fetch_canonical_url(response_body) if next_url.to_s.present? && next_url != @uri @follow_canonical = false diff --git a/spec/components/final_destination_spec.rb b/spec/components/final_destination_spec.rb index e9548ead0b9..b1ce1a262a0 100644 --- a/spec/components/final_destination_spec.rb +++ b/spec/components/final_destination_spec.rb @@ -49,6 +49,13 @@ describe FinalDestination do } end + let(:body_response) do + { + status: 200, + body: "test" + } + end + def canonical_follow(from, dest) stub_request(:get, from).to_return( status: 200, @@ -182,6 +189,20 @@ describe FinalDestination do end end + it 'raises error when response is too big' do + stub_const(described_class, "MAX_REQUEST_SIZE_BYTES", 1) do + stub_request(:get, "https://codinghorror.com/blog").to_return(body_response) + final = FinalDestination.new('https://codinghorror.com/blog', opts.merge(follow_canonical: true)) + expect { final.resolve }.to raise_error(Excon::Errors::ExpectationFailed, "response size too big: https://codinghorror.com/blog") + end + end + + it 'raises error when response is too slow' do + stub_request(:get, "https://codinghorror.com/blog").to_return(lambda { |request| freeze_time(11.seconds.from_now) ; body_response }) + final = FinalDestination.new('https://codinghorror.com/blog', opts.merge(follow_canonical: true)) + expect { final.resolve }.to raise_error(Excon::Errors::ExpectationFailed, "connect timeout reached: https://codinghorror.com/blog") + end + context 'follows canonical links' do it 'resolves the canonical link as the final destination' do canonical_follow("https://eviltrout.com", "https://codinghorror.com/blog")