diff --git a/app/models/opengraph/fetch.rb b/app/models/opengraph/fetch.rb index 7fa85ae..8bf5b1d 100644 --- a/app/models/opengraph/fetch.rb +++ b/app/models/opengraph/fetch.rb @@ -5,6 +5,8 @@ class Opengraph::Fetch ALLOWED_DOCUMENT_CONTENT_TYPE = "text/html" MAX_BODY_SIZE = 5.megabytes MAX_REDIRECTS = 10 + TIMEOUT = 7.seconds # to connect (TLS included), and for each read and write, as Webhook does + DEADLINE = 10.seconds # for the whole fetch: redirects, headers and body class TooManyRedirectsError < StandardError; end class RedirectDeniedError < StandardError; end @@ -22,20 +24,30 @@ class Opengraph::Fetch end private + # A member triggers this from a web request, and the host on the other end decides how fast it answers. + # The timeouts bound each operation, but Net::HTTP reads headers and body in as many reads as the host + # likes, so only a deadline over the whole fetch stops one that sends a byte at a time. def request(url, request_class, ip:) - MAX_REDIRECTS.times do - Net::HTTP.start(url.host, url.port, ipaddr: ip, use_ssl: url.scheme == "https") do |http| - http.request request_class.new(url) do |response| - if response.is_a?(Net::HTTPRedirection) - url, ip = resolve_redirect(response["location"]) - else - yield response + Timeout.timeout(DEADLINE) do + MAX_REDIRECTS.times do + Net::HTTP.start(url.host, url.port, ipaddr: ip, use_ssl: url.scheme == "https", **timeouts) do |http| + http.request request_class.new(url) do |response| + if response.is_a?(Net::HTTPRedirection) + url, ip = resolve_redirect(response["location"]) + else + yield response + end end end end - end - raise TooManyRedirectsError + raise TooManyRedirectsError + end + end + + # Without max_retries: 0, Net::HTTP sends a GET or HEAD that timed out a second time. + def timeouts + { open_timeout: TIMEOUT, read_timeout: TIMEOUT, write_timeout: TIMEOUT, max_retries: 0 } end def resolve_redirect(location) diff --git a/test/models/opengraph/fetch_test.rb b/test/models/opengraph/fetch_test.rb index dc510cc..e007ae7 100644 --- a/test/models/opengraph/fetch_test.rb +++ b/test/models/opengraph/fetch_test.rb @@ -74,6 +74,54 @@ class Opengraph::FetchTest < ActiveSupport::TestCase end end + test "#fetch_document gives up on a host that accepts the connection and never answers" do + with_local_host do |url, connections| + stub_const(Opengraph::Fetch, :TIMEOUT, 0.2.seconds) do + assert_gives_up_within(1.second, Net::ReadTimeout) { @fetch.fetch_document(url, ip: "127.0.0.1") } + end + assert_equal 1, connections.size, "the GET is not retried" + end + end + + test "#fetch_document gives up on headers that trickle in past the deadline" do + trickle_headers = ->(client) do + client.write "HTTP/1.1 200 OK\r\nX-Padding: " + 100.times { client.write "x"; sleep 0.05 } + client.write "\r\nContent-Type: text/html\r\nContent-Length: 2\r\n\r\nok" + end + + with_local_host(trickle_headers) do |url| + stub_const(Opengraph::Fetch, :DEADLINE, 0.5.seconds) do + assert_gives_up_within(1.5.seconds, Timeout::Error) { @fetch.fetch_document(url, ip: "127.0.0.1") } + end + end + end + + test "#fetch_document gives up on a body that trickles in past the deadline" do + trickle_body = ->(client) do + client.write "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: 100\r\n\r\n" + 100.times { client.write "x"; sleep 0.05 } + end + + with_local_host(trickle_body) do |url| + stub_const(Opengraph::Fetch, :DEADLINE, 0.5.seconds) do + assert_gives_up_within(1.5.seconds, Timeout::Error) { @fetch.fetch_document(url, ip: "127.0.0.1") } + end + end + end + + test "#fetch_document gives up on redirects that keep coming past the deadline" do + stub_dns_resolution("1.2.3.4") + WebMock.stub_request(:get, "https://www.example.com/").to_return do + sleep 0.2 + { status: 302, headers: { location: "https://www.example.com/" } } + end + + stub_const(Opengraph::Fetch, :DEADLINE, 0.5.seconds) do + assert_gives_up_within(1.second, Timeout::Error) { @fetch.fetch_document(@url, ip: "1.2.3.4") } + end + end + test "#fetch_document is empty following redirects that never finish" do WebMock.stub_request(:get, "https://www.example.com/") .to_return(status: 302, headers: { location: "https://www.example.com/" }) @@ -115,4 +163,31 @@ class Opengraph::FetchTest < ActiveSupport::TestCase def large_body_content "x" * (Opengraph::Fetch::MAX_BODY_SIZE + 1) end + + # A real socket, because WebMock reads the whole response before the code under test sees any of it. + def with_local_host(respond = ->(client) { }) + server = TCPServer.new("127.0.0.1", 0) + connections = Queue.new + Thread.new do + loop do + client = server.accept + connections << client + client.readpartial(1024) + respond.call(client) + end + rescue IOError, SystemCallError + end + + WebMock.disable! + yield URI.parse("http://www.example.com:#{server.addr[1]}/"), connections + ensure + WebMock.enable! + server&.close + end + + def assert_gives_up_within(limit, error, &block) + started = Process.clock_gettime(Process::CLOCK_MONOTONIC) + assert_raises(error, &block) + assert_operator Process.clock_gettime(Process::CLOCK_MONOTONIC) - started, :<, limit + end end