Merge branch 'hchouraria-url-blocker-getaddrinfo-timeout' into 'master'

Apply a 15s (finite) timeout to getaddrinfo operations performed when checking URLs to block

See merge request gitlab-org/gitlab!52895

lib/gitlab/url_blocker.rb

@@ -7,6 +7,9 @@ module Gitlab
   class UrlBlocker
     BlockedUrlError = Class.new(StandardError)
 
+    GETADDRINFO_TIMEOUT_SECONDS = 15
+    private_constant :GETADDRINFO_TIMEOUT_SECONDS
+
     class << self
       # Validates the given url according to the constraints specified by arguments.
       #
@@ -110,7 +113,7 @@ module Gitlab
       end
 
       def get_address_info(uri, dns_rebind_protection)
-        Addrinfo.getaddrinfo(uri.hostname, get_port(uri), nil, :STREAM).map do |addr|
+        Addrinfo.getaddrinfo(uri.hostname, get_port(uri), nil, :STREAM, timeout: GETADDRINFO_TIMEOUT_SECONDS).map do |addr|
           addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr
         end
       rescue SocketError

spec/lib/gitlab/url_blocker_spec.rb

@@ -160,6 +160,47 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do
         end
       end
     end
+
+    context 'when resolving runs into a timeout' do
+      let(:import_url) { 'http://example.com' }
+
+      subject { described_class.validate!(import_url, dns_rebind_protection: dns_rebind_protection) }
+
+      before do
+        skip 'timeout is not available' unless timeout_available?
+
+        stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
+        stub_const("#{described_class}::GETADDRINFO_TIMEOUT_SECONDS", 0)
+      end
+
+      context 'with dns rebinding enabled' do
+        let(:dns_rebind_protection) { true }
+
+        it 'raises an error due to DNS timeout' do
+          expect { subject }.to raise_error(described_class::BlockedUrlError)
+        end
+      end
+
+      context 'with dns rebinding disabled' do
+        let(:dns_rebind_protection) { false }
+
+        it_behaves_like 'validates URI and hostname' do
+          let(:expected_uri) { import_url }
+          let(:expected_hostname) { nil }
+        end
+      end
+
+      # Detect whether the timeout option is available.
+      #
+      # See https://bugs.ruby-lang.org/issues/15553
+      def timeout_available?
+        Addrinfo.getaddrinfo('localhost', nil, timeout: 0)
+
+        false
+      rescue SocketError
+        true
+      end
+    end
   end
 
   describe '#blocked_url?' do

spec/support/helpers/dns_helpers.rb

@@ -12,19 +12,38 @@ module DnsHelpers
   end
 
   def stub_all_dns!
-    allow(Addrinfo).to receive(:getaddrinfo).with(anything, anything, nil, :STREAM).and_return([])
-    allow(Addrinfo).to receive(:getaddrinfo).with(anything, anything, nil, :STREAM, anything, anything).and_return([])
+    allow(Addrinfo).to receive(:getaddrinfo).and_return([])
   end
 
   def stub_invalid_dns!
-    allow(Addrinfo).to receive(:getaddrinfo).with(/\Afoobar\.\w|(\d{1,3}\.){4,}\d{1,3}\z/i, anything, nil, :STREAM) do
-      raise SocketError.new("getaddrinfo: Name or service not known")
-    end
+    invalid_addresses = %r{
+      \A
+        (?:
+          foobar\.\w |
+          (?:\d{1,3}\.){4,}\d{1,3}
+        )
+      \z
+    }ix
+
+    allow(Addrinfo).to receive(:getaddrinfo)
+      .with(invalid_addresses, any_args)
+      .and_raise(SocketError, 'getaddrinfo: Name or service not known')
   end
 
   def permit_local_dns!
-    local_addresses = /\A(127|10)\.0\.0\.\d{1,3}|(192\.168|172\.16)\.\d{1,3}\.\d{1,3}|0\.0\.0\.0|localhost\z/i
-    allow(Addrinfo).to receive(:getaddrinfo).with(local_addresses, anything, nil, :STREAM).and_call_original
-    allow(Addrinfo).to receive(:getaddrinfo).with(local_addresses, anything, nil, :STREAM, anything, anything, any_args).and_call_original
+    local_addresses = %r{
+      \A
+        (?:
+          (?:127|10)\.0\.0\.\d{1,3} |
+          (?:192\.168|172\.16)\.\d{1,3}\.\d{1,3} |
+          0\.0\.0\.0 |
+          localhost
+        )
+      \z
+    }ix
+
+    allow(Addrinfo).to receive(:getaddrinfo)
+      .with(local_addresses, any_args)
+      .and_call_original
   end
 end

spec/support/helpers/stub_requests.rb

@@ -18,14 +18,13 @@ module StubRequests
   end
 
   def stub_dns(url, ip_address:, port: 80)
-    url = parse_url(url)
+    url = URI(url)
     socket = Socket.sockaddr_in(port, ip_address)
     addr = Addrinfo.new(socket)
 
-    # See Gitlab::UrlBlocker
     allow(Addrinfo).to receive(:getaddrinfo)
-                         .with(url.hostname, url.port, nil, :STREAM)
-                         .and_return([addr])
+      .with(url.hostname, url.port, any_args)
+      .and_return([addr])
   end
 
   def stub_all_dns(url, ip_address:)
@@ -34,22 +33,14 @@ module StubRequests
     socket = Socket.sockaddr_in(port, ip_address)
     addr = Addrinfo.new(socket)
 
-    # See Gitlab::UrlBlocker
-    allow(Addrinfo).to receive(:getaddrinfo).and_call_original
     allow(Addrinfo).to receive(:getaddrinfo)
-      .with(url.hostname, anything, nil, :STREAM)
+      .with(url.hostname, any_args)
       .and_return([addr])
   end
 
   def stubbed_hostname(url, hostname: IP_ADDRESS_STUB)
-    url = parse_url(url)
+    url = URI(url)
     url.hostname = hostname
     url.to_s
   end
-
-  private
-
-  def parse_url(url)
-    url.is_a?(URI) ? url : URI(url)
-  end
 end