Use 'git update-ref' for safer web commits

CHANGELOG

@@ -27,6 +27,7 @@ v 8.12.0 (unreleased)
   - Remove prefixes from transition CSS property (ClemMakesApps)
   - Add Sentry logging to API calls
   - Add BroadcastMessage API
+  - Use 'git update-ref' for safer web commits !6130
   - Automatically expand hidden discussions when accessed by a permalink !5585 (Mike Greiling)
   - Remove unused mixins (ClemMakesApps)
   - Add search to all issue board lists

app/models/repository.rb

@@ -149,7 +149,7 @@ class Repository
     return false unless target
 
     GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do
-      rugged.branches.create(branch_name, target)
+      update_ref!(ref, target, oldrev)
     end
 
     after_create_branch
@@ -181,7 +181,7 @@ class Repository
     ref    = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
 
     GitHooksService.new.execute(user, path_to_repo, oldrev, newrev, ref) do
-      rugged.branches.delete(branch_name)
+      update_ref!(ref, newrev, oldrev)
     end
 
     after_remove_branch
@@ -215,6 +215,21 @@ class Repository
     rugged.references.exist?(ref)
   end
 
+  def update_ref!(name, newrev, oldrev)
+    # We use 'git update-ref' because libgit2/rugged currently does not
+    # offer 'compare and swap' ref updates. Without compare-and-swap we can
+    # (and have!) accidentally reset the ref to an earlier state, clobbering
+    # commits. See also https://github.com/libgit2/libgit2/issues/1534.
+    command = %w[git update-ref --stdin -z]
+    output, status = Gitlab::Popen.popen(command, path_to_repo) do |stdin|
+      stdin.write("update #{name}\x00#{newrev}\x00#{oldrev}\x00")
+    end
+
+    return if status.zero?
+
+    raise CommitError.new("error updating ref #{name} #{oldrev}->#{newrev}\n#{output}")
+  end
+
   # Makes sure a commit is kept around when Git garbage collection runs.
   # Git GC will delete commits from the repository that are no longer in any
   # branches or tags, but we want to keep some of these commits around, for
@@ -1014,15 +1029,10 @@ class Repository
   def commit_with_hooks(current_user, branch)
     update_autocrlf_option
 
-    oldrev = Gitlab::Git::BLANK_SHA
     ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
     target_branch = find_branch(branch)
     was_empty = empty?
 
-    if !was_empty && target_branch
-      oldrev = target_branch.target.id
-    end
-
     # Make commit
     newrev = yield(ref)
 
@@ -1030,24 +1040,15 @@ class Repository
       raise CommitError.new('Failed to create commit')
     end
 
-    GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do
-      if was_empty || !target_branch
-        # Create branch
-        rugged.references.create(ref, newrev)
+    oldrev = rugged.lookup(newrev).parent_ids.first || Gitlab::Git::BLANK_SHA
 
+    GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do
+      update_ref!(ref, newrev, oldrev)
+      
+      if was_empty || !target_branch
         # If repo was empty expire cache
         after_create if was_empty
         after_create_branch
-      else
-        # Update head
-        current_head = find_branch(branch).target.id
-
-        # Make sure target branch was not changed during pre-receive hook
-        if current_head == oldrev
-          rugged.references.update(ref, newrev)
-        else
-          raise CommitError.new('Commit was rejected because branch received new push')
-        end
       end
     end
 

lib/gitlab/popen.rb

@@ -21,9 +21,9 @@ module Gitlab
       @cmd_output = ""
       @cmd_status = 0
       Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
-        # We are not using stdin so we should close it, in case the command we
-        # are running waits for input.
+        yield(stdin) if block_given?
         stdin.close
+
         @cmd_output << stdout.read
         @cmd_output << stderr.read
         @cmd_status = wait_thr.value.exitstatus

spec/lib/gitlab/popen_spec.rb

@@ -40,4 +40,13 @@ describe 'Gitlab::Popen', lib: true, no_db: true do
     it { expect(@status).to be_zero }
     it { expect(@output).to include('spec') }
   end
+
+  context 'use stdin' do
+    before do
+      @output, @status = @klass.new.popen(%w[cat]) { |stdin| stdin.write 'hello' }
+    end
+  
+    it { expect(@status).to be_zero }
+    it { expect(@output).to eq('hello') }
+  end
 end

spec/models/repository_spec.rb

@@ -443,31 +443,32 @@ describe Repository, models: true do
 
   describe '#commit_with_hooks' do
     let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
+    let(:new_rev) { 'a74ae73c1ccde9b974a70e82b901588071dc142a' } # commit whose parent is old_rev
 
     context 'when pre hooks were successful' do
       before do
         expect_any_instance_of(GitHooksService).to receive(:execute).
-          with(user, repository.path_to_repo, old_rev, sample_commit.id, 'refs/heads/feature').
+          with(user, repository.path_to_repo, old_rev, new_rev, 'refs/heads/feature').
           and_yield.and_return(true)
       end
 
       it 'runs without errors' do
         expect do
-          repository.commit_with_hooks(user, 'feature') { sample_commit.id }
+          repository.commit_with_hooks(user, 'feature') { new_rev }
         end.not_to raise_error
       end
 
       it 'ensures the autocrlf Git option is set to :input' do
         expect(repository).to receive(:update_autocrlf_option)
 
-        repository.commit_with_hooks(user, 'feature') { sample_commit.id }
+        repository.commit_with_hooks(user, 'feature') { new_rev }
       end
 
       context "when the branch wasn't empty" do
         it 'updates the head' do
           expect(repository.find_branch('feature').target.id).to eq(old_rev)
-          repository.commit_with_hooks(user, 'feature') { sample_commit.id }
-          expect(repository.find_branch('feature').target.id).to eq(sample_commit.id)
+          repository.commit_with_hooks(user, 'feature') { new_rev }
+          expect(repository.find_branch('feature').target.id).to eq(new_rev)
         end
       end
     end
@@ -477,7 +478,7 @@ describe Repository, models: true do
         allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
-          repository.commit_with_hooks(user, 'feature') { sample_commit.id }
+          repository.commit_with_hooks(user, 'feature') { new_rev }
         end.to raise_error(GitHooksService::PreReceiveError)
       end
     end
@@ -485,6 +486,7 @@ describe Repository, models: true do
     context 'when target branch is different from source branch' do
       before do
         allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, ''])
+        allow(repository).to receive(:update_ref!)
       end
 
       it 'expires branch cache' do
@@ -495,7 +497,7 @@ describe Repository, models: true do
         expect(repository).to     receive(:expire_has_visible_content_cache)
         expect(repository).to     receive(:expire_branch_count_cache)
 
-        repository.commit_with_hooks(user, 'new-feature') { sample_commit.id }
+        repository.commit_with_hooks(user, 'new-feature') { new_rev }
       end
     end
 
@@ -1268,4 +1270,18 @@ describe Repository, models: true do
       File.delete(path)
     end
   end
+
+  describe '#update_ref!' do
+    it 'can create a ref' do
+      repository.update_ref!('refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+
+      expect(repository.find_branch('foobar')).not_to be_nil
+    end
+
+    it 'raises CommitError when the ref update fails' do
+      expect do
+        repository.update_ref!('refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+      end.to raise_error(Repository::CommitError)
+    end
+  end
 end