Don't use Gitlab::Utils.nlbr in Gitlab::Git

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

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

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

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}"

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

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

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

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

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

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

lib/gitlab/git/hooks_service.rb

 module Gitlab
   module Git
     class HooksService
-      PreReceiveError = Class.new(StandardError)
-
       attr_accessor :oldrev, :newrev, :ref
       def execute(pusher, repository, oldrev, newrev, ref)

lib/gitlab/git/pre_receive_error.rb

+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

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

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

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

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

spec/lib/gitlab/git/pre_receive_error_spec.rb

+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

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

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

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)