Wrap all UntrustedRegexp patterns in () for RE2untrusted-regexp-match-groups-bugfix

- Add () wrapping to all patterns in UntrustedRegexp#scan_regex
- Opt in to match-data formatting for patterns with match groups, since
  they all do now
- Add Matches operator and statement specs reproducing the reported
  error, with correct result values

5 files changed, 39 insertions, 13 deletions

diff --git a/changelogs/unreleased/untrusted-regexp-match-groups-bugfix.yml b/changelogs/unreleased/untrusted-regexp-match-groups-bugfix.yml
new file mode 100644
index 00000000000..daa0ae5bff3
--- /dev/null
+++ b/changelogs/unreleased/untrusted-regexp-match-groups-bugfix.yml
@@ -0,0 +1,5 @@
+---
+title: consistently wrap UntrustedRegexp patterns in () for RE2
+merge_request: 29575
+author:
+type: fixed
diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb
index ecfab627226..61f2402fad4 100644
--- a/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb
+++ b/lib/gitlab/ci/pipeline/expression/lexeme/matches.rb
@@ -12,8 +12,6 @@ module Gitlab
               text = @left.evaluate(variables)
               regexp = @right.evaluate(variables)
 
-              regexp.scan(text.to_s).any?
-
               if ci_variables_complex_expressions?
                 # return offset of first match, or nil if no matches
                 if match = regexp.scan(text.to_s).first
diff --git a/lib/gitlab/untrusted_regexp.rb b/lib/gitlab/untrusted_regexp.rb
index c237f4a7404..893ec7c7fea 100644
--- a/lib/gitlab/untrusted_regexp.rb
+++ b/lib/gitlab/untrusted_regexp.rb
@@ -30,9 +30,7 @@ module Gitlab
     end
 
     def scan(text)
-      matches = scan_regexp.scan(text).to_a
-      matches.map!(&:first) if regexp.number_of_capturing_groups.zero?
-      matches
+      scan_regexp.scan(text).to_a.map(&:first)
     end
 
     def match?(text)
@@ -65,14 +63,9 @@ module Gitlab
     attr_reader :regexp
 
     # RE2 scan operates differently to Ruby scan when there are no capture
-    # groups, so work around it
+    # groups, so always add one
     def scan_regexp
-      @scan_regexp ||=
-        if regexp.number_of_capturing_groups.zero?
-          RE2::Regexp.new('(' + regexp.source + ')')
-        else
-          regexp
-        end
+      @scan_regexp ||= RE2::Regexp.new('(' + regexp.source + ')')
     end
   end
 end
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 97da66d2bcc..fb630ea1d09 100644
--- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/matches_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/matches_spec.rb
@@ -1,4 +1,4 @@
-require 'fast_spec_helper'
+require 'spec_helper'
 require_dependency 're2'
 
 describe Gitlab::Ci::Pipeline::Expression::Lexeme::Matches do
@@ -67,6 +67,27 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::Matches do
       it { is_expected.to eq(nil) }
     end
 
+    context 'with an internal match group' do
+      let(:left_value)  { 'v11.11.3-rc1' }
+      let(:right_value) { Gitlab::UntrustedRegexp.new('^v\d+\.\d+\.\d+(-rc\d+)?$') }
+
+      it { is_expected.to eq(0) }
+    end
+
+    context 'with an all-compassing match group' do
+      let(:left_value)  { 'v11.11.3-rc1' }
+      let(:right_value) { Gitlab::UntrustedRegexp.new('(^v\d+\.\d+\.\d+-rc\d+?$)') }
+
+      it { is_expected.to eq(0) }
+    end
+
+    context 'with a nested match group' do
+      let(:left_value)  { 'v11.11.3-rc1' }
+      let(:right_value) { Gitlab::UntrustedRegexp.new('(^v\d+\.\d+\.\d+(-rc\d+)?$)') }
+
+      it { is_expected.to eq(0) }
+    end
+
     context 'when left is a multiline string and matches right' do
       let(:left_value) do
         <<~TEXT
diff --git a/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb
index 057e2f3fbe8..9aab9ce5a78 100644
--- a/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb
@@ -10,6 +10,9 @@ describe Gitlab::Ci::Pipeline::Expression::Statement do
 
   let(:variables) do
     {
+      'CI_COMMIT_TAG'      => 'v11.11.3',
+      'CI_COMMIT_REF_NAME' => '11-11-stable',
+      'GITLAB_VERSION'     => 'v11.11.3-ee',
       'PRESENT_VARIABLE'   => 'my variable',
       'PATH_VARIABLE'      => 'a/path/variable/value',
       'FULL_PATH_VARIABLE' => '/a/full/path/variable/value',
@@ -67,6 +70,12 @@ describe Gitlab::Ci::Pipeline::Expression::Statement do
       "$UNDEFINED_VARIABLE !~ /var.*/"                              | true
       "$PRESENT_VARIABLE !~ /VAR.*/i"                               | false
 
+      '$CI_COMMIT_TAG =~ /^v\d+\.\d+\.\d+(-rc\d+)?$/'       | 0
+      '$CI_COMMIT_REF_NAME =~ /^\d+-\d+-stable$/'           | 0
+      '$CI_COMMIT_TAG =~ /^v\d+\.\d+\.\d+(-rc\d+)?-ee$/'    | nil
+      '$CI_COMMIT_REF_NAME =~ /^\d+-\d+-stable-ee$/'        | nil
+      '$GITLAB_VERSION =~ /^v\d+\.\d+\.\d+(-rc\d+)?-ee$/'   | 0
+
       '$PRESENT_VARIABLE && "string"'          | 'string'
       '$PRESENT_VARIABLE && $PRESENT_VARIABLE' | 'my variable'
       '$PRESENT_VARIABLE && $EMPTY_VARIABLE'   | ''