Validate urls before attempting to download

Changelog: changed

lib/gitlab/http_connection_adapter.rb

@@ -47,6 +47,7 @@ module Gitlab
     def validate_url!(url)
       Gitlab::UrlBlocker.validate!(url, allow_local_network: allow_local_requests?,
                                         allow_localhost: allow_local_requests?,
+                                        allow_object_storage: allow_object_storage?,
                                         dns_rebind_protection: dns_rebind_protection?)
     rescue Gitlab::UrlBlocker::BlockedUrlError => e
       raise Gitlab::HTTP::BlockedUrlError, "URL '#{url}' is blocked: #{e.message}"
@@ -56,6 +57,10 @@ module Gitlab
       options.fetch(:allow_local_requests, allow_settings_local_requests?)
     end
 
+    def allow_object_storage?
+      options.fetch(:allow_object_storage, false)
+    end
+
     def dns_rebind_protection?
       return false if Gitlab.http_proxy_env?
 

lib/gitlab/import_export/command_line_util.rb

@@ -62,23 +62,27 @@ module Gitlab
       end
 
       def download(url, upload_path, size_limit: nil)
-        File.open(upload_path, 'w') do |file|
-          # Download (stream) file from the uploader's location
-          IO.copy_stream(
-            URI.parse(url).open(progress_proc: file_size_limiter(size_limit)),
-            file
-          )
+        File.open(upload_path, 'wb') do |file|
+          current_size = 0
+
+          Gitlab::HTTP.get(url, stream_body: true, allow_object_storage: true) do |fragment|
+            if [301, 302, 307].include?(fragment.code)
+              Gitlab::Import::Logger.warn(message: "received redirect fragment", fragment_code: fragment.code)
+            elsif fragment.code == 200
+              current_size += fragment.bytesize
+
+              raise FileOversizedError if size_limit.present? && current_size > size_limit
+
+              file.write(fragment)
+            else
+              raise Gitlab::ImportExport::Error, "unsupported response downloading fragment #{fragment.code}"
+            end
+          end
         end
       rescue FileOversizedError
         nil
       end
 
-      def file_size_limiter(limit)
-        return if limit.blank?
-
-        -> (current_size) { raise FileOversizedError if current_size > limit }
-      end
-
       def tar_with_options(archive:, dir:, options:)
         execute_cmd(%W(tar -#{options} #{archive} -C #{dir} .))
       end

lib/gitlab/url_blocker.rb

@@ -13,6 +13,7 @@ module Gitlab
       # ports - Raises error if the given URL port does is not between given ports.
       # allow_localhost - Raises error if URL resolves to a localhost IP address and argument is false.
       # allow_local_network - Raises error if URL resolves to a link-local address and argument is false.
+      # allow_object_storage - Avoid raising an error if URL resolves to an object storage endpoint and argument is true.
       # ascii_only - Raises error if URL has unicode characters and argument is true.
       # enforce_user - Raises error if URL user doesn't start with alphanumeric characters and argument is true.
       # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true.
@@ -25,6 +26,7 @@ module Gitlab
         schemes: [],
         allow_localhost: false,
         allow_local_network: true,
+        allow_object_storage: false,
         ascii_only: false,
         enforce_user: false,
         enforce_sanitization: false,
@@ -58,6 +60,8 @@ module Gitlab
         # Allow url from the GitLab instance itself but only for the configured hostname and ports
         return protected_uri_with_hostname if internal?(uri)
 
+        return protected_uri_with_hostname if allow_object_storage && object_storage_endpoint?(uri)
+
         validate_local_request(
           address_info: address_info,
           allow_localhost: allow_localhost,
@@ -269,6 +273,30 @@ module Gitlab
           get_port(uri) == config.gitlab_shell.ssh_port
       end
 
+      def enabled_object_storage_endpoints
+        ObjectStoreSettings::SUPPORTED_TYPES.collect do |type|
+          section_setting = config.try(type)
+
+          next unless section_setting
+
+          object_store_setting = section_setting['object_store']
+
+          next unless object_store_setting && object_store_setting['enabled']
+
+          object_store_setting.dig('connection', 'endpoint')
+        end.compact.uniq
+      end
+
+      def object_storage_endpoint?(uri)
+        enabled_object_storage_endpoints.any? do |endpoint|
+          endpoint_uri = URI(endpoint)
+
+          uri.scheme == endpoint_uri.scheme &&
+            uri.hostname == endpoint_uri.hostname &&
+            get_port(uri) == get_port(endpoint_uri)
+        end
+      end
+
       def domain_allowed?(uri)
         Gitlab::UrlBlockers::UrlAllowlist.domain_allowed?(uri.normalized_host, port: get_port(uri))
       end

spec/lib/gitlab/import_export/command_line_util_spec.rb

@@ -68,31 +68,126 @@ RSpec.describe Gitlab::ImportExport::CommandLineUtil do
   end
 
   describe '#download' do
-    before do
-      stub_request(:get, 'http://localhost:3000/file')
-        .to_return(
-          status: 200,
-          body: File.open(archive),
-          headers: {
-            'Content-Type' => 'application/x-tar'
-          }
-        )
-    end
+    let(:content) { File.open('spec/fixtures/rails_sample.tif') }
+
+    context 'a non-localhost uri' do
+      before do
+        stub_request(:get, url)
+          .to_return(
+            status: status,
+            body: content
+          )
+      end
+
+      let(:url) { 'https://gitlab.com/file' }
+
+      context 'with ok status code' do
+        let(:status) { HTTP::Status::OK }
+
+        it 'gets the contents' do
+          Tempfile.create('test') do |file|
+            subject.download(url, file.path)
+            expect(file.read).to eq(File.open('spec/fixtures/rails_sample.tif').read)
+          end
+        end
+
+        it 'streams the contents via Gitlab::HTTP' do
+          expect(Gitlab::HTTP).to receive(:get).with(url, hash_including(stream_body: true))
+
+          Tempfile.create('test') do |file|
+            subject.download(url, file.path)
+          end
+        end
+
+        it 'does not get the content over the size_limit' do
+          Tempfile.create('test') do |file|
+            subject.download(url, file.path, size_limit: 300.kilobytes)
+            expect(file.read).to eq('')
+          end
+        end
+
+        it 'gets the content within the size_limit' do
+          Tempfile.create('test') do |file|
+            subject.download(url, file.path, size_limit: 400.kilobytes)
+            expect(file.read).to eq(File.open('spec/fixtures/rails_sample.tif').read)
+          end
+        end
+      end
 
-    let(:tempfile) { Tempfile.new('test', path) }
+      %w[MOVED_PERMANENTLY FOUND TEMPORARY_REDIRECT].each do |code|
+        context "with a redirect status code #{code}" do
+          let(:status) { HTTP::Status.const_get(code, false) }
 
-    it 'downloads the file in the given path' do
-      subject.download('http://localhost:3000/file', tempfile)
+          it 'logs the redirect' do
+            expect(Gitlab::Import::Logger).to receive(:warn)
 
-      expect(File.exist?(tempfile)).to eq(true)
-      expect(tempfile.size).to eq(File.size(archive))
+            Tempfile.create('test') do |file|
+              subject.download(url, file.path)
+            end
+          end
+        end
+      end
+
+      %w[ACCEPTED UNAUTHORIZED BAD_REQUEST].each do |code|
+        context "with an invalid status code #{code}" do
+          let(:status) { HTTP::Status.const_get(code, false) }
+
+          it 'throws an error' do
+            Tempfile.create('test') do |file|
+              expect { subject.download(url, file.path) }.to raise_error(Gitlab::ImportExport::Error)
+            end
+          end
+        end
+      end
     end
 
-    it 'limit the size of the downloaded file' do
-      subject.download('http://localhost:3000/file', tempfile, size_limit: 1.byte)
+    context 'a localhost uri' do
+      include StubRequests
+
+      let(:status) { HTTP::Status::OK }
+      let(:url) { "#{host}/foo/bar" }
+      let(:host) { 'http://localhost:8081' }
 
-      expect(File.exist?(tempfile)).to eq(true)
-      expect(tempfile.size).to eq(0)
+      before do
+        # Note: the hostname gets changed to an ip address due to dns_rebind_protection
+        stub_dns(url, ip_address: '127.0.0.1')
+        stub_request(:get, 'http://127.0.0.1:8081/foo/bar')
+          .to_return(
+            status: status,
+            body: content
+          )
+      end
+
+      it 'throws a blocked url error' do
+        Tempfile.create('test') do |file|
+          expect { subject.download(url, file.path) }.to raise_error((Gitlab::HTTP::BlockedUrlError))
+        end
+      end
+
+      context 'for object_storage uri' do
+        let(:enabled_object_storage_setting) do
+          {
+            'object_store' =>
+            {
+              'enabled' => true,
+              'connection' => {
+                'endpoint' => host
+              }
+            }
+          }
+        end
+
+        before do
+          allow(Settings).to receive(:external_diffs).and_return(enabled_object_storage_setting)
+        end
+
+        it 'gets the content' do
+          Tempfile.create('test') do |file|
+            subject.download(url, file.path)
+            expect(file.read).to eq(File.open('spec/fixtures/rails_sample.tif').read)
+          end
+        end
+      end
     end
   end
 

spec/lib/gitlab/url_blocker_spec.rb

@@ -39,6 +39,73 @@ RSpec.describe Gitlab::UrlBlocker, :stub_invalid_dns_only do
       end
     end
 
+    context 'when URI is for a local object storage' do
+      let(:import_url) { "#{host}/external-diffs/merge_request_diffs/mr-1/diff-1" }
+      let(:enabled_object_storage_setting) do
+        {
+          'object_store' =>
+          {
+            'enabled' => true,
+            'connection' => {
+              'endpoint' => host
+            }
+          }
+        }
+      end
+
+      before do
+        allow(Settings).to receive(:external_diffs).and_return(enabled_object_storage_setting)
+      end
+
+      context 'when allow_object_storage is true' do
+        subject { described_class.validate!(import_url, allow_object_storage: true) }
+
+        context 'with a local domain name' do
+          let(:host) { 'http://review-minio-svc.svc:9000' }
+
+          before do
+            stub_dns(host, ip_address: '127.0.0.1')
+          end
+
+          it_behaves_like 'validates URI and hostname' do
+            let(:expected_uri) { 'http://127.0.0.1:9000/external-diffs/merge_request_diffs/mr-1/diff-1' }
+            let(:expected_hostname) { 'review-minio-svc.svc' }
+          end
+        end
+
+        context 'with an IP address' do
+          let(:host) { 'http://127.0.0.1:9000' }
+
+          it_behaves_like 'validates URI and hostname' do
+            let(:expected_uri) { 'http://127.0.0.1:9000/external-diffs/merge_request_diffs/mr-1/diff-1' }
+            let(:expected_hostname) { nil }
+          end
+        end
+      end
+
+      context 'when allow_object_storage is false' do
+        context 'with a local domain name' do
+          let(:host) { 'http://review-minio-svc.svc:9000' }
+
+          before do
+            stub_dns(host, ip_address: '127.0.0.1')
+          end
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(described_class::BlockedUrlError)
+          end
+        end
+
+        context 'with an IP address' do
+          let(:host) { 'http://127.0.0.1:9000' }
+
+          it 'raises an error' do
+            expect { subject }.to raise_error(described_class::BlockedUrlError)
+          end
+        end
+      end
+    end
+
     context 'when the URL hostname is a domain' do
       context 'when domain can be resolved' do
         let(:import_url) { 'https://example.org' }