From fd8145d2322d8b13b6aaa9cd2a61331ff660c9dd Mon Sep 17 00:00:00 2001
From: Eugen Rochko <eugen@zeonfederated.com>
Date: Thu, 22 Nov 2018 20:12:04 +0100
Subject: [PATCH] Fix connect timeout not being enforced (#9329)

* Fix connect timeout not being enforced

The loop was catching the timeout exception that should stop execution, so the next IP would no longer be within a timed block, which led to requests taking much longer than 10 seconds.

* Use timeout on each IP attempt, but limit to 2 attempts

* Fix code style issue

* Do not break Request#perform if no block given

* Update method stub in spec for Request

* Move timeout inside the begin/rescue block

* Use Resolv::DNS with timeout of 1 to get IP addresses

* Update Request spec to stub Resolv::DNS instead of Addrinfo

* Fix Resolve::DNS stubs in Request spec
---
 app/lib/request.rb       | 32 +++++++++++++++++++++++---------
 spec/lib/request_spec.rb | 17 +++++++++++------
 2 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/app/lib/request.rb b/app/lib/request.rb
index fdaaf36369..bb6ef4661a 100644
--- a/app/lib/request.rb
+++ b/app/lib/request.rb
@@ -2,6 +2,7 @@
 
 require 'ipaddr'
 require 'socket'
+require 'resolv'
 
 class Request
   REQUEST_TARGET = '(request-target)'
@@ -45,7 +46,7 @@ class Request
     end
 
     begin
-      yield response.extend(ClientLimit)
+      yield response.extend(ClientLimit) if block_given?
     ensure
       http_client.close
     end
@@ -94,7 +95,7 @@ class Request
   end
 
   def timeout
-    { connect: 10, read: 10, write: 10 }
+    { connect: nil, read: 10, write: 10 }
   end
 
   def http_client
@@ -139,16 +140,29 @@ class Request
   class Socket < TCPSocket
     class << self
       def open(host, *args)
-        return super host, *args if thru_hidden_service? host
+        return super(host, *args) if thru_hidden_service?(host)
+
         outer_e = nil
-        Addrinfo.foreach(host, nil, nil, :SOCK_STREAM) do |address|
-          begin
-            raise Mastodon::HostValidationError if PrivateAddressCheck.private_address? IPAddr.new(address.ip_address)
-            return super address.ip_address, *args
-          rescue => e
-            outer_e = e
+
+        Resolv::DNS.open do |dns|
+          dns.timeouts = 1
+
+          addresses = dns.getaddresses(host).take(2)
+          time_slot = 10.0 / addresses.size
+
+          addresses.each do |address|
+            begin
+              raise Mastodon::HostValidationError if PrivateAddressCheck.private_address?(IPAddr.new(address.to_s))
+
+              ::Timeout.timeout(time_slot, HTTP::TimeoutError) do
+                return super(address.to_s, *args)
+              end
+            rescue => e
+              outer_e = e
+            end
           end
         end
+
         raise outer_e if outer_e
       end
 
diff --git a/spec/lib/request_spec.rb b/spec/lib/request_spec.rb
index 8cc5d90ce3..2d300f18d6 100644
--- a/spec/lib/request_spec.rb
+++ b/spec/lib/request_spec.rb
@@ -48,9 +48,11 @@ describe Request do
       end
 
       it 'executes a HTTP request when the first address is private' do
-        allow(Addrinfo).to receive(:foreach).with('example.com', nil, nil, :SOCK_STREAM)
-                                            .and_yield(Addrinfo.new(["AF_INET", 0, "example.com", "0.0.0.0"], :PF_INET, :SOCK_STREAM))
-                                            .and_yield(Addrinfo.new(["AF_INET6", 0, "example.com", "2001:4860:4860::8844"], :PF_INET6, :SOCK_STREAM))
+        resolver = double
+
+        allow(resolver).to receive(:getaddresses).with('example.com').and_return(%w(0.0.0.0 2001:4860:4860::8844))
+        allow(resolver).to receive(:timeouts=).and_return(nil)
+        allow(Resolv::DNS).to receive(:open).and_yield(resolver)
 
         expect { |block| subject.perform &block }.to yield_control
         expect(a_request(:get, 'http://example.com')).to have_been_made.once
@@ -81,9 +83,12 @@ describe Request do
       end
 
       it 'raises Mastodon::ValidationError' do
-        allow(Addrinfo).to receive(:foreach).with('example.com', nil, nil, :SOCK_STREAM)
-                                            .and_yield(Addrinfo.new(["AF_INET", 0, "example.com", "0.0.0.0"], :PF_INET, :SOCK_STREAM))
-                                            .and_yield(Addrinfo.new(["AF_INET6", 0, "example.com", "2001:db8::face"], :PF_INET6, :SOCK_STREAM))
+        resolver = double
+
+        allow(resolver).to receive(:getaddresses).with('example.com').and_return(%w(0.0.0.0 2001:db8::face))
+        allow(resolver).to receive(:timeouts=).and_return(nil)
+        allow(Resolv::DNS).to receive(:open).and_yield(resolver)
+
         expect { subject.perform }.to raise_error Mastodon::ValidationError
       end
     end