From 3ed2efb3d4c7a1abfb079ba361e04bc9d7849747 Mon Sep 17 00:00:00 2001
From: David Fernandez <dfernandez@gitlab.com>
Date: Tue, 16 Jun 2020 04:31:50 +0000
Subject: [PATCH] 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 | 4 ++
.../container_expiration_policy_service.rb | 7 +++
.../cleanup_tags_service.rb | 12 ++++++
.../container_expiration_policy_worker.rb | 2 +
...n-policy-when-invalid-regex-is-present.yml | 5 +++
config/initializers/grape_validators.rb | 1 +
doc/api/container_registry.md | 6 +--
lib/api/project_container_repositories.rb | 6 +--
.../validators/untrusted_regexp.rb | 19 ++++++++
.../validators/untrusted_regexp_spec.rb | 28 ++++++++++++
.../container_expiration_policy_spec.rb | 10 +++++
.../project_container_repositories_spec.rb | 34 +++++++++++++++
...ontainer_expiration_policy_service_spec.rb | 15 +++++++
.../cleanup_tags_service_spec.rb | 43 ++++++++++++++++++-
...container_expiration_policy_worker_spec.rb | 17 ++++++++
15 files changed, 202 insertions(+), 7 deletions(-)
create mode 100644 changelogs/unreleased/216088-disable-container-expiration-policy-when-invalid-regex-is-present.yml
create mode 100644 lib/api/validations/validators/untrusted_regexp.rb
create mode 100644 spec/lib/api/validations/validators/untrusted_regexp_spec.rb
diff --git a/app/models/container_expiration_policy.rb b/app/models/container_expiration_policy.rb
index fb1e0ce255e0d..b1dd720d9082f 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 82274fd866889..80f3229832323 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 b53a9c1561e4d..c5809c11ea9a0 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 e1544be5aed13..96590e165ae13 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 0000000000000..0a8df78d0c71b
--- /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 9d2b6dc9bd121..22f2c9ecf9247 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 dbfda65a131cb..d4a4fc1a73302 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 f378b87d4b649..2a0099018d94b 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 0000000000000..ec623684e67e8
--- /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 0000000000000..491bf94fd7930
--- /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 7d9c763889747..588685b04bffd 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 91905635c3fdd..471fc99117b3d 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 b2f2b2e1236e5..97715b990efe8 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 01f09f208fd6f..11ea7d5167352 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 48ab161463375..b15a28dcdca51 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
--
GitLab