diff --git a/app/models/container_expiration_policy.rb b/app/models/container_expiration_policy.rb index fb1e0ce255e0d16e389f0cc7830a9ac8879bba66..b1dd720d9082f0658991b127e084ec7c60dec824 100644 --- a/app/models/container_expiration_policy.rb +++ b/app/models/container_expiration_policy.rb @@ -52,4 +52,8 @@ def self.older_than_options def set_next_run_at self.next_run_at = Time.zone.now + ChronicDuration.parse(cadence).seconds end + + def disable! + update_attribute(:enabled, false) + end end diff --git a/app/services/container_expiration_policy_service.rb b/app/services/container_expiration_policy_service.rb index 82274fd866889e4377567ea1b49657c0da4bfd33..80f3229832323c06a193e898534d1a1dd9cd3e96 100644 --- a/app/services/container_expiration_policy_service.rb +++ b/app/services/container_expiration_policy_service.rb @@ -1,7 +1,14 @@ # frozen_string_literal: true class ContainerExpirationPolicyService < BaseService + InvalidPolicyError = Class.new(StandardError) + def execute(container_expiration_policy) + unless container_expiration_policy.valid? + container_expiration_policy.disable! + raise InvalidPolicyError + end + container_expiration_policy.schedule_next_run! container_expiration_policy.container_repositories.find_each do |container_repository| diff --git a/app/services/projects/container_repository/cleanup_tags_service.rb b/app/services/projects/container_repository/cleanup_tags_service.rb index b53a9c1561e4d52021011975a9029835e64196c4..c5809c11ea9a01f0fe40e4d6c4904034893b3d4d 100644 --- a/app/services/projects/container_repository/cleanup_tags_service.rb +++ b/app/services/projects/container_repository/cleanup_tags_service.rb @@ -6,6 +6,7 @@ class CleanupTagsService < BaseService def execute(container_repository) return error('feature disabled') unless can_use? return error('access denied') unless can_destroy? + return error('invalid regex') unless valid_regex? tags = container_repository.tags tags = without_latest(tags) @@ -76,6 +77,17 @@ def can_destroy? def can_use? Feature.enabled?(:container_registry_cleanup, project, default_enabled: true) end + + def valid_regex? + %w(name_regex_delete name_regex name_regex_keep).each do |param_name| + regex = params[param_name] + Gitlab::UntrustedRegexp.new(regex) unless regex.blank? + end + true + rescue RegexpError => e + Gitlab::ErrorTracking.log_exception(e, project_id: project.id) + false + end end end end diff --git a/app/workers/container_expiration_policy_worker.rb b/app/workers/container_expiration_policy_worker.rb index e1544be5aed13d335f4d650d60f08c45789aa3d1..96590e165ae13d4d202558c2111be1d6857ac485 100644 --- a/app/workers/container_expiration_policy_worker.rb +++ b/app/workers/container_expiration_policy_worker.rb @@ -12,6 +12,8 @@ def perform user: container_expiration_policy.project.owner) do |project:, user:| ContainerExpirationPolicyService.new(project, user) .execute(container_expiration_policy) + rescue ContainerExpirationPolicyService::InvalidPolicyError => e + Gitlab::ErrorTracking.log_exception(e, container_expiration_policy_id: container_expiration_policy.id) end end end diff --git a/changelogs/unreleased/216088-disable-container-expiration-policy-when-invalid-regex-is-present.yml b/changelogs/unreleased/216088-disable-container-expiration-policy-when-invalid-regex-is-present.yml new file mode 100644 index 0000000000000000000000000000000000000000..0a8df78d0c71bbd3dd745e9e1d4cb11ee39385a7 --- /dev/null +++ b/changelogs/unreleased/216088-disable-container-expiration-policy-when-invalid-regex-is-present.yml @@ -0,0 +1,5 @@ +--- +title: Validate regex before sending them to CleanupContainerRepositoryWorker +merge_request: 34282 +author: +type: added diff --git a/config/initializers/grape_validators.rb b/config/initializers/grape_validators.rb index 9d2b6dc9bd12131806b3172f999b810a291d8795..22f2c9ecf924778cb489c6c15a96e63fbdd7c474 100644 --- a/config/initializers/grape_validators.rb +++ b/config/initializers/grape_validators.rb @@ -7,3 +7,4 @@ Grape::Validations.register_validator(:integer_none_any, ::API::Validations::Validators::IntegerNoneAny) Grape::Validations.register_validator(:array_none_any, ::API::Validations::Validators::ArrayNoneAny) Grape::Validations.register_validator(:check_assignees_count, ::API::Validations::Validators::CheckAssigneesCount) +Grape::Validations.register_validator(:untrusted_regexp, ::API::Validations::Validators::UntrustedRegexp) diff --git a/doc/api/container_registry.md b/doc/api/container_registry.md index dbfda65a131cb56fb404d89a9d90b156d4f3f1e5..d4a4fc1a7330275fc85c94f94aa4abdc4c087db8 100644 --- a/doc/api/container_registry.md +++ b/doc/api/container_registry.md @@ -240,9 +240,9 @@ DELETE /projects/:id/registry/repositories/:repository_id/tags | --------- | ---- | -------- | ----------- | | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user. | | `repository_id` | integer | yes | The ID of registry repository. | -| `name_regex` | string | no | The [re2](https://github.com/google/re2/wiki/Syntax) regex of the name to delete. To delete all tags specify `.*`. **Note:** `name_regex` is deprecated in favor of `name_regex_delete`.| -| `name_regex_delete` | string | yes | The [re2](https://github.com/google/re2/wiki/Syntax) regex of the name to delete. To delete all tags specify `.*`.| -| `name_regex_keep` | string | no | The [re2](https://github.com/google/re2/wiki/Syntax) regex of the name to keep. This value will override any matches from `name_regex_delete`. Note: setting to `.*` will result in a no-op. | +| `name_regex` | string | no | The [re2](https://github.com/google/re2/wiki/Syntax) regex of the name to delete. To delete all tags specify `.*`. **Note:** `name_regex` is deprecated in favor of `name_regex_delete`. This field is validated. | +| `name_regex_delete` | string | yes | The [re2](https://github.com/google/re2/wiki/Syntax) regex of the name to delete. To delete all tags specify `.*`. This field is validated. | +| `name_regex_keep` | string | no | The [re2](https://github.com/google/re2/wiki/Syntax) regex of the name to keep. This value will override any matches from `name_regex_delete`. This field is validated. Note: setting to `.*` will result in a no-op. | | `keep_n` | integer | no | The amount of latest tags of given name to keep. | | `older_than` | string | no | Tags to delete that are older than the given time, written in human readable form `1h`, `1d`, `1month`. | diff --git a/lib/api/project_container_repositories.rb b/lib/api/project_container_repositories.rb index f378b87d4b6490d2434c0a68b4875eb2d3df1976..2a0099018d94b659145fd617b4759e5798f3e1bb 100644 --- a/lib/api/project_container_repositories.rb +++ b/lib/api/project_container_repositories.rb @@ -70,11 +70,11 @@ class ProjectContainerRepositories < Grape::API end params do requires :repository_id, type: Integer, desc: 'The ID of the repository' - optional :name_regex_delete, type: String, desc: 'The tag name regexp to delete, specify .* to delete all' - optional :name_regex, type: String, desc: 'The tag name regexp to delete, specify .* to delete all' + optional :name_regex_delete, type: String, untrusted_regexp: true, desc: 'The tag name regexp to delete, specify .* to delete all' + optional :name_regex, type: String, untrusted_regexp: true, desc: 'The tag name regexp to delete, specify .* to delete all' # require either name_regex (deprecated) or name_regex_delete, it is ok to have both at_least_one_of :name_regex, :name_regex_delete - optional :name_regex_keep, type: String, desc: 'The tag name regexp to retain' + optional :name_regex_keep, type: String, untrusted_regexp: true, desc: 'The tag name regexp to retain' optional :keep_n, type: Integer, desc: 'Keep n of latest tags with matching name' optional :older_than, type: String, desc: 'Delete older than: 1h, 1d, 1month' end diff --git a/lib/api/validations/validators/untrusted_regexp.rb b/lib/api/validations/validators/untrusted_regexp.rb new file mode 100644 index 0000000000000000000000000000000000000000..ec623684e67e8d0cc4456481edd8de8f72cb3203 --- /dev/null +++ b/lib/api/validations/validators/untrusted_regexp.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module API + module Validations + module Validators + class UntrustedRegexp < Grape::Validations::Base + def validate_param!(attr_name, params) + value = params[attr_name] + return unless value + + Gitlab::UntrustedRegexp.new(value) + rescue RegexpError => e + message = "is an invalid regexp: #{e.message}" + raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message + end + end + end + end +end diff --git a/spec/lib/api/validations/validators/untrusted_regexp_spec.rb b/spec/lib/api/validations/validators/untrusted_regexp_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..491bf94fd793042d99a837b5c539719dd2d8678a --- /dev/null +++ b/spec/lib/api/validations/validators/untrusted_regexp_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Validations::Validators::UntrustedRegexp do + include ApiValidatorsHelpers + + subject do + described_class.new(['test'], {}, false, scope.new) + end + + context 'valid regex' do + it 'does not raise a validation error' do + expect_no_validation_error('test' => 'test') + expect_no_validation_error('test' => '.*') + expect_no_validation_error('test' => Gitlab::Regex.environment_name_regex_chars) + end + end + + context 'invalid regex' do + it 'raises a validation error' do + expect_validation_error('test' => '[') + expect_validation_error('test' => '*foobar') + expect_validation_error('test' => '?foobar') + expect_validation_error('test' => '\A[^/%\s]+(..\z') + end + end +end diff --git a/spec/models/container_expiration_policy_spec.rb b/spec/models/container_expiration_policy_spec.rb index 7d9c763889747d9637c8cf0bbda8d11e1f653f65..588685b04bffd64e2cfb347794bdb14e8ea9b262 100644 --- a/spec/models/container_expiration_policy_spec.rb +++ b/spec/models/container_expiration_policy_spec.rb @@ -103,4 +103,14 @@ end end end + + describe '#disable!' do + let_it_be(:container_expiration_policy) { create(:container_expiration_policy) } + + subject { container_expiration_policy.disable! } + + it 'disables the container expiration policy' do + expect { subject }.to change { container_expiration_policy.reload.enabled }.from(true).to(false) + end + end end diff --git a/spec/requests/api/project_container_repositories_spec.rb b/spec/requests/api/project_container_repositories_spec.rb index 91905635c3fdd6e61b49bbc1c9160cc8dfb8af50..471fc99117b3db2e3bc60a1db504d980abd24a3e 100644 --- a/spec/requests/api/project_container_repositories_spec.rb +++ b/spec/requests/api/project_container_repositories_spec.rb @@ -223,6 +223,40 @@ expect(response).to have_gitlab_http_status(:accepted) end end + + context 'with invalid regex' do + let(:invalid_regex) { '*v10.' } + let(:lease_key) { "container_repository:cleanup_tags:#{root_repository.id}" } + + RSpec.shared_examples 'rejecting the invalid regex' do |param_name| + it 'does not enqueue a job' do + expect(CleanupContainerRepositoryWorker).not_to receive(:perform_async) + + subject + end + + it_behaves_like 'returning response status', :bad_request + + it 'returns an error message' do + subject + + expect(json_response['error']).to include("#{param_name} is an invalid regexp") + end + end + + before do + stub_last_activity_update + stub_exclusive_lease(lease_key, timeout: 1.hour) + end + + %i[name_regex_delete name_regex name_regex_keep].each do |param_name| + context "for #{param_name}" do + let(:params) { { param_name => invalid_regex } } + + it_behaves_like 'rejecting the invalid regex', param_name + end + end + end end end diff --git a/spec/services/container_expiration_policy_service_spec.rb b/spec/services/container_expiration_policy_service_spec.rb index b2f2b2e1236e553f83e19f46e90bf4ba53551422..97715b990efe8881fac95f5fed31aa88215a4bf5 100644 --- a/spec/services/container_expiration_policy_service_spec.rb +++ b/spec/services/container_expiration_policy_service_spec.rb @@ -27,5 +27,20 @@ expect(container_expiration_policy.next_run_at).to be > Time.zone.now end + + context 'with an invalid container expiration policy' do + before do + allow(container_expiration_policy).to receive(:valid?).and_return(false) + end + + it 'disables it' do + expect(container_expiration_policy).not_to receive(:schedule_next_run!) + expect(CleanupContainerRepositoryWorker).not_to receive(:perform_async) + + expect { subject } + .to change { container_expiration_policy.reload.enabled }.from(true).to(false) + .and raise_error(ContainerExpirationPolicyService::InvalidPolicyError) + end + end end end diff --git a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb index 01f09f208fd6f60e73c1ebe8eac1de4ba873e82d..11ea7d5167352df8d49d97400f0cd450f2980452 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -4,7 +4,7 @@ describe Projects::ContainerRepository::CleanupTagsService do let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :private) } + let_it_be(:project, reload: true) { create(:project, :private) } let_it_be(:repository) { create(:container_repository, :root, project: project) } let(:service) { described_class.new(project, user, params) } @@ -72,6 +72,47 @@ end end + context 'with invalid regular expressions' do + RSpec.shared_examples 'handling an invalid regex' do + it 'keeps all tags' do + expect(Projects::ContainerRepository::DeleteTagsService) + .not_to receive(:new) + subject + end + + it 'returns an error' do + response = subject + + expect(response[:status]).to eq(:error) + expect(response[:message]).to eq('invalid regex') + end + + it 'calls error tracking service' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).and_call_original + + subject + end + end + + context 'when name_regex_delete is invalid' do + let(:params) { { 'name_regex_delete' => '*test*' } } + + it_behaves_like 'handling an invalid regex' + end + + context 'when name_regex is invalid' do + let(:params) { { 'name_regex' => '*test*' } } + + it_behaves_like 'handling an invalid regex' + end + + context 'when name_regex_keep is invalid' do + let(:params) { { 'name_regex_keep' => '*test*' } } + + it_behaves_like 'handling an invalid regex' + end + end + context 'when delete regex matching specific tags is used' do let(:params) do { 'name_regex_delete' => 'C|D' } diff --git a/spec/workers/container_expiration_policy_worker_spec.rb b/spec/workers/container_expiration_policy_worker_spec.rb index 48ab16146337520122b6764aa617fa0339d88d8c..b15a28dcdca511e1704e6fe751f897dec32abb5a 100644 --- a/spec/workers/container_expiration_policy_worker_spec.rb +++ b/spec/workers/container_expiration_policy_worker_spec.rb @@ -53,5 +53,22 @@ subject end end + + context 'an invalid policy' do + let_it_be(:container_expiration_policy) { create(:container_expiration_policy, :runnable) } + let_it_be(:user) {container_expiration_policy.project.owner } + + before do + container_expiration_policy.update_column(:name_regex, '*production') + end + + it 'runs the policy and tracks an error' do + expect(ContainerExpirationPolicyService) + .to receive(:new).with(container_expiration_policy.project, user).and_call_original + expect(Gitlab::ErrorTracking).to receive(:log_exception).with(instance_of(ContainerExpirationPolicyService::InvalidPolicyError), container_expiration_policy_id: container_expiration_policy.id) + + expect { subject }.to change { container_expiration_policy.reload.enabled }.from(true).to(false) + end + end end end