From b4186113913f6aebe7a87a024d50b4a03b98cc6c Mon Sep 17 00:00:00 2001 From: lma-git Date: Wed, 8 Feb 2023 16:11:22 -0800 Subject: [PATCH] Fix to prevent unlimited includes This MR updates the external file verifier so that it appends the file to expandset prior to file validation. This fixes the bug that allowed unlimited recursive includes. The fix is developed behind a feature flag: ci_fix_max_includes. --- .../development/ci_fix_max_includes.yml | 8 + lib/gitlab/ci/config/external/context.rb | 2 +- .../ci/config/external/mapper/verifier.rb | 34 ++- .../gitlab/ci/config/external/context_spec.rb | 31 ++- .../config/external/mapper/verifier_spec.rb | 261 +++++++++++------- .../gitlab/ci/config/external/mapper_spec.rb | 15 +- 6 files changed, 244 insertions(+), 107 deletions(-) create mode 100644 config/feature_flags/development/ci_fix_max_includes.yml diff --git a/config/feature_flags/development/ci_fix_max_includes.yml b/config/feature_flags/development/ci_fix_max_includes.yml new file mode 100644 index 00000000000000..8abcf2640cd76a --- /dev/null +++ b/config/feature_flags/development/ci_fix_max_includes.yml @@ -0,0 +1,8 @@ +--- +name: ci_fix_max_includes +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111391 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/390909 +milestone: '15.9' +type: development +group: group::pipeline authoring +default_enabled: false diff --git a/lib/gitlab/ci/config/external/context.rb b/lib/gitlab/ci/config/external/context.rb index 138e79db33181d..71e21f06f19dd9 100644 --- a/lib/gitlab/ci/config/external/context.rb +++ b/lib/gitlab/ci/config/external/context.rb @@ -27,7 +27,7 @@ def initialize( @user = user @parent_pipeline = parent_pipeline @variables = variables || Ci::Variables::Collection.new - @expandset = Set.new + @expandset = Feature.enabled?(:ci_fix_max_includes, project) ? [] : Set.new @execution_deadline = 0 @logger = logger || Gitlab::Ci::Pipeline::Logger.new(project: project) @max_includes = MAX_INCLUDES diff --git a/lib/gitlab/ci/config/external/mapper/verifier.rb b/lib/gitlab/ci/config/external/mapper/verifier.rb index beb4dd1add7f8f..896bd18a3aa8f9 100644 --- a/lib/gitlab/ci/config/external/mapper/verifier.rb +++ b/lib/gitlab/ci/config/external/mapper/verifier.rb @@ -15,6 +15,11 @@ def process_without_instrumentation(files) end files.each do |file| + if ::Feature.enabled?(:ci_fix_max_includes, context.project) + context.expandset << file + verify_max_includes! + end + verify_execution_time! file.validate_location! @@ -25,31 +30,44 @@ def process_without_instrumentation(files) # We do not combine the loops because we need to load the content of all files before continuing # to call `BatchLoader` for all locations. files.each do |file| # rubocop:disable Style/CombinableLoops - # Checking the max includes will be changed with https://gitlab.com/gitlab-org/gitlab/-/issues/367150 - verify_max_includes! + verify_max_includes! if ::Feature.disabled?(:ci_fix_max_includes, context.project) + verify_execution_time! file.validate_content! if file.valid? file.load_and_validate_expanded_hash! if file.valid? - context.expandset.add(file) + context.expandset.add(file) if ::Feature.disabled?(:ci_fix_max_includes, context.project) end end # Will be removed with the FF ci_batch_request_for_local_and_project_includes def legacy_process_without_instrumentation(files) files.select do |file| - verify_max_includes! - verify_execution_time! + if ::Feature.enabled?(:ci_fix_max_includes, context.project) + context.expandset << file + verify_max_includes! - file.validate! + verify_execution_time! - context.expandset.add(file) + file.validate! + else + verify_max_includes! + verify_execution_time! + + file.validate! + + context.expandset.add(file) + end end end def verify_max_includes! - return if context.expandset.count < context.max_includes + if ::Feature.enabled?(:ci_fix_max_includes, context.project) + return if context.expandset.count <= context.max_includes + else + return if context.expandset.count < context.max_includes # rubocop:disable Style/IfInsideElse + end raise Mapper::TooManyIncludesError, "Maximum of #{context.max_includes} nested includes are allowed!" end diff --git a/spec/lib/gitlab/ci/config/external/context_spec.rb b/spec/lib/gitlab/ci/config/external/context_spec.rb index 40702e7540442b..73167be40d7803 100644 --- a/spec/lib/gitlab/ci/config/external/context_spec.rb +++ b/spec/lib/gitlab/ci/config/external/context_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Ci::Config::External::Context do +RSpec.describe Gitlab::Ci::Config::External::Context, feature_category: :pipeline_authoring do let(:project) { build(:project) } let(:user) { double('User') } let(:sha) { '12345' } @@ -14,7 +14,7 @@ describe 'attributes' do context 'with values' do it { is_expected.to have_attributes(**attributes) } - it { expect(subject.expandset).to eq(Set.new) } + it { expect(subject.expandset).to eq([]) } it { expect(subject.execution_deadline).to eq(0) } it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) } it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) } @@ -25,11 +25,36 @@ let(:attributes) { { project: nil, user: nil, sha: nil } } it { is_expected.to have_attributes(**attributes) } - it { expect(subject.expandset).to eq(Set.new) } + it { expect(subject.expandset).to eq([]) } it { expect(subject.execution_deadline).to eq(0) } it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) } it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) } end + + context 'when FF ci_fix_max_includes is disabled' do + before do + stub_feature_flags(ci_fix_max_includes: false) + end + + context 'with values' do + it { is_expected.to have_attributes(**attributes) } + it { expect(subject.expandset).to eq(Set.new) } + it { expect(subject.execution_deadline).to eq(0) } + it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) } + it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) } + it { expect(subject.variables_hash).to include('a' => 'b') } + end + + context 'without values' do + let(:attributes) { { project: nil, user: nil, sha: nil } } + + it { is_expected.to have_attributes(**attributes) } + it { expect(subject.expandset).to eq(Set.new) } + it { expect(subject.execution_deadline).to eq(0) } + it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) } + it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) } + end + end end describe '#set_deadline' do diff --git a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb index 4303f279cacb0c..e39a1c6786b767 100644 --- a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb @@ -58,121 +58,184 @@ describe '#process' do subject(:process) { verifier.process(files) } - context 'when files are local' do - let(:files) do - [ - Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), - Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file2.yml' }, context), - Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file3.yml' }, context) - ] - end + shared_examples 'when FF ci_fix_max_includes is enabled' do + context 'when files are local' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file2.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file3.yml' }, context) + ] + end - it 'returns an array of file objects' do - expect(process.map(&:location)).to contain_exactly( - 'myfolder/file1.yml', 'myfolder/file2.yml', 'myfolder/file3.yml' - ) - end + it 'returns an array of file objects' do + expect(process.map(&:location)).to contain_exactly( + 'myfolder/file1.yml', 'myfolder/file2.yml', 'myfolder/file3.yml' + ) + end - it 'adds files to the expandset' do - expect { process }.to change { context.expandset.count }.by(3) - end + it 'adds files to the expandset' do + expect { process }.to change { context.expandset.count }.by(3) + end + + it 'calls Gitaly only once for all files', :request_store do + # 1 for project.commit.id, 1 for the files + expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(2) + end + + context 'when the FF ci_batch_request_for_local_and_project_includes is disabled' do + before do + stub_feature_flags(ci_batch_request_for_local_and_project_includes: false) + end - it 'calls Gitaly only once for all files', :request_store do - # 1 for project.commit.id, 1 for the files - expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(2) + it 'calls Gitaly for each file', :request_store do + # 1 for project.commit.id, 3 for the files + expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(4) + end + end end - context 'when the FF ci_batch_request_for_local_and_project_includes is disabled' do - before do - stub_feature_flags(ci_batch_request_for_local_and_project_includes: false) + context 'when files are project files' do + let_it_be(:included_project) { create(:project, :repository, namespace: project.namespace, creator: user) } + + let(:files) do + [ + Gitlab::Ci::Config::External::File::Project.new( + { file: 'myfolder/file1.yml', project: included_project.full_path }, context + ), + Gitlab::Ci::Config::External::File::Project.new( + { file: 'myfolder/file2.yml', project: included_project.full_path }, context + ), + Gitlab::Ci::Config::External::File::Project.new( + { file: 'myfolder/file3.yml', project: included_project.full_path }, context + ) + ] end - it 'calls Gitaly for each file', :request_store do - # 1 for project.commit.id, 3 for the files - expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(4) + around(:all) do |example| + create_and_delete_files(included_project, project_files) do + example.run + end end - end - end - context 'when files are project files' do - let_it_be(:included_project) { create(:project, :repository, namespace: project.namespace, creator: user) } - - let(:files) do - [ - Gitlab::Ci::Config::External::File::Project.new( - { file: 'myfolder/file1.yml', project: included_project.full_path }, context - ), - Gitlab::Ci::Config::External::File::Project.new( - { file: 'myfolder/file2.yml', project: included_project.full_path }, context - ), - Gitlab::Ci::Config::External::File::Project.new( - { file: 'myfolder/file3.yml', project: included_project.full_path }, context + it 'returns an array of file objects' do + expect(process.map(&:location)).to contain_exactly( + 'myfolder/file1.yml', 'myfolder/file2.yml', 'myfolder/file3.yml' ) - ] - end + end - around(:all) do |example| - create_and_delete_files(included_project, project_files) do - example.run + it 'adds files to the expandset' do + expect { process }.to change { context.expandset.count }.by(3) end - end - it 'returns an array of file objects' do - expect(process.map(&:location)).to contain_exactly( - 'myfolder/file1.yml', 'myfolder/file2.yml', 'myfolder/file3.yml' - ) - end + it 'calls Gitaly only once for all files', :request_store do + # 1 for project.commit.id, 3 for the sha check, 1 for the files + expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(5) + end + + context 'when the FF ci_batch_request_for_local_and_project_includes is disabled' do + before do + stub_feature_flags(ci_batch_request_for_local_and_project_includes: false) + end - it 'adds files to the expandset' do - expect { process }.to change { context.expandset.count }.by(3) + it 'calls Gitaly for each file', :request_store do + # 1 for project.commit.id, 3 for the sha check, 3 for the files + expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(7) + end + end end - it 'calls Gitaly only once for all files', :request_store do - # 1 for project.commit.id, 3 for the sha check, 1 for the files - expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(5) + context 'when a file includes other files' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'nested_configs.yml' }, context) + ] + end + + it 'returns an array of file objects with combined hash' do + expect(process.map(&:to_hash)).to contain_exactly( + { my_build: { script: 'echo Hello World' }, + my_test: { script: 'echo Hello World' }, + remote_test: { script: 'echo Hello World' } } + ) + end end - context 'when the FF ci_batch_request_for_local_and_project_includes is disabled' do - before do - stub_feature_flags(ci_batch_request_for_local_and_project_includes: false) + context 'when there is an invalid file' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/invalid.yml' }, context) + ] end - it 'calls Gitaly for each file', :request_store do - # 1 for project.commit.id, 3 for the sha check, 3 for the files - expect { process }.to change { Gitlab::GitalyClient.get_request_count }.by(7) + it 'adds an error to the file' do + expect(process.first.errors).to include("Local file `myfolder/invalid.yml` does not exist!") end end end - context 'when a file includes other files' do - let(:files) do - [ - Gitlab::Ci::Config::External::File::Local.new({ local: 'nested_configs.yml' }, context) - ] + it_behaves_like 'when FF ci_fix_max_includes is enabled' + + context 'when FF ci_fix_max_includes is disabled' do + before do + stub_feature_flags(ci_fix_max_includes: false) end - it 'returns an array of file objects with combined hash' do - expect(process.map(&:to_hash)).to contain_exactly( - { my_build: { script: 'echo Hello World' }, - my_test: { script: 'echo Hello World' }, - remote_test: { script: 'echo Hello World' } } - ) + it_behaves_like 'when FF ci_fix_max_includes is enabled' + + context 'when max_includes is exceeded' do + context 'when files are nested' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'nested_configs.yml' }, context) + ] + end + + before do + allow(context).to receive(:max_includes).and_return(1) + end + + it 'raises Processor::IncludeError' do + expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) + end + end + + context 'when files are not nested' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file2.yml' }, context) + ] + end + + before do + allow(context).to receive(:max_includes).and_return(1) + end + + it 'raises Mapper::TooManyIncludesError' do + expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + end + end end end - context 'when there is an invalid file' do - let(:files) do - [ - Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/invalid.yml' }, context) - ] - end + describe 'max includes detection' do + shared_examples 'when total file count is equal to max_includes' do + before do + allow(context).to receive(:max_includes).and_return(expected_total_file_count) + end + + it 'adds the expected number of files to expandset' do + process - it 'adds an error to the file' do - expect(process.first.errors).to include("Local file `myfolder/invalid.yml` does not exist!") + expect(context.expandset.count).to eq(expected_total_file_count) + end + + it 'does not raise an error' do + expect { process }.not_to raise_error + end end - end - context 'when max_includes is exceeded' do context 'when files are nested' do let(:files) do [ @@ -180,13 +243,19 @@ ] end - before do - allow(context).to receive(:max_includes).and_return(1) - end + let(:expected_total_file_count) { 4 } - it 'raises Processor::IncludeError' do - expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) + context 'when total file count exceeds max_includes' do + before do + allow(context).to receive(:max_includes).and_return(expected_total_file_count - 1) + end + + it 'raises Processor::IncludeError' do + expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) + end end + + it_behaves_like 'when total file count is equal to max_includes' end context 'when files are not nested' do @@ -197,13 +266,19 @@ ] end - before do - allow(context).to receive(:max_includes).and_return(1) - end + let(:expected_total_file_count) { files.count } - it 'raises Mapper::TooManyIncludesError' do - expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + context 'when total file count exceeds max_includes' do + before do + allow(context).to receive(:max_includes).and_return(expected_total_file_count - 1) + end + + it 'raises Mapper::TooManyIncludesError' do + expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + end end + + it_behaves_like 'when total file count is equal to max_includes' end end end diff --git a/spec/lib/gitlab/ci/config/external/mapper_spec.rb b/spec/lib/gitlab/ci/config/external/mapper_spec.rb index a053c3047de2f1..bc05ac380deab1 100644 --- a/spec/lib/gitlab/ci/config/external/mapper_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper_spec.rb @@ -236,9 +236,20 @@ expect { process }.not_to raise_error end - it 'has expanset with one' do + it 'has expanset with two' do process - expect(context.expandset.size).to eq(1) + expect(context.expandset.size).to eq(2) + end + + context 'when FF ci_fix_max_includes is disabled' do + before do + stub_feature_flags(ci_fix_max_includes: false) + end + + it 'has expanset with one' do + process + expect(context.expandset.size).to eq(1) + end end end -- GitLab