Merge branch 'security-protectedbranch-egress-status-checks' into 'master'

Do not allow status checks to exist with external protected branches

See merge request gitlab-org/security/gitlab!1716

ee/app/models/merge_requests/external_status_check.rb

@@ -19,6 +19,7 @@ module MergeRequests
 
     validates :external_url, presence: true, uniqueness: { scope: :project_id }, addressable_url: true
     validates :name, uniqueness: { scope: :project_id }, presence: true
+    validate :protected_branches_must_belong_to_project
 
     def async_execute(data)
       return unless protected_branches.none? || protected_branches.by_name(data[:object_attributes][:target_branch]).any?
@@ -43,5 +44,9 @@ module MergeRequests
     def payload_data(merge_request_hook_data)
       merge_request_hook_data.merge(external_approval_rule: self.to_h)
     end
+
+    def protected_branches_must_belong_to_project
+      errors.add(:base, 'all protected branches must exist within the project') unless protected_branches.all? { |b| project.protected_branches.include?(b) }
+    end
   end
 end

ee/spec/models/merge_requests/external_status_check_spec.rb

@@ -14,6 +14,15 @@ RSpec.describe MergeRequests::ExternalStatusCheck, type: :model do
     it { is_expected.to validate_presence_of(:external_url) }
     it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) }
     it { is_expected.to validate_uniqueness_of(:external_url).scoped_to(:project_id) }
+
+    describe 'protected_branches_must_belong_to_project' do
+      let(:check) { build(:external_status_check, protected_branches: [create(:protected_branch)]) }
+
+      it 'is invalid' do
+        expect(check).to be_invalid
+        expect(check.errors.messages[:base]).to eq ['all protected branches must exist within the project']
+      end
+    end
   end
 
   describe 'to_h' do
@@ -26,8 +35,8 @@ RSpec.describe MergeRequests::ExternalStatusCheck, type: :model do
     let_it_be(:merge_request) { create(:merge_request) }
     let_it_be(:check_belonging_to_different_project) { create(:external_status_check) }
     let_it_be(:check_with_no_protected_branches) { create(:external_status_check, project: merge_request.project, protected_branches: []) }
-    let_it_be(:check_with_applicable_protected_branches) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: merge_request.target_branch)]) }
-    let_it_be(:check_with_non_applicable_protected_branches) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: 'testbranch')]) }
+    let_it_be(:check_with_applicable_protected_branches) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: merge_request.target_branch, project: merge_request.project)]) }
+    let_it_be(:check_with_non_applicable_protected_branches) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: 'testbranch', project: merge_request.project)]) }
 
     it 'returns the correct collection of checks' do
       expect(merge_request.project.external_status_checks.applicable_to_branch(merge_request.target_branch)).to contain_exactly(check_with_no_protected_branches, check_with_applicable_protected_branches)
@@ -58,7 +67,7 @@ RSpec.describe MergeRequests::ExternalStatusCheck, type: :model do
     end
 
     context 'when data target branch matches a protected branch' do
-      let_it_be(:check) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: 'test')]) }
+      let_it_be(:check) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: 'test', project: merge_request.project)]) }
 
       it 'enqueues the status check' do
         expect(ApprovalRules::ExternalApprovalRulePayloadWorker).to receive(:perform_async).once
@@ -68,7 +77,7 @@ RSpec.describe MergeRequests::ExternalStatusCheck, type: :model do
     end
 
     context 'when data target branch does not match a protected branch' do
-      let_it_be(:check) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: 'new-branch')]) }
+      let_it_be(:check) { create(:external_status_check, project: merge_request.project, protected_branches: [create(:protected_branch, name: 'new-branch', project: merge_request.project)]) }
 
       it 'does not enqueue the status check' do
         expect(ApprovalRules::ExternalApprovalRulePayloadWorker).to receive(:perform_async).never

ee/spec/presenters/merge_request_presenter_spec.rb

@@ -211,7 +211,7 @@ RSpec.describe MergeRequestPresenter do
 
       context 'without applicable branches' do
         before do
-          create(:external_status_check, project: project, protected_branches: [create(:protected_branch, name: 'testbranch')])
+          create(:external_status_check, project: project, protected_branches: [create(:protected_branch, name: 'testbranch', project: project)])
         end
 
         it { is_expected.to eq(nil) }
@@ -227,7 +227,7 @@ RSpec.describe MergeRequestPresenter do
 
       context 'with applicable branches' do
         before do
-          create(:external_status_check, project: project, protected_branches: [create(:protected_branch, name: merge_request.target_branch)])
+          create(:external_status_check, project: project, protected_branches: [create(:protected_branch, name: merge_request.target_branch, project: project)])
         end
 
         it { is_expected.to eq(exposed_path) }

ee/spec/requests/api/status_checks_spec.rb

@@ -39,8 +39,8 @@ RSpec.describe API::StatusChecks do
       end
 
       context 'when merge request has received status check responses' do
-        let!(:non_applicable_check) { create(:external_status_check, project: project, protected_branches: [create(:protected_branch, name: 'different-branch')]) }
-        let!(:branch_specific_check) { create(:external_status_check, project: project, protected_branches: [create(:protected_branch, name: merge_request.target_branch)]) }
+        let!(:non_applicable_check) { create(:external_status_check, project: project, protected_branches: [create(:protected_branch, name: 'different-branch', project: project)]) }
+        let!(:branch_specific_check) { create(:external_status_check, project: project, protected_branches: [create(:protected_branch, name: merge_request.target_branch, project: project)]) }
         let!(:status_check_response) { create(:status_check_response, external_status_check: rule, merge_request: merge_request, sha: sha) }
 
         it 'returns a 200' do
@@ -288,6 +288,24 @@ RSpec.describe API::StatusChecks do
         expect(response).to have_gitlab_http_status(:success)
       end
 
+      context 'when referencing a protected branch outside of the project' do
+        let_it_be(:protected_branch) { create(:protected_branch) }
+
+        let(:params) do
+          { name: 'New rule', external_url: 'https://gitlab.com/test/example.json', protected_branch_ids: protected_branch.id }
+        end
+
+        subject do
+          put api(single_object_url, project.owner), params: params
+        end
+
+        it 'is invalid' do
+          subject
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        end
+      end
+
       context 'with protected branches' do
         let_it_be(:protected_branch) { create(:protected_branch, project: project) }