Merge branch '216088-regex-validation-on-container-expiration-policy' into 'master'

Add regexp validation on ContainerExpirationPolicy

See merge request gitlab-org/gitlab!34063

app/models/container_expiration_policy.rb

@@ -13,6 +13,8 @@ class ContainerExpirationPolicy < ApplicationRecord
   validates :cadence, presence: true, inclusion: { in: ->(_) { self.cadence_options.stringify_keys } }
   validates :older_than, inclusion: { in: ->(_) { self.older_than_options.stringify_keys } }, allow_nil: true
   validates :keep_n, inclusion: { in: ->(_) { self.keep_n_options.keys } }, allow_nil: true
+  validates :name_regex, untrusted_regexp: true, if: :enabled?
+  validates :name_regex_keep, untrusted_regexp: true, if: :enabled?
 
   scope :active, -> { where(enabled: true) }
   scope :preloaded, -> { preload(project: [:route]) }

changelogs/unreleased/216088-regex-validation-on-container-expiration-policy.yml

+---
+title: Container expiration policy regular expressions are now validated
+merge_request: 34063
+author:
+type: added

doc/user/packages/container_registry/index.md

@@ -532,6 +532,17 @@ The UI allows you to configure the following:
 - **Docker tags with names matching this regex pattern will expire:** the regex used to determine what tags should be expired. To qualify all tags for expiration, use the default value of `.*`.
 - **Docker tags with names matching this regex pattern will be preserved:** the regex used to determine what tags should be preserved. To preserve all tags, use the default value of `.*`.
 
+#### Troubleshooting expiration policies
+
+If you see the following message:
+
+"Something went wrong while updating the expiration policy."
+
+Check the regex patterns to ensure they are valid.
+
+You can use [Rubular](https://rubular.com/) to check your regex.
+View some common [regex pattern examples](#regex-pattern-examples).
+
 ### Managing project expiration policy through the API
 
 You can set, update, and disable the expiration policies using the GitLab API.

spec/features/projects/settings/registry_settings_spec.rb

@@ -29,7 +29,7 @@ RSpec.describe 'Project > Settings > CI/CD > Container registry tag expiration p
           select('7 days until tags are automatically removed', from: 'Expiration interval:')
           select('Every day', from: 'Expiration schedule:')
           select('50 tags per image name', from: 'Number of tags to retain:')
-          fill_in('Tags with names matching this regex pattern will expire:', with: '*-production')
+          fill_in('Tags with names matching this regex pattern will expire:', with: '.*-production')
         end
         submit_button = find('.card-footer .btn.btn-success')
         expect(submit_button).not_to be_disabled
@@ -38,6 +38,19 @@ RSpec.describe 'Project > Settings > CI/CD > Container registry tag expiration p
       toast = find('.gl-toast')
       expect(toast).to have_content('Expiration policy successfully saved.')
     end
+
+    it 'does not save expiration policy submit form with invalid regex' do
+      within '#js-registry-policies' do
+        within '.card-body' do
+          fill_in('Tags with names matching this regex pattern will expire:', with: '*-production')
+        end
+        submit_button = find('.card-footer .btn.btn-success')
+        expect(submit_button).not_to be_disabled
+        submit_button.click
+      end
+      toast = find('.gl-toast')
+      expect(toast).to have_content('Something went wrong while updating the expiration policy.')
+    end
   end
 
   context 'when registry is disabled' do

spec/models/container_expiration_policy_spec.rb

@@ -37,6 +37,37 @@ RSpec.describe ContainerExpirationPolicy, type: :model do
       it { is_expected.to allow_value(nil).for(:keep_n) }
       it { is_expected.not_to allow_value('foo').for(:keep_n) }
     end
+
+    context 'with a set of regexps' do
+      valid_regexps = %w[master .* v.+ v10.1.* (?:v.+|master|release)]
+      invalid_regexps = ['[', '(?:v.+|master|release']
+
+      valid_regexps.each do |valid_regexp|
+        it { is_expected.to allow_value(valid_regexp).for(:name_regex) }
+        it { is_expected.to allow_value(valid_regexp).for(:name_regex_keep) }
+      end
+
+      invalid_regexps.each do |invalid_regexp|
+        it { is_expected.not_to allow_value(invalid_regexp).for(:name_regex) }
+        it { is_expected.not_to allow_value(invalid_regexp).for(:name_regex_keep) }
+      end
+
+      context 'with a disabled container expiration policy' do
+        let_it_be(:container_expiration_policy) { create(:container_expiration_policy, :disabled) }
+
+        subject { container_expiration_policy }
+
+        valid_regexps.each do |valid_regexp|
+          it { is_expected.to allow_value(valid_regexp).for(:name_regex) }
+          it { is_expected.to allow_value(valid_regexp).for(:name_regex_keep) }
+        end
+
+        invalid_regexps.each do |invalid_regexp|
+          it { is_expected.to allow_value(invalid_regexp).for(:name_regex) }
+          it { is_expected.to allow_value(invalid_regexp).for(:name_regex_keep) }
+        end
+      end
+    end
   end
 
   describe '.preloaded' do

spec/requests/api/projects_spec.rb

@@ -2516,6 +2516,21 @@ describe API::Projects do
         expect(json_response['container_expiration_policy']['keep_n']).to eq(1)
         expect(json_response['container_expiration_policy']['name_regex_keep']).to eq('foo.*')
       end
+
+      it "doesn't update container_expiration_policy with invalid regex" do
+        project_param = {
+          container_expiration_policy_attributes: {
+            cadence: '1month',
+            keep_n: 1,
+            name_regex_keep: '['
+          }
+        }
+
+        put api("/projects/#{project3.id}", user4), params: project_param
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['message']['container_expiration_policy.name_regex_keep']).to contain_exactly('not valid RE2 syntax: missing ]: [')
+      end
     end
 
     context 'when authenticated as project developer' do