Automatic merge of gitlab-org/gitlab-ce master

changelogs/unreleased/fj-66723-add-dns-rebinding-protection-check.yml

+---
+title: Allow not resolvable urls when dns rebind protection is disabled
+merge_request: 32523
+author:
+type: fixed

lib/gitlab/url_blocker.rb

@@ -49,7 +49,7 @@ module Gitlab
         hostname = uri.hostname
         port = get_port(uri)
 
-        address_info = get_address_info(hostname, port)
+        address_info = get_address_info(hostname, port, dns_rebind_protection)
         return [uri, nil] unless address_info
 
         ip_address = ip_address(address_info)
@@ -110,11 +110,15 @@ module Gitlab
         validate_unicode_restriction(uri) if ascii_only
       end
 
-      def get_address_info(hostname, port)
+      def get_address_info(hostname, port, dns_rebind_protection)
         Addrinfo.getaddrinfo(hostname, port, nil, :STREAM).map do |addr|
           addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr
         end
       rescue SocketError
+        # If the dns rebinding protection is not enabled, we allow
+        # urls that can't be resolved at this point.
+        return unless dns_rebind_protection
+
         # In the test suite we use a lot of mocked urls that are either invalid or
         # don't exist. In order to avoid modifying a ton of tests and factories
         # we allow invalid urls unless the environment variable RSPEC_ALLOW_INVALID_URLS

spec/lib/gitlab/url_blocker_spec.rb

@@ -4,80 +4,114 @@
 require 'spec_helper'
 
 describe Gitlab::UrlBlocker do
+  include StubRequests
+
   describe '#validate!' do
+    subject { described_class.validate!(import_url) }
+
+    shared_examples 'validates URI and hostname' do
+      it 'runs the url validations' do
+        uri, hostname = subject
+
+        expect(uri).to eq(Addressable::URI.parse(expected_uri))
+        expect(hostname).to eq(expected_hostname)
+      end
+    end
+
     context 'when URI is nil' do
       let(:import_url) { nil }
 
-      it 'returns no URI and hostname' do
-        uri, hostname = described_class.validate!(import_url)
-
-        expect(uri).to be(nil)
-        expect(hostname).to be(nil)
+      it_behaves_like 'validates URI and hostname' do
+        let(:expected_uri) { nil }
+        let(:expected_hostname) { nil }
       end
     end
 
     context 'when URI is internal' do
       let(:import_url) { 'http://localhost' }
 
-      it 'returns URI and no hostname' do
-        uri, hostname = described_class.validate!(import_url)
-
-        expect(uri).to eq(Addressable::URI.parse('http://[::1]'))
-        expect(hostname).to eq('localhost')
+      it_behaves_like 'validates URI and hostname' do
+        let(:expected_uri) { 'http://[::1]' }
+        let(:expected_hostname) { 'localhost' }
       end
     end
 
     context 'when the URL hostname is a domain' do
-      let(:import_url) { 'https://example.org' }
+      context 'when domain can be resolved' do
+        let(:import_url) { 'https://example.org' }
 
-      it 'returns URI and hostname' do
-        uri, hostname = described_class.validate!(import_url)
+        before do
+          stub_dns(import_url, ip_address: '93.184.216.34')
+        end
 
-        expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34'))
-        expect(hostname).to eq('example.org')
+        it_behaves_like 'validates URI and hostname' do
+          let(:expected_uri) { 'https://93.184.216.34' }
+          let(:expected_hostname) { 'example.org' }
+        end
+      end
+
+      context 'when domain cannot be resolved' do
+        let(:import_url) { 'http://foobar.x' }
+
+        it 'raises an error' do
+          stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
+
+          expect { subject }.to raise_error(described_class::BlockedUrlError)
+        end
       end
     end
 
     context 'when the URL hostname is an IP address' do
       let(:import_url) { 'https://93.184.216.34' }
 
-      it 'returns URI and no hostname' do
-        uri, hostname = described_class.validate!(import_url)
+      it_behaves_like 'validates URI and hostname' do
+        let(:expected_uri) { import_url }
+        let(:expected_hostname) { nil }
+      end
+
+      context 'when the address is invalid' do
+        let(:import_url) { 'http://1.1.1.1.1' }
 
-        expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34'))
-        expect(hostname).to be(nil)
+        it 'raises an error' do
+          stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
+
+          expect { subject }.to raise_error(described_class::BlockedUrlError)
+        end
       end
     end
 
     context 'disabled DNS rebinding protection' do
+      subject { described_class.validate!(import_url, dns_rebind_protection: false) }
+
       context 'when URI is internal' do
         let(:import_url) { 'http://localhost' }
 
-        it 'returns URI and no hostname' do
-          uri, hostname = described_class.validate!(import_url, dns_rebind_protection: false)
-
-          expect(uri).to eq(Addressable::URI.parse('http://localhost'))
-          expect(hostname).to be(nil)
+        it_behaves_like 'validates URI and hostname' do
+          let(:expected_uri) { import_url }
+          let(:expected_hostname) { nil }
         end
       end
 
       context 'when the URL hostname is a domain' do
         let(:import_url) { 'https://example.org' }
 
-        it 'returns URI and no hostname' do
-          uri, hostname = described_class.validate!(import_url, dns_rebind_protection: false)
+        before do
+          stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
+        end
 
-          expect(uri).to eq(Addressable::URI.parse('https://example.org'))
-          expect(hostname).to eq(nil)
+        context 'when domain can be resolved' do
+          it_behaves_like 'validates URI and hostname' do
+            let(:expected_uri) { import_url }
+            let(:expected_hostname) { nil }
+          end
         end
 
-        context 'when it cannot be resolved' do
+        context 'when domain cannot be resolved' do
           let(:import_url) { 'http://foobar.x' }
 
-          it 'raises error' do
-            stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
-
-            expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError)
+          it_behaves_like 'validates URI and hostname' do
+            let(:expected_uri) { import_url }
+            let(:expected_hostname) { nil }
           end
         end
       end
@@ -85,20 +119,17 @@ describe Gitlab::UrlBlocker do
       context 'when the URL hostname is an IP address' do
         let(:import_url) { 'https://93.184.216.34' }
 
-        it 'returns URI and no hostname' do
-          uri, hostname = described_class.validate!(import_url, dns_rebind_protection: false)
-
-          expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34'))
-          expect(hostname).to be(nil)
+        it_behaves_like 'validates URI and hostname' do
+          let(:expected_uri) { import_url }
+          let(:expected_hostname) { nil }
         end
 
         context 'when it is invalid' do
           let(:import_url) { 'http://1.1.1.1.1' }
 
-          it 'raises an error' do
-            stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
-
-            expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError)
+          it_behaves_like 'validates URI and hostname' do
+            let(:expected_uri) { import_url }
+            let(:expected_hostname) { nil }
           end
         end
       end