Skip to content
Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lis2 committed Feb 14, 2022
1 parent 55007fb commit a34075d
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 3 deletions.
17 changes: 14 additions & 3 deletions lib/final_destination.rb
Expand Up @@ -8,6 +8,8 @@

# 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)
Expand Down Expand Up @@ -203,12 +205,21 @@ def resolve
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
Expand All @@ -220,12 +231,12 @@ def resolve
# 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
Expand Down
21 changes: 21 additions & 0 deletions spec/components/final_destination_spec.rb
Expand Up @@ -49,6 +49,13 @@
}
end

let(:body_response) do
{
status: 200,
body: "<body>test</body>"
}
end

def canonical_follow(from, dest)
stub_request(:get, from).to_return(
status: 200,
Expand Down Expand Up @@ -182,6 +189,20 @@ def fd(url)
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")
Expand Down

0 comments on commit a34075d

Please sign in to comment.