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

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>

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')

changelogs/unreleased/48132-display-output-from-pre-receive-scripts.yml

+---
+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

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)

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

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

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',
       );
     });

spec/lib/gitlab/git/pre_receive_error_spec.rb

 require 'spec_helper'
 describe Gitlab::Git::PreReceiveError do
-  it 'makes its message HTML-friendly' do
-    ex = described_class.new("hello\nworld\n")
-    expect(ex.message).to eq('hello<br>world<br>')
+  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,')
+      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

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")

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)

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)

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')