diff options
-rw-r--r-- | app/services/ci/list_config_variables_service.rb | 3 | ||||
-rw-r--r-- | lib/gitlab/checks/tag_check.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/ci/lint.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/ci/yaml_processor.rb | 31 | ||||
-rw-r--r-- | spec/lib/gitlab/checks/tag_check_spec.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/lint_spec.rb | 56 | ||||
-rw-r--r-- | spec/lib/gitlab/ci/yaml_processor_spec.rb | 82 | ||||
-rw-r--r-- | spec/models/blob_viewer/gitlab_ci_yml_spec.rb | 36 | ||||
-rw-r--r-- | spec/services/ci/list_config_variables_service_spec.rb | 45 | ||||
-rw-r--r-- | spec/support/shared_contexts/ci/yaml_processor_shared_context.rb | 22 |
10 files changed, 276 insertions, 13 deletions
diff --git a/app/services/ci/list_config_variables_service.rb b/app/services/ci/list_config_variables_service.rb index 1020e98f463..e028d7252ae 100644 --- a/app/services/ci/list_config_variables_service.rb +++ b/app/services/ci/list_config_variables_service.rb @@ -32,7 +32,8 @@ module Ci config.content, project: project, user: current_user, - sha: sha + sha: sha, + verify_project_sha: true ).execute result.valid? ? result.root_variables_with_prefill_data : {} diff --git a/lib/gitlab/checks/tag_check.rb b/lib/gitlab/checks/tag_check.rb index 5c43ca946b5..4505bcb5411 100644 --- a/lib/gitlab/checks/tag_check.rb +++ b/lib/gitlab/checks/tag_check.rb @@ -39,6 +39,10 @@ module Gitlab def prohibited_tag_checks return if deletion? + unless Gitlab::GitRefValidator.validate(tag_name) + raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_tag_name] + end + if tag_name.start_with?("refs/tags/") # rubocop: disable Style/GuardClause raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_tag_name] end diff --git a/lib/gitlab/ci/lint.rb b/lib/gitlab/ci/lint.rb index e0112a1b1c2..54861e2769e 100644 --- a/lib/gitlab/ci/lint.rb +++ b/lib/gitlab/ci/lint.rb @@ -28,6 +28,9 @@ module Gitlab def initialize(project:, current_user:, sha: nil) @project = project @current_user = current_user + # If the `sha` is not provided, the default is the project's head commit (or nil). In such case, we + # don't need to call `YamlProcessor.verify_project_sha!`, which prevents redundant calls to Gitaly. + @verify_project_sha = sha.present? @sha = sha || project&.repository&.commit&.sha end @@ -77,6 +80,7 @@ module Gitlab Gitlab::Ci::YamlProcessor.new(content, project: @project, user: @current_user, sha: @sha, + verify_project_sha: @verify_project_sha, logger: logger).execute end end diff --git a/lib/gitlab/ci/yaml_processor.rb b/lib/gitlab/ci/yaml_processor.rb index 3a0173d1548..289f41b4ec7 100644 --- a/lib/gitlab/ci/yaml_processor.rb +++ b/lib/gitlab/ci/yaml_processor.rb @@ -8,6 +8,8 @@ module Gitlab module Ci class YamlProcessor + include Gitlab::Utils::StrongMemoize + ValidationError = Class.new(StandardError) def initialize(config_content, opts = {}) @@ -28,6 +30,8 @@ module Gitlab return Result.new(errors: ['Please provide content of .gitlab-ci.yml']) end + verify_project_sha! if verify_project_sha? + @ci_config = Gitlab::Ci::Config.new(@config_content, **@opts) unless @ci_config.valid? @@ -47,6 +51,15 @@ module Gitlab @opts[:project] end + def sha + @opts[:sha] + end + + def verify_project_sha? + @opts.delete(:verify_project_sha) || false + end + strong_memoize_attr :verify_project_sha? + def run_logical_validations! @stages = @ci_config.stages @jobs = @ci_config.normalized_jobs @@ -191,6 +204,24 @@ module Gitlab def error!(message) raise ValidationError, message end + + def verify_project_sha! + return unless project && sha && project.repository_exists? && project.commit(sha) + + unless project_ref_contains_sha? + error!('Could not validate configuration. Config originates from external project') + end + end + + def project_ref_contains_sha? + # A 5-minute cache TTL is sufficient to prevent Gitaly load issues while also mitigating rare + # use cases concerning stale data. For example, when an external commit gets merged into the + # project, there may be at most a 5-minute window where the `sha` is still considered external. + Rails.cache.fetch(['project', project.id, 'ref/contains/sha', sha], expires_in: 5.minutes) do + repo = project.repository + repo.branch_names_contains(sha, limit: 1).any? || repo.tag_names_contains(sha, limit: 1).any? + end + end end end end diff --git a/spec/lib/gitlab/checks/tag_check_spec.rb b/spec/lib/gitlab/checks/tag_check_spec.rb index e75b0459337..60d3eb4bfb3 100644 --- a/spec/lib/gitlab/checks/tag_check_spec.rb +++ b/spec/lib/gitlab/checks/tag_check_spec.rb @@ -15,6 +15,12 @@ RSpec.describe Gitlab::Checks::TagCheck, feature_category: :source_code_manageme end context "prohibited tags check" do + it 'prohibits tags name that include refs/heads at the head' do + allow(subject).to receive(:tag_name).and_return("refs/heads/foo") + + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, "You cannot create a tag with a prohibited pattern.") + end + it "prohibits tag names that include refs/tags/ at the head" do allow(subject).to receive(:tag_name).and_return("refs/tags/foo") diff --git a/spec/lib/gitlab/ci/lint_spec.rb b/spec/lib/gitlab/ci/lint_spec.rb index b238e9161eb..4196aad2db4 100644 --- a/spec/lib/gitlab/ci/lint_spec.rb +++ b/spec/lib/gitlab/ci/lint_spec.rb @@ -6,8 +6,9 @@ RSpec.describe Gitlab::Ci::Lint, feature_category: :pipeline_composition do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } - let(:lint) { described_class.new(project: project, current_user: user) } + let(:sha) { nil } let(:ref) { project.default_branch } + let(:lint) { described_class.new(project: project, current_user: user, sha: sha) } describe '#validate' do subject { lint.validate(content, dry_run: dry_run, ref: ref) } @@ -250,6 +251,59 @@ RSpec.describe Gitlab::Ci::Lint, feature_category: :pipeline_composition do subject end + + context 'when sha is provided' do + let(:sha) { project.commit.sha } + + it 'runs YamlProcessor with verify_project_sha: true' do + expect(Gitlab::Ci::YamlProcessor) + .to receive(:new) + .with(content, a_hash_including(verify_project_sha: true)) + .and_call_original + + subject + end + + it_behaves_like 'content is valid' + + context 'when the sha is invalid' do + let(:sha) { 'invalid-sha' } + + it_behaves_like 'content is valid' + end + + context 'when the sha is from a fork' do + include_context 'when a project repository contains a forked commit' + + let(:sha) { forked_commit_sha } + + context 'when a project ref contains the sha' do + before do + mock_branch_contains_forked_commit_sha + end + + it_behaves_like 'content is valid' + end + + context 'when a project ref does not contain the sha' do + it 'returns an error' do + expect(subject).not_to be_valid + expect(subject.errors).to include(/Could not validate configuration/) + end + end + end + end + + context 'when sha is not provided' do + it 'runs YamlProcessor with verify_project_sha: false' do + expect(Gitlab::Ci::YamlProcessor) + .to receive(:new) + .with(content, a_hash_including(verify_project_sha: false)) + .and_call_original + + subject + end + end end context 'when using dry run mode' do diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index c09c0b31e97..5cfd8d9b9fb 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -3607,6 +3607,88 @@ module Gitlab end end end + + describe 'verify project sha', :use_clean_rails_redis_caching do + include_context 'when a project repository contains a forked commit' + + let(:config) { YAML.dump(job: { script: 'echo' }) } + let(:verify_project_sha) { true } + let(:sha) { forked_commit_sha } + + let(:processor) { described_class.new(config, project: project, sha: sha, verify_project_sha: verify_project_sha) } + + subject { processor.execute } + + shared_examples 'when the processor is executed twice consecutively' do |branch_names_contains_sha = false| + it 'calls Gitaly only once for each ref type' do + expect(repository).to receive(:branch_names_contains).once.and_call_original + expect(repository).to receive(:tag_names_contains).once.and_call_original unless branch_names_contains_sha + + 2.times { processor.execute } + end + end + + context 'when a project branch contains the forked commit sha' do + before_all do + repository.add_branch(project.owner, 'branch1', forked_commit_sha) + end + + after(:all) do + repository.rm_branch(project.owner, 'branch1') + end + + it { is_expected.to be_valid } + + it_behaves_like 'when the processor is executed twice consecutively', true + end + + context 'when a project tag contains the forked commit sha' do + before_all do + repository.add_tag(project.owner, 'tag1', forked_commit_sha) + end + + after(:all) do + repository.rm_tag(project.owner, 'tag1') + end + + it { is_expected.to be_valid } + + it_behaves_like 'when the processor is executed twice consecutively' + end + + context 'when a project ref does not contain the forked commit sha' do + it 'returns an error' do + is_expected.not_to be_valid + expect(subject.errors).to include(/Could not validate configuration/) + end + + it_behaves_like 'when the processor is executed twice consecutively' + end + + context 'when verify_project_sha option is false' do + let(:verify_project_sha) { false } + + it { is_expected.to be_valid } + end + + context 'when project is not provided' do + let(:project) { nil } + + it { is_expected.to be_valid } + end + + context 'when sha is not provided' do + let(:sha) { nil } + + it { is_expected.to be_valid } + end + + context 'when sha is invalid' do + let(:sha) { 'invalid-sha' } + + it { is_expected.to be_valid } + end + end end end end diff --git a/spec/models/blob_viewer/gitlab_ci_yml_spec.rb b/spec/models/blob_viewer/gitlab_ci_yml_spec.rb index 803614d90a5..36b75e5338a 100644 --- a/spec/models/blob_viewer/gitlab_ci_yml_spec.rb +++ b/spec/models/blob_viewer/gitlab_ci_yml_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe BlobViewer::GitlabCiYml do +RSpec.describe BlobViewer::GitlabCiYml, feature_category: :source_code_management do include FakeBlobHelpers include RepoHelpers @@ -13,18 +13,20 @@ RSpec.describe BlobViewer::GitlabCiYml do let(:blob) { fake_blob(path: '.gitlab-ci.yml', data: data) } let(:sha) { sample_commit.id } - subject { described_class.new(blob) } + subject(:blob_viewer) { described_class.new(blob) } describe '#validation_message' do + subject(:validation_message) { blob_viewer.validation_message(project: project, sha: sha, user: user) } + it 'calls prepare! on the viewer' do - expect(subject).to receive(:prepare!) + expect(blob_viewer).to receive(:prepare!) - subject.validation_message(project: project, sha: sha, user: user) + validation_message end context 'when the configuration is valid' do it 'returns nil' do - expect(subject.validation_message(project: project, sha: sha, user: user)).to be_nil + expect(validation_message).to be_nil end end @@ -32,7 +34,29 @@ RSpec.describe BlobViewer::GitlabCiYml do let(:data) { 'oof' } it 'returns the error message' do - expect(subject.validation_message(project: project, sha: sha, user: user)).to eq('Invalid configuration format') + expect(validation_message).to eq('Invalid configuration format') + end + end + + context 'when the sha is from a fork' do + include_context 'when a project repository contains a forked commit' + + let(:sha) { forked_commit_sha } + + context 'when a project ref contains the sha' do + before do + mock_branch_contains_forked_commit_sha + end + + it 'returns nil' do + expect(validation_message).to be_nil + end + end + + context 'when a project ref does not contain the sha' do + it 'returns an error' do + expect(validation_message).to match(/Could not validate configuration/) + end end end end diff --git a/spec/services/ci/list_config_variables_service_spec.rb b/spec/services/ci/list_config_variables_service_spec.rb index 07c9085b83a..ccc8b77f6b2 100644 --- a/spec/services/ci/list_config_variables_service_spec.rb +++ b/spec/services/ci/list_config_variables_service_spec.rb @@ -32,15 +32,50 @@ RSpec.describe Ci::ListConfigVariablesService, } end + let(:expected_result) do + { + 'KEY1' => { value: 'val 1', description: 'description 1' }, + 'KEY2' => { value: 'val 2', description: '' }, + 'KEY3' => { value: 'val 3' }, + 'KEY4' => { value: 'val 4' } + } + end + before do synchronous_reactive_cache(service) end - it 'returns variable list' do - expect(result['KEY1']).to eq({ value: 'val 1', description: 'description 1' }) - expect(result['KEY2']).to eq({ value: 'val 2', description: '' }) - expect(result['KEY3']).to eq({ value: 'val 3' }) - expect(result['KEY4']).to eq({ value: 'val 4' }) + it 'returns variables list' do + expect(result).to eq(expected_result) + end + + context 'when the ref is a sha from a fork' do + include_context 'when a project repository contains a forked commit' + + before do + allow_next_instance_of(Gitlab::Ci::ProjectConfig) do |instance| + allow(instance).to receive(:exists?).and_return(true) + allow(instance).to receive(:content).and_return(YAML.dump(ci_config)) + end + end + + let(:ref) { forked_commit_sha } + + context 'when a project ref contains the sha' do + before do + mock_branch_contains_forked_commit_sha + end + + it 'returns variables list' do + expect(result).to eq(expected_result) + end + end + + context 'when a project ref does not contain the sha' do + it 'returns empty response' do + expect(result).to eq({}) + end + end end end diff --git a/spec/support/shared_contexts/ci/yaml_processor_shared_context.rb b/spec/support/shared_contexts/ci/yaml_processor_shared_context.rb new file mode 100644 index 00000000000..3b4d6fcef6f --- /dev/null +++ b/spec/support/shared_contexts/ci/yaml_processor_shared_context.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +RSpec.shared_context 'when a project repository contains a forked commit' do + include ProjectForksHelper + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:repository) { project.repository } + let_it_be(:forked_project) { fork_project(project, project.owner, repository: true) } + + let_it_be(:forked_commit_sha) do + forked_project.repository.create_file(project.owner, 'file.txt', 'file', message: 'test', branch_name: 'master') + end + + before_all do + # Creating this MR moves the forked commit to the project repository + create(:merge_request, source_project: forked_project, target_project: project) + end + + def mock_branch_contains_forked_commit_sha + allow(repository).to receive(:branch_names_contains).with(forked_commit_sha, limit: 1).and_return(['branch1']) + end +end |