diff --git a/app/services/commits/create_service.rb b/app/services/commits/create_service.rb
index f96f2931508..4d0578becbe 100644
--- a/app/services/commits/create_service.rb
+++ b/app/services/commits/create_service.rb
@@ -17,7 +17,7 @@ module Commits
new_commit = create_commit!
success(result: new_commit)
- rescue ValidationError, ChangeError, Gitlab::Git::Index::IndexError, Gitlab::Git::CommitError, Gitlab::Git::HooksService::PreReceiveError => ex
+ rescue ValidationError, ChangeError, Gitlab::Git::Index::IndexError, Gitlab::Git::CommitError, Gitlab::Git::PreReceiveError => ex
error(ex.message)
end
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index 0ba6a0ac6b5..9b1a4d960e2 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -14,7 +14,7 @@ class CreateBranchService < BaseService
else
error('Invalid reference name')
end
- rescue Gitlab::Git::HooksService::PreReceiveError => ex
+ rescue Gitlab::Git::PreReceiveError => ex
error(ex.message)
end
diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb
index 1f059c31944..e1499dcee64 100644
--- a/app/services/delete_branch_service.rb
+++ b/app/services/delete_branch_service.rb
@@ -16,7 +16,7 @@ class DeleteBranchService < BaseService
else
error('Failed to remove branch')
end
- rescue Gitlab::Git::HooksService::PreReceiveError => ex
+ rescue Gitlab::Git::PreReceiveError => ex
error(ex.message)
end
diff --git a/app/services/merge_requests/ff_merge_service.rb b/app/services/merge_requests/ff_merge_service.rb
index ba6853b835a..bffc09c34f0 100644
--- a/app/services/merge_requests/ff_merge_service.rb
+++ b/app/services/merge_requests/ff_merge_service.rb
@@ -13,7 +13,7 @@ module MergeRequests
source,
merge_request.target_branch,
merge_request: merge_request)
- rescue Gitlab::Git::HooksService::PreReceiveError => e
+ rescue Gitlab::Git::PreReceiveError => e
raise MergeError, e.message
rescue StandardError => e
raise MergeError, "Something went wrong during merge: #{e.message}"
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 126da891c78..3d587f97906 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -79,7 +79,7 @@ module MergeRequests
message = params[:commit_message] || merge_request.merge_commit_message
repository.merge(current_user, source, merge_request, message)
- rescue Gitlab::Git::HooksService::PreReceiveError => e
+ rescue Gitlab::Git::PreReceiveError => e
handle_merge_error(log_message: e.message)
raise MergeError, 'Something went wrong during merge pre-receive hook'
rescue => e
diff --git a/app/services/tags/create_service.rb b/app/services/tags/create_service.rb
index cc76d0df3a1..3cc88d77ba1 100644
--- a/app/services/tags/create_service.rb
+++ b/app/services/tags/create_service.rb
@@ -13,7 +13,7 @@ module Tags
new_tag = repository.add_tag(current_user, tag_name, target, message)
rescue Gitlab::Git::Repository::TagExistsError
return error("Tag #{tag_name} already exists")
- rescue Gitlab::Git::HooksService::PreReceiveError => ex
+ rescue Gitlab::Git::PreReceiveError => ex
return error(ex.message)
end
diff --git a/app/services/tags/destroy_service.rb b/app/services/tags/destroy_service.rb
index d3d46064729..b84b061e460 100644
--- a/app/services/tags/destroy_service.rb
+++ b/app/services/tags/destroy_service.rb
@@ -21,7 +21,7 @@ module Tags
else
error('Failed to remove tag')
end
- rescue Gitlab::Git::HooksService::PreReceiveError => ex
+ rescue Gitlab::Git::PreReceiveError => ex
error(ex.message)
end
diff --git a/app/services/validate_new_branch_service.rb b/app/services/validate_new_branch_service.rb
index 7d1ed768ee8..643f2ce1481 100644
--- a/app/services/validate_new_branch_service.rb
+++ b/app/services/validate_new_branch_service.rb
@@ -13,7 +13,7 @@ class ValidateNewBranchService < BaseService
end
success
- rescue Gitlab::Git::HooksService::PreReceiveError => ex
+ rescue Gitlab::Git::PreReceiveError => ex
error(ex.message)
end
end
diff --git a/lib/gitlab/git/committer_with_hooks.rb b/lib/gitlab/git/committer_with_hooks.rb
index a8a59f998cd..4198be7c9c9 100644
--- a/lib/gitlab/git/committer_with_hooks.rb
+++ b/lib/gitlab/git/committer_with_hooks.rb
@@ -20,7 +20,7 @@ module Gitlab
end
result[:newrev]
- rescue Gitlab::Git::HooksService::PreReceiveError => e
+ rescue Gitlab::Git::PreReceiveError => e
message = "Custom Hook failed: #{e.message}"
raise Gitlab::Git::Wiki::OperationError, message
end
diff --git a/lib/gitlab/git/hook.rb b/lib/gitlab/git/hook.rb
index 7c201c6169b..94ff5b4980a 100644
--- a/lib/gitlab/git/hook.rb
+++ b/lib/gitlab/git/hook.rb
@@ -11,7 +11,7 @@ module Gitlab
def initialize(name, repository)
@name = name
@repository = repository
- @path = File.join(repo_path.strip, 'hooks', name)
+ @path = File.join(repo_path, 'hooks', name)
end
def repo_path
@@ -95,13 +95,13 @@ module Gitlab
args = [ref, oldrev, newrev]
stdout, stderr, status = Open3.capture3(env, path, *args, options)
- [status.success?, Gitlab::Utils.nlbr(stderr.presence || stdout)]
+ [status.success?, stderr.presence || stdout]
end
def retrieve_error_message(stderr, stdout)
err_message = stderr.read
err_message = err_message.blank? ? stdout.read : err_message
- Gitlab::Utils.nlbr(err_message)
+ err_message
end
end
end
diff --git a/lib/gitlab/git/hooks_service.rb b/lib/gitlab/git/hooks_service.rb
index f302b852b35..e67cacdb95a 100644
--- a/lib/gitlab/git/hooks_service.rb
+++ b/lib/gitlab/git/hooks_service.rb
@@ -1,8 +1,6 @@
module Gitlab
module Git
class HooksService
- PreReceiveError = Class.new(StandardError)
-
attr_accessor :oldrev, :newrev, :ref
def execute(pusher, repository, oldrev, newrev, ref)
diff --git a/lib/gitlab/git/pre_receive_error.rb b/lib/gitlab/git/pre_receive_error.rb
new file mode 100644
index 00000000000..ac1ab7c39d5
--- /dev/null
+++ b/lib/gitlab/git/pre_receive_error.rb
@@ -0,0 +1,21 @@
+module Gitlab
+ module Git
+ #
+ # PreReceiveError is special because its message gets displayed to users
+ # in the web UI. To prevent XSS we sanitize the message on
+ # initialization.
+ class PreReceiveError < StandardError
+ def initialize(msg = '')
+ super(nlbr(msg))
+ end
+
+ private
+
+ # In gitaly-ruby we override this method to do nothing, so that
+ # sanitization happens in gitlab-rails only.
+ def nlbr(str)
+ Gitlab::Utils.nlbr(str)
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb
index 44b0e517bf0..e9d4bb4c4b6 100644
--- a/lib/gitlab/gitaly_client/operation_service.rb
+++ b/lib/gitlab/gitaly_client/operation_service.rb
@@ -20,7 +20,7 @@ module Gitlab
response = GitalyClient.call(@repository.storage, :operation_service, :user_delete_tag, request)
if pre_receive_error = response.pre_receive_error.presence
- raise Gitlab::Git::HooksService::PreReceiveError, pre_receive_error
+ raise Gitlab::Git::PreReceiveError, pre_receive_error
end
end
@@ -35,7 +35,7 @@ module Gitlab
response = GitalyClient.call(@repository.storage, :operation_service, :user_create_tag, request)
if pre_receive_error = response.pre_receive_error.presence
- raise Gitlab::Git::HooksService::PreReceiveError, pre_receive_error
+ raise Gitlab::Git::PreReceiveError, pre_receive_error
elsif response.exists
raise Gitlab::Git::Repository::TagExistsError
end
@@ -56,7 +56,7 @@ module Gitlab
:user_create_branch, request)
if response.pre_receive_error.present?
- raise Gitlab::Git::HooksService::PreReceiveError.new(response.pre_receive_error)
+ raise Gitlab::Git::PreReceiveError.new(response.pre_receive_error)
end
branch = response.branch
@@ -76,7 +76,7 @@ module Gitlab
response = GitalyClient.call(@repository.storage, :operation_service, :user_delete_branch, request)
if pre_receive_error = response.pre_receive_error.presence
- raise Gitlab::Git::HooksService::PreReceiveError, pre_receive_error
+ raise Gitlab::Git::PreReceiveError, pre_receive_error
end
end
@@ -106,7 +106,7 @@ module Gitlab
second_response = response_enum.next
if second_response.pre_receive_error.present?
- raise Gitlab::Git::HooksService::PreReceiveError, second_response.pre_receive_error
+ raise Gitlab::Git::PreReceiveError, second_response.pre_receive_error
end
branch_update = second_response.branch_update
@@ -175,7 +175,7 @@ module Gitlab
)
if response.pre_receive_error.presence
- raise Gitlab::Git::HooksService::PreReceiveError, response.pre_receive_error
+ raise Gitlab::Git::PreReceiveError, response.pre_receive_error
elsif response.git_error.presence
raise Gitlab::Git::Repository::GitError, response.git_error
else
@@ -242,7 +242,7 @@ module Gitlab
:user_commit_files, req_enum, remote_storage: start_repository.storage)
if (pre_receive_error = response.pre_receive_error.presence)
- raise Gitlab::Git::HooksService::PreReceiveError, pre_receive_error
+ raise Gitlab::Git::PreReceiveError, pre_receive_error
end
if (index_error = response.index_error.presence)
@@ -280,7 +280,7 @@ module Gitlab
def handle_cherry_pick_or_revert_response(response)
if response.pre_receive_error.presence
- raise Gitlab::Git::HooksService::PreReceiveError, response.pre_receive_error
+ raise Gitlab::Git::PreReceiveError, response.pre_receive_error
elsif response.commit_error.presence
raise Gitlab::Git::CommitError, response.commit_error
elsif response.create_tree_error.presence
diff --git a/spec/features/tags/master_deletes_tag_spec.rb b/spec/features/tags/master_deletes_tag_spec.rb
index c0b4fa52526..9981bfa4609 100644
--- a/spec/features/tags/master_deletes_tag_spec.rb
+++ b/spec/features/tags/master_deletes_tag_spec.rb
@@ -38,7 +38,7 @@ feature 'Master deletes tag' do
context 'when Gitaly operation_user_delete_tag feature is enabled' do
before do
allow_any_instance_of(Gitlab::GitalyClient::OperationService).to receive(:rm_tag)
- .and_raise(Gitlab::Git::HooksService::PreReceiveError, 'Do not delete tags')
+ .and_raise(Gitlab::Git::PreReceiveError, 'Do not delete tags')
end
scenario 'shows the error message' do
@@ -51,7 +51,7 @@ feature 'Master deletes tag' do
context 'when Gitaly operation_user_delete_tag feature is disabled', :skip_gitaly_mock do
before do
allow_any_instance_of(Gitlab::Git::HooksService).to receive(:execute)
- .and_raise(Gitlab::Git::HooksService::PreReceiveError, 'Do not delete tags')
+ .and_raise(Gitlab::Git::PreReceiveError, 'Do not delete tags')
end
scenario 'shows the error message' do
diff --git a/spec/lib/gitlab/git/hook_spec.rb b/spec/lib/gitlab/git/hook_spec.rb
index 2fe1f5603ce..d9b3d0cf419 100644
--- a/spec/lib/gitlab/git/hook_spec.rb
+++ b/spec/lib/gitlab/git/hook_spec.rb
@@ -9,24 +9,31 @@ describe Gitlab::Git::Hook do
end
describe "#trigger" do
- let(:project) { create(:project, :repository) }
+ set(:project) { create(:project, :repository) }
let(:repository) { project.repository.raw_repository }
let(:repo_path) { repository.path }
+ let(:hooks_dir) { File.join(repo_path, 'hooks') }
let(:user) { create(:user) }
let(:gl_id) { Gitlab::GlId.gl_id(user) }
let(:gl_username) { user.username }
def create_hook(name)
- FileUtils.mkdir_p(File.join(repo_path, 'hooks'))
- File.open(File.join(repo_path, 'hooks', name), 'w', 0755) do |f|
- f.write('exit 0')
+ FileUtils.mkdir_p(hooks_dir)
+ hook_path = File.join(hooks_dir, name)
+ File.open(hook_path, 'w', 0755) do |f|
+ f.write(<<~HOOK)
+ #!/bin/sh
+ exit 0
+ HOOK
end
end
def create_failing_hook(name)
- FileUtils.mkdir_p(File.join(repo_path, 'hooks'))
- File.open(File.join(repo_path, 'hooks', name), 'w', 0755) do |f|
- f.write(<<-HOOK)
+ FileUtils.mkdir_p(hooks_dir)
+ hook_path = File.join(hooks_dir, name)
+ File.open(hook_path, 'w', 0755) do |f|
+ f.write(<<~HOOK)
+ #!/bin/sh
echo 'regular message from the hook'
echo 'error message from the hook' 1>&2
echo 'error message from the hook line 2' 1>&2
@@ -38,7 +45,7 @@ describe Gitlab::Git::Hook do
['pre-receive', 'post-receive', 'update'].each do |hook_name|
context "when triggering a #{hook_name} hook" do
context "when the hook is successful" do
- let(:hook_path) { File.join(repo_path, 'hooks', hook_name) }
+ let(:hook_path) { File.join(hooks_dir, hook_name) }
let(:gl_repository) { Gitlab::GlRepository.gl_repository(project, false) }
let(:env) do
{
@@ -76,7 +83,7 @@ describe Gitlab::Git::Hook do
status, errors = hook.trigger(gl_id, gl_username, blank, blank, ref)
expect(status).to be false
- expect(errors).to eq("error message from the hook
error message from the hook line 2
")
+ expect(errors).to eq("error message from the hook\nerror message from the hook line 2\n")
end
end
end
diff --git a/spec/lib/gitlab/git/hooks_service_spec.rb b/spec/lib/gitlab/git/hooks_service_spec.rb
index 3ed3feb4c74..9337aa39e13 100644
--- a/spec/lib/gitlab/git/hooks_service_spec.rb
+++ b/spec/lib/gitlab/git/hooks_service_spec.rb
@@ -26,24 +26,24 @@ describe Gitlab::Git::HooksService, seed_helper: true do
context 'when pre-receive hook failed' do
it 'does not call post-receive hook' do
- expect(service).to receive(:run_hook).with('pre-receive').and_return([false, ''])
+ expect(service).to receive(:run_hook).with('pre-receive').and_return([false, 'hello world'])
expect(service).not_to receive(:run_hook).with('post-receive')
expect do
service.execute(user, repository, blankrev, newrev, ref)
- end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+ end.to raise_error(Gitlab::Git::PreReceiveError, 'hello world')
end
end
context 'when update hook failed' do
it 'does not call post-receive hook' do
expect(service).to receive(:run_hook).with('pre-receive').and_return([true, nil])
- expect(service).to receive(:run_hook).with('update').and_return([false, ''])
+ expect(service).to receive(:run_hook).with('update').and_return([false, 'hello world'])
expect(service).not_to receive(:run_hook).with('post-receive')
expect do
service.execute(user, repository, blankrev, newrev, ref)
- end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+ end.to raise_error(Gitlab::Git::PreReceiveError, 'hello world')
end
end
end
diff --git a/spec/lib/gitlab/git/pre_receive_error_spec.rb b/spec/lib/gitlab/git/pre_receive_error_spec.rb
new file mode 100644
index 00000000000..1b8be62dec6
--- /dev/null
+++ b/spec/lib/gitlab/git/pre_receive_error_spec.rb
@@ -0,0 +1,9 @@
+require 'spec_helper'
+
+describe Gitlab::Git::PreReceiveError do
+ it 'makes its message HTML-friendly' do
+ ex = described_class.new("hello\nworld\n")
+
+ expect(ex.message).to eq('hello
world
')
+ end
+end
diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
index 9fbdd73ee0e..9709f1f5646 100644
--- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
@@ -48,7 +48,7 @@ describe Gitlab::GitalyClient::OperationService do
.and_return(response)
expect { subject }.to raise_error(
- Gitlab::Git::HooksService::PreReceiveError, "something failed")
+ Gitlab::Git::PreReceiveError, "something failed")
end
end
end
@@ -85,7 +85,7 @@ describe Gitlab::GitalyClient::OperationService do
.and_return(response)
expect { subject }.to raise_error(
- Gitlab::Git::HooksService::PreReceiveError, "something failed")
+ Gitlab::Git::PreReceiveError, "something failed")
end
end
end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 7c0a1cd967c..b6df048d4ca 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1195,7 +1195,7 @@ describe Repository do
Gitlab::Git::OperationService.new(git_user, repository.raw_repository).with_branch('feature') do
new_rev
end
- end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+ end.to raise_error(Gitlab::Git::PreReceiveError)
end
end
@@ -1938,13 +1938,13 @@ describe Repository do
context 'when pre hooks failed' do
before do
allow_any_instance_of(Gitlab::GitalyClient::OperationService)
- .to receive(:user_delete_branch).and_raise(Gitlab::Git::HooksService::PreReceiveError)
+ .to receive(:user_delete_branch).and_raise(Gitlab::Git::PreReceiveError)
end
it 'gets an error and does not delete the branch' do
expect do
repository.rm_branch(user, 'feature')
- end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+ end.to raise_error(Gitlab::Git::PreReceiveError)
expect(repository.find_branch('feature')).not_to be_nil
end
@@ -1980,7 +1980,7 @@ describe Repository do
expect do
repository.rm_branch(user, 'feature')
- end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+ end.to raise_error(Gitlab::Git::PreReceiveError)
end
it 'does not delete the branch' do
@@ -1988,7 +1988,7 @@ describe Repository do
expect do
repository.rm_branch(user, 'feature')
- end.to raise_error(Gitlab::Git::HooksService::PreReceiveError)
+ end.to raise_error(Gitlab::Git::PreReceiveError)
expect(repository.find_branch('feature')).not_to be_nil
end
end
diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb
index 5ef6365fcc9..28f56d19657 100644
--- a/spec/services/merge_requests/ff_merge_service_spec.rb
+++ b/spec/services/merge_requests/ff_merge_service_spec.rb
@@ -72,7 +72,7 @@ describe MergeRequests::FfMergeService do
it 'logs and saves error if there is an PreReceiveError exception' do
error_message = 'error message'
- allow(service).to receive(:repository).and_raise(Gitlab::Git::HooksService::PreReceiveError, error_message)
+ allow(service).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, error_message)
allow(service).to receive(:execute_hooks)
service.execute(merge_request)
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index dc30a9bccc1..ef2738ef504 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -226,7 +226,7 @@ describe MergeRequests::MergeService do
it 'logs and saves error if there is an PreReceiveError exception' do
error_message = 'error message'
- allow(service).to receive(:repository).and_raise(Gitlab::Git::HooksService::PreReceiveError, error_message)
+ allow(service).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, error_message)
allow(service).to receive(:execute_hooks)
service.execute(merge_request)
diff --git a/spec/services/tags/create_service_spec.rb b/spec/services/tags/create_service_spec.rb
index e7e9080b6b0..0cbe57352be 100644
--- a/spec/services/tags/create_service_spec.rb
+++ b/spec/services/tags/create_service_spec.rb
@@ -41,7 +41,7 @@ describe Tags::CreateService do
it 'returns an error' do
expect(repository).to receive(:add_tag)
.with(user, 'v1.1.0', 'master', 'Foo')
- .and_raise(Gitlab::Git::HooksService::PreReceiveError, 'something went wrong')
+ .and_raise(Gitlab::Git::PreReceiveError, 'something went wrong')
response = service.execute('v1.1.0', 'master', 'Foo')