Handle and report SSL errors in Web hook test. Check for status 200 for success.

If a Web hook test fails due to an SSL error or some other error, report
the result back to the user instead of an Error 500.

Closes #3656

Handle response

4 files changed, 28 insertions, 20 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 58a0a3c8944..faee22405dd 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.3.0 (unreleased)
+  - Handle and report SSL errors in Web hook test (Stan Hu)
   - Fix: Assignee selector is empty when 'Unassigned' is selected (Jose Corcuera)
   - Fix 500 error when update group member permission
   - Trim leading and trailing whitespace of milestone and issueable titles (Jose Corcuera)
diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb
index c7569541899..6a62880cb71 100644
--- a/app/controllers/projects/hooks_controller.rb
+++ b/app/controllers/projects/hooks_controller.rb
@@ -25,13 +25,12 @@ class Projects::HooksController < Projects::ApplicationController
 
   def test
     if !@project.empty_repo?
-      status = TestHookService.new.execute(hook, current_user)
+      status, message = TestHookService.new.execute(hook, current_user)
 
       if status
         flash[:notice] = 'Hook successfully executed.'
       else
-        flash[:alert] = 'Hook execution failed. '\
-                        'Ensure hook URL is correct and service is up.'
+        flash[:alert] = "Hook execution failed: #{message}"
       end
     else
       flash[:alert] = 'Hook execution failed. Ensure the project has commits.'
diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index d6c6f415c4a..2caf26cc8c9 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -37,31 +37,33 @@ class WebHook < ActiveRecord::Base
   def execute(data, hook_name)
     parsed_url = URI.parse(url)
     if parsed_url.userinfo.blank?
-      WebHook.post(url,
-                   body: data.to_json,
-                   headers: {
-                     "Content-Type" => "application/json",
-                     "X-Gitlab-Event" => hook_name.singularize.titleize
-                   },
-                   verify: enable_ssl_verification)
+      response = WebHook.post(url,
+                              body: data.to_json,
+                              headers: {
+                                  "Content-Type" => "application/json",
+                                  "X-Gitlab-Event" => hook_name.singularize.titleize
+                              },
+                              verify: enable_ssl_verification)
     else
       post_url = url.gsub("#{parsed_url.userinfo}@", "")
       auth = {
         username: URI.decode(parsed_url.user),
         password: URI.decode(parsed_url.password),
       }
-      WebHook.post(post_url,
-                   body: data.to_json,
-                   headers: {
-                     "Content-Type" => "application/json",
-                     "X-Gitlab-Event" => hook_name.singularize.titleize
-                   },
-                   verify: enable_ssl_verification,
-                   basic_auth: auth)
+      response = WebHook.post(post_url,
+                              body: data.to_json,
+                              headers: {
+                                  "Content-Type" => "application/json",
+                                  "X-Gitlab-Event" => hook_name.singularize.titleize
+                              },
+                              verify: enable_ssl_verification,
+                              basic_auth: auth)
     end
-  rescue SocketError, Errno::ECONNRESET, Errno::ECONNREFUSED, Net::OpenTimeout => e
+
+    [response.code == 200, ActionView::Base.full_sanitizer.sanitize(response.to_s)]
+  rescue SocketError, OpenSSL::SSL::SSLError, Errno::ECONNRESET, Errno::ECONNREFUSED, Net::OpenTimeout => e
     logger.error("WebHook Error => #{e}")
-    false
+    [false, e.to_s]
   end
 
   def async_execute(data, hook_name)
diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb
index 2fdc49f02ee..35042788c65 100644
--- a/spec/models/hooks/web_hook_spec.rb
+++ b/spec/models/hooks/web_hook_spec.rb
@@ -71,5 +71,11 @@ describe ProjectHook do
 
       expect { @project_hook.execute(@data, 'push_hooks') }.to raise_error(RuntimeError)
     end
+
+    it "handles SSL exceptions" do
+      expect(WebHook).to receive(:post).and_raise(OpenSSL::SSL::SSLError.new('SSL error'))
+
+      expect(@project_hook.execute(@data, 'push_hooks')).to eq([false, 'SSL error'])
+    end
   end
 end