Merge branch 'georgekoltsov/55474-outbound-setting-system-hooks' into 'master'

Add outbound setting for system hooks

See merge request gitlab-org/gitlab-ce!31177

13 files changed, 123 insertions, 50 deletions

diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb
index c77605f3869..ddd87404003 100644
--- a/spec/features/admin/admin_settings_spec.rb
+++ b/spec/features/admin/admin_settings_spec.rb
@@ -338,14 +338,17 @@ describe 'Admin updates settings' do
       visit network_admin_application_settings_path
 
       page.within('.as-outbound') do
-        check 'Allow requests to the local network from hooks and services'
+        check 'Allow requests to the local network from web hooks and services'
+        # Enabled by default
+        uncheck 'Allow requests to the local network from system hooks'
         # Enabled by default
         uncheck 'Enforce DNS rebinding attack protection'
         click_button 'Save changes'
       end
 
       expect(page).to have_content "Application settings saved successfully"
-      expect(current_settings.allow_local_requests_from_hooks_and_services).to be true
+      expect(current_settings.allow_local_requests_from_web_hooks_and_services).to be true
+      expect(current_settings.allow_local_requests_from_system_hooks).to be false
       expect(current_settings.dns_rebinding_protection_enabled).to be false
     end
   end
diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb
index 158f77cab2c..d3f9be845dd 100644
--- a/spec/lib/gitlab/http_spec.rb
+++ b/spec/lib/gitlab/http_spec.rb
@@ -23,14 +23,14 @@ describe Gitlab::HTTP do
     end
   end
 
-  describe 'allow_local_requests_from_hooks_and_services is' do
+  describe 'allow_local_requests_from_web_hooks_and_services is' do
     before do
       WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success')
     end
 
     context 'disabled' do
       before do
-        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false)
+        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_web_hooks_and_services?).and_return(false)
       end
 
       it 'deny requests to localhost' do
@@ -52,7 +52,7 @@ describe Gitlab::HTTP do
 
     context 'enabled' do
       before do
-        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true)
+        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_web_hooks_and_services?).and_return(true)
       end
 
       it 'allow requests to localhost' do
diff --git a/spec/lib/gitlab/kubernetes/kube_client_spec.rb b/spec/lib/gitlab/kubernetes/kube_client_spec.rb
index 97ebb5f1554..f49d4e23e39 100644
--- a/spec/lib/gitlab/kubernetes/kube_client_spec.rb
+++ b/spec/lib/gitlab/kubernetes/kube_client_spec.rb
@@ -58,7 +58,7 @@ describe Gitlab::Kubernetes::KubeClient do
 
       context 'when local requests are allowed' do
         before do
-          stub_application_setting(allow_local_requests_from_hooks_and_services: true)
+          stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
         end
 
         it 'allows local addresses' do
diff --git a/spec/lib/gitlab/octokit/middleware_spec.rb b/spec/lib/gitlab/octokit/middleware_spec.rb
index 7f2b523f5b7..43f6d13f7ba 100644
--- a/spec/lib/gitlab/octokit/middleware_spec.rb
+++ b/spec/lib/gitlab/octokit/middleware_spec.rb
@@ -30,7 +30,7 @@ describe Gitlab::Octokit::Middleware do
 
       context 'when localhost requests are not allowed' do
         before do
-          stub_application_setting(allow_local_requests_from_hooks_and_services: false)
+          stub_application_setting(allow_local_requests_from_web_hooks_and_services: false)
         end
 
         it_behaves_like 'Local URL'
@@ -38,7 +38,7 @@ describe Gitlab::Octokit::Middleware do
 
       context 'when localhost requests are allowed' do
         before do
-          stub_application_setting(allow_local_requests_from_hooks_and_services: true)
+          stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
         end
 
         it_behaves_like 'Public URL'
@@ -50,7 +50,7 @@ describe Gitlab::Octokit::Middleware do
 
       context 'when local network requests are not allowed' do
         before do
-          stub_application_setting(allow_local_requests_from_hooks_and_services: false)
+          stub_application_setting(allow_local_requests_from_web_hooks_and_services: false)
         end
 
         it_behaves_like 'Local URL'
@@ -58,7 +58,7 @@ describe Gitlab::Octokit::Middleware do
 
       context 'when local network requests are allowed' do
         before do
-          stub_application_setting(allow_local_requests_from_hooks_and_services: true)
+          stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
         end
 
         it_behaves_like 'Public URL'
diff --git a/spec/models/clusters/platforms/kubernetes_spec.rb b/spec/models/clusters/platforms/kubernetes_spec.rb
index 471769e4aab..5811016ea4d 100644
--- a/spec/models/clusters/platforms/kubernetes_spec.rb
+++ b/spec/models/clusters/platforms/kubernetes_spec.rb
@@ -106,7 +106,7 @@ describe Clusters::Platforms::Kubernetes do
           before do
             allow(ApplicationSetting)
               .to receive(:current)
-              .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: true))
+              .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: true))
           end
 
           it { expect(kubernetes.save).to be_truthy }
diff --git a/spec/models/lfs_download_object_spec.rb b/spec/models/lfs_download_object_spec.rb
index effd8b08124..8b53effe98f 100644
--- a/spec/models/lfs_download_object_spec.rb
+++ b/spec/models/lfs_download_object_spec.rb
@@ -50,7 +50,7 @@ describe LfsDownloadObject do
         before do
           allow(ApplicationSetting)
             .to receive(:current)
-              .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: setting))
+              .and_return(ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: setting))
         end
 
         context 'are allowed' do
diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb
index 8a60980fe80..184c00a356a 100644
--- a/spec/requests/api/settings_spec.rb
+++ b/spec/requests/api/settings_spec.rb
@@ -25,6 +25,9 @@ describe API::Settings, 'Settings' do
       expect(json_response['ed25519_key_restriction']).to eq(0)
       expect(json_response['performance_bar_allowed_group_id']).to be_nil
       expect(json_response['instance_statistics_visibility_private']).to be(false)
+      expect(json_response['allow_local_requests_from_hooks_and_services']).to be(false)
+      expect(json_response['allow_local_requests_from_web_hooks_and_services']).to be(false)
+      expect(json_response['allow_local_requests_from_system_hooks']).to be(true)
       expect(json_response).not_to have_key('performance_bar_allowed_group_path')
       expect(json_response).not_to have_key('performance_bar_enabled')
     end
@@ -67,7 +70,9 @@ describe API::Settings, 'Settings' do
             instance_statistics_visibility_private: true,
             diff_max_patch_bytes: 150_000,
             default_branch_protection: ::Gitlab::Access::PROTECTION_DEV_CAN_MERGE,
-            local_markdown_version: 3
+            local_markdown_version: 3,
+            allow_local_requests_from_web_hooks_and_services: true,
+            allow_local_requests_from_system_hooks: false
           }
 
         expect(response).to have_gitlab_http_status(200)
@@ -95,6 +100,8 @@ describe API::Settings, 'Settings' do
         expect(json_response['diff_max_patch_bytes']).to eq(150_000)
         expect(json_response['default_branch_protection']).to eq(Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
         expect(json_response['local_markdown_version']).to eq(3)
+        expect(json_response['allow_local_requests_from_web_hooks_and_services']).to eq(true)
+        expect(json_response['allow_local_requests_from_system_hooks']).to eq(false)
       end
     end
 
@@ -117,6 +124,14 @@ describe API::Settings, 'Settings' do
       expect(json_response['performance_bar_allowed_group_id']).to be_nil
     end
 
+    it 'supports legacy allow_local_requests_from_hooks_and_services' do
+      put api("/application/settings", admin),
+          params: { allow_local_requests_from_hooks_and_services: true }
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['allow_local_requests_from_hooks_and_services']).to eq(true)
+    end
+
     context 'external policy classification settings' do
       let(:settings) do
         {
diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb
index 75d534c59bf..970e82e7107 100644
--- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb
+++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb
@@ -17,7 +17,7 @@ describe Projects::LfsPointers::LfsDownloadService do
   before do
     ApplicationSetting.create_from_defaults
 
-    stub_application_setting(allow_local_requests_from_hooks_and_services: local_request_setting)
+    stub_application_setting(allow_local_requests_from_web_hooks_and_services: local_request_setting)
     allow(project).to receive(:lfs_enabled?).and_return(true)
   end
 
diff --git a/spec/services/self_monitoring/project/create_service_spec.rb b/spec/services/self_monitoring/project/create_service_spec.rb
index a1e7aaf45f2..7d4faba526b 100644
--- a/spec/services/self_monitoring/project/create_service_spec.rb
+++ b/spec/services/self_monitoring/project/create_service_spec.rb
@@ -37,7 +37,7 @@ describe SelfMonitoring::Project::CreateService do
         allow(ApplicationSetting)
           .to receive(:current)
           .and_return(
-            ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: true)
+            ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: true)
           )
       end
 
@@ -95,7 +95,7 @@ describe SelfMonitoring::Project::CreateService do
           allow(ApplicationSetting)
             .to receive(:current)
             .and_return(
-              ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: false)
+              ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: false)
             )
         end
 
diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb
index 37bafc0c002..50167a2e059 100644
--- a/spec/services/web_hook_service_spec.rb
+++ b/spec/services/web_hook_service_spec.rb
@@ -19,17 +19,37 @@ describe WebHookService do
   let(:service_instance) { described_class.new(project_hook, data, :push_hooks) }
 
   describe '#initialize' do
-    it 'allow_local_requests is true if hook is a SystemHook' do
-      instance = described_class.new(build(:system_hook), data, :system_hook)
-      expect(instance.request_options[:allow_local_requests]).to be_truthy
+    before do
+      stub_application_setting(setting_name => setting)
     end
 
-    it 'allow_local_requests is false if hook is not a SystemHook' do
-      %i(project_hook service_hook web_hook_log).each do |hook|
-        instance = described_class.new(build(hook), data, hook)
-        expect(instance.request_options[:allow_local_requests]).to be_falsey
+    shared_examples_for 'respects outbound network setting' do
+      context 'when local requests are allowed' do
+        let(:setting) { true }
+
+        it { expect(hook.request_options[:allow_local_requests]).to be_truthy }
+      end
+
+      context 'when local requests are not allowed' do
+        let(:setting) { false }
+
+        it { expect(hook.request_options[:allow_local_requests]).to be_falsey }
       end
     end
+
+    context 'when SystemHook' do
+      let(:setting_name) { :allow_local_requests_from_system_hooks }
+      let(:hook) { described_class.new(build(:system_hook), data, :system_hook) }
+
+      include_examples 'respects outbound network setting'
+    end
+
+    context 'when ProjectHook' do
+      let(:setting_name) { :allow_local_requests_from_web_hooks_and_services }
+      let(:hook) { described_class.new(build(:project_hook), data, :project_hook) }
+
+      include_examples 'respects outbound network setting'
+    end
   end
 
   describe '#execute' do
diff --git a/spec/support/shared_examples/url_validator_examples.rb b/spec/support/shared_examples/url_validator_examples.rb
index 16fceddb605..c5a775fefb6 100644
--- a/spec/support/shared_examples/url_validator_examples.rb
+++ b/spec/support/shared_examples/url_validator_examples.rb
@@ -1,12 +1,12 @@
 # frozen_string_literal: true
 
 RSpec.shared_examples 'url validator examples' do |schemes|
-  let(:validator) { described_class.new(attributes: [:link_url], **options) }
-  let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
+  describe '#validate' do
+    let(:validator) { described_class.new(attributes: [:link_url], **options) }
+    let(:badge) { build(:badge, link_url: 'http://www.example.com') }
 
-  subject { validator.validate(badge) }
+    subject { validator.validate(badge) }
 
-  describe '#validate' do
     context 'with no options' do
       let(:options) { {} }
 
@@ -42,3 +42,52 @@ RSpec.shared_examples 'url validator examples' do |schemes|
     end
   end
 end
+
+RSpec.shared_examples 'public url validator examples' do |setting|
+  let(:validator) { described_class.new(attributes: [:link_url]) }
+  let(:badge) { build(:badge, link_url: 'http://www.example.com') }
+
+  subject { validator.validate(badge) }
+
+  context 'by default' do
+    it 'blocks urls pointing to localhost' do
+      badge.link_url = 'https://127.0.0.1'
+
+      subject
+
+      expect(badge.errors).to be_present
+    end
+
+    it 'blocks urls pointing to the local network' do
+      badge.link_url = 'https://192.168.1.1'
+
+      subject
+
+      expect(badge.errors).to be_present
+    end
+  end
+
+  context 'when local requests are allowed' do
+    let!(:settings) { create(:application_setting) }
+
+    before do
+      stub_application_setting(setting)
+    end
+
+    it 'does not block urls pointing to localhost' do
+      badge.link_url = 'https://127.0.0.1'
+
+      subject
+
+      expect(badge.errors).not_to be_present
+    end
+
+    it 'does not block urls pointing to the local network' do
+      badge.link_url = 'https://192.168.1.1'
+
+      subject
+
+      expect(badge.errors).not_to be_present
+    end
+  end
+end
diff --git a/spec/validators/public_url_validator_spec.rb b/spec/validators/public_url_validator_spec.rb
index f6364fb1dd5..3cbf1002730 100644
--- a/spec/validators/public_url_validator_spec.rb
+++ b/spec/validators/public_url_validator_spec.rb
@@ -2,27 +2,5 @@ require 'spec_helper'
 
 describe PublicUrlValidator do
   include_examples 'url validator examples', AddressableUrlValidator::DEFAULT_OPTIONS[:schemes]
-
-  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(badge) }
-
-    it 'blocks urls pointing to localhost' do
-      badge.link_url = 'https://127.0.0.1'
-
-      subject
-
-      expect(badge.errors).to be_present
-    end
-
-    it 'blocks urls pointing to the local network' do
-      badge.link_url = 'https://192.168.1.1'
-
-      subject
-
-      expect(badge.errors).to be_present
-    end
-  end
+  include_examples 'public url validator examples', allow_local_requests_from_web_hooks_and_services: true
 end
diff --git a/spec/validators/system_hook_url_validator_spec.rb b/spec/validators/system_hook_url_validator_spec.rb
new file mode 100644
index 00000000000..02384bbd1ce
--- /dev/null
+++ b/spec/validators/system_hook_url_validator_spec.rb
@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe SystemHookUrlValidator do
+  include_examples 'url validator examples', AddressableUrlValidator::DEFAULT_OPTIONS[:schemes]
+  include_examples 'public url validator examples', allow_local_requests_from_system_hooks: true
+end