Merge branch...

Merge branch '216088-disable-container-expiration-policy-when-invalid-regex-is-present' into 'master'

Validate regex before enqueuing a CleanupContainerRepositoryWorker job

See merge request gitlab-org/gitlab!34282

app/models/container_expiration_policy.rb

@@ -52,4 +52,8 @@ class ContainerExpirationPolicy < ApplicationRecord
   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 @@ module Projects
       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 @@ module Projects
       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 @@ class ContainerExpirationPolicyWorker # rubocop:disable Scalability/IdempotentWo
                    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(:git_sha, ::API::Validations::Validators::
 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 @@ module 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 @@ RSpec.describe ContainerExpirationPolicy, type: :model do
       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 @@ describe API::ProjectContainerRepositories do
           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 @@ describe ContainerExpirationPolicyService do
 
       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 @@ require 'spec_helper'
 
 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 @@ describe Projects::ContainerRepository::CleanupTagsService do
       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 @@ describe ContainerExpirationPolicyWorker do
         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