Merge branch 'security-proctect-internal-builds-from-external-overrides' into 'master'

Protect internal builds from external overrides

Closes #22

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

app/models/generic_commit_status.rb

 # frozen_string_literal: true
 
 class GenericCommitStatus < CommitStatus
+  EXTERNAL_STAGE_IDX = 1_000_000
+
   before_validation :set_default_values
 
   validates :target_url, addressable_url: true,
                          length: { maximum: 255 },
                          allow_nil: true
+  validate :name_uniqueness_across_types, unless: :importing?
 
   # GitHub compatible API
   alias_attribute :context, :name
@@ -13,7 +16,7 @@ class GenericCommitStatus < CommitStatus
   def set_default_values
     self.context ||= 'default'
     self.stage ||= 'external'
-    self.stage_idx ||= 1000000
+    self.stage_idx ||= EXTERNAL_STAGE_IDX
   end
 
   def tags
@@ -25,4 +28,14 @@ class GenericCommitStatus < CommitStatus
       .new(self, current_user)
       .fabricate!
   end
+
+  private
+
+  def name_uniqueness_across_types
+    return if !pipeline || name.blank?
+
+    if pipeline.statuses.by_name(name).where.not(type: type).exists?
+      errors.add(:name, :taken)
+    end
+  end
 end

changelogs/unreleased/security-proctect-internal-builds-from-external-overrides.yml

+---
+title: Protect internal CI builds from external overrides
+merge_request:
+author:
+type: security

lib/api/commit_statuses.rb

@@ -83,6 +83,8 @@ module API
           user: current_user,
           protected: user_project.protected_for?(ref))
 
+        authorize! :update_pipeline, pipeline
+
         status = GenericCommitStatus.running_or_pending.find_or_initialize_by(
           project: user_project,
           pipeline: pipeline,

spec/models/generic_commit_status_spec.rb

@@ -19,6 +19,74 @@ describe GenericCommitStatus do
     it { is_expected.not_to allow_value('javascript:alert(1)').for(:target_url) }
   end
 
+  describe '#name_uniqueness_across_types' do
+    let(:attributes) { {} }
+    let(:commit_status) { described_class.new(attributes) }
+    let(:status_name) { 'test-job' }
+
+    subject(:errors) { commit_status.errors[:name] }
+
+    shared_examples 'it does not have uniqueness errors' do
+      it 'does not return errors' do
+        commit_status.valid?
+
+        is_expected.to be_empty
+      end
+    end
+
+    context 'without attributes' do
+      it_behaves_like 'it does not have uniqueness errors'
+    end
+
+    context 'with only a pipeline' do
+      let(:attributes) { { pipeline: pipeline } }
+
+      context 'without name' do
+        it_behaves_like 'it does not have uniqueness errors'
+      end
+    end
+
+    context 'with only a name' do
+      let(:attributes) { { name: status_name } }
+
+      context 'without pipeline' do
+        it_behaves_like 'it does not have uniqueness errors'
+      end
+    end
+
+    context 'with pipeline and name' do
+      let(:attributes) do
+        {
+          pipeline: pipeline,
+          name: status_name
+        }
+      end
+
+      context 'without other statuses' do
+        it_behaves_like 'it does not have uniqueness errors'
+      end
+
+      context 'with generic statuses' do
+        before do
+          create(:generic_commit_status, pipeline: pipeline, name: status_name)
+        end
+
+        it_behaves_like 'it does not have uniqueness errors'
+      end
+
+      context 'with ci_build statuses' do
+        before do
+          create(:ci_build, pipeline: pipeline, name: status_name)
+        end
+
+        it 'returns name error' do
+          expect(commit_status).to be_invalid
+          is_expected.to include('has already been taken')
+        end
+      end
+    end
+  end
+
   describe '#context' do
     subject { generic_commit_status.context }
 
@@ -79,6 +147,12 @@ describe GenericCommitStatus do
 
       it { is_expected.not_to be_nil }
     end
+
+    describe '#stage_idx' do
+      subject { generic_commit_status.stage_idx }
+
+      it { is_expected.not_to be_nil }
+    end
   end
 
   describe '#present' do

spec/requests/api/commit_statuses_spec.rb

@@ -164,6 +164,7 @@ describe API::CommitStatuses do
 
               expect(response).to have_gitlab_http_status(201)
               expect(job.status).to eq('pending')
+              expect(job.stage_idx).to eq(GenericCommitStatus::EXTERNAL_STAGE_IDX)
             end
           end
 
@@ -331,6 +332,29 @@ describe API::CommitStatuses do
         end
       end
 
+      context 'when updating a protected ref' do
+        before do
+          create(:protected_branch, project: project, name: 'master')
+          post api(post_url, user), params: { state: 'running', ref: 'master' }
+        end
+
+        context 'with user as developer' do
+          let(:user) { developer }
+
+          it 'does not create commit status' do
+            expect(response).to have_gitlab_http_status(403)
+          end
+        end
+
+        context 'with user as maintainer' do
+          let(:user) { create_user(:maintainer) }
+
+          it 'creates commit status' do
+            expect(response).to have_gitlab_http_status(201)
+          end
+        end
+      end
+
       context 'when commit SHA is invalid' do
         let(:sha) { 'invalid_sha' }
 
@@ -372,6 +396,22 @@ describe API::CommitStatuses do
               .to include 'is blocked: Only allowed schemes are http, https'
         end
       end
+
+      context 'when trying to update a status of a different type' do
+        let!(:pipeline) { create(:ci_pipeline, project: project, sha: sha, ref: 'ref') }
+        let!(:ci_build) { create(:ci_build, pipeline: pipeline, name: 'test-job') }
+        let(:params) { { state: 'pending', name: 'test-job' } }
+
+        before do
+          post api(post_url, developer), params: params
+        end
+
+        it 'responds with bad request status and validation errors' do
+          expect(response).to have_gitlab_http_status(400)
+          expect(json_response['message']['name'])
+              .to include 'has already been taken'
+        end
+      end
     end
 
     context 'reporter user' do