Add latest changes from gitlab-org/security/gitlab@12-6-stable-ee

spec/lib/gitlab/git/repository_spec.rb

@@ -1962,66 +1962,15 @@ describe Gitlab::Git::Repository, :seed_helper do
   end
   describe '#compare_source_branch' do
-    let(:repository) { Gitlab::Git::Repository.new('default', TEST_GITATTRIBUTES_REPO_PATH, '', 'group/project') }
-
-    context 'within same repository' do
-      it 'does not create a temp ref' do
-        expect(repository).not_to receive(:fetch_source_branch!)
-        expect(repository).not_to receive(:delete_refs)
-
-        compare = repository.compare_source_branch('master', repository, 'feature', straight: false)
-        expect(compare).to be_a(Gitlab::Git::Compare)
-        expect(compare.commits.count).to be > 0
-      end
-
-      it 'returns empty commits when source ref does not exist' do
-        compare = repository.compare_source_branch('master', repository, 'non-existent-branch', straight: false)
-        expect(compare.commits).to be_empty
+    it 'delegates to Gitlab::Git::CrossRepoComparer' do
+      expect_next_instance_of(::Gitlab::Git::CrossRepoComparer) do |instance|
+        expect(instance.source_repo).to eq(:source_repository)
+        expect(instance.target_repo).to eq(repository)
+        expect(instance).to receive(:compare).with('feature', 'master', straight: :straight)
       end
-    end
-    context 'with different repositories' do
-      context 'when ref is known by source repo, but not by target' do
-        before do
-          mutable_repository.write_ref('another-branch', 'feature')
-        end
-
-        it 'creates temp ref' do
-          expect(repository).not_to receive(:fetch_source_branch!)
-          expect(repository).not_to receive(:delete_refs)
-
-          compare = repository.compare_source_branch('master', mutable_repository, 'another-branch', straight: false)
-          expect(compare).to be_a(Gitlab::Git::Compare)
-          expect(compare.commits.count).to be > 0
-        end
-      end
-
-      context 'when ref is known by source and target repos' do
-        before do
-          mutable_repository.write_ref('another-branch', 'feature')
-          repository.write_ref('another-branch', 'feature')
-        end
-
-        it 'does not create a temp ref' do
-          expect(repository).not_to receive(:fetch_source_branch!)
-          expect(repository).not_to receive(:delete_refs)
-
-          compare = repository.compare_source_branch('master', mutable_repository, 'another-branch', straight: false)
-          expect(compare).to be_a(Gitlab::Git::Compare)
-          expect(compare.commits.count).to be > 0
-        end
-      end
-
-      context 'when ref is unknown by source repo' do
-        it 'returns nil when source ref does not exist' do
-          expect(repository).to receive(:fetch_source_branch!).and_call_original
-          expect(repository).to receive(:delete_refs).and_call_original
-
-          compare = repository.compare_source_branch('master', mutable_repository, 'non-existent-branch', straight: false)
-          expect(compare).to be_nil
-        end
-      end
+      repository.compare_source_branch('master', :source_repository, 'feature', straight: :straight)
     end
   end

spec/lib/gitlab/reference_extractor_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 describe Gitlab::ReferenceExtractor do
-  let(:project) { create(:project) }
+  let_it_be(:project) { create(:project) }
   before do
     project.add_developer(project.creator)
@@ -293,4 +293,34 @@ describe Gitlab::ReferenceExtractor do
       end
     end
   end
+
+  describe '#references' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:issue) { create(:issue, project: project) }
+    let(:text) { "Ref. #{issue.to_reference}" }
+
+    subject { described_class.new(project, user) }
+
+    before do
+      subject.analyze(text)
+    end
+
+    context 'when references are visible' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'returns visible references of given type' do
+        expect(subject.references(:issue)).to eq([issue])
+      end
+
+      it 'does not increase stateful_not_visible_counter' do
+        expect { subject.references(:issue) }.not_to change { subject.stateful_not_visible_counter }
+      end
+    end
+
+    it 'increases stateful_not_visible_counter' do
+      expect { subject.references(:issue) }.to change { subject.stateful_not_visible_counter }.by(1)
+    end
+  end
 end

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/models/grafana_integration_spec.rb

@@ -9,7 +9,7 @@ describe GrafanaIntegration do
   describe 'validations' do
     it { is_expected.to validate_presence_of(:project) }
-    it { is_expected.to validate_presence_of(:token) }
+    it { is_expected.to validate_presence_of(:encrypted_token) }
     it 'disallows invalid urls for grafana_url' do
       unsafe_url = %{https://replaceme.com/'><script>alert(document.cookie)</script>}
@@ -66,4 +66,24 @@ describe GrafanaIntegration do
       end
     end
   end
+
+  describe 'attribute encryption' do
+    subject(:grafana_integration) { create(:grafana_integration, token: 'super-secret') }
+
+    context 'token' do
+      it 'encrypts original value into encrypted_token attribute' do
+        expect(grafana_integration.encrypted_token).not_to be_nil
+      end
+
+      it 'locks access to raw value in private method', :aggregate_failures do
+        expect { grafana_integration.token }.to raise_error(NoMethodError, /private method .token. called/)
+        expect(grafana_integration.send(:token)).to eql('super-secret')
+      end
+
+      it 'prevents overriding token value with its encrypted or masked version', :aggregate_failures do
+        expect { grafana_integration.update(token: grafana_integration.encrypted_token) }.not_to change { grafana_integration.reload.send(:token) }
+        expect { grafana_integration.update(token: grafana_integration.masked_token) }.not_to change { grafana_integration.reload.send(:token) }
+      end
+    end
+  end
 end

spec/models/note_spec.rb

@@ -350,12 +350,12 @@ describe Note do
   end
   describe "cross_reference_not_visible_for?" do
-    let(:private_user)    { create(:user) }
-    let(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } }
-    let(:private_issue)   { create(:issue, project: private_project) }
-    let(:ext_proj)  { create(:project, :public) }
-    let(:ext_issue) { create(:issue, project: ext_proj) }
+    let_it_be(:private_user)    { create(:user) }
+    let_it_be(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } }
+    let_it_be(:private_issue)   { create(:issue, project: private_project) }
+    let_it_be(:ext_proj)  { create(:project, :public) }
+    let_it_be(:ext_issue) { create(:issue, project: ext_proj) }
     shared_examples "checks references" do
       it "returns true" do
@@ -393,10 +393,24 @@ describe Note do
       it_behaves_like "checks references"
     end
-    context "when there are two references in note" do
+    context "when there is a reference to a label" do
+      let_it_be(:private_label) { create(:label, project: private_project) }
       let(:note) do
         create :note,
           noteable: ext_issue, project: ext_proj,
+          note: "added label #{private_label.to_reference(ext_proj)}",
+          system: true
+      end
+      let!(:system_note_metadata) { create(:system_note_metadata, note: note, action: :label) }
+
+      it_behaves_like "checks references"
+    end
+
+    context "when there are two references in note" do
+      let_it_be(:ext_issue2) { create(:issue, project: ext_proj) }
+      let(:note) do
+        create :note,
+          noteable: ext_issue2, project: ext_proj,
           note: "mentioned in issue #{private_issue.to_reference(ext_proj)} and " \
                 "public issue #{ext_issue.to_reference(ext_proj)}",
           system: true

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

spec/requests/api/runner_spec.rb

@@ -1509,7 +1509,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
           authorize_artifacts
-          expect(response).to have_gitlab_http_status(500)
+          expect(response).to have_gitlab_http_status(:forbidden)
         end
         context 'authorization token is invalid' do
@@ -1639,6 +1639,18 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             end
           end
+          context 'Is missing GitLab Workhorse token headers' do
+            let(:jwt_token) { JWT.encode({ 'iss' => 'invalid-header' }, Gitlab::Workhorse.secret, 'HS256') }
+
+            it 'fails to post artifacts without GitLab-Workhorse' do
+              expect(Gitlab::ErrorTracking).to receive(:track_exception).once
+
+              upload_artifacts(file_upload, headers_with_token)
+
+              expect(response).to have_gitlab_http_status(:forbidden)
+            end
+          end
+
           context 'when setting an expire date' do
             let(:default_artifacts_expire_in) {}
             let(:post_data) do

spec/services/projects/group_links/destroy_service_spec.rb

@@ -3,8 +3,8 @@
 require 'spec_helper'
 describe Projects::GroupLinks::DestroyService, '#execute' do
-  let(:group_link) { create :project_group_link }
-  let(:project) { group_link.project }
+  let(:project) { create(:project, :private) }
+  let!(:group_link) { create(:project_group_link, project: project) }
   let(:user) { create :user }
   let(:subject) { described_class.new(project, user) }
@@ -15,4 +15,39 @@ describe Projects::GroupLinks::DestroyService, '#execute' do
   it 'returns false if group_link is blank' do
     expect { subject.execute(nil) }.not_to change { project.project_group_links.count }
   end
+
+  describe 'todos cleanup' do
+    context 'when project is private' do
+      it 'triggers todos cleanup' do
+        expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id)
+        expect(project.private?).to be true
+
+        subject.execute(group_link)
+      end
+    end
+
+    context 'when project is public or internal' do
+      shared_examples_for 'removes confidential todos' do
+        it 'does not trigger todos cleanup' do
+          expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id)
+          expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, nil, project.id)
+          expect(project.private?).to be false
+
+          subject.execute(group_link)
+        end
+      end
+
+      context 'when project is public' do
+        let(:project) { create(:project, :public) }
+
+        it_behaves_like 'removes confidential todos'
+      end
+
+      context 'when project is internal' do
+        let(:project) { create(:project, :public) }
+
+        it_behaves_like 'removes confidential todos'
+      end
+    end
+  end
 end

spec/services/projects/operations/update_service_spec.rb

@@ -210,7 +210,7 @@ describe Projects::Operations::UpdateService do
           integration = project.reload.grafana_integration
           expect(integration.grafana_url).to eq(expected_attrs[:grafana_url])
-          expect(integration.token).to eq(expected_attrs[:token])
+          expect(integration.send(:token)).to eq(expected_attrs[:token])
         end
       end
@@ -226,7 +226,7 @@ describe Projects::Operations::UpdateService do
           integration = project.reload.grafana_integration
           expect(integration.grafana_url).to eq(expected_attrs[:grafana_url])
-          expect(integration.token).to eq(expected_attrs[:token])
+          expect(integration.send(:token)).to eq(expected_attrs[:token])
         end
         context 'with all grafana attributes blank in params' do

spec/services/projects/update_pages_service_spec.rb

@@ -5,7 +5,7 @@ require "spec_helper"
 describe Projects::UpdatePagesService do
   set(:project) { create(:project, :repository) }
   set(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) }
-  set(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') }
+  let(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') }
   let(:invalid_file) { fixture_file_upload('spec/fixtures/dk.png') }
   let(:file) { fixture_file_upload("spec/fixtures/pages.zip") }
@@ -242,6 +242,32 @@ describe Projects::UpdatePagesService do
     end
   end
+  context 'when file size is spoofed' do
+    let(:metadata) { spy('metadata') }
+
+    include_context 'pages zip with spoofed size'
+
+    before do
+      file = fixture_file_upload(fake_zip_path, 'pages.zip')
+      metafile = fixture_file_upload('spec/fixtures/pages.zip.meta')
+
+      create(:ci_job_artifact, :archive, file: file, job: build)
+      create(:ci_job_artifact, :metadata, file: metafile, job: build)
+
+      allow(build).to receive(:artifacts_metadata_entry)
+                        .and_return(metadata)
+      allow(metadata).to receive(:total_size).and_return(100)
+    end
+
+    it 'raises an error' do
+      expect do
+        subject.execute
+      end.to raise_error(Projects::UpdatePagesService::FailedToExtractError,
+                         'Entry public/index.html should be 1B but is larger when inflated')
+      expect(deploy_status).to be_script_failure
+    end
+  end
+
   def deploy_status
     GenericCommitStatus.find_by(name: 'pages:deploy')
   end

spec/support/helpers/reference_parser_helpers.rb

@@ -5,6 +5,11 @@ module ReferenceParserHelpers
     Nokogiri::HTML.fragment('<a></a>').children[0]
   end
+  def expect_gathered_references(result, visible, not_visible_count)
+    expect(result[:visible]).to eq(visible)
+    expect(result[:not_visible].count).to eq(not_visible_count)
+  end
+
   shared_examples 'no project N+1 queries' do
     it 'avoids N+1 queries in #nodes_visible_to_user', :request_store do
       context = Banzai::RenderContext.new(project, user)

spec/support/shared_contexts/pages_zip_with_spoofed_size_shared_context.rb

+# frozen_string_literal: true
+
+# the idea of creating zip archive with spoofed size is borrowed from
+# https://github.com/rubyzip/rubyzip/pull/403/files#diff-118213fb4baa6404a40f89e1147661ebR88
+RSpec.shared_context 'pages zip with spoofed size' do
+  let(:real_zip_path) { Tempfile.new(['real', '.zip']).path }
+  let(:fake_zip_path) { Tempfile.new(['fake', '.zip']).path }
+
+  before do
+    full_file_name = 'public/index.html'
+    true_size = 500_000
+    fake_size = 1
+
+    ::Zip::File.open(real_zip_path, ::Zip::File::CREATE) do |zf|
+      zf.get_output_stream(full_file_name) do |os|
+        os.write 'a' * true_size
+      end
+    end
+
+    compressed_size = nil
+    ::Zip::File.open(real_zip_path) do |zf|
+      a_entry = zf.find_entry(full_file_name)
+      compressed_size = a_entry.compressed_size
+    end
+
+    true_size_bytes = [compressed_size, true_size, full_file_name.size].pack('LLS')
+    fake_size_bytes = [compressed_size, fake_size, full_file_name.size].pack('LLS')
+
+    data = File.binread(real_zip_path)
+    data.gsub! true_size_bytes, fake_size_bytes
+
+    File.open(fake_zip_path, 'wb') do |file|
+      file.write data
+    end
+  end
+
+  after do
+    File.delete(real_zip_path) if File.exist?(real_zip_path)
+    File.delete(fake_zip_path) if File.exist?(fake_zip_path)
+  end
+end