Update security/webhooks.md doc page & specs

Updating security/webhooks.md to match new behaviour
as well as drying up few specs to extract shared
examples

5 files changed, 43 insertions, 46 deletions

diff --git a/app/validators/system_hook_url_validator.rb b/app/validators/system_hook_url_validator.rb
index c8c0007e35b..e482828685d 100644
--- a/app/validators/system_hook_url_validator.rb
+++ b/app/validators/system_hook_url_validator.rb
@@ -2,7 +2,7 @@
 
 # SystemHookUrlValidator
 #
-# Custom validator specifically for SystemHook URLs. This validator works like AddressableUrlValidator but
+# Custom validator specific to 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
 #
@@ -14,8 +14,8 @@
 #
 class SystemHookUrlValidator < AddressableUrlValidator
   DEFAULT_OPTIONS = {
-    allow_localhost: true,
-    allow_local_network: true
+    allow_localhost: false,
+    allow_local_network: false
   }.freeze
 
   def initialize(options)
diff --git a/doc/security/img/outbound_requests_section_v2.png b/doc/security/img/outbound_requests_section_v2.png
new file mode 100644
index 00000000000..4fd3c7d9fce
--- /dev/null
+++ b/doc/security/img/outbound_requests_section_v2.png
diff --git a/doc/security/webhooks.md b/doc/security/webhooks.md
index 1194234a295..401df02e5c3 100644
--- a/doc/security/webhooks.md
+++ b/doc/security/webhooks.md
@@ -34,15 +34,15 @@ to 127.0.0.1, ::1 and 0.0.0.0, as well as IPv4 10.0.0.0/8, 172.16.0.0/12,
 192.168.0.0/16 and IPv6 site-local (ffc0::/10) addresses won't be allowed.
 
 This behavior can be overridden by enabling the option *"Allow requests to the
-local network from hooks and services"* in the *"Outbound requests"* section
+local network from web hooks and services"* in the *"Outbound requests"* section
 inside the Admin area under **Settings**
 (`/admin/application_settings/network`):
 
-![Outbound requests admin settings](img/outbound_requests_section.png)
+![Outbound requests admin settings](img/outbound_requests_section_v2.png)
 
 >**Note:**
-*System hooks* are exempt from this protection because they are set up by
-admins.
+*System hooks* are enabled to make requests to local network by default since they are set up by admins. 
+However, it can be turned off by disabling *"Allow requests to the local network from system hooks"* option.
 
 <!-- ## Troubleshooting
 
diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb
index 8353fade5ea..115599bd1a4 100644
--- a/spec/services/web_hook_service_spec.rb
+++ b/spec/services/web_hook_service_spec.rb
@@ -19,44 +19,36 @@ describe WebHookService do
   let(:service_instance) { described_class.new(project_hook, data, :push_hooks) }
 
   describe '#initialize' do
-    context 'when SystemHook' do
-      context 'when allow_local_requests_from_system_hooks application setting is true' do
-        it 'allows local requests' do
-          stub_application_setting(allow_local_requests_from_system_hooks: true)
-          instance = described_class.new(build(:system_hook), data, :system_hook)
+    before do
+      stub_application_setting(setting_name => setting)
+    end
 
-          expect(instance.request_options[:allow_local_requests]).to be_truthy
-        end
+    shared_examples_for 'respecting outbound network setting' do
+      context 'local requests are allowed' do
+        let(:setting) { true }
+
+        it { expect(hook.request_options[:allow_local_requests]).to be_truthy }
       end
 
-      context 'when allow_local_requests_from_system_hooks application setting is false' do
-        it 'denies local requests' do
-          stub_application_setting(allow_local_requests_from_system_hooks: false)
-          instance = described_class.new(build(:system_hook), data, :system_hook)
+      context 'local requests are not allowed' do
+        let(:setting) { false }
 
-          expect(instance.request_options[:allow_local_requests]).to be_falsey
-        end
+        it { expect(hook.request_options[:allow_local_requests]).to be_falsey }
       end
+    end
 
-      context 'when ProjectHook' do
-        context 'when allow_local_requests_from_web_hooks_and_services application setting is true' do
-          it 'allows local requests' do
-            stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
-            instance = described_class.new(build(:project_hook), data, :project_hook)
+    context 'when SystemHook' do
+      let(:setting_name) { :allow_local_requests_from_system_hooks }
+      let(:hook) { described_class.new(build(:system_hook), data, :system_hook) }
 
-            expect(instance.request_options[:allow_local_requests]).to be_truthy
-          end
-        end
+      include_examples 'respecting outbound network setting'
+    end
 
-        context 'when allow_local_requests_from_system_hooks application setting is false' do
-          it 'denies local requests' do
-            stub_application_setting(allow_local_requests_from_web_hooks_and_services: false)
-            instance = described_class.new(build(:project_hook), data, :project_hook)
+    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) }
 
-            expect(instance.request_options[:allow_local_requests]).to be_falsey
-          end
-        end
-      end
+      include_examples 'respecting outbound network setting'
     end
   end
 
diff --git a/spec/validators/system_hook_url_validator_spec.rb b/spec/validators/system_hook_url_validator_spec.rb
index fc4261666e7..78e95db2b47 100644
--- a/spec/validators/system_hook_url_validator_spec.rb
+++ b/spec/validators/system_hook_url_validator_spec.rb
@@ -11,43 +11,48 @@ describe SystemHookUrlValidator do
 
     subject { validator.validate(badge) }
 
-    it 'does not block urls pointing to localhost' do
+    it 'blocks urls pointing to localhost' do
       badge.link_url = 'https://127.0.0.1'
 
       subject
 
-      expect(badge.errors).not_to be_present
+      expect(badge.errors).to be_present
     end
 
-    it 'does not block urls pointing to the local network' do
+    it 'blocks urls pointing to the local network' do
       badge.link_url = 'https://192.168.1.1'
 
       subject
 
-      expect(badge.errors).not_to be_present
+      expect(badge.errors).to be_present
     end
   end
 
-  context 'when local requests are not allowed' do
-    let(:validator) { described_class.new(attributes: [:link_url], allow_localhost: false, allow_local_network: false) }
+  context 'when local requests are allowed' do
+    let(:validator) { described_class.new(attributes: [:link_url]) }
     let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
+    let!(:settings) { create(:application_setting) }
 
     subject { validator.validate(badge) }
 
-    it 'blocks urls pointing to localhost' do
+    before do
+      stub_application_setting(allow_local_requests_from_system_hooks: true)
+    end
+
+    it 'does not block urls pointing to localhost' do
       badge.link_url = 'https://127.0.0.1'
 
       subject
 
-      expect(badge.errors).to be_present
+      expect(badge.errors).not_to be_present
     end
 
-    it 'blocks urls pointing to the local network' do
+    it 'does not block urls pointing to the local network' do
       badge.link_url = 'https://192.168.1.1'
 
       subject
 
-      expect(badge.errors).to be_present
+      expect(badge.errors).not_to be_present
     end
   end
 end