Add validation to webhook and service URLs to ensure they are not blocked because of SSRF

10 files changed, 147 insertions, 71 deletions

diff --git a/spec/controllers/projects/mirrors_controller_spec.rb b/spec/controllers/projects/mirrors_controller_spec.rb
index 45c1218a39c..5d64f362252 100644
--- a/spec/controllers/projects/mirrors_controller_spec.rb
+++ b/spec/controllers/projects/mirrors_controller_spec.rb
@@ -54,7 +54,7 @@ describe Projects::MirrorsController do
         do_put(project, remote_mirrors_attributes: remote_mirror_attributes)
 
         expect(response).to redirect_to(project_settings_repository_path(project))
-        expect(flash[:alert]).to match(/must be a valid URL/)
+        expect(flash[:alert]).to match(/Only allowed protocols are/)
       end
 
       it 'should not create a RemoteMirror object' do
diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb
index e4dc61b3a68..61f35cf325b 100644
--- a/spec/controllers/projects/services_controller_spec.rb
+++ b/spec/controllers/projects/services_controller_spec.rb
@@ -102,7 +102,7 @@ describe Projects::ServicesController do
 
         expect(response.status).to eq(200)
         expect(JSON.parse(response.body))
-          .to eq('error' => true, 'message' => 'Test failed.', 'service_response' => 'Bad test')
+          .to eq('error' => true, 'message' => 'Test failed.', 'service_response' => 'Bad test', 'test_failed' => true)
       end
     end
   end
diff --git a/spec/javascripts/integrations/integration_settings_form_spec.js b/spec/javascripts/integrations/integration_settings_form_spec.js
index 050b1f2074e..e07343810d2 100644
--- a/spec/javascripts/integrations/integration_settings_form_spec.js
+++ b/spec/javascripts/integrations/integration_settings_form_spec.js
@@ -143,6 +143,7 @@ describe('IntegrationSettingsForm', () => {
         error: true,
         message: errorMessage,
         service_response: 'some error',
+        test_failed: true,
       });
 
       integrationSettingsForm.testSettings(formData)
@@ -157,6 +158,27 @@ describe('IntegrationSettingsForm', () => {
         .catch(done.fail);
     });
 
+    it('should not show error Flash with `Save anyway` action if ajax request responds with error in validation', (done) => {
+      const errorMessage = 'Validations failed.';
+      mock.onPut(integrationSettingsForm.testEndPoint).reply(200, {
+        error: true,
+        message: errorMessage,
+        service_response: 'some error',
+        test_failed: false,
+      });
+
+      integrationSettingsForm.testSettings(formData)
+        .then(() => {
+          const $flashContainer = $('.flash-container');
+          expect($flashContainer.find('.flash-text').text().trim()).toEqual('Validations failed. some error');
+          expect($flashContainer.find('.flash-action')).toBeDefined();
+          expect($flashContainer.find('.flash-action').text().trim()).toEqual('');
+
+          done();
+        })
+        .catch(done.fail);
+    });
+
     it('should submit form if ajax request responds without any error in test', (done) => {
       spyOn(integrationSettingsForm.$form, 'submit');
 
@@ -180,6 +202,7 @@ describe('IntegrationSettingsForm', () => {
       mock.onPut(integrationSettingsForm.testEndPoint).reply(200, {
         error: true,
         message: errorMessage,
+        test_failed: true,
       });
 
       integrationSettingsForm.testSettings(formData)
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
index a3b3dc3be6d..81dbbb962dd 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Gitlab::UrlBlocker do
   describe '#blocked_url?' do
-    let(:valid_ports) { Project::VALID_IMPORT_PORTS }
+    let(:ports) { Project::VALID_IMPORT_PORTS }
 
     it 'allows imports from configured web host and port' do
       import_url = "http://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git"
@@ -19,7 +19,13 @@ describe Gitlab::UrlBlocker do
     end
 
     it 'returns true for bad port' do
-      expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git', valid_ports: valid_ports)).to be true
+      expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git', ports: ports)).to be true
+    end
+
+    it 'returns true for bad protocol' do
+      expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['https'])).to be false
+      expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false
+      expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['http'])).to be true
     end
 
     it 'returns true for alternative version of 127.0.0.1 (0177.1)' do
diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb
index a80800c6c92..1d94abe4195 100644
--- a/spec/models/remote_mirror_spec.rb
+++ b/spec/models/remote_mirror_spec.rb
@@ -12,8 +12,8 @@ describe RemoteMirror do
     context 'with an invalid URL' do
       it 'should not be valid' do
         remote_mirror = build(:remote_mirror, url: 'ftp://invalid.invalid')
+
         expect(remote_mirror).not_to be_valid
-        expect(remote_mirror.errors[:url].size).to eq(2)
       end
     end
   end
diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb
index f246bb79ab7..cd43bec35df 100644
--- a/spec/requests/api/commit_statuses_spec.rb
+++ b/spec/requests/api/commit_statuses_spec.rb
@@ -304,7 +304,7 @@ describe API::CommitStatuses do
         it 'responds with bad request status and validation errors' do
           expect(response).to have_gitlab_http_status(400)
           expect(json_response['message']['target_url'])
-            .to include 'must be a valid URL'
+            .to include 'is blocked: Only allowed protocols are http, https'
         end
       end
     end
diff --git a/spec/support/shared_examples/url_validator_examples.rb b/spec/support/shared_examples/url_validator_examples.rb
new file mode 100644
index 00000000000..b4757a70984
--- /dev/null
+++ b/spec/support/shared_examples/url_validator_examples.rb
@@ -0,0 +1,42 @@
+RSpec.shared_examples 'url validator examples' do |protocols|
+  let(:validator) { described_class.new(attributes: [:link_url],  **options) }
+  let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
+
+  subject { validator.validate_each(badge, :link_url, badge.link_url) }
+
+  describe '#validates_each' do
+    context 'with no options' do
+      let(:options) { {} }
+
+      it "allows #{protocols.join(',')} protocols by default" do
+        expect(validator.send(:default_options)[:protocols]).to eq protocols
+      end
+
+      it 'checks that the url structure is valid' do
+        badge.link_url = "#{badge.link_url}:invalid_port"
+
+        subject
+
+        expect(badge.errors.empty?).to be false
+      end
+    end
+
+    context 'with protocols' do
+      let(:options) { { protocols: %w[http] } }
+
+      it 'allows urls with the defined protocols' do
+        subject
+
+        expect(badge.errors.empty?).to be true
+      end
+
+      it 'add error if the url protocol does not match the selected ones' do
+        badge.link_url = 'https://www.example.com'
+
+        subject
+
+        expect(badge.errors.empty?).to be false
+      end
+    end
+  end
+end
diff --git a/spec/validators/public_url_validator_spec.rb b/spec/validators/public_url_validator_spec.rb
new file mode 100644
index 00000000000..710dd3dc38e
--- /dev/null
+++ b/spec/validators/public_url_validator_spec.rb
@@ -0,0 +1,28 @@
+require 'spec_helper'
+
+describe PublicUrlValidator do
+  include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS
+
+  context 'by default' do
+    let(:validator) { described_class.new(attributes: [:link_url]) }
+    let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
+
+    subject { validator.validate_each(badge, :link_url, badge.link_url) }
+
+    it 'blocks urls pointing to localhost' do
+      badge.link_url = 'https://127.0.0.1'
+
+      subject
+
+      expect(badge.errors.empty?).to be false
+    end
+
+    it 'blocks urls pointing to the local network' do
+      badge.link_url = 'https://192.168.1.1'
+
+      subject
+
+      expect(badge.errors.empty?).to be false
+    end
+  end
+end
diff --git a/spec/validators/url_placeholder_validator_spec.rb b/spec/validators/url_placeholder_validator_spec.rb
deleted file mode 100644
index b76d8acdf88..00000000000
--- a/spec/validators/url_placeholder_validator_spec.rb
+++ /dev/null
@@ -1,39 +0,0 @@
-require 'spec_helper'
-
-describe UrlPlaceholderValidator do
-  let(:validator) { described_class.new(attributes: [:link_url],  **options) }
-  let!(:badge) { build(:badge) }
-  let(:placeholder_url) { 'http://www.example.com/%{project_path}/%{project_id}/%{default_branch}/%{commit_sha}' }
-
-  subject { validator.validate_each(badge, :link_url, badge.link_url) }
-
-  describe '#validates_each' do
-    context 'with no options' do
-      let(:options) { {} }
-
-      it 'allows http and https protocols by default' do
-        expect(validator.send(:default_options)[:protocols]).to eq %w(http https)
-      end
-
-      it 'checks that the url structure is valid' do
-        badge.link_url = placeholder_url
-
-        subject
-
-        expect(badge.errors.empty?).to be false
-      end
-    end
-
-    context 'with placeholder regex' do
-      let(:options) { { placeholder_regex: /(project_path|project_id|commit_sha|default_branch)/ } }
-
-      it 'checks that the url is valid and obviate placeholders that match regex' do
-        badge.link_url = placeholder_url
-
-        subject
-
-        expect(badge.errors.empty?).to be true
-      end
-    end
-  end
-end
diff --git a/spec/validators/url_validator_spec.rb b/spec/validators/url_validator_spec.rb
index 763dff181d2..2d719263fc8 100644
--- a/spec/validators/url_validator_spec.rb
+++ b/spec/validators/url_validator_spec.rb
@@ -1,46 +1,62 @@
 require 'spec_helper'
 
 describe UrlValidator do
-  let(:validator) { described_class.new(attributes: [:link_url],  **options) }
-  let!(:badge) { build(:badge) }
-
+  let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
   subject { validator.validate_each(badge, :link_url, badge.link_url) }
 
-  describe '#validates_each' do
-    context 'with no options' do
-      let(:options) { {} }
+  include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS
+
+  context 'by default' do
+    let(:validator) { described_class.new(attributes: [:link_url]) }
+
+    it 'does not block urls pointing to localhost' do
+      badge.link_url = 'https://127.0.0.1'
+
+      subject
+
+      expect(badge.errors.empty?).to be true
+    end
+
+    it 'does not block urls pointing to the local network' do
+      badge.link_url = 'https://192.168.1.1'
 
-      it 'allows http and https protocols by default' do
-        expect(validator.send(:default_options)[:protocols]).to eq %w(http https)
-      end
+      subject
 
-      it 'checks that the url structure is valid' do
-        badge.link_url = 'http://www.google.es/%{whatever}'
+      expect(badge.errors.empty?).to be true
+    end
+  end
+
+  context 'when allow_localhost is set to false' do
+    let(:validator) { described_class.new(attributes: [:link_url], allow_localhost: false) }
+
+    it 'blocks urls pointing to localhost' do
+      badge.link_url = 'https://127.0.0.1'
 
-        subject
+      subject
 
-        expect(badge.errors.empty?).to be false
-      end
+      expect(badge.errors.empty?).to be false
     end
+  end
 
-    context 'with protocols' do
-      let(:options) { { protocols: %w(http) } }
+  context 'when allow_local_network is set to false' do
+    let(:validator) { described_class.new(attributes: [:link_url], allow_local_network: false) }
 
-      it 'allows urls with the defined protocols' do
-        badge.link_url = 'http://www.example.com'
+    it 'blocks urls pointing to the local network' do
+      badge.link_url = 'https://192.168.1.1'
 
-        subject
+      subject
 
-        expect(badge.errors.empty?).to be true
-      end
+      expect(badge.errors.empty?).to be false
+    end
+  end
 
-      it 'add error if the url protocol does not match the selected ones' do
-        badge.link_url = 'https://www.example.com'
+  context 'when ports is set' do
+    let(:validator) { described_class.new(attributes: [:link_url], ports: [443]) }
 
-        subject
+    it 'blocks urls with a different port' do
+      subject
 
-        expect(badge.errors.empty?).to be false
-      end
+      expect(badge.errors.empty?).to be false
     end
   end
 end