Skip to content
This repository has been archived by the owner on May 16, 2019. It is now read-only.

Commit

Permalink
Fix connect timeout not being enforced (mastodon#9329)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
Gargron committed Nov 23, 2018
1 parent 12bdd7d commit a2cda74
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 15 deletions.
32 changes: 23 additions & 9 deletions app/lib/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

require 'ipaddr'
require 'socket'
require 'resolv'

class Request
REQUEST_TARGET = '(request-target)'
Expand Down Expand Up @@ -45,7 +46,7 @@ def perform
end

begin
yield response.extend(ClientLimit)
yield response.extend(ClientLimit) if block_given?
ensure
http_client.close
end
Expand Down Expand Up @@ -94,7 +95,7 @@ def key_id
end

def timeout
{ write: 10, connect: 10, read: 10 }
{ connect: nil, read: 10, write: 10 }
end

def http_client
Expand Down Expand Up @@ -139,16 +140,29 @@ def body_with_limit(limit = 1.megabyte)
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

Expand Down
17 changes: 11 additions & 6 deletions spec/lib/request_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,11 @@
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
Expand Down Expand Up @@ -81,9 +83,12 @@
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
Expand Down

0 comments on commit a2cda74

Please sign in to comment.