Bound how long link unfurling can hold a request

Opengraph::Fetch runs inside POST /unfurl_link, against a host the member
picked, with Net::HTTP's defaults: 60 s to connect, 60 s for each read, and
one retry of the GET. A per-read timeout doesn't bound a host that sends a
byte at a time, in its headers or its body, so nothing limited how long a
fetch could hold the request thread.

Give each operation 7 s, as Webhook does, turn off the retry, and put the
whole fetch, redirects included, under one 10 s deadline.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa

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)

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