diff options
22 files changed, 82 insertions, 47 deletions
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<br>error message from the hook line 2<br>") + 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<br>world<br>') + 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') |