Add outbound requests setting for system hooks

This MR adds new application setting to network section
`allow_local_requests_from_system_hooks`. Prior to this change
system hooks were allowed to do local network requests by default
and we are adding an ability for admins to control it.

8 files changed, 51 insertions, 10 deletions

diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb
index 3847a35fbab..a646b62027a 100644
--- a/app/helpers/application_settings_helper.rb
+++ b/app/helpers/application_settings_helper.rb
@@ -159,7 +159,8 @@ module ApplicationSettingsHelper
       :after_sign_up_text,
       :akismet_api_key,
       :akismet_enabled,
-      :allow_local_requests_from_hooks_and_services,
+      :allow_local_requests_from_web_hooks_and_services,
+      :allow_local_requests_from_system_hooks,
       :dns_rebinding_protection_enabled,
       :archive_builds_in_human_readable,
       :authorized_keys_enabled,
diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb
index 4bb09bf3b53..b7a4d7aa803 100644
--- a/app/models/application_setting_implementation.rb
+++ b/app/models/application_setting_implementation.rb
@@ -21,7 +21,8 @@ module ApplicationSettingImplementation
       {
         after_sign_up_text: nil,
         akismet_enabled: false,
-        allow_local_requests_from_hooks_and_services: false,
+        allow_local_requests_from_web_hooks_and_services: false,
+        allow_local_requests_from_system_hooks: true,
         dns_rebinding_protection_enabled: true,
         authorized_keys_enabled: true, # TODO default to false if the instance is configured to use AuthorizedKeysCommand
         container_registry_token_expire_delay: 5,
diff --git a/app/models/hooks/system_hook.rb b/app/models/hooks/system_hook.rb
index 90b4588a325..b83913c845f 100644
--- a/app/models/hooks/system_hook.rb
+++ b/app/models/hooks/system_hook.rb
@@ -14,8 +14,11 @@ class SystemHook < WebHook
   default_value_for :repository_update_events, true
   default_value_for :merge_requests_events, false
 
+  validates :url, presence: true, public_url: false, system_hook_url: { allow_localhost: lambda(&:allow_local_requests?),
+                                                                        allow_local_network: lambda(&:allow_local_requests?) }
+
   # Allow urls pointing localhost and the local network
   def allow_local_requests?
-    true
+    Gitlab::CurrentSettings.allow_local_requests_from_system_hooks?
   end
 end
diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index daf7ff4b771..3203bb024ab 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -35,6 +35,6 @@ class WebHook < ApplicationRecord
 
   # Allow urls pointing localhost and the local network
   def allow_local_requests?
-    false
+    Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?
   end
 end
diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb
index 6d675c026bb..8c294218708 100644
--- a/app/services/web_hook_service.rb
+++ b/app/services/web_hook_service.rb
@@ -17,8 +17,10 @@ class WebHookService
     @hook = hook
     @data = data
     @hook_name = hook_name.to_s
-    @request_options = { timeout: Gitlab.config.gitlab.webhook_timeout }
-    @request_options.merge!(allow_local_requests: true) if @hook.is_a?(SystemHook)
+    @request_options = {
+      timeout: Gitlab.config.gitlab.webhook_timeout,
+      allow_local_requests: hook.allow_local_requests?
+    }
   end
 
   def execute
diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index 273e15ef925..bb445499cee 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -107,6 +107,6 @@ class AddressableUrlValidator < ActiveModel::EachValidator
     # calls this validator.
     #
     # See https://gitlab.com/gitlab-org/gitlab-ee/issues/9833
-    ApplicationSetting.current&.allow_local_requests_from_hooks_and_services?
+    ApplicationSetting.current&.allow_local_requests_from_web_hooks_and_services?
   end
 end
diff --git a/app/validators/system_hook_url_validator.rb b/app/validators/system_hook_url_validator.rb
new file mode 100644
index 00000000000..c8c0007e35b
--- /dev/null
+++ b/app/validators/system_hook_url_validator.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+# SystemHookUrlValidator
+#
+# Custom validator specifically for SystemHook URLs. This validator works like AddressableUrlValidator but
+# it blocks urls pointing to localhost or the local network depending on
+# ApplicationSetting.allow_local_requests_from_system_hooks
+#
+# Example:
+#
+#   class SystemHook < WebHook
+#     validates :url, system_hook_url: { allow_localhost: true, allow_local_network: true }
+#   end
+#
+class SystemHookUrlValidator < AddressableUrlValidator
+  DEFAULT_OPTIONS = {
+    allow_localhost: true,
+    allow_local_network: true
+  }.freeze
+
+  def initialize(options)
+    options.reverse_merge!(DEFAULT_OPTIONS)
+
+    super(options)
+  end
+
+  def self.allow_setting_local_requests?
+    ApplicationSetting.current&.allow_local_requests_from_system_hooks?
+  end
+end
diff --git a/app/views/admin/application_settings/_outbound.html.haml b/app/views/admin/application_settings/_outbound.html.haml
index 4fecdb59e1d..d39e192d604 100644
--- a/app/views/admin/application_settings/_outbound.html.haml
+++ b/app/views/admin/application_settings/_outbound.html.haml
@@ -4,9 +4,13 @@
   %fieldset
     .form-group
       .form-check
-        = f.check_box :allow_local_requests_from_hooks_and_services, class: 'form-check-input'
-        = f.label :allow_local_requests_from_hooks_and_services, class: 'form-check-label' do
-          Allow requests to the local network from hooks and services
+        = f.check_box :allow_local_requests_from_web_hooks_and_services, class: 'form-check-input'
+        = f.label :allow_local_requests_from_web_hooks_and_services, class: 'form-check-label' do
+          Allow requests to the local network from web hooks and services
+      .form-check
+        = f.check_box :allow_local_requests_from_system_hooks, class: 'form-check-input'
+        = f.label :allow_local_requests_from_system_hooks, class: 'form-check-label' do
+          Allow requests to the local network from system hooks
 
     .form-group
       = f.label :outbound_local_requests_whitelist_raw, class: 'label-bold' do