Commit f82380b9 authored by Luke Duncalfe's avatar Luke Duncalfe

Allow custom hooks errors to appear in GitLab UI

Error messages from custom pre-receive hooks now appear in the GitLab
UI.

This is re-enabling a feature that had been disabled in merge request
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18646

The feature had been disabled due to security concerns that information
which was not intended to be public (like stack traces) would leak into
public view.

PreReceiveErrors (from pre-receive, post-receive and update custom
hooks) are now filtered for messages that have been prefixed in a
particular way.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/48132
parent a3b3da72
...@@ -80,7 +80,7 @@ export default { ...@@ -80,7 +80,7 @@ export default {
<status-icon :show-disabled-button="true" status="warning" /> <status-icon :show-disabled-button="true" status="warning" />
<div class="media-body space-children"> <div class="media-body space-children">
<span class="bold"> <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 v-else> {{ s__('mrWidget|Merge failed.') }} </span>
<span :class="{ 'has-custom-error': mr.mergeError }"> {{ timerText }} </span> <span :class="{ 'has-custom-error': mr.mergeError }"> {{ timerText }} </span>
</span> </span>
......
...@@ -76,8 +76,8 @@ module MergeRequests ...@@ -76,8 +76,8 @@ module MergeRequests
def try_merge def try_merge
repository.merge(current_user, source, merge_request, commit_message) repository.merge(current_user, source, merge_request, commit_message)
rescue Gitlab::Git::PreReceiveError => e rescue Gitlab::Git::PreReceiveError => e
handle_merge_error(log_message: e.message) raise MergeError,
raise_error('Something went wrong during merge pre-receive hook') "Something went wrong during merge pre-receive hook. #{e.message}".strip
rescue => e rescue => e
handle_merge_error(log_message: e.message) handle_merge_error(log_message: e.message)
raise_error('Something went wrong during merge') raise_error('Something went wrong during merge')
......
...@@ -51,7 +51,7 @@ Hooks can be also placed in `hooks/<hook_name>.d` (global) or ...@@ -51,7 +51,7 @@ Hooks can be also placed in `hooks/<hook_name>.d` (global) or
execution of the hooks. execution of the hooks.
NOTE: **Note:** `<hook_name>.d` would need to be either `pre-receive.d`, 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 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 `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. ...@@ -76,9 +76,21 @@ first script exiting with a non-zero value.
> [Introduced][5073] in GitLab 8.10. > [Introduced][5073] in GitLab 8.10.
If the commit is declined or an error occurs during the Git hook check, To have custom error messages appear in GitLab's UI when the commit is
the STDERR or STDOUT message of the hook will be present in GitLab's UI. declined or an error occurs during the Git hook, your script should:
STDERR takes precedence over STDOUT.
- 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) ![Custom message from custom Git hook](img/custom_hooks_error_msg.png)
......
...@@ -4,19 +4,38 @@ module Gitlab ...@@ -4,19 +4,38 @@ module Gitlab
module Git module Git
# #
# PreReceiveError is special because its message gets displayed to users # PreReceiveError is special because its message gets displayed to users
# in the web UI. To prevent XSS we sanitize the message on # in the web UI. Because of this, we:
# initialization. # - 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 class PreReceiveError < StandardError
def initialize(msg = '') SAFE_MESSAGE_PREFIXES = [
super(nlbr(msg)) '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 end
private private
# In gitaly-ruby we override this method to do nothing, so that # In gitaly-ruby we override this method to do nothing, so that
# sanitization happens in gitlab-rails only. # sanitization happens in gitlab-rails only.
def nlbr(str) def sanitize(message)
Gitlab::Utils.nlbr(str) 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 end
end end
......
...@@ -37,7 +37,7 @@ describe 'Maintainer deletes tag' do ...@@ -37,7 +37,7 @@ describe 'Maintainer deletes tag' do
context 'when pre-receive hook fails', :js do context 'when pre-receive hook fails', :js do
before do before do
allow_any_instance_of(Gitlab::GitalyClient::OperationService).to receive(:rm_tag) 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 end
it 'shows the error message' do it 'shows the error message' do
......
...@@ -120,7 +120,7 @@ describe('MRWidgetFailedToMerge', () => { ...@@ -120,7 +120,7 @@ describe('MRWidgetFailedToMerge', () => {
it('renders given error', () => { it('renders given error', () => {
expect(vm.$el.querySelector('.has-error-message').textContent.trim()).toEqual( expect(vm.$el.querySelector('.has-error-message').textContent.trim()).toEqual(
'Merge error happened.', 'Merge error happened',
); );
}); });
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::Git::PreReceiveError do describe Gitlab::Git::PreReceiveError do
it 'makes its message HTML-friendly' do Gitlab::Git::PreReceiveError::SAFE_MESSAGE_PREFIXES.each do |prefix|
ex = described_class.new("hello\nworld\n") 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<br>world<br>') 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
end end
...@@ -39,7 +39,7 @@ describe Gitlab::GitalyClient::OperationService do ...@@ -39,7 +39,7 @@ describe Gitlab::GitalyClient::OperationService do
context "when pre_receive_error is present" do context "when pre_receive_error is present" do
let(:response) do let(:response) do
Gitaly::UserCreateBranchResponse.new(pre_receive_error: "something failed") Gitaly::UserCreateBranchResponse.new(pre_receive_error: "GitLab: something failed")
end end
it "throws a PreReceive exception" do it "throws a PreReceive exception" do
...@@ -80,7 +80,7 @@ describe Gitlab::GitalyClient::OperationService do ...@@ -80,7 +80,7 @@ describe Gitlab::GitalyClient::OperationService do
context "when pre_receive_error is present" do context "when pre_receive_error is present" do
let(:response) do let(:response) do
Gitaly::UserUpdateBranchResponse.new(pre_receive_error: "something failed") Gitaly::UserUpdateBranchResponse.new(pre_receive_error: "GitLab: something failed")
end end
it "throws a PreReceive exception" do it "throws a PreReceive exception" do
...@@ -117,7 +117,7 @@ describe Gitlab::GitalyClient::OperationService do ...@@ -117,7 +117,7 @@ describe Gitlab::GitalyClient::OperationService do
context "when pre_receive_error is present" do context "when pre_receive_error is present" do
let(:response) do let(:response) do
Gitaly::UserDeleteBranchResponse.new(pre_receive_error: "something failed") Gitaly::UserDeleteBranchResponse.new(pre_receive_error: "GitLab: something failed")
end end
it "throws a PreReceive exception" do it "throws a PreReceive exception" do
...@@ -175,7 +175,7 @@ describe Gitlab::GitalyClient::OperationService do ...@@ -175,7 +175,7 @@ describe Gitlab::GitalyClient::OperationService do
shared_examples 'cherry pick and revert errors' do shared_examples 'cherry pick and revert errors' do
context 'when a pre_receive_error is present' 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 it 'raises a PreReceiveError' do
expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed") expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
...@@ -313,7 +313,7 @@ describe Gitlab::GitalyClient::OperationService do ...@@ -313,7 +313,7 @@ describe Gitlab::GitalyClient::OperationService do
end end
context 'when a pre_receive_error is present' do 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 it 'raises a PreReceiveError' do
expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed") expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
......
...@@ -72,7 +72,7 @@ describe MergeRequests::FfMergeService do ...@@ -72,7 +72,7 @@ describe MergeRequests::FfMergeService do
it 'logs and saves error if there is an PreReceiveError exception' do it 'logs and saves error if there is an PreReceiveError exception' do
error_message = 'error message' 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) allow(service).to receive(:execute_hooks)
service.execute(merge_request) service.execute(merge_request)
......
...@@ -239,7 +239,7 @@ describe MergeRequests::MergeService do ...@@ -239,7 +239,7 @@ describe MergeRequests::MergeService do
it 'logs and saves error if there is an PreReceiveError exception' do it 'logs and saves error if there is an PreReceiveError exception' do
error_message = 'error message' 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) allow(service).to receive(:execute_hooks)
service.execute(merge_request) service.execute(merge_request)
......
...@@ -41,7 +41,7 @@ describe Tags::CreateService do ...@@ -41,7 +41,7 @@ describe Tags::CreateService do
it 'returns an error' do it 'returns an error' do
expect(repository).to receive(:add_tag) expect(repository).to receive(:add_tag)
.with(user, 'v1.1.0', 'master', 'Foo') .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') response = service.execute('v1.1.0', 'master', 'Foo')
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment