From 36a3a19aece0434636c4cdc339b34433c2bdaac6 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan <furkanayhn@gmail.com> Date: Mon, 18 Apr 2022 12:49:55 +0300 Subject: [PATCH 1/6] Fix CI rules-if comparison with Regexp variables When the right side of a comparison is a regexp variable, we consider this as a string. However, we need to try converting it to a regexp if possible. --- ...les_if_comparison_with_regexp_variable.yml | 8 ++ .../ci/pipeline/expression/lexeme/matches.rb | 7 ++ .../pipeline/expression/lexeme/not_matches.rb | 7 ++ .../ci/build/rules/rule/clause/if_spec.rb | 74 +++++++++++++++++++ spec/lib/gitlab/ci/build/rules_spec.rb | 13 ++++ .../expression/lexeme/matches_spec.rb | 28 +++++++ .../expression/lexeme/not_matches_spec.rb | 28 +++++++ .../ci/pipeline/expression/statement_spec.rb | 45 +++++++++-- 8 files changed, 205 insertions(+), 5 deletions(-) create mode 100644 config/feature_flags/development/ci_fix_rules_if_comparison_with_regexp_variable.yml create mode 100644 spec/lib/gitlab/ci/build/rules/rule/clause/if_spec.rb diff --git a/config/feature_flags/development/ci_fix_rules_if_comparison_with_regexp_variable.yml b/config/feature_flags/development/ci_fix_rules_if_comparison_with_regexp_variable.yml new file mode 100644 index 0000000000000000..be21707d3768ce2d --- /dev/null +++ b/config/feature_flags/development/ci_fix_rules_if_comparison_with_regexp_variable.yml @@ -0,0 +1,8 @@ +--- +name: ci_fix_rules_if_comparison_with_regexp_variable +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/85310 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/359740 +milestone: '15.0' +type: development +group: group::pipeline authoring +default_enabled: false diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb index 4d65b914d8d4442d..9d0037e70426c7a4 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb @@ -11,6 +11,13 @@ class Matches < Lexeme::LogicalOperator def evaluate(variables = {}) text = @left.evaluate(variables) regexp = @right.evaluate(variables) + + if ::Feature.enabled?(:ci_fix_rules_if_comparison_with_regexp_variable, default_enabled: :yaml) + # All variables are evaluated as strings, even if they are regexps. + # So, we need to convert them to regexps. + regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(regexp) || regexp + end + return false unless regexp regexp.scan(text.to_s).present? diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb index 29c5aa5d753d812d..101f5c807c6be8c0 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb @@ -11,6 +11,13 @@ class NotMatches < Lexeme::LogicalOperator def evaluate(variables = {}) text = @left.evaluate(variables) regexp = @right.evaluate(variables) + + if ::Feature.enabled?(:ci_fix_rules_if_comparison_with_regexp_variable, default_enabled: :yaml) + # All variables are evaluated as strings, even if they are regexps. + # So, we need to convert them to regexps. + regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(regexp) || regexp + end + return true unless regexp regexp.scan(text.to_s).empty? diff --git a/spec/lib/gitlab/ci/build/rules/rule/clause/if_spec.rb b/spec/lib/gitlab/ci/build/rules/rule/clause/if_spec.rb new file mode 100644 index 0000000000000000..81bce98983318938 --- /dev/null +++ b/spec/lib/gitlab/ci/build/rules/rule/clause/if_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'support/helpers/stubbed_feature' +require 'support/helpers/stub_feature_flags' + +RSpec.describe Gitlab::Ci::Build::Rules::Rule::Clause::If do + include StubFeatureFlags + + subject(:if_clause) { described_class.new(expression) } + + describe '#satisfied_by?' do + let(:context_class) { Gitlab::Ci::Build::Context::Base } + let(:rules_context) { instance_double(context_class, variables_hash: {}) } + + subject(:satisfied_by?) { if_clause.satisfied_by?(nil, rules_context) } + + context 'when expression is a basic string comparison' do + context 'when comparison is true' do + let(:expression) { '"value" == "value"' } + + it { is_expected.to eq(true) } + end + + context 'when comparison is false' do + let(:expression) { '"value" == "other"' } + + it { is_expected.to eq(false) } + end + end + + context 'when expression is a regexp' do + context 'when comparison is true' do + let(:expression) { '"abcde" =~ /^ab.*/' } + + it { is_expected.to eq(true) } + end + + context 'when comparison is false' do + let(:expression) { '"abcde" =~ /^af.*/' } + + it { is_expected.to eq(false) } + end + + context 'when both side of the expression are variables' do + let(:expression) { '$teststring =~ $pattern' } + + context 'when comparison is true' do + let(:rules_context) do + instance_double(context_class, variables_hash: { 'teststring' => 'abcde', 'pattern' => '/^ab.*/' }) + end + + it { is_expected.to eq(true) } + + context 'when the FF ci_fix_rules_if_comparison_with_regexp_variable is disabled' do + before do + stub_feature_flags(ci_fix_rules_if_comparison_with_regexp_variable: false) + end + + it { is_expected.to eq(false) } + end + end + + context 'when comparison is false' do + let(:rules_context) do + instance_double(context_class, variables_hash: { 'teststring' => 'abcde', 'pattern' => '/^af.*/' }) + end + + it { is_expected.to eq(false) } + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/build/rules_spec.rb b/spec/lib/gitlab/ci/build/rules_spec.rb index 37bfdca4d1d75b39..e82dcd0254d24e65 100644 --- a/spec/lib/gitlab/ci/build/rules_spec.rb +++ b/spec/lib/gitlab/ci/build/rules_spec.rb @@ -188,6 +188,19 @@ it { is_expected.to eq(described_class::Result.new('on_success', nil, nil, { MY_VAR: 'my var' })) } end end + + context 'with a regexp variable matching rule' do + let(:rule_list) { [{ if: '"abcde" =~ $pattern' }] } + + before do + allow(ci_build).to receive(:scoped_variables).and_return( + Gitlab::Ci::Variables::Collection.new + .append(key: 'pattern', value: '/^ab.*/', public: true) + ) + end + + it { is_expected.to eq(described_class::Result.new('on_success')) } + end end describe 'Gitlab::Ci::Build::Rules::Result' do diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/matches_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/matches_spec.rb index 0da04d8dcf7be20c..83742699d3d6858b 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/matches_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/matches_spec.rb @@ -1,9 +1,13 @@ # frozen_string_literal: true require 'fast_spec_helper' +require 'support/helpers/stubbed_feature' +require 'support/helpers/stub_feature_flags' require_dependency 're2' RSpec.describe Gitlab::Ci::Pipeline::Expression::Lexeme::Matches do + include StubFeatureFlags + let(:left) { double('left') } let(:right) { double('right') } @@ -148,5 +152,29 @@ it { is_expected.to eq(false) } end + + context 'when right value is a regexp string' do + let(:right_value) { '/^ab.*/' } + + context 'when matching' do + let(:left_value) { 'abcde' } + + it { is_expected.to eq(true) } + + context 'when the FF ci_fix_rules_if_comparison_with_regexp_variable is disabled' do + before do + stub_feature_flags(ci_fix_rules_if_comparison_with_regexp_variable: false) + end + + it { is_expected.to eq(false) } + end + end + + context 'when not matching' do + let(:left_value) { 'dfg' } + + it { is_expected.to eq(false) } + end + end end end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_matches_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_matches_spec.rb index 9bff2355d58462c7..aad33106647b6d88 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_matches_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/not_matches_spec.rb @@ -1,9 +1,13 @@ # frozen_string_literal: true require 'fast_spec_helper' +require 'support/helpers/stubbed_feature' +require 'support/helpers/stub_feature_flags' require_dependency 're2' RSpec.describe Gitlab::Ci::Pipeline::Expression::Lexeme::NotMatches do + include StubFeatureFlags + let(:left) { double('left') } let(:right) { double('right') } @@ -148,5 +152,29 @@ it { is_expected.to eq(true) } end + + context 'when right value is a regexp string' do + let(:right_value) { '/^ab.*/' } + + context 'when matching' do + let(:left_value) { 'abcde' } + + it { is_expected.to eq(false) } + + context 'when the FF ci_fix_rules_if_comparison_with_regexp_variable is disabled' do + before do + stub_feature_flags(ci_fix_rules_if_comparison_with_regexp_variable: false) + end + + it { is_expected.to eq(true) } + end + end + + context 'when not matching' do + let(:left_value) { 'dfg' } + + it { is_expected.to eq(true) } + end + end end end diff --git a/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb index 84713e2a798ffd5a..bbd11a00149a0151 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb @@ -12,7 +12,7 @@ .to_hash end - subject do + subject(:statement) do described_class.new(text, variables) end @@ -29,6 +29,8 @@ describe '#evaluate' do using RSpec::Parameterized::TableSyntax + subject(:evaluate) { statement.evaluate } + where(:expression, :value) do '$PRESENT_VARIABLE == "my variable"' | true '"my variable" == $PRESENT_VARIABLE' | true @@ -125,7 +127,7 @@ let(:text) { expression } it "evaluates to `#{params[:value].inspect}`" do - expect(subject.evaluate).to eq(value) + expect(evaluate).to eq(value) end end end @@ -133,6 +135,8 @@ describe '#truthful?' do using RSpec::Parameterized::TableSyntax + subject(:truthful?) { statement.truthful? } + where(:expression, :value) do '$PRESENT_VARIABLE == "my variable"' | true "$PRESENT_VARIABLE == 'no match'" | false @@ -151,7 +155,7 @@ let(:text) { expression } it "returns `#{params[:value].inspect}`" do - expect(subject.truthful?).to eq value + expect(truthful?).to eq value end end @@ -159,10 +163,41 @@ let(:text) { '$PRESENT_VARIABLE' } it 'returns false' do - allow(subject).to receive(:evaluate) + allow(statement).to receive(:evaluate) .and_raise(described_class::StatementError) - expect(subject.truthful?).to be_falsey + expect(truthful?).to be_falsey + end + end + + context 'when variables have patterns' do + let(:variables) do + Gitlab::Ci::Variables::Collection.new + .append(key: 'teststring', value: 'abcde') + .append(key: 'pattern1', value: '/^ab.*/') + .append(key: 'pattern2', value: '/^at.*/') + .to_hash + end + + where(:expression, :ff, :result) do + '$teststring =~ "abcde"' | true | true + '$teststring =~ "abcde"' | false | true + '$teststring =~ $teststring' | true | true + '$teststring =~ $teststring' | false | true + '$teststring =~ $pattern1' | true | true + '$teststring =~ $pattern1' | false | false + '$teststring =~ $pattern2' | true | false + '$teststring =~ $pattern2' | false | false + end + + with_them do + let(:text) { expression } + + before do + stub_feature_flags(ci_fix_rules_if_comparison_with_regexp_variable: ff) + end + + it { is_expected.to eq(result) } end end end -- GitLab From b197cb1e90076e6ffc92b67ee3269bfc360b4a42 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan <furkanayhn@gmail.com> Date: Thu, 21 Apr 2022 15:16:15 +0300 Subject: [PATCH 2/6] Add the documentation --- doc/ci/yaml/index.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 1806a677c2341a6f..0fe639d4d8b08f65 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -3090,6 +3090,8 @@ job: - Unlike variables in [`script`](../variables/index.md#use-cicd-variables-in-job-scripts) sections, variables in rules expressions are always formatted as `$VARIABLE`. - You can use `rules:if` with `include` to [conditionally include other configuration files](includes.md#use-rules-with-include). +- In [GitLab 15.0 and later](https://gitlab.com/gitlab-org/gitlab/-/issues/35438), + variables in the right side of `=~` and `!~` expressions are evaluated as Regexp. **Related topics**: -- GitLab From 4be17e0cfd648feae2c307f731087614b0d568f6 Mon Sep 17 00:00:00 2001 From: Marcel Amirault <mamirault@gitlab.com> Date: Fri, 22 Apr 2022 08:49:36 +0000 Subject: [PATCH 3/6] Apply 1 suggestion(s) to 1 file(s) --- doc/ci/yaml/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/ci/yaml/index.md b/doc/ci/yaml/index.md index 0fe639d4d8b08f65..ac21fdb3714099f3 100644 --- a/doc/ci/yaml/index.md +++ b/doc/ci/yaml/index.md @@ -3091,7 +3091,7 @@ job: sections, variables in rules expressions are always formatted as `$VARIABLE`. - You can use `rules:if` with `include` to [conditionally include other configuration files](includes.md#use-rules-with-include). - In [GitLab 15.0 and later](https://gitlab.com/gitlab-org/gitlab/-/issues/35438), - variables in the right side of `=~` and `!~` expressions are evaluated as Regexp. + variables on the right side of `=~` and `!~` expressions are evaluated as regular expressions. **Related topics**: -- GitLab From 452ecb4a51215a192b20896ce694b85cb3f3e73a Mon Sep 17 00:00:00 2001 From: Furkan Ayhan <furkanayhn@gmail.com> Date: Wed, 27 Apr 2022 11:46:05 +0300 Subject: [PATCH 4/6] Change the comments --- lib/gitlab/ci/pipeline/expression/lexeme/matches.rb | 4 ++-- lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb index 9d0037e70426c7a4..1f4e1527fb4089fd 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb @@ -13,8 +13,8 @@ def evaluate(variables = {}) regexp = @right.evaluate(variables) if ::Feature.enabled?(:ci_fix_rules_if_comparison_with_regexp_variable, default_enabled: :yaml) - # All variables are evaluated as strings, even if they are regexps. - # So, we need to convert them to regexps. + # All variables are evaluated as strings, even if they are regexp strings. + # So, we need to convert them to regexp objects. regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(regexp) || regexp end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb index 101f5c807c6be8c0..2b4c007b12880784 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb @@ -13,8 +13,8 @@ def evaluate(variables = {}) regexp = @right.evaluate(variables) if ::Feature.enabled?(:ci_fix_rules_if_comparison_with_regexp_variable, default_enabled: :yaml) - # All variables are evaluated as strings, even if they are regexps. - # So, we need to convert them to regexps. + # All variables are evaluated as strings, even if they are regexp strings. + # So, we need to convert them to regexp objects. regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(regexp) || regexp end -- GitLab From 184da5698fc55b634ef1b778122e96530803fec1 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan <furkanayhn@gmail.com> Date: Fri, 29 Apr 2022 16:04:59 +0300 Subject: [PATCH 5/6] Use lexeme pattern --- .../ci/pipeline/expression/lexeme/matches.rb | 6 +-- .../pipeline/expression/lexeme/not_matches.rb | 6 +-- .../ci/pipeline/expression/lexeme/pattern.rb | 18 ++++++-- .../ci/pipeline/expression/lexeme/value.rb | 2 + .../expression/lexeme/pattern_spec.rb | 46 ++++++++++++++++++- 5 files changed, 67 insertions(+), 11 deletions(-) diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb index 1f4e1527fb4089fd..ada21911f2c45123 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb @@ -12,14 +12,14 @@ def evaluate(variables = {}) text = @left.evaluate(variables) regexp = @right.evaluate(variables) + return false unless regexp + if ::Feature.enabled?(:ci_fix_rules_if_comparison_with_regexp_variable, default_enabled: :yaml) # All variables are evaluated as strings, even if they are regexp strings. # So, we need to convert them to regexp objects. - regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(regexp) || regexp + regexp = Lexeme::Pattern.build_safe(regexp)&.evaluate(variables) || regexp end - return false unless regexp - regexp.scan(text.to_s).present? end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb index 2b4c007b12880784..4eff3e59aa4897d5 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb @@ -12,14 +12,14 @@ def evaluate(variables = {}) text = @left.evaluate(variables) regexp = @right.evaluate(variables) + return true unless regexp + if ::Feature.enabled?(:ci_fix_rules_if_comparison_with_regexp_variable, default_enabled: :yaml) # All variables are evaluated as strings, even if they are regexp strings. # So, we need to convert them to regexp objects. - regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(regexp) || regexp + regexp = Lexeme::Pattern.build_safe(regexp)&.evaluate(variables) || regexp end - return true unless regexp - regexp.scan(text.to_s).empty? end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb b/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb index c7106f3ec390ea18..0a6b0ad618fa07ee 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb @@ -11,10 +11,14 @@ class Pattern < Lexeme::Value PATTERN = %r{^\/([^\/]|\\/)+[^\\]\/[ismU]*}.freeze def initialize(regexp) - super(regexp.gsub(%r{\\/}, '/')) - - unless Gitlab::UntrustedRegexp::RubySyntax.valid?(@value) - raise Lexer::SyntaxError, 'Invalid regular expression!' + if regexp.is_a?(Gitlab::UntrustedRegexp) + super("/#{regexp.source}/") + else + super(regexp.gsub(%r{\\/}, '/')) + + unless Gitlab::UntrustedRegexp::RubySyntax.valid?(@value) + raise Lexer::SyntaxError, 'Invalid regular expression!' + end end end @@ -35,6 +39,12 @@ def self.pattern def self.build(string) new(string) end + + def self.build_safe(string) + new(string) + rescue Lexer::SyntaxError + nil + end end end end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/value.rb b/lib/gitlab/ci/pipeline/expression/lexeme/value.rb index 6d872fee39d095eb..fa82bbe3275d9f12 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/value.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/value.rb @@ -10,6 +10,8 @@ def self.type :value end + attr_reader :value + def initialize(value) @value = value end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb index fa4f8a20984b5c3c..91d0a8b8dc06a3bd 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb @@ -1,8 +1,40 @@ # frozen_string_literal: true -require 'spec_helper' +require 'fast_spec_helper' RSpec.describe Gitlab::Ci::Pipeline::Expression::Lexeme::Pattern do + describe '#initialize' do + context 'when the value is a valid regular expression' do + it 'initializes the pattern' do + pattern = described_class.new('/foo/') + + expect(pattern.value).to eq('/foo/') + end + end + + context 'when the value is a valid regular expression with escaped slashes' do + it 'initializes the pattern' do + pattern = described_class.new('/foo\\/bar/') + + expect(pattern.value).to eq('/foo/bar/') + end + end + + context 'when the value is a Gitlab::UntrustedRegexp' do + it 'initializes the pattern' do + pattern = described_class.new(Gitlab::UntrustedRegexp::RubySyntax.fabricate!('/foo/')) + + expect(pattern.value).to eq('/foo/') + end + end + + context 'when the value is not a valid regular expression' do + it 'raises an error' do + expect { described_class.new('foo') }.to raise_error(Gitlab::Ci::Pipeline::Expression::Lexer::SyntaxError) + end + end + end + describe '.build' do it 'creates a new instance of the token' do expect(described_class.build('/.*/')) @@ -15,6 +47,18 @@ end end + describe '.build_safe' do + it 'creates a new instance of the token' do + expect(described_class.build_safe('/.*/')) + .to be_a(described_class) + end + + it 'returns nil if pattern is invalid' do + expect(described_class.build_safe('/ some ( thin/i')) + .to be_nil + end + end + describe '.type' do it 'is a value lexeme' do expect(described_class.type).to eq :value -- GitLab From e9b42a6e348c39ccae2267baa718a1ce2d3f54f2 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan <furkanayhn@gmail.com> Date: Fri, 13 May 2022 13:30:22 +0300 Subject: [PATCH 6/6] Move the logic to build_and_evaluate --- .../ci/pipeline/expression/lexeme/matches.rb | 4 +-- .../pipeline/expression/lexeme/not_matches.rb | 4 +-- .../ci/pipeline/expression/lexeme/pattern.rb | 26 ++++++++------- .../expression/lexeme/pattern_spec.rb | 33 ++++++++++--------- 4 files changed, 36 insertions(+), 31 deletions(-) diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb index ada21911f2c45123..6efb3a4f16a18593 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb @@ -14,10 +14,10 @@ def evaluate(variables = {}) return false unless regexp - if ::Feature.enabled?(:ci_fix_rules_if_comparison_with_regexp_variable, default_enabled: :yaml) + if ::Feature.enabled?(:ci_fix_rules_if_comparison_with_regexp_variable) # All variables are evaluated as strings, even if they are regexp strings. # So, we need to convert them to regexp objects. - regexp = Lexeme::Pattern.build_safe(regexp)&.evaluate(variables) || regexp + regexp = Lexeme::Pattern.build_and_evaluate(regexp, variables) end regexp.scan(text.to_s).present? diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb index 4eff3e59aa4897d5..a72e5dbc822f5659 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/not_matches.rb @@ -14,10 +14,10 @@ def evaluate(variables = {}) return true unless regexp - if ::Feature.enabled?(:ci_fix_rules_if_comparison_with_regexp_variable, default_enabled: :yaml) + if ::Feature.enabled?(:ci_fix_rules_if_comparison_with_regexp_variable) # All variables are evaluated as strings, even if they are regexp strings. # So, we need to convert them to regexp objects. - regexp = Lexeme::Pattern.build_safe(regexp)&.evaluate(variables) || regexp + regexp = Lexeme::Pattern.build_and_evaluate(regexp, variables) end regexp.scan(text.to_s).empty? diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb b/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb index 0a6b0ad618fa07ee..cd4106b16bb16431 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb @@ -11,14 +11,10 @@ class Pattern < Lexeme::Value PATTERN = %r{^\/([^\/]|\\/)+[^\\]\/[ismU]*}.freeze def initialize(regexp) - if regexp.is_a?(Gitlab::UntrustedRegexp) - super("/#{regexp.source}/") - else - super(regexp.gsub(%r{\\/}, '/')) - - unless Gitlab::UntrustedRegexp::RubySyntax.valid?(@value) - raise Lexer::SyntaxError, 'Invalid regular expression!' - end + super(regexp.gsub(%r{\\/}, '/')) + + unless Gitlab::UntrustedRegexp::RubySyntax.valid?(@value) + raise Lexer::SyntaxError, 'Invalid regular expression!' end end @@ -40,10 +36,16 @@ def self.build(string) new(string) end - def self.build_safe(string) - new(string) - rescue Lexer::SyntaxError - nil + def self.build_and_evaluate(data, variables = {}) + return data if data.is_a?(Gitlab::UntrustedRegexp) + + begin + new_pattern = build(data) + rescue Lexer::SyntaxError + return data + end + + new_pattern.evaluate(variables) end end end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb index 91d0a8b8dc06a3bd..be205395b6940088 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb @@ -20,14 +20,6 @@ end end - context 'when the value is a Gitlab::UntrustedRegexp' do - it 'initializes the pattern' do - pattern = described_class.new(Gitlab::UntrustedRegexp::RubySyntax.fabricate!('/foo/')) - - expect(pattern.value).to eq('/foo/') - end - end - context 'when the value is not a valid regular expression' do it 'raises an error' do expect { described_class.new('foo') }.to raise_error(Gitlab::Ci::Pipeline::Expression::Lexer::SyntaxError) @@ -47,15 +39,26 @@ end end - describe '.build_safe' do - it 'creates a new instance of the token' do - expect(described_class.build_safe('/.*/')) - .to be_a(described_class) + describe '.build_and_evaluate' do + context 'when the value is a valid regular expression' do + it 'returns the value as a Gitlab::UntrustedRegexp' do + expect(described_class.build_and_evaluate('/foo/')) + .to eq(Gitlab::UntrustedRegexp.new('foo')) + end + end + + context 'when the value is a Gitlab::UntrustedRegexp' do + it 'returns the value itself' do + expect(described_class.build_and_evaluate(Gitlab::UntrustedRegexp.new('foo'))) + .to eq(Gitlab::UntrustedRegexp.new('foo')) + end end - it 'returns nil if pattern is invalid' do - expect(described_class.build_safe('/ some ( thin/i')) - .to be_nil + context 'when the value is not a valid regular expression' do + it 'returns the value itself' do + expect(described_class.build_and_evaluate('foo')) + .to eq('foo') + end end end -- GitLab