diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-02-10 23:13:51 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-02-10 23:14:04 +0000 |
commit | 0e71ffb85425829c094aa0ff32f9a28048b1403d (patch) | |
tree | 12cca41149acef7f17e187ec0d3e22adc60c1efd | |
parent | 29339ca07c9e7c3a5eb5f4219c4c3863c8d7fbdd (diff) | |
download | gitlab-ce-0e71ffb85425829c094aa0ff32f9a28048b1403d.tar.gz |
Add latest changes from gitlab-org/security/gitlab@13-6-stable-ee
17 files changed, 220 insertions, 45 deletions
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 8707d635e03..3778add83bf 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -15,6 +15,7 @@ module Ci include ShaAttribute include FromUnion include UpdatedAtFilterable + include EachBatch PROJECT_ROUTE_AND_NAMESPACE_ROUTE = { project: [:project_feature, :route, { namespace: :route }] diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index ee9c2501bfc..f9de9dec072 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -55,6 +55,7 @@ class CommitStatus < ApplicationRecord scope :for_ids, -> (ids) { where(id: ids) } scope :for_ref, -> (ref) { where(ref: ref) } scope :by_name, -> (name) { where(name: name) } + scope :in_pipelines, ->(pipelines) { where(pipeline: pipelines) } scope :for_project_paths, -> (paths) do where(project: Project.where_full_path_in(Array(paths))) diff --git a/app/services/ci/abort_project_pipelines_service.rb b/app/services/ci/abort_project_pipelines_service.rb new file mode 100644 index 00000000000..d4f228e37a1 --- /dev/null +++ b/app/services/ci/abort_project_pipelines_service.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Ci + class AbortProjectPipelinesService + # Danger: Cancels in bulk without callbacks + # Only for pipeline abandonment scenarios (current example: project delete) + def execute(project) + return unless Feature.enabled?(:abort_deleted_project_pipelines, default_enabled: true) + + pipelines = project.all_pipelines.cancelable + bulk_abort!(pipelines, status: :canceled) + + ServiceResponse.success(message: 'Pipelines canceled') + end + + private + + def bulk_abort!(pipelines, status:) + pipelines.each_batch do |pipeline_batch| + CommitStatus.in_pipelines(pipeline_batch).in_batches.update_all(status: status) # rubocop: disable Cop/InBatches + pipeline_batch.update_all(status: status) + end + end + end +end diff --git a/app/services/ci/cancel_user_pipelines_service.rb b/app/services/ci/cancel_user_pipelines_service.rb index 3a8b5e91088..3d3a8032e8e 100644 --- a/app/services/ci/cancel_user_pipelines_service.rb +++ b/app/services/ci/cancel_user_pipelines_service.rb @@ -6,6 +6,7 @@ module Ci # This is a bug with CodeReuse/ActiveRecord cop # https://gitlab.com/gitlab-org/gitlab/issues/32332 def execute(user) + # TODO: fix N+1 queries https://gitlab.com/gitlab-org/gitlab/-/issues/300685 user.pipelines.cancelable.find_each(&:cancel_running) ServiceResponse.success(message: 'Pipeline canceled') diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index bec75657530..c1501625300 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -21,11 +21,14 @@ module Projects def execute return false unless can?(current_user, :remove_project, project) + project.update_attribute(:pending_delete, true) # Flush the cache for both repositories. This has to be done _before_ # removing the physical repositories as some expiration code depends on # Git data (e.g. a list of branch names). flush_caches(project) + ::Ci::AbortProjectPipelinesService.new.execute(project) + Projects::UnlinkForkService.new(project, current_user).execute attempt_destroy(project) diff --git a/changelogs/unreleased/security-cancel-pipelines-for-deleted-project.yml b/changelogs/unreleased/security-cancel-pipelines-for-deleted-project.yml new file mode 100644 index 00000000000..de92707cb8f --- /dev/null +++ b/changelogs/unreleased/security-cancel-pipelines-for-deleted-project.yml @@ -0,0 +1,5 @@ +--- +title: Cancel running and pending jobs when a project is deleted +merge_request: 1220 +author: +type: security diff --git a/changelogs/unreleased/security-confidential-titles.yml b/changelogs/unreleased/security-confidential-titles.yml new file mode 100644 index 00000000000..506cbc095c4 --- /dev/null +++ b/changelogs/unreleased/security-confidential-titles.yml @@ -0,0 +1,5 @@ +--- +title: Prevent exposure of confidential issue titles in file browser +merge_request: +author: +type: security diff --git a/config/feature_flags/development/abort_deleted_project_pipelines.yml b/config/feature_flags/development/abort_deleted_project_pipelines.yml new file mode 100644 index 00000000000..f09cc9dd86b --- /dev/null +++ b/config/feature_flags/development/abort_deleted_project_pipelines.yml @@ -0,0 +1,8 @@ +--- +name: abort_deleted_project_pipelines +introduced_by_url: https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/1220 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/301106 +milestone: '13.9' +type: development +group: group::continuous integration +default_enabled: true diff --git a/config/feature_flags/development/codequality_mr_diff.yml b/config/feature_flags/development/codequality_mr_diff.yml deleted file mode 100644 index ca6846b9390..00000000000 --- a/config/feature_flags/development/codequality_mr_diff.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: codequality_mr_diff -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/47938 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/284140 -milestone: '13.7' -type: development -group: group::testing -default_enabled: false diff --git a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb index 8f1e690c081..4f90b7756eb 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb @@ -10,6 +10,10 @@ module Gitlab include Chain::Helpers def perform! + if project.pending_delete? + return error('Project is deleted!') + end + unless project.builds_enabled? return error('Pipelines are disabled!') end diff --git a/lib/gitlab/tree_summary.rb b/lib/gitlab/tree_summary.rb index 9b67599668a..bc7b8bd2b94 100644 --- a/lib/gitlab/tree_summary.rb +++ b/lib/gitlab/tree_summary.rb @@ -40,21 +40,17 @@ module Gitlab # - An Array of the unique ::Commit objects in the first value def summarize summary = contents - .map { |content| build_entry(content) } .tap { |summary| fill_last_commits!(summary) } [summary, commits] end def fetch_logs - cache_key = ['projects', project.id, 'logs', commit.id, path, offset] - Rails.cache.fetch(cache_key, expires_in: CACHE_EXPIRE_IN) do - logs, _ = summarize + logs, _ = summarize - new_offset = next_offset if more? + new_offset = next_offset if more? - [logs.as_json, new_offset] - end + [logs.as_json, new_offset] end # Does the tree contain more entries after the given offset + limit? @@ -71,7 +67,7 @@ module Gitlab private def contents - all_contents[offset, limit] + all_contents[offset, limit] || [] end def commits @@ -82,22 +78,17 @@ module Gitlab project.repository end - def entry_path(entry) - File.join(*[path, entry[:file_name]].compact).force_encoding(Encoding::ASCII_8BIT) + # Ensure the path is in "path/" format + def ensured_path + File.join(*[path, ""]) if path end - def build_entry(entry) - { file_name: entry.name, type: entry.type } + def entry_path(entry) + File.join(*[path, entry[:file_name]].compact).force_encoding(Encoding::ASCII_8BIT) end def fill_last_commits!(entries) - # Ensure the path is in "path/" format - ensured_path = - if path - File.join(*[path, ""]) - end - - commits_hsh = repository.list_last_commits_for_tree(commit.id, ensured_path, offset: offset, limit: limit, literal_pathspec: true) + commits_hsh = fetch_last_cached_commits_list prerender_commit_full_titles!(commits_hsh.values) entries.each do |entry| @@ -112,6 +103,18 @@ module Gitlab end end + def fetch_last_cached_commits_list + cache_key = ['projects', project.id, 'last_commits_list', commit.id, ensured_path, offset, limit] + + commits = Rails.cache.fetch(cache_key, expires_in: CACHE_EXPIRE_IN) do + repository + .list_last_commits_for_tree(commit.id, ensured_path, offset: offset, limit: limit, literal_pathspec: true) + .transform_values!(&:to_hash) + end + + commits.transform_values! { |value| Commit.from_hash(value, project) } + end + def cache_commit(commit) return unless commit.present? @@ -123,12 +126,18 @@ module Gitlab end def all_contents - strong_memoize(:all_contents) do + strong_memoize(:all_contents) { cached_contents } + end + + def cached_contents + cache_key = ['projects', project.id, 'content', commit.id, path] + + Rails.cache.fetch(cache_key, expires_in: CACHE_EXPIRE_IN) do [ *tree.trees, *tree.blobs, *tree.submodules - ] + ].map { |entry| { file_name: entry.name, type: entry.type } } end end diff --git a/spec/controllers/projects/refs_controller_spec.rb b/spec/controllers/projects/refs_controller_spec.rb index d10351feb9e..b625ce35d61 100644 --- a/spec/controllers/projects/refs_controller_spec.rb +++ b/spec/controllers/projects/refs_controller_spec.rb @@ -56,18 +56,6 @@ RSpec.describe Projects::RefsController do expect(response).to be_successful expect(json_response).to be_kind_of(Array) end - - it 'caches tree summary data', :use_clean_rails_memory_store_caching do - expect_next_instance_of(::Gitlab::TreeSummary) do |instance| - expect(instance).to receive_messages(summarize: ['logs'], next_offset: 50, more?: true) - end - - xhr_get(:json, offset: 25) - - cache_key = "projects/#{project.id}/logs/#{project.commit.id}/#{path}/25" - expect(Rails.cache.fetch(cache_key)).to eq(['logs', 50]) - expect(response.headers['More-Logs-Offset']).to eq("50") - end end end end diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb index ae3270cb9b2..7aaeee32f49 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb @@ -74,6 +74,14 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do it 'does not break the chain' do expect(step.break?).to eq false end + + context 'when project is deleted' do + before do + project.update!(pending_delete: true) + end + + specify { expect(step.perform!).to contain_exactly('Project is deleted!') } + end end describe '#allowed_to_write_ref?' do diff --git a/spec/lib/gitlab/tree_summary_spec.rb b/spec/lib/gitlab/tree_summary_spec.rb index 303a4a80581..d2c5844b0fa 100644 --- a/spec/lib/gitlab/tree_summary_spec.rb +++ b/spec/lib/gitlab/tree_summary_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::TreeSummary do + include RepoHelpers using RSpec::Parameterized::TableSyntax let(:project) { create(:project, :empty_repo) } @@ -44,6 +45,40 @@ RSpec.describe Gitlab::TreeSummary do expect(commits).to match_array(entries.map { |entry| entry[:commit] }) end end + + context 'when offset is over the limit' do + let(:offset) { 100 } + + it 'returns an empty array' do + expect(summarized).to eq([[], []]) + end + end + + context 'with caching', :use_clean_rails_memory_store_caching do + subject { Rails.cache.fetch(key) } + + before do + summarized + end + + context 'Repository tree cache' do + let(:key) { ['projects', project.id, 'content', commit.id, path] } + + it 'creates a cache for repository content' do + is_expected.to eq([{ file_name: 'a.txt', type: :blob }]) + end + end + + context 'Commits list cache' do + let(:offset) { 0 } + let(:limit) { 25 } + let(:key) { ['projects', project.id, 'last_commits_list', commit.id, path, offset, limit] } + + it 'creates a cache for commits list' do + is_expected.to eq('a.txt' => commit.to_hash) + end + end + end end describe '#summarize (entries)' do @@ -167,6 +202,46 @@ RSpec.describe Gitlab::TreeSummary do end end + describe 'References in commit messages' do + let_it_be(:project) { create(:project, :empty_repo) } + let_it_be(:issue) { create(:issue, project: project) } + let(:entries) { summary.summarize.first } + let(:entry) { entries.find { |entry| entry[:file_name] == 'issue.txt' } } + + before_all do + create_file_in_repo(project, 'master', 'master', 'issue.txt', '', commit_message: "Issue ##{issue.iid}") + end + + where(:project_visibility, :user_role, :issue_confidential, :expected_result) do + 'private' | :guest | false | true + 'private' | :guest | true | false + 'private' | :reporter | false | true + 'private' | :reporter | true | true + + 'internal' | :guest | false | true + 'internal' | :guest | true | false + 'internal' | :reporter | false | true + 'internal' | :reporter | true | true + + 'public' | :guest | false | true + 'public' | :guest | true | false + 'public' | :reporter | false | true + 'public' | :reporter | true | true + end + + with_them do + subject { entry[:commit_title_html].include?("title=\"#{issue.title}\"") } + + before do + project.add_role(user, user_role) + project.update!(visibility_level: Gitlab::VisibilityLevel.level_value(project_visibility)) + issue.update!(confidential: issue_confidential) + end + + it { is_expected.to eq(expected_result) } + end + end + describe '#more?' do let(:path) { 'tmp/more' } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 1ca370dc950..4c26cc09656 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -34,6 +34,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do it { is_expected.to have_many(:auto_canceled_jobs) } it { is_expected.to have_many(:sourced_pipelines) } it { is_expected.to have_many(:triggered_pipelines) } + it { is_expected.to have_many(:pipeline_artifacts) } it { is_expected.to have_one(:chat_data) } it { is_expected.to have_one(:source_pipeline) } @@ -41,14 +42,15 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do it { is_expected.to have_one(:source_job) } it { is_expected.to have_one(:pipeline_config) } - it { is_expected.to validate_presence_of(:sha) } - it { is_expected.to validate_presence_of(:status) } - it { is_expected.to respond_to :git_author_name } it { is_expected.to respond_to :git_author_email } it { is_expected.to respond_to :short_sha } it { is_expected.to delegate_method(:full_path).to(:project).with_prefix } - it { is_expected.to have_many(:pipeline_artifacts) } + + describe 'validations' do + it { is_expected.to validate_presence_of(:sha) } + it { is_expected.to validate_presence_of(:status) } + end describe 'associations' do it 'has a bidirectional relationship with projects' do diff --git a/spec/services/ci/abort_project_pipelines_service_spec.rb b/spec/services/ci/abort_project_pipelines_service_spec.rb new file mode 100644 index 00000000000..9af909ac2ab --- /dev/null +++ b/spec/services/ci/abort_project_pipelines_service_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::AbortProjectPipelinesService do + let_it_be(:project) { create(:project) } + let_it_be(:pipeline) { create(:ci_pipeline, :running, project: project) } + let_it_be(:build) { create(:ci_build, :running, pipeline: pipeline) } + + describe '#execute' do + it 'cancels all running pipelines and related jobs' do + result = described_class.new.execute(project) + + expect(result).to be_success + expect(pipeline.reload).to be_canceled + expect(build.reload).to be_canceled + end + + it 'avoids N+1 queries' do + control_count = ActiveRecord::QueryRecorder.new { described_class.new.execute(project) }.count + + pipelines = create_list(:ci_pipeline, 5, :running, project: project) + create_list(:ci_build, 5, :running, pipeline: pipelines.first) + + expect { described_class.new.execute(project) }.not_to exceed_query_limit(control_count) + end + end + + context 'when feature disabled' do + before do + stub_feature_flags(abort_deleted_project_pipelines: false) + end + + it 'does not abort the pipeline' do + result = described_class.new.execute(project) + + expect(result).to be(nil) + expect(pipeline.reload).to be_running + expect(build.reload).to be_running + end + end +end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index f0f09218b06..75d1c98923a 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -69,6 +69,12 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do destroy_project(project, user, {}) end + it 'performs cancel for project ci pipelines' do + expect(::Ci::AbortProjectPipelinesService).to receive_message_chain(:new, :execute).with(project) + + destroy_project(project, user, {}) + end + context 'when project has remote mirrors' do let!(:project) do create(:project, :repository, namespace: user.namespace).tap do |project| |