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 7fa85ae..bd62812 100644 --- a/app/models/opengraph/fetch.rb +++ b/app/models/opengraph/fetch.rb @@ -5,6 +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 class TooManyRedirectsError < StandardError; end class RedirectDeniedError < StandardError; end @@ -22,9 +23,11 @@ class Opengraph::Fetch end private + # 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:) MAX_REDIRECTS.times do - Net::HTTP.start(url.host, url.port, ipaddr: ip, use_ssl: url.scheme == "https") do |http| + 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"]) @@ -38,6 +41,11 @@ class Opengraph::Fetch raise TooManyRedirectsError 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) url = URI.parse(location) raise RedirectDeniedError unless url.is_a?(URI::HTTP) 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,