Merge branch 'feature/gb/remove-unsafe-regexps' into 'master'

Remove support for unsafe regular expressions

See merge request gitlab-org/gitlab!79611

config/feature_flags/development/allow_unsafe_ruby_regexp.yml → config/feature_flags/development/disable_unsafe_regexp.yml

 ---
-name: allow_unsafe_ruby_regexp
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/10566
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/257849
-milestone: '11.10'
+name: disable_unsafe_regexp
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79611
+rollout_issue_url: 
+milestone: '14.9'
 type: development
 group: group::pipeline execution
 default_enabled: false

doc/ci/jobs/job_control.md

@@ -834,13 +834,9 @@ due to computational complexity, and some features, like negative lookaheads, be
 Only a subset of features provided by [Ruby Regexp](https://ruby-doc.org/core/Regexp.html)
 are now supported.
 
-From GitLab 11.9.7 to GitLab 12.0, GitLab provided a feature flag to
-let you use unsafe regexp syntax. After migrating to safe syntax, you should disable
-this feature flag again:
-
-```ruby
-Feature.enable(:allow_unsafe_ruby_regexp)
-```
+From GitLab 11.9.7 to GitLab 14.9, GitLab provided a feature flag to let you
+use unsafe regexp syntax. We've fully migrated to RE2 now, and that feature
+flag is no longer available.
 
 ## CI/CD variable expressions
 

ee/spec/models/push_rule_spec.rb

@@ -65,11 +65,26 @@ RSpec.describe PushRule, :saas do
         subject.branch_name_allowed?('ee-feature-ee')
       end
 
-      it 'falls back to ruby regex engine' do
-        push_rule.update_column(:branch_name_regex, '(ee|ce).*\1')
+      context 'when unsafe regexps are available' do
+        it 'falls back to ruby regex engine' do
+          stub_feature_flags(disable_unsafe_regexp: false)
 
-        expect(subject.branch_name_allowed?('ee-feature-ee')).to be true
-        expect(subject.branch_name_allowed?('ee-feature-ce')).to be false
+          push_rule.update_column(:branch_name_regex, '(ee|ce).*\1')
+
+          expect(subject.branch_name_allowed?('ee-feature-ee')).to be true
+          expect(subject.branch_name_allowed?('ee-feature-ce')).to be false
+        end
+      end
+
+      context 'when unsafe regexps are disabled' do
+        it 'raises an exception' do
+          stub_feature_flags(disable_unsafe_regexp: true)
+
+          push_rule.update_column(:branch_name_regex, '(ee|ce).*\1')
+
+          expect { subject.branch_name_allowed?('ee-feature-ee') }
+            .to raise_error(described_class::MatchError)
+        end
       end
     end
   end

lib/gitlab/ci/build/policy/refs.rb

@@ -36,7 +36,7 @@ module Gitlab
             # the pattern matching does not work for merge requests pipelines
             if pipeline.branch? || pipeline.tag?
               regexp = Gitlab::UntrustedRegexp::RubySyntax
-                .fabricate(pattern, fallback: true, project: pipeline.project)
+                .fabricate(pattern, project: pipeline.project)
 
               if regexp
                 regexp.match?(pipeline.ref)

lib/gitlab/ci/config/entry/policy.rb

@@ -17,7 +17,7 @@ module Gitlab
             include ::Gitlab::Config::Entry::Validatable
 
             validations do
-              validates :config, array_of_strings_or_regexps_with_fallback: true
+              validates :config, array_of_strings_or_regexps: true
             end
 
             def value
@@ -38,7 +38,7 @@ module Gitlab
               validate :variables_expressions_syntax
 
               with_options allow_nil: true do
-                validates :refs, array_of_strings_or_regexps_with_fallback: true
+                validates :refs, array_of_strings_or_regexps: true
                 validates :kubernetes, allowed_values: %w[active]
                 validates :variables, array_of_strings: true
                 validates :changes, array_of_strings: true

lib/gitlab/config/entry/validators.rb

@@ -228,12 +228,6 @@ module Gitlab
             end
           end
 
-          protected
-
-          def fallback
-            false
-          end
-
           private
 
           def matches_syntax?(value)
@@ -242,7 +236,7 @@ module Gitlab
 
           def validate_regexp(value)
             matches_syntax?(value) &&
-              Gitlab::UntrustedRegexp::RubySyntax.valid?(value, fallback: fallback)
+              Gitlab::UntrustedRegexp::RubySyntax.valid?(value)
           end
         end
 
@@ -271,27 +265,6 @@ module Gitlab
           end
         end
 
-        class ArrayOfStringsOrRegexpsWithFallbackValidator < ArrayOfStringsOrRegexpsValidator
-          protected
-
-          # TODO
-          #
-          # Remove ArrayOfStringsOrRegexpsWithFallbackValidator class too when
-          # you are removing the `:allow_unsafe_ruby_regexp` feature flag.
-          #
-          def validation_message
-            if ::Feature.enabled?(:allow_unsafe_ruby_regexp, default_enabled: :yaml)
-              'should be an array of strings or regular expressions'
-            else
-              super
-            end
-          end
-
-          def fallback
-            true
-          end
-        end
-
         class ArrayOfStringsOrStringValidator < RegexpValidator
           def validate_each(record, attribute, value)
             unless validate_array_of_strings_or_string(value)

lib/gitlab/untrusted_regexp.rb

@@ -61,6 +61,16 @@ module Gitlab
     def self.with_fallback(pattern, multiline: false)
       UntrustedRegexp.new(pattern, multiline: multiline)
     rescue RegexpError
+      raise if Feature.enabled?(:disable_unsafe_regexp, default_enabled: :yaml)
+
+      if Feature.enabled?(:ci_unsafe_regexp_logger, type: :ops, default_enabled: :yaml)
+        Gitlab::AppJsonLogger.info(
+          class: self.name,
+          regexp: pattern.to_s,
+          fabricated: 'unsafe ruby regexp'
+        )
+      end
+
       Regexp.new(pattern)
     end
 

lib/gitlab/untrusted_regexp/ruby_syntax.rb

@@ -16,40 +16,23 @@ module Gitlab
 
       # The regexp can match the pattern `/.../`, but may not be fabricatable:
       # it can be invalid or incomplete: `/match ( string/`
-      def self.valid?(pattern, fallback: false)
-        !!self.fabricate(pattern, fallback: fallback)
+      def self.valid?(pattern)
+        !!self.fabricate(pattern)
       end
 
-      def self.fabricate(pattern, fallback: false, project: nil)
-        self.fabricate!(pattern, fallback: fallback, project: project)
+      def self.fabricate(pattern, project: nil)
+        self.fabricate!(pattern, project: project)
       rescue RegexpError
         nil
       end
 
-      def self.fabricate!(pattern, fallback: false, project: nil)
+      def self.fabricate!(pattern, project: nil)
         raise RegexpError, 'Pattern is not string!' unless pattern.is_a?(String)
 
         matches = pattern.match(PATTERN)
         raise RegexpError, 'Invalid regular expression!' if matches.nil?
 
-        begin
-          create_untrusted_regexp(matches[:regexp], matches[:flags])
-        rescue RegexpError
-          raise unless fallback &&
-              Feature.enabled?(:allow_unsafe_ruby_regexp, default_enabled: :yaml)
-
-          if Feature.enabled?(:ci_unsafe_regexp_logger, type: :ops, default_enabled: :yaml)
-            Gitlab::AppJsonLogger.info(
-              class: self.name,
-              regexp: pattern.to_s,
-              fabricated: 'unsafe ruby regexp',
-              project_id: project&.id,
-              project_path: project&.full_path
-            )
-          end
-
-          create_ruby_regexp(matches[:regexp], matches[:flags])
-        end
+        create_untrusted_regexp(matches[:regexp], matches[:flags])
       end
 
       def self.create_untrusted_regexp(pattern, flags)
@@ -58,15 +41,6 @@ module Gitlab
         UntrustedRegexp.new(pattern, multiline: false)
       end
       private_class_method :create_untrusted_regexp
-
-      def self.create_ruby_regexp(pattern, flags)
-        options = 0
-        options += Regexp::IGNORECASE if flags&.include?('i')
-        options += Regexp::MULTILINE if flags&.include?('m')
-
-        Regexp.new(pattern, options)
-      end
-      private_class_method :create_ruby_regexp
     end
   end
 end

spec/lib/gitlab/ci/build/policy/refs_spec.rb

@@ -149,26 +149,9 @@ RSpec.describe Gitlab::Ci::Build::Policy::Refs do
       context 'when unsafe regexp is used' do
         let(:subject) { described_class.new(['/^(?!master).+/']) }
 
-        context 'when allow_unsafe_ruby_regexp is disabled' do
-          before do
-            stub_feature_flags(allow_unsafe_ruby_regexp: false)
-          end
-
-          it 'ignores invalid regexp' do
-            expect(subject)
-              .not_to be_satisfied_by(pipeline)
-          end
-        end
-
-        context 'when allow_unsafe_ruby_regexp is enabled' do
-          before do
-            stub_feature_flags(allow_unsafe_ruby_regexp: true)
-          end
-
-          it 'is satisfied by regexp' do
-            expect(subject)
-              .to be_satisfied_by(pipeline)
-          end
+        it 'ignores invalid regexp' do
+          expect(subject)
+            .not_to be_satisfied_by(pipeline)
         end
       end
     end

spec/lib/gitlab/ci/config/entry/include/rules/rule_spec.rb

@@ -59,9 +59,7 @@ RSpec.describe Gitlab::Ci::Config::Entry::Include::Rules::Rule do
     context 'when using an if: clause with lookahead regex character "?"' do
       let(:config) { { if: '$CI_COMMIT_REF =~ /^(?!master).+/' } }
 
-      context 'when allow_unsafe_ruby_regexp is disabled' do
-        it_behaves_like 'an invalid config', /invalid expression syntax/
-      end
+      it_behaves_like 'an invalid config', /invalid expression syntax/
     end
 
     context 'when specifying unknown policy' do

spec/lib/gitlab/ci/config/entry/policy_spec.rb

 # frozen_string_literal: true
 
-require 'spec_helper'
+require 'fast_spec_helper'
 
 RSpec.describe Gitlab::Ci::Config::Entry::Policy do
   let(:entry) { described_class.new(config) }
@@ -45,29 +45,10 @@ RSpec.describe Gitlab::Ci::Config::Entry::Policy do
         end
 
         context 'when using unsafe regexp' do
-          # When removed we could use `require 'fast_spec_helper'` again.
-          include StubFeatureFlags
-
           let(:config) { ['/^(?!master).+/'] }
 
-          context 'when allow_unsafe_ruby_regexp is disabled' do
-            before do
-              stub_feature_flags(allow_unsafe_ruby_regexp: false)
-            end
-
-            it 'is not valid' do
-              expect(entry).not_to be_valid
-            end
-          end
-
-          context 'when allow_unsafe_ruby_regexp is enabled' do
-            before do
-              stub_feature_flags(allow_unsafe_ruby_regexp: true)
-            end
-
-            it 'is valid' do
-              expect(entry).to be_valid
-            end
+          it 'is not valid' do
+            expect(entry).not_to be_valid
           end
         end
 
@@ -106,29 +87,10 @@ RSpec.describe Gitlab::Ci::Config::Entry::Policy do
     end
 
     context 'when using unsafe regexp' do
-      # When removed we could use `require 'fast_spec_helper'` again.
-      include StubFeatureFlags
-
       let(:config) { { refs: ['/^(?!master).+/'] } }
 
-      context 'when allow_unsafe_ruby_regexp is disabled' do
-        before do
-          stub_feature_flags(allow_unsafe_ruby_regexp: false)
-        end
-
-        it 'is not valid' do
-          expect(entry).not_to be_valid
-        end
-      end
-
-      context 'when allow_unsafe_ruby_regexp is enabled' do
-        before do
-          stub_feature_flags(allow_unsafe_ruby_regexp: true)
-        end
-
-        it 'is valid' do
-          expect(entry).to be_valid
-        end
+      it 'is not valid' do
+        expect(entry).not_to be_valid
       end
     end
 

spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb

@@ -92,12 +92,10 @@ RSpec.describe Gitlab::Ci::Config::Entry::Rules::Rule do
     context 'when using an if: clause with lookahead regex character "?"' do
       let(:config) { { if: '$CI_COMMIT_REF =~ /^(?!master).+/' } }
 
-      context 'when allow_unsafe_ruby_regexp is disabled' do
-        it { is_expected.not_to be_valid }
+      it { is_expected.not_to be_valid }
 
-        it 'reports an error about invalid expression syntax' do
-          expect(subject.errors).to include(/invalid expression syntax/)
-        end
+      it 'reports an error about invalid expression syntax' do
+        expect(subject.errors).to include(/invalid expression syntax/)
       end
     end
 

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -9,10 +9,6 @@ module Gitlab
 
       subject { described_class.new(config, user: nil).execute }
 
-      before do
-        stub_feature_flags(allow_unsafe_ruby_regexp: false)
-      end
-
       shared_examples 'returns errors' do |error_message|
         it 'adds a message when an error is encountered' do
           expect(subject.errors).to include(error_message)

spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb

 # frozen_string_literal: true
 
-require 'spec_helper'
+require 'fast_spec_helper'
 
 RSpec.describe Gitlab::UntrustedRegexp::RubySyntax do
   describe '.matches_syntax?' do
@@ -71,44 +71,6 @@ RSpec.describe Gitlab::UntrustedRegexp::RubySyntax do
       end
     end
 
-    context 'when unsafe regexp is used' do
-      include StubFeatureFlags
-
-      before do
-        # When removed we could use `require 'fast_spec_helper'` again.
-        stub_feature_flags(allow_unsafe_ruby_regexp: true)
-
-        allow(Gitlab::UntrustedRegexp).to receive(:new).and_raise(RegexpError)
-      end
-
-      context 'when no fallback is enabled' do
-        it 'raises an exception' do
-          expect { described_class.fabricate!('/something/') }
-            .to raise_error(RegexpError)
-        end
-      end
-
-      context 'when fallback is used' do
-        it 'fabricates regexp with a single flag' do
-          regexp = described_class.fabricate!('/something/i', fallback: true)
-
-          expect(regexp).to eq Regexp.new('something', Regexp::IGNORECASE)
-        end
-
-        it 'fabricates regexp with multiple flags' do
-          regexp = described_class.fabricate!('/something/im', fallback: true)
-
-          expect(regexp).to eq Regexp.new('something', Regexp::IGNORECASE | Regexp::MULTILINE)
-        end
-
-        it 'fabricates regexp without flags' do
-          regexp = described_class.fabricate!('/something/', fallback: true)
-
-          expect(regexp).to eq Regexp.new('something')
-        end
-      end
-    end
-
     context 'when regexp is a raw pattern' do
       it 'raises an error' do
         expect { described_class.fabricate!('some .* thing') }