Merge branch '48132-display-output-from-pre-receive-scripts' into 'master'

Allow custom hooks errors to appear in GitLab UI

Closes #48132

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

13 files changed, 72 insertions, 26 deletions

diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_failed_to_merge.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_failed_to_merge.vue
index 2a4dff71d9b..11bc8c73ee9 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_failed_to_merge.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_failed_to_merge.vue
@@ -80,7 +80,7 @@ export default {
       <status-icon :show-disabled-button="true" status="warning" />
       <div class="media-body space-children">
         <span class="bold">
-          <span v-if="mr.mergeError" class="has-error-message"> {{ mergeError }}. </span>
+          <span v-if="mr.mergeError" class="has-error-message"> {{ mergeError }} </span>
           <span v-else> {{ s__('mrWidget|Merge failed.') }} </span>
           <span :class="{ 'has-custom-error': mr.mergeError }"> {{ timerText }} </span>
         </span>
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 8241e408ce5..d8a78001b79 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -76,8 +76,8 @@ module MergeRequests
     def try_merge
       repository.merge(current_user, source, merge_request, commit_message)
     rescue Gitlab::Git::PreReceiveError => e
-      handle_merge_error(log_message: e.message)
-      raise_error('Something went wrong during merge pre-receive hook')
+      raise MergeError,
+            "Something went wrong during merge pre-receive hook. #{e.message}".strip
     rescue => e
       handle_merge_error(log_message: e.message)
       raise_error('Something went wrong during merge')
diff --git a/changelogs/unreleased/48132-display-output-from-pre-receive-scripts.yml b/changelogs/unreleased/48132-display-output-from-pre-receive-scripts.yml
new file mode 100644
index 00000000000..e06a4d5ee75
--- /dev/null
+++ b/changelogs/unreleased/48132-display-output-from-pre-receive-scripts.yml
@@ -0,0 +1,5 @@
+---
+title: "Allow failed custom hook script errors to safely appear in GitLab UI by filtering error messages by the prefix GL-HOOK-ERR:"
+merge_request: 25625
+author:
+type: changed
diff --git a/doc/administration/custom_hooks.md b/doc/administration/custom_hooks.md
index 60ad4bf4e8f..28afaf84f5a 100644
--- a/doc/administration/custom_hooks.md
+++ b/doc/administration/custom_hooks.md
@@ -51,7 +51,7 @@ Hooks can be also placed in `hooks/<hook_name>.d` (global) or
 execution of the hooks.
 
 NOTE: **Note:** `<hook_name>.d` would need to be either `pre-receive.d`,
-`post-receive.d`, or `update.d` to work properly. Any other names will be ignored. 
+`post-receive.d`, or `update.d` to work properly. Any other names will be ignored.
 
 To look in a different directory for the global custom hooks (those in
 `hooks/<hook_name.d>`), set `custom_hooks_dir` in gitlab-shell config. For
@@ -76,9 +76,21 @@ first script exiting with a non-zero value.
 
 > [Introduced][5073] in GitLab 8.10.
 
-If the commit is declined or an error occurs during the Git hook check,
-the STDERR or STDOUT message of the hook will be present in GitLab's UI.
-STDERR takes precedence over STDOUT.
+To have custom error messages appear in GitLab's UI when the commit is
+declined or an error occurs during the Git hook, your script should:
+
+- Send the custom error messages to either the script's `stdout` or `stderr`.
+- Prefix each message with `GL-HOOK-ERR:` with no characters appearing before the prefix.
+
+### Example custom error message
+
+This hook script written in bash will generate the following message in GitLab's UI:
+
+```bash
+#!/bin/sh
+echo "GL-HOOK-ERR: My custom error message.";
+exit 1
+```
 
 ![Custom message from custom Git hook](img/custom_hooks_error_msg.png)
 
diff --git a/doc/administration/img/custom_hooks_error_msg.png b/doc/administration/img/custom_hooks_error_msg.png
index 845f0de19ce..4f25c471908 100644
--- a/doc/administration/img/custom_hooks_error_msg.png
+++ b/doc/administration/img/custom_hooks_error_msg.png
diff --git a/lib/gitlab/git/pre_receive_error.rb b/lib/gitlab/git/pre_receive_error.rb
index 03caace6fce..b46d4ba0b02 100644
--- a/lib/gitlab/git/pre_receive_error.rb
+++ b/lib/gitlab/git/pre_receive_error.rb
@@ -4,19 +4,38 @@ module Gitlab
   module Git
     #
     # PreReceiveError is special because its message gets displayed to users
-    # in the web UI. To prevent XSS we sanitize the message on
-    # initialization.
+    # in the web UI. Because of this, we:
+    # - Only display errors that have been marked as safe with a prefix.
+    #   This is to prevent leaking of stacktraces, or other sensitive info.
+    # - Sanitize the string of any XSS
     class PreReceiveError < StandardError
-      def initialize(msg = '')
-        super(nlbr(msg))
+      SAFE_MESSAGE_PREFIXES = [
+        'GitLab:', # Messages from gitlab-shell
+        'GL-HOOK-ERR:' # Messages marked as safe by user
+      ].freeze
+
+      SAFE_MESSAGE_REGEX = /^(#{SAFE_MESSAGE_PREFIXES.join('|')})\s*(?<safe_message>.+)/
+
+      def initialize(message = '')
+        super(sanitize(message))
       end
 
       private
 
       # In gitaly-ruby we override this method to do nothing, so that
       # sanitization happens in gitlab-rails only.
-      def nlbr(str)
-        Gitlab::Utils.nlbr(str)
+      def sanitize(message)
+        return message if message.blank?
+
+        safe_messages = message.split("\n").map do |msg|
+          if (match = msg.match(SAFE_MESSAGE_REGEX))
+            match[:safe_message].presence
+          end
+        end
+
+        safe_messages = safe_messages.compact.join("\n")
+
+        Gitlab::Utils.nlbr(safe_messages)
       end
     end
   end
diff --git a/spec/features/tags/master_deletes_tag_spec.rb b/spec/features/tags/master_deletes_tag_spec.rb
index 8d567e925ef..bdbbe645779 100644
--- a/spec/features/tags/master_deletes_tag_spec.rb
+++ b/spec/features/tags/master_deletes_tag_spec.rb
@@ -37,7 +37,7 @@ describe 'Maintainer deletes tag' do
   context 'when pre-receive hook fails', :js do
     before do
       allow_any_instance_of(Gitlab::GitalyClient::OperationService).to receive(:rm_tag)
-        .and_raise(Gitlab::Git::PreReceiveError, 'Do not delete tags')
+        .and_raise(Gitlab::Git::PreReceiveError, 'GitLab: Do not delete tags')
     end
 
     it 'shows the error message' do
diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_failed_to_merge_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_failed_to_merge_spec.js
index 3229ddd5e27..780bed1bf69 100644
--- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_failed_to_merge_spec.js
+++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_failed_to_merge_spec.js
@@ -120,7 +120,7 @@ describe('MRWidgetFailedToMerge', () => {
 
     it('renders given error', () => {
       expect(vm.$el.querySelector('.has-error-message').textContent.trim()).toEqual(
-        'Merge error happened.',
+        'Merge error happened',
       );
     });
 
diff --git a/spec/lib/gitlab/git/pre_receive_error_spec.rb b/spec/lib/gitlab/git/pre_receive_error_spec.rb
index 1b8be62dec6..cb030e38032 100644
--- a/spec/lib/gitlab/git/pre_receive_error_spec.rb
+++ b/spec/lib/gitlab/git/pre_receive_error_spec.rb
@@ -1,9 +1,19 @@
 require 'spec_helper'
 
 describe Gitlab::Git::PreReceiveError do
-  it 'makes its message HTML-friendly' do
-    ex = described_class.new("hello\nworld\n")
+  Gitlab::Git::PreReceiveError::SAFE_MESSAGE_PREFIXES.each do |prefix|
+    context "error messages prefixed with #{prefix}" do
+      it 'accepts only errors lines with the prefix' do
+        ex = described_class.new("#{prefix} Hello,\nworld!")
 
-    expect(ex.message).to eq('hello<br>world<br>')
+        expect(ex.message).to eq('Hello,')
+      end
+
+      it 'makes its message HTML-friendly' do
+        ex = described_class.new("#{prefix} Hello,\n#{prefix} world!\n")
+
+        expect(ex.message).to eq('Hello,<br>world!')
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
index b37fe2686b6..7579a6577b9 100644
--- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
@@ -39,7 +39,7 @@ describe Gitlab::GitalyClient::OperationService do
 
     context "when pre_receive_error is present" do
       let(:response) do
-        Gitaly::UserCreateBranchResponse.new(pre_receive_error: "something failed")
+        Gitaly::UserCreateBranchResponse.new(pre_receive_error: "GitLab: something failed")
       end
 
       it "throws a PreReceive exception" do
@@ -80,7 +80,7 @@ describe Gitlab::GitalyClient::OperationService do
 
     context "when pre_receive_error is present" do
       let(:response) do
-        Gitaly::UserUpdateBranchResponse.new(pre_receive_error: "something failed")
+        Gitaly::UserUpdateBranchResponse.new(pre_receive_error: "GitLab: something failed")
       end
 
       it "throws a PreReceive exception" do
@@ -117,7 +117,7 @@ describe Gitlab::GitalyClient::OperationService do
 
     context "when pre_receive_error is present" do
       let(:response) do
-        Gitaly::UserDeleteBranchResponse.new(pre_receive_error: "something failed")
+        Gitaly::UserDeleteBranchResponse.new(pre_receive_error: "GitLab: something failed")
       end
 
       it "throws a PreReceive exception" do
@@ -175,7 +175,7 @@ describe Gitlab::GitalyClient::OperationService do
 
   shared_examples 'cherry pick and revert errors' do
     context 'when a pre_receive_error is present' do
-      let(:response) { response_class.new(pre_receive_error: "something failed") }
+      let(:response) { response_class.new(pre_receive_error: "GitLab: something failed") }
 
       it 'raises a PreReceiveError' do
         expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
@@ -313,7 +313,7 @@ describe Gitlab::GitalyClient::OperationService do
       end
 
       context 'when a pre_receive_error is present' do
-        let(:response) { Gitaly::UserCommitFilesResponse.new(pre_receive_error: "something failed") }
+        let(:response) { Gitaly::UserCommitFilesResponse.new(pre_receive_error: "GitLab: something failed") }
 
         it 'raises a PreReceiveError' do
           expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb
index fe673de46aa..1430e12a07e 100644
--- a/spec/services/merge_requests/ff_merge_service_spec.rb
+++ b/spec/services/merge_requests/ff_merge_service_spec.rb
@@ -72,7 +72,7 @@ describe MergeRequests::FfMergeService do
       it 'logs and saves error if there is an PreReceiveError exception' do
         error_message = 'error message'
 
-        allow(service).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, error_message)
+        allow(service).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, "GitLab: #{error_message}")
         allow(service).to receive(:execute_hooks)
 
         service.execute(merge_request)
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index ede79b87bcc..887ec17171e 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -239,7 +239,7 @@ describe MergeRequests::MergeService do
       it 'logs and saves error if there is an PreReceiveError exception' do
         error_message = 'error message'
 
-        allow(service).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, error_message)
+        allow(service).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, "GitLab: #{error_message}")
         allow(service).to receive(:execute_hooks)
 
         service.execute(merge_request)
diff --git a/spec/services/tags/create_service_spec.rb b/spec/services/tags/create_service_spec.rb
index 0cbe57352be..e112cdc8881 100644
--- a/spec/services/tags/create_service_spec.rb
+++ b/spec/services/tags/create_service_spec.rb
@@ -41,7 +41,7 @@ describe Tags::CreateService do
       it 'returns an error' do
         expect(repository).to receive(:add_tag)
           .with(user, 'v1.1.0', 'master', 'Foo')
-          .and_raise(Gitlab::Git::PreReceiveError, 'something went wrong')
+          .and_raise(Gitlab::Git::PreReceiveError, 'GitLab: something went wrong')
 
         response = service.execute('v1.1.0', 'master', 'Foo')