Merge branch 'jprovazn-generic-error' into 'master'

Save and expose only generic merge error

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

3 files changed, 27 insertions, 12 deletions

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
-      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}")
+      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
 
-      raise MergeError, 'Conflicts detected during merge' unless commit_id
+      merge_request.update!(merge_commit_sha: commit_id)
+    end
+
+    def try_merge
+      message = params[:commit_message] || merge_request.merge_commit_message
 
-      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