From d973872072097152bf16ebb808a2c65912b9f3d0 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Sun, 29 Apr 2018 17:30:49 +0200
Subject: [PATCH] Save and expose only generic merge error

When an error occurs during merge, the error message is exposed to user
and it is also saved in DB. This error message may be user unfriendly
(as in !41820) and it could also expose a detailed backend information.

Instead of displaying the specific error message, only sanitized generic
message is displayed. This is potentially controversial change because
disadvantage is that user doesn't get specific reason of failure.

Additional changes:
* repository.merge including exceptions is is extracted into a
separate method to make things clearer
* update! is used instead of update so we don't silently ignore
an error

Related to !41857
---
 app/services/merge_requests/merge_service.rb  | 31 ++++++++++++-------
 .../unreleased/jprovazn-generic-error.yml     |  6 ++++
 .../merge_requests/merge_service_spec.rb      |  4 +--
 3 files changed, 28 insertions(+), 13 deletions(-)
 create mode 100644 changelogs/unreleased/jprovazn-generic-error.yml

diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index cedfcb50e09..2209a60a840 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -50,21 +50,30 @@ module MergeRequests
     end
 
     def commit
+      log_info("Git merge started on JID #{merge_jid}")
+      commit_id = try_merge
+
+      if commit_id
+        log_info("Git merge finished on JID #{merge_jid} commit #{commit_id}")
+      else
+        raise MergeError, 'Conflicts detected during merge'
+      end
+
+      merge_request.update!(merge_commit_sha: commit_id)
+    end
+
+    def try_merge
       message = params[:commit_message] || merge_request.merge_commit_message
 
-      log_info("Git merge started on JID #{merge_jid}")
-      commit_id = repository.merge(current_user, source, merge_request, message)
-      log_info("Git merge finished on JID #{merge_jid} commit #{commit_id}")
-
-      raise MergeError, 'Conflicts detected during merge' unless commit_id
-
-      merge_request.update(merge_commit_sha: commit_id)
+      repository.merge(current_user, source, merge_request, message)
     rescue Gitlab::Git::HooksService::PreReceiveError => e
-      raise MergeError, e.message
-    rescue StandardError => e
-      raise MergeError, "Something went wrong during merge: #{e.message}"
+      handle_merge_error(log_message: e.message)
+      raise MergeError, 'Something went wrong during merge pre-receive hook'
+    rescue => e
+      handle_merge_error(log_message: e.message)
+      raise MergeError, 'Something went wrong during merge'
     ensure
-      merge_request.update(in_progress_merge_commit_sha: nil)
+      merge_request.update!(in_progress_merge_commit_sha: nil)
     end
 
     def after_merge
diff --git a/changelogs/unreleased/jprovazn-generic-error.yml b/changelogs/unreleased/jprovazn-generic-error.yml
new file mode 100644
index 00000000000..ced3b84fe02
--- /dev/null
+++ b/changelogs/unreleased/jprovazn-generic-error.yml
@@ -0,0 +1,6 @@
+---
+title: Display only generic message on merge error to avoid exposing any potentially
+  sensitive or user unfriendly backend messages.
+merge_request:
+author:
+type: fixed
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index c38ddf4612b..e8568bf8bb3 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -219,7 +219,7 @@ describe MergeRequests::MergeService do
 
         service.execute(merge_request)
 
-        expect(merge_request.merge_error).to include(error_message)
+        expect(merge_request.merge_error).to include('Something went wrong during merge')
         expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
       end
 
@@ -231,7 +231,7 @@ describe MergeRequests::MergeService do
 
         service.execute(merge_request)
 
-        expect(merge_request.merge_error).to include(error_message)
+        expect(merge_request.merge_error).to include('Something went wrong during merge pre-receive hook')
         expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
       end