From a196a93ed2390b995214118eed35b2baf0136119 Mon Sep 17 00:00:00 2001 From: Marcello Costagliola Date: Mon, 5 Oct 2026 03:06:26 +0200 Subject: [PATCH 1/2] 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 Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa --- app/models/opengraph/fetch.rb | 30 ++++++++---- test/models/opengraph/fetch_test.rb | 75 +++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 9 deletions(-) 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 From 414ff3376eb55e19f5b3a87757d08444c122aa3d Mon Sep 17 00:00:00 2001 From: Marcello Costagliola Date: Tue, 6 Oct 2026 14:03:26 +0200 Subject: [PATCH 2/2] Put the whole unfurl, lookups included, under one deadline The deadline in Opengraph::Fetch started after the lookup of the pasted host, and an unfurl looks up more hosts outside any fetch: the canonical URL's, and the image's, once to check its content type and again to validate it. Each of those waits as long as the resolver takes to give up. Move the deadline up to UnfurlLinksController, around everything the link leads to; Opengraph::Fetch keeps its per-operation timeouts and makes no retries. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa --- app/controllers/unfurl_links_controller.rb | 17 +++- app/models/opengraph/fetch.rb | 28 +++--- .../unfurl_links_controller_test.rb | 87 +++++++++++++++++++ test/models/opengraph/fetch_test.rb | 75 ---------------- 4 files changed, 113 insertions(+), 94 deletions(-) diff --git a/app/controllers/unfurl_links_controller.rb b/app/controllers/unfurl_links_controller.rb index 5149216..331405e 100644 --- a/app/controllers/unfurl_links_controller.rb +++ b/app/controllers/unfurl_links_controller.rb @@ -1,8 +1,9 @@ class UnfurlLinksController < ApplicationController - def create - opengraph = Opengraph::Metadata.from_url(url_param) + # The pasted link decides which hosts an unfurl looks up and fetches from, and how quickly they answer. + DEADLINE = 10.seconds - if opengraph.valid? + def create + if opengraph = unfurl(url_param) render json: opengraph else head :no_content @@ -13,4 +14,14 @@ class UnfurlLinksController < ApplicationController def url_param params.require(:url) end + + # One deadline for everything the link leads to: DNS lookups, redirects, the page, the image check. + def unfurl(url) + Timeout.timeout(DEADLINE) do + Opengraph::Metadata.from_url(url).then { |opengraph| opengraph if opengraph.valid? } + end + rescue Timeout::Error + Rails.logger.warn "Gave up unfurling #{url} after #{DEADLINE.inspect}" + nil + end end diff --git a/app/models/opengraph/fetch.rb b/app/models/opengraph/fetch.rb index 8bf5b1d..bd62812 100644 --- a/app/models/opengraph/fetch.rb +++ b/app/models/opengraph/fetch.rb @@ -5,8 +5,7 @@ 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 + TIMEOUT = 7.seconds # to connect (TLS included), and for each read and write, as Webhook does class TooManyRedirectsError < StandardError; end class RedirectDeniedError < StandardError; end @@ -24,25 +23,22 @@ 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. + # The timeouts bound each operation. A host can still send its headers or body a byte at a time, + # in as many reads as it likes: UnfurlLinksController puts the whole unfurl under one deadline. def request(url, request_class, ip:) - 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 + 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 - - raise TooManyRedirectsError end + + raise TooManyRedirectsError end # Without max_retries: 0, Net::HTTP sends a GET or HEAD that timed out a second time. diff --git a/test/controllers/unfurl_links_controller_test.rb b/test/controllers/unfurl_links_controller_test.rb index a97d045..f0311f2 100644 --- a/test/controllers/unfurl_links_controller_test.rb +++ b/test/controllers/unfurl_links_controller_test.rb @@ -75,7 +75,94 @@ class UnfurlLinksControllerTest < ActionDispatch::IntegrationTest assert_equal "Hey!", JSON.parse(response.body)["title"] end + test "create 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_unfurls_nothing_within(1.second) { post unfurl_link_url, params: { url: url } } + end + assert_equal 1, connections.size, "the GET is not retried" + end + end + + test "create 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(UnfurlLinksController, :DEADLINE, 0.5.seconds) do + assert_unfurls_nothing_within(1.5.seconds) { post unfurl_link_url, params: { url: url } } + end + end + end + + test "create 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(UnfurlLinksController, :DEADLINE, 0.5.seconds) do + assert_unfurls_nothing_within(1.5.seconds) { post unfurl_link_url, params: { url: url } } + end + end + end + + test "create 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(UnfurlLinksController, :DEADLINE, 0.5.seconds) do + assert_unfurls_nothing_within(1.second) { post unfurl_link_url, params: { url: "https://www.example.com" } } + end + end + + test "create gives up on a host name that takes too long to look up" do + Resolv.stubs(:getaddresses).with { sleep 2 }.returns([ "1.2.3.4" ]) # each lookup takes 2 s + WebMock.stub_request(:get, "https://www.example.com/").to_return(status: 200, body: "", headers: { content_type: "text/html" }) + + stub_const(UnfurlLinksController, :DEADLINE, 0.5.seconds) do + assert_unfurls_nothing_within(1.second) { post unfurl_link_url, params: { url: "https://www.example.com" } } + end + end + private + # A real socket, because WebMock reads the whole response before the code under test sees any of it. + # The guard lets the local address through, as it would a public one. + 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 + + RestrictedHTTP::PrivateNetworkGuard.stubs(:resolve).returns("127.0.0.1") + WebMock.disable! + yield "http://www.example.com:#{server.addr[1]}/", connections + ensure + WebMock.enable! + server&.close + end + + def assert_unfurls_nothing_within(limit) + started = Process.clock_gettime(Process::CLOCK_MONOTONIC) + yield + assert_response :no_content + assert_operator Process.clock_gettime(Process::CLOCK_MONOTONIC) - started, :<, limit + end + def stub_successful_request(url: "https://www.example.com/", title: "Hey!", description: "desc..") WebMock.stub_request(:get, url).to_return( status: 200, diff --git a/test/models/opengraph/fetch_test.rb b/test/models/opengraph/fetch_test.rb index e007ae7..dc510cc 100644 --- a/test/models/opengraph/fetch_test.rb +++ b/test/models/opengraph/fetch_test.rb @@ -74,54 +74,6 @@ 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/" }) @@ -163,31 +115,4 @@ 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