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