From 0ddbb03976e18f9255f0cac60261ddbaf24733ae Mon Sep 17 00:00:00 2001
From: Bob Weinand <b.weinand@talkwalker.com>
Date: Tue, 3 Sep 2019 09:59:06 +0200
Subject: [PATCH] Do not strip HTML from / doubly escape sanitized pre-receive
 hook error (!18646)

---
 .../states/mr_widget_failed_to_merge.vue      | 13 +++++++--
 .../mr_widget_options.vue                     | 28 ++++++++++++++++---
 app/assets/stylesheets/framework/common.scss  |  4 +++
 app/services/merge_requests/merge_service.rb  |  2 +-
 ...lti-line-display-of-pre-receive-errors.yml |  4 +++
 lib/gitlab/git/pre_receive_error.rb           | 15 ++++------
 locale/gitlab.pot                             |  2 +-
 .../states/mr_widget_failed_to_merge_spec.js  |  8 +++---
 spec/lib/gitlab/git/pre_receive_error_spec.rb |  4 +--
 9 files changed, 56 insertions(+), 24 deletions(-)
 create mode 100644 changelogs/unreleased/20947-multi-line-display-of-pre-receive-errors.yml

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 11bc8c73ee94af..703186ac947288 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
@@ -1,6 +1,5 @@
 <script>
 import { n__ } from '~/locale';
-import { stripHtml } from '~/lib/utils/text_utility';
 import statusIcon from '../mr_widget_status_icon.vue';
 import eventHub from '../../event_hub';
 
@@ -29,7 +28,13 @@ export default {
 
   computed: {
     mergeError() {
-      return this.mr.mergeError ? stripHtml(this.mr.mergeError, ' ').trim() : '';
+      if (!this.mr.mergeError) {
+        return '';
+      }
+      if (this.mr.mergeError.indexOf('\n') < 0) {
+        return `${this.mr.mergeError.trim()}.`;
+      }
+      return this.mr.mergeError;
     },
     timerText() {
       return n__(
@@ -80,7 +85,9 @@ 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 text-pre-formatted">{{
+            mergeError
+          }}</span>
           <span v-else> {{ s__('mrWidget|Merge failed.') }} </span>
           <span :class="{ 'has-custom-error': mr.mergeError }"> {{ timerText }} </span>
         </span>
diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue
index edd21a81f8bfee..fed196ca629230 100644
--- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue
@@ -120,9 +120,25 @@ export default {
       );
     },
     mergeError() {
-      return sprintf(s__('mrWidget|Merge failed: %{mergeError}. Please try again.'), {
-        mergeError: this.mr.mergeError,
-      });
+      return sprintf(
+        s__('mrWidget|Merge failed: %{mergeError} Please try again.'),
+        {
+          mergeError:
+            this.mr.mergeError.indexOf('\n') < 0
+              ? `${this.mr.mergeError.trim()}.`
+              : `${this.mr.mergeError}\n`,
+        },
+        false,
+      );
+    },
+    mergeErrorIsMultiLine() {
+      return this.mr.mergeError.indexOf('\n') >= 0;
+    },
+    mergeErrorFirstLine() {
+      return this.mergeError.split('\n')[0];
+    },
+    mergeErrorWithoutFirstLine() {
+      return this.mergeError.substring(this.mergeErrorFirstLine.length);
     },
   },
   watch: {
@@ -362,7 +378,11 @@ export default {
           </mr-widget-alert-message>
 
           <mr-widget-alert-message v-if="mr.mergeError" type="danger">
-            {{ mergeError }}
+            <details v-if="mergeErrorIsMultiLine" class="text-pre-formatted">
+              <summary>{{ mergeErrorFirstLine }}</summary>
+              {{ mergeErrorWithoutFirstLine }}
+            </details>
+            <span v-else>{{ mergeError }}</span>
           </mr-widget-alert-message>
 
           <source-branch-removal-status v-if="shouldRenderSourceBranchRemovalStatus" />
diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss
index 4b89a2f2b0485d..b10cbe73dd1dec 100644
--- a/app/assets/stylesheets/framework/common.scss
+++ b/app/assets/stylesheets/framework/common.scss
@@ -240,6 +240,10 @@ li.note {
   @include message($red-100, $red-200, $red-900);
 }
 
+.text-pre-formatted {
+  white-space: pre-line;
+}
+
 .gitlab-promo {
   a {
     color: $gl-gray-350;
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 6309052244de77..5090482b3eeb29 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -77,7 +77,7 @@ def try_merge
       repository.merge(current_user, source, merge_request, commit_message)
     rescue Gitlab::Git::PreReceiveError => e
       raise MergeError,
-            "Something went wrong during merge pre-receive hook. #{e.message}".strip
+            "Something went wrong during merge pre-receive hook.\n#{e.message}"
     rescue => e
       handle_merge_error(log_message: e.message)
       raise_error('Something went wrong during merge')
diff --git a/changelogs/unreleased/20947-multi-line-display-of-pre-receive-errors.yml b/changelogs/unreleased/20947-multi-line-display-of-pre-receive-errors.yml
new file mode 100644
index 00000000000000..64ccaba1a1140c
--- /dev/null
+++ b/changelogs/unreleased/20947-multi-line-display-of-pre-receive-errors.yml
@@ -0,0 +1,4 @@
+title: Fix display of multi-line pre-receive hook error output when merging
+merge_request: 18646
+author: Bob Weinand
+type: fixed
diff --git a/lib/gitlab/git/pre_receive_error.rb b/lib/gitlab/git/pre_receive_error.rb
index ef9b1bf522481a..ba187bee78b539 100644
--- a/lib/gitlab/git/pre_receive_error.rb
+++ b/lib/gitlab/git/pre_receive_error.rb
@@ -4,10 +4,9 @@ module Gitlab
   module Git
     #
     # PreReceiveError is special because its message gets displayed to users
-    # 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
+    # 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.
     class PreReceiveError < StandardError
       SAFE_MESSAGE_PREFIXES = [
         'GitLab:', # Messages from gitlab-shell
@@ -17,14 +16,14 @@ class PreReceiveError < StandardError
       SAFE_MESSAGE_REGEX = /^(#{SAFE_MESSAGE_PREFIXES.join('|')})\s*(?<safe_message>.+)/.freeze
 
       def initialize(message = '')
-        super(sanitize(message))
+        super(collect_safe_messages(message))
       end
 
       private
 
       # In gitaly-ruby we override this method to do nothing, so that
       # sanitization happens in gitlab-rails only.
-      def sanitize(message)
+      def collect_safe_messages(message)
         return message if message.blank?
 
         safe_messages = message.split("\n").map do |msg|
@@ -33,9 +32,7 @@ def sanitize(message)
           end
         end
 
-        safe_messages = safe_messages.compact.join("\n")
-
-        Gitlab::Utils.nlbr(safe_messages)
+        safe_messages.compact.join("\n")
       end
     end
   end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 524ac9046b50ed..2b766592226d4f 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -19998,7 +19998,7 @@ msgstr ""
 msgid "mrWidget|Merge failed."
 msgstr ""
 
-msgid "mrWidget|Merge failed: %{mergeError}. Please try again."
+msgid "mrWidget|Merge failed: %{mergeError} Please try again."
 msgstr ""
 
 msgid "mrWidget|Merge locally"
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 780bed1bf69d6d..21ac25865f2006 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
@@ -49,12 +49,12 @@ describe('MRWidgetFailedToMerge', () => {
     });
 
     describe('mergeError', () => {
-      it('removes forced line breaks', done => {
-        mr.mergeError = 'contains<br />line breaks<br />';
+      it('preserves line breaks', done => {
+        mr.mergeError = 'contains\nline breaks';
 
         Vue.nextTick()
           .then(() => {
-            expect(vm.mergeError).toBe('contains line breaks');
+            expect(vm.mergeError).toBe('contains\nline breaks');
           })
           .then(done)
           .catch(done.fail);
@@ -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 cb030e380329b9..b5b67d14962b38 100644
--- a/spec/lib/gitlab/git/pre_receive_error_spec.rb
+++ b/spec/lib/gitlab/git/pre_receive_error_spec.rb
@@ -9,10 +9,10 @@
         expect(ex.message).to eq('Hello,')
       end
 
-      it 'makes its message HTML-friendly' do
+      it 'preserves newlines' do
         ex = described_class.new("#{prefix} Hello,\n#{prefix} world!\n")
 
-        expect(ex.message).to eq('Hello,<br>world!')
+        expect(ex.message).to eq("Hello,\nworld!")
       end
     end
   end
-- 
GitLab