Merge branch 'bvl-validate-sha-when-merging' into 'master'

Validate the sha when merging all the way down

See merge request gitlab-org/gitlab!19579

app/controllers/projects/merge_requests_controller.rb

@@ -255,7 +255,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   end
 
   def merge_params_attributes
-    [:should_remove_source_branch, :commit_message, :squash_commit_message, :squash, :auto_merge_strategy]
+    MergeRequest::KNOWN_MERGE_PARAMS
   end
 
   def auto_merge_requested?
@@ -295,7 +295,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
     return :sha_mismatch if params[:sha] != @merge_request.diff_head_sha
 
-    @merge_request.update(merge_error: nil, squash: merge_params.fetch(:squash, false))
+    @merge_request.update(merge_error: nil, squash: params.fetch(:squash, false))
 
     if auto_merge_requested?
       if merge_request.auto_merge_enabled?

app/services/merge_requests/merge_base_service.rb

@@ -19,10 +19,12 @@ module MergeRequests
     end
 
     def source
-      if merge_request.squash
-        squash_sha!
-      else
-        merge_request.diff_head_sha
+      strong_memoize(:source) do
+        if merge_request.squash
+          squash_sha!
+        else
+          merge_request.diff_head_sha
+        end
       end
     end
 
@@ -58,16 +60,14 @@ module MergeRequests
     end
 
     def squash_sha!
-      strong_memoize(:squash_sha) do
-        params[:merge_request] = merge_request
-        squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute
-
-        case squash_result[:status]
-        when :success
-          squash_result[:squash_sha]
-        when :error
-          raise ::MergeRequests::MergeService::MergeError, squash_result[:message]
-        end
+      params[:merge_request] = merge_request
+      squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute
+
+      case squash_result[:status]
+      when :success
+        squash_result[:squash_sha]
+      when :error
+        raise ::MergeRequests::MergeService::MergeError, squash_result[:message]
       end
     end
   end

app/services/merge_requests/merge_service.rb

@@ -37,6 +37,7 @@ module MergeRequests
     def validate!
       authorization_check!
       error_check!
+      updated_check!
     end
 
     def authorization_check!
@@ -60,6 +61,15 @@ module MergeRequests
       raise_error(error) if error
     end
 
+    def updated_check!
+      return unless Feature.enabled?(:validate_merge_sha, merge_request.target_project, default_enabled: false)
+
+      unless source_matches?
+        raise_error('Branch has been updated since the merge was requested. '\
+                    'Please review the changes.')
+      end
+    end
+
     def commit
       log_info("Git merge started on JID #{merge_jid}")
       commit_id = try_merge
@@ -125,5 +135,11 @@ module MergeRequests
     def merge_request_info
       merge_request.to_reference(full: true)
     end
+
+    def source_matches?
+      # params-keys are symbols coming from the controller, but when they get
+      # loaded from the database they're strings
+      params.with_indifferent_access[:sha] == merge_request.diff_head_sha
+    end
   end
 end

app/services/merge_requests/update_service.rb

@@ -88,9 +88,9 @@ module MergeRequests
       merge_request.update(merge_error: nil)
 
       if merge_request.head_pipeline && merge_request.head_pipeline.active?
-        AutoMergeService.new(project, current_user).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
+        AutoMergeService.new(project, current_user, { sha: last_diff_sha }).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
       else
-        merge_request.merge_async(current_user.id, {})
+        merge_request.merge_async(current_user.id, { sha: last_diff_sha })
       end
     end
 

ee/spec/features/merge_trains/two_merge_requests_on_train_spec.rb

@@ -39,10 +39,10 @@ describe 'Two merge requests on a merge train' do
     allow(merge_request_1).to receive(:actual_head_pipeline) { head_pipeline }
     allow(merge_request_2).to receive(:actual_head_pipeline) { head_pipeline }
 
-    AutoMergeService.new(project, maintainer_1)
+    AutoMergeService.new(project, maintainer_1, { sha: merge_request_1.diff_head_sha })
       .execute(merge_request_1, AutoMergeService::STRATEGY_MERGE_TRAIN)
-    AutoMergeService.new(project, maintainer_2)
-      .execute(merge_request_2, AutoMergeService::STRATEGY_MERGE_TRAIN)
+    AutoMergeService.new(project, maintainer_2, { sha: merge_request_2.diff_head_sha })
+      .execute(merge_request_2, AutoMergeService::STRATEGY_MERGE_TRAIN )
 
     merge_request_1.reload
     merge_request_2.reload

ee/spec/services/merge_requests/merge_service_spec.rb

@@ -6,7 +6,7 @@ describe MergeRequests::MergeService do
   let(:user) { create(:user) }
   let(:merge_request) { create(:merge_request, :simple) }
   let(:project) { merge_request.project }
-  let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
+  let(:service) { described_class.new(project, user, sha: merge_request.diff_head_sha, commit_message: 'Awesome message') }
 
   before do
     project.add_maintainer(user)

lib/api/merge_requests.rb

@@ -407,7 +407,8 @@ module API
         merge_params = HashWithIndifferentAccess.new(
           commit_message: params[:merge_commit_message],
           squash_commit_message: params[:squash_commit_message],
-          should_remove_source_branch: params[:should_remove_source_branch]
+          should_remove_source_branch: params[:should_remove_source_branch],
+          sha: params[:sha] || merge_request.diff_head_sha
         )
 
         if merge_when_pipeline_succeeds && merge_request.head_pipeline && merge_request.head_pipeline.active?

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -405,7 +405,7 @@ describe Projects::MergeRequestsController do
       end
 
       it 'starts the merge immediately with permitted params' do
-        expect(MergeWorker).to receive(:perform_async).with(merge_request.id, anything, { 'squash' => false })
+        expect(MergeWorker).to receive(:perform_async).with(merge_request.id, anything, { 'sha' => merge_request.diff_head_sha })
 
         merge_with_sha
       end
@@ -432,9 +432,14 @@ describe Projects::MergeRequestsController do
         let(:message) { 'My custom squash commit message' }
 
         it 'passes the same message to SquashService', :sidekiq_might_not_need_inline do
-          params = { squash: '1', squash_commit_message: message }
-
-          expect_next_instance_of(MergeRequests::SquashService, project, user, params.merge(merge_request: merge_request)) do |squash_service|
+          params = { squash: '1',
+                     squash_commit_message: message,
+                     sha: merge_request.diff_head_sha }
+          expected_squash_params = { squash_commit_message: message,
+                                     sha: merge_request.diff_head_sha,
+                                     merge_request: merge_request }
+
+          expect_next_instance_of(MergeRequests::SquashService, project, user, expected_squash_params) do |squash_service|
             expect(squash_service).to receive(:execute).and_return({
               status: :success,
               squash_sha: SecureRandom.hex(20)

spec/factories/merge_requests.rb

@@ -100,6 +100,7 @@ FactoryBot.define do
       auto_merge_enabled { true }
       auto_merge_strategy { AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS }
       merge_user { author }
+      merge_params { { sha: diff_head_sha } }
     end
 
     trait :remove_source_branch do

spec/features/merge_request/user_sees_cherry_pick_modal_spec.rb

@@ -15,7 +15,7 @@ describe 'Merge request > User cherry-picks', :js do
 
   context 'Viewing a merged merge request' do
     before do
-      service = MergeRequests::MergeService.new(project, user)
+      service = MergeRequests::MergeService.new(project, user, sha: merge_request.diff_head_sha)
 
       perform_enqueued_jobs do
         service.execute(merge_request)

spec/models/merge_request_spec.rb

@@ -1934,7 +1934,7 @@ describe MergeRequest do
     context 'when the MR has been merged' do
       before do
         MergeRequests::MergeService
-          .new(subject.target_project, subject.author)
+          .new(subject.target_project, subject.author, { sha: subject.diff_head_sha })
           .execute(subject)
       end
 

spec/services/merge_requests/ff_merge_service_spec.rb

@@ -13,6 +13,7 @@ describe MergeRequests::FfMergeService do
            author: create(:user))
   end
   let(:project) { merge_request.project }
+  let(:valid_merge_params) { { sha: merge_request.diff_head_sha } }
 
   before do
     project.add_maintainer(user)
@@ -21,7 +22,7 @@ describe MergeRequests::FfMergeService do
 
   describe '#execute' do
     context 'valid params' do
-      let(:service) { described_class.new(project, user, {}) }
+      let(:service) { described_class.new(project, user, valid_merge_params) }
 
       before do
         allow(service).to receive(:execute_hooks)
@@ -52,8 +53,8 @@ describe MergeRequests::FfMergeService do
       end
     end
 
-    context "error handling" do
-      let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
+    context 'error handling' do
+      let(:service) { described_class.new(project, user, valid_merge_params.merge(commit_message: 'Awesome message')) }
 
       before do
         allow(Rails.logger).to receive(:error)

spec/services/merge_requests/merge_service_spec.rb

@@ -14,9 +14,12 @@ describe MergeRequests::MergeService do
   end
 
   describe '#execute' do
-    context 'valid params' do
-      let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
+    let(:service) { described_class.new(project, user, merge_params) }
+    let(:merge_params) do
+      { commit_message: 'Awesome message', sha: merge_request.diff_head_sha }
+    end
 
+    context 'valid params' do
       before do
         allow(service).to receive(:execute_hooks)
 
@@ -38,11 +41,80 @@ describe MergeRequests::MergeService do
         note = merge_request.notes.last
         expect(note.note).to include 'merged'
       end
+
+      context 'when squashing' do
+        let(:merge_params) do
+          { commit_message: 'Merge commit message',
+            squash_commit_message: 'Squash commit message',
+            sha: merge_request.diff_head_sha }
+        end
+
+        let(:merge_request) do
+          # A merge reqeust with 5 commits
+          create(:merge_request, :simple,
+                 author: user2,
+                 assignees: [user2],
+                 squash: true,
+                 source_branch: 'improve/awesome',
+                 target_branch: 'fix')
+        end
+
+        it 'merges the merge request with squashed commits' do
+          expect(merge_request).to be_merged
+
+          merge_commit = merge_request.merge_commit
+          squash_commit = merge_request.merge_commit.parents.last
+
+          expect(merge_commit.message).to eq('Merge commit message')
+          expect(squash_commit.message).to eq("Squash commit message\n")
+        end
+      end
     end
 
-    context 'closes related issues' do
-      let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
+    context 'when an invalid sha is passed' do
+      let(:merge_request) do
+        create(:merge_request, :simple,
+               author: user2,
+               assignees: [user2],
+               squash: true,
+               source_branch: 'improve/awesome',
+               target_branch: 'fix')
+      end
+
+      let(:merge_params) do
+        { sha: merge_request.commits.second.sha }
+      end
+
+      it 'does not merge the MR' do
+        service.execute(merge_request)
+
+        expect(merge_request).not_to be_merged
+        expect(merge_request.merge_error).to match(/Branch has been updated/)
+      end
+    end
+
+    context 'when the `sha` param is missing' do
+      let(:merge_params) { {} }
+
+      it 'returns the error' do
+        merge_error = 'Branch has been updated since the merge was requested. '\
+                      'Please review the changes.'
+
+        expect { service.execute(merge_request) }
+          .to change { merge_request.merge_error }
+                .from(nil).to(merge_error)
+      end
+
+      it 'merges the MR when the feature is disabled' do
+        stub_feature_flags(validate_merge_sha: false)
 
+        service.execute(merge_request)
+
+        expect(merge_request).to be_merged
+      end
+    end
+
+    context 'closes related issues' do
       before do
         allow(project).to receive(:default_branch).and_return(merge_request.target_branch)
       end
@@ -83,12 +155,12 @@ describe MergeRequests::MergeService do
           service.execute(merge_request)
         end
 
-        context "when jira_issue_transition_id is not present" do
+        context 'when jira_issue_transition_id is not present' do
           before do
             allow_any_instance_of(JIRA::Resource::Issue).to receive(:resolution).and_return(nil)
           end
 
-          it "does not close issue" do
+          it 'does not close issue' do
             allow(jira_tracker).to receive_messages(jira_issue_transition_id: nil)
 
             expect_any_instance_of(JiraService).not_to receive(:transition_issue)
@@ -97,7 +169,7 @@ describe MergeRequests::MergeService do
           end
         end
 
-        context "wrong issue markdown" do
+        context 'wrong issue markdown' do
           it 'does not close issues on Jira issue tracker' do
             jira_issue = ExternalIssue.new('#JIRA-123', project)
             stub_jira_urls(jira_issue)
@@ -115,7 +187,7 @@ describe MergeRequests::MergeService do
     context 'closes related todos' do
       let(:merge_request) { create(:merge_request, assignees: [user], author: user) }
       let(:project) { merge_request.project }
-      let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
+
       let!(:todo) do
         create(:todo, :assigned,
           project: project,
@@ -139,7 +211,7 @@ describe MergeRequests::MergeService do
     context 'source branch removal' do
       context 'when the source branch is protected' do
         let(:service) do
-          described_class.new(project, user, 'should_remove_source_branch' => true)
+          described_class.new(project, user, merge_params.merge('should_remove_source_branch' => true))
         end
 
         before do
@@ -154,7 +226,7 @@ describe MergeRequests::MergeService do
 
       context 'when the source branch is the default branch' do
         let(:service) do
-          described_class.new(project, user, 'should_remove_source_branch' => true)
+          described_class.new(project, user, merge_params.merge('should_remove_source_branch' => true))
         end
 
         before do
@@ -169,8 +241,6 @@ describe MergeRequests::MergeService do
 
       context 'when the source branch can be removed' do
         context 'when MR author set the source branch to be removed' do
-          let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
-
           before do
             merge_request.update_attribute(:merge_params, { 'force_remove_source_branch' => '1' })
           end
@@ -183,7 +253,7 @@ describe MergeRequests::MergeService do
           end
 
           context 'when the merger set the source branch not to be removed' do
-            let(:service) { described_class.new(project, user, commit_message: 'Awesome message', 'should_remove_source_branch' => false) }
+            let(:service) { described_class.new(project, user, merge_params.merge('should_remove_source_branch' => false)) }
 
             it 'does not delete the source branch' do
               expect(DeleteBranchService).not_to receive(:new)
@@ -194,7 +264,7 @@ describe MergeRequests::MergeService do
 
         context 'when MR merger set the source branch to be removed' do
           let(:service) do
-            described_class.new(project, user, commit_message: 'Awesome message', 'should_remove_source_branch' => true)
+            described_class.new(project, user, merge_params.merge('should_remove_source_branch' => true))
           end
 
           it 'removes the source branch using the current user' do
@@ -207,9 +277,7 @@ describe MergeRequests::MergeService do
       end
     end
 
-    context "error handling" do
-      let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
-
+    context 'error handling' do
       before do
         allow(Rails.logger).to receive(:error)
       end
@@ -230,7 +298,7 @@ describe MergeRequests::MergeService do
       it 'logs and saves error if there is an exception' do
         error_message = 'error message'
 
-        allow(service).to receive(:repository).and_raise("error message")
+        allow(service).to receive(:repository).and_raise('error message')
         allow(service).to receive(:execute_hooks)
 
         service.execute(merge_request)
@@ -310,7 +378,7 @@ describe MergeRequests::MergeService do
           expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message))
         end
 
-        context "when fast-forward merge is not allowed" do
+        context 'when fast-forward merge is not allowed' do
           before do
             allow_any_instance_of(Repository).to receive(:ancestor?).and_return(nil)
           end

spec/services/merge_requests/merge_to_ref_service_spec.rb

@@ -76,7 +76,7 @@ describe MergeRequests::MergeToRefService do
       described_class.new(project, user, **params)
     end
 
-    let(:params) { { commit_message: 'Awesome message', should_remove_source_branch: true } }
+    let(:params) { { commit_message: 'Awesome message', should_remove_source_branch: true, sha: merge_request.diff_head_sha } }
 
     def process_merge_to_ref
       perform_enqueued_jobs do
@@ -103,7 +103,7 @@ describe MergeRequests::MergeToRefService do
       end
 
       let(:merge_service) do
-        MergeRequests::MergeService.new(project, user, {})
+        MergeRequests::MergeService.new(project, user, { sha: merge_request.diff_head_sha })
       end
 
       context 'when merge commit' do
@@ -205,7 +205,7 @@ describe MergeRequests::MergeToRefService do
     end
 
     context 'when target ref is passed as a parameter' do
-      let(:params) { { commit_message: 'merge train', target_ref: target_ref } }
+      let(:params) { { commit_message: 'merge train', target_ref: target_ref, sha: merge_request.diff_head_sha } }
 
       it_behaves_like 'successfully merges to ref with merge method' do
         let(:first_parent_ref) { 'refs/heads/master' }
@@ -215,7 +215,7 @@ describe MergeRequests::MergeToRefService do
 
     describe 'cascading merge refs' do
       set(:project) { create(:project, :repository) }
-      let(:params) { { commit_message: 'Cascading merge', first_parent_ref: first_parent_ref, target_ref: target_ref } }
+      let(:params) { { commit_message: 'Cascading merge', first_parent_ref: first_parent_ref, target_ref: target_ref, sha: merge_request.diff_head_sha } }
 
       context 'when first merge happens' do
         let(:merge_request) do

spec/services/merge_requests/update_service_spec.rb

@@ -219,7 +219,7 @@ describe MergeRequests::UpdateService, :mailer do
             head_pipeline_of: merge_request
           )
 
-          expect(AutoMerge::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user, {})
+          expect(AutoMerge::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user, { sha: merge_request.diff_head_sha })
             .and_return(service_mock)
           allow(service_mock).to receive(:available_for?) { true }
           expect(service_mock).to receive(:execute).with(merge_request)
@@ -411,7 +411,7 @@ describe MergeRequests::UpdateService, :mailer do
 
       context 'when auto merge is enabled and target branch changed' do
         before do
-          AutoMergeService.new(project, user).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
+          AutoMergeService.new(project, user, { sha: merge_request.diff_head_sha }).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
 
           update_merge_request({ target_branch: 'target' })
         end

spec/support/helpers/cycle_analytics_helpers.rb

@@ -77,7 +77,7 @@ module CycleAnalyticsHelpers
                        .new(project, user)
                        .closed_by_merge_requests(issue)
 
-    merge_requests.each { |merge_request| MergeRequests::MergeService.new(project, user).execute(merge_request) }
+    merge_requests.each { |merge_request| MergeRequests::MergeService.new(project, user, sha: merge_request.diff_head_sha).execute(merge_request) }
   end
 
   def deploy_master(user, project, environment: 'production')

spec/workers/merge_worker_spec.rb

@@ -20,6 +20,7 @@ describe MergeWorker do
       described_class.new.perform(
         merge_request.id, merge_request.author_id,
         commit_message: 'wow such merge',
+        sha: merge_request.diff_head_sha,
         should_remove_source_branch: true)
 
       merge_request.reload

spec/workers/process_commit_worker_spec.rb

@@ -81,9 +81,10 @@ describe ProcessCommitWorker do
 
       let(:commit) do
         project.repository.create_branch('feature-merged', 'feature')
+        project.repository.after_create_branch
 
         MergeRequests::MergeService
-          .new(project, merge_request.author)
+          .new(project, merge_request.author, { sha: merge_request.diff_head_sha })
           .execute(merge_request)
 
         merge_request.reload.merge_commit