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
This commit is contained in:
parent
ee189fd511
commit
d973872072
|
@ -50,21 +50,30 @@ module MergeRequests
|
||||||
end
|
end
|
||||||
|
|
||||||
def commit
|
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
|
message = params[:commit_message] || merge_request.merge_commit_message
|
||||||
|
|
||||||
log_info("Git merge started on JID #{merge_jid}")
|
repository.merge(current_user, source, merge_request, message)
|
||||||
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)
|
|
||||||
rescue Gitlab::Git::HooksService::PreReceiveError => e
|
rescue Gitlab::Git::HooksService::PreReceiveError => e
|
||||||
raise MergeError, e.message
|
handle_merge_error(log_message: e.message)
|
||||||
rescue StandardError => e
|
raise MergeError, 'Something went wrong during merge pre-receive hook'
|
||||||
raise MergeError, "Something went wrong during merge: #{e.message}"
|
rescue => e
|
||||||
|
handle_merge_error(log_message: e.message)
|
||||||
|
raise MergeError, 'Something went wrong during merge'
|
||||||
ensure
|
ensure
|
||||||
merge_request.update(in_progress_merge_commit_sha: nil)
|
merge_request.update!(in_progress_merge_commit_sha: nil)
|
||||||
end
|
end
|
||||||
|
|
||||||
def after_merge
|
def after_merge
|
||||||
|
|
|
@ -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
|
|
@ -219,7 +219,7 @@ describe MergeRequests::MergeService do
|
||||||
|
|
||||||
service.execute(merge_request)
|
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))
|
expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -231,7 +231,7 @@ describe MergeRequests::MergeService do
|
||||||
|
|
||||||
service.execute(merge_request)
|
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))
|
expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue