Validate regex before using `CleanupContainerRepositoryWorker`

Three guards have been added:
1. The service used by the worker will not raise an error
upon receiving an invalid regex but it will return an error
response
2. The expiration container policy will not schedule the
next run for the given container expiration policy if it is
valid. In addition, the given container expiration policy
will be disabled
3. Added a new UntrustedRegexp validator for Grape APIs.
This one has been used in `API::ProjectContainerRepositories` to prevent
enqueuing a job with invalid regex

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

app/services/container_expiration_policy_service.rb

 # 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|

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

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

changelogs/unreleased/216088-disable-container-expiration-policy-when-invalid-regex-is-present.yml

+---
+title: Validate regex before sending them to CleanupContainerRepositoryWorker
+merge_request: 34282
+author:
+type: added

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)

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`. |
 

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

lib/api/validations/validators/untrusted_regexp.rb

+# 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

spec/lib/api/validations/validators/untrusted_regexp_spec.rb

+# 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

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

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
 

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

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' }

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