Merge branch 'pedropombeiro/26345/7-add-expression-parser' into 'master'

Do not expand escaped sequences while expanding variables sent to Runner

See merge request gitlab-org/gitlab!56603

lib/gitlab/ci/variables/collection.rb

@@ -68,11 +68,19 @@ module Gitlab
         end
 
         def expand_value(value, keep_undefined: false)
-          value.gsub(ExpandVariables::VARIABLES_REGEXP) do
+          value.gsub(Item::VARIABLES_REGEXP) do
             match = Regexp.last_match
-            result = @variables_by_key[match[1] || match[2]]&.value
-            result ||= match[0] if keep_undefined
-            result
+            if match[:key]
+              # we matched variable
+              if variable = @variables_by_key[match[:key]]
+                variable.value
+              elsif keep_undefined
+                match[0]
+              end
+            else
+              # we escape sequence
+              match[0]
+            end
           end
         end
 

lib/gitlab/ci/variables/collection/item.rb

@@ -7,6 +7,9 @@ module Gitlab
         class Item
           include Gitlab::Utils::StrongMemoize
 
+          VARIABLES_REGEXP = /\$\$|%%|\$(?<key>[a-zA-Z_][a-zA-Z0-9_]*)|\${\g<key>?}|%\g<key>%/.freeze.freeze
+          VARIABLE_REF_CHARS = %w[$ %].freeze
+
           def initialize(key:, value:, public: true, file: false, masked: false, raw: false)
             raise ArgumentError, "`#{key}` must be of type String or nil value, while it was: #{value.class}" unless
               value.is_a?(String) || value.nil?
@@ -34,9 +37,9 @@ module Gitlab
             strong_memoize(:depends_on) do
               next if raw
 
-              next unless ExpandVariables.possible_var_reference?(value)
+              next unless self.class.possible_var_reference?(value)
 
-              value.scan(ExpandVariables::VARIABLES_REGEXP).map(&:first)
+              value.scan(VARIABLES_REGEXP).filter_map(&:last)
             end
           end
 
@@ -64,6 +67,12 @@ module Gitlab
             end
           end
 
+          def self.possible_var_reference?(value)
+            return unless value
+
+            VARIABLE_REF_CHARS.any? { |symbol| value.include?(symbol) }
+          end
+
           def to_s
             return to_runner_variable.to_s unless depends_on
 

spec/lib/expand_variables_spec.rb

 # frozen_string_literal: true
 
-require 'spec_helper'
+require 'fast_spec_helper'
+require 'rspec-parameterized'
 
 RSpec.describe ExpandVariables do
   shared_examples 'common variable expansion' do |expander|
@@ -231,41 +232,4 @@ RSpec.describe ExpandVariables do
       end
     end
   end
-
-  describe '#possible_var_reference?' do
-    context 'table tests' do
-      using RSpec::Parameterized::TableSyntax
-
-      where do
-        {
-          "empty value": {
-            value: '',
-            result: false
-          },
-          "normal value": {
-            value: 'some value',
-            result: false
-          },
-          "simple expansions": {
-            value: 'key$variable',
-            result: true
-          },
-          "complex expansions": {
-            value: 'key${variable}${variable2}',
-            result: true
-          },
-          "complex expansions for Windows": {
-            value: 'key%variable%%variable2%',
-            result: true
-          }
-        }
-      end
-
-      with_them do
-        subject { ExpandVariables.possible_var_reference?(value) }
-
-        it { is_expected.to eq(result) }
-      end
-    end
-  end
 end

spec/lib/gitlab/ci/variables/collection/item_spec.rb

@@ -70,6 +70,43 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Item do
     end
   end
 
+  describe '.possible_var_reference?' do
+    context 'table tests' do
+      using RSpec::Parameterized::TableSyntax
+
+      where do
+        {
+          "empty value": {
+            value: '',
+            result: false
+          },
+          "normal value": {
+            value: 'some value',
+            result: false
+          },
+          "simple expansions": {
+            value: 'key$variable',
+            result: true
+          },
+          "complex expansions": {
+            value: 'key${variable}${variable2}',
+            result: true
+          },
+          "complex expansions for Windows": {
+            value: 'key%variable%%variable2%',
+            result: true
+          }
+        }
+      end
+
+      with_them do
+        subject { Gitlab::Ci::Variables::Collection::Item.possible_var_reference?(value) }
+
+        it { is_expected.to eq(result) }
+      end
+    end
+  end
+
   describe '#depends_on' do
     let(:item) { Gitlab::Ci::Variables::Collection::Item.new(**variable) }
 
@@ -128,7 +165,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Item do
     end
 
     it 'supports using an active record resource' do
-      variable = create(:ci_variable, key: 'CI_VAR', value: '123')
+      variable = build(:ci_variable, key: 'CI_VAR', value: '123')
       resource = described_class.fabricate(variable)
 
       expect(resource).to be_a(described_class)

spec/lib/gitlab/ci/variables/collection/sort_spec.rb

 # frozen_string_literal: true
 
-require 'spec_helper'
+require 'fast_spec_helper'
+require 'rspec-parameterized'
 
 RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
   describe '#initialize with non-Collection value' do
@@ -57,9 +58,9 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
           },
           "variable containing escaped variable reference": {
             variables: [
-              { key: 'variable_a', value: 'value' },
               { key: 'variable_b', value: '$$variable_a' },
-              { key: 'variable_c', value: '$variable_b' }
+              { key: 'variable_c', value: '$variable_a' },
+              { key: 'variable_a', value: 'value' }
             ],
             expected_errors: nil
           }
@@ -144,11 +145,11 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
           },
           "variable containing escaped variable reference": {
             variables: [
-              { key: 'variable_c', value: '$variable_b' },
               { key: 'variable_b', value: '$$variable_a' },
+              { key: 'variable_c', value: '$variable_a' },
               { key: 'variable_a', value: 'value' }
             ],
-            result: %w[variable_a variable_b variable_c]
+            result: %w[variable_b variable_a variable_c]
           }
         }
       end

spec/lib/gitlab/ci/variables/collection_spec.rb

@@ -253,6 +253,11 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
             value: 'key${MISSING_VAR}-${CI_JOB_NAME}',
             result: 'key${MISSING_VAR}-test-1',
             keep_undefined: true
+          },
+          "escaped characters are kept intact": {
+            value: 'key-$TEST1-%%HOME%%-$${HOME}',
+            result: 'key-test-3-%%HOME%%-$${HOME}',
+            keep_undefined: false
           }
         }
       end
@@ -315,6 +320,14 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
               ],
               keep_undefined: false
             },
+            "escaped characters in complex expansions are kept intact": {
+              variables: [
+                { key: 'variable3', value: 'key_${variable}_$${HOME}_%%HOME%%' },
+                { key: 'variable', value: '$variable2' },
+                { key: 'variable2', value: 'value2' }
+              ],
+              keep_undefined: false
+            },
             "array with cyclic dependency": {
               variables: [
                 { key: 'variable', value: '$variable2' },
@@ -415,6 +428,30 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
                 { key: 'variable3', value: 'keyvalueresult' }
               ]
             },
+            "escaped characters in complex expansions keeping undefined are kept intact": {
+              variables: [
+                { key: 'variable3', value: 'key_${variable}_$${HOME}_%%HOME%%' },
+                { key: 'variable', value: '$variable2' },
+                { key: 'variable2', value: 'value' }
+              ],
+              keep_undefined: true,
+              result: [
+                { key: 'variable', value: 'value' },
+                { key: 'variable2', value: 'value' },
+                { key: 'variable3', value: 'key_value_$${HOME}_%%HOME%%' }
+              ]
+            },
+            "escaped characters in complex expansions discarding undefined are kept intact": {
+              variables: [
+                { key: 'variable2', value: 'key_${variable4}_$${HOME}_%%HOME%%' },
+                { key: 'variable', value: 'value_$${HOME}_%%HOME%%' }
+              ],
+              keep_undefined: false,
+              result: [
+                { key: 'variable', value: 'value_$${HOME}_%%HOME%%' },
+                { key: 'variable2', value: 'key__$${HOME}_%%HOME%%' }
+              ]
+            },
             "out-of-order expansion": {
               variables: [
                 { key: 'variable3', value: 'key$variable2$variable' },
@@ -441,7 +478,7 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
                 { key: 'variable3', value: 'keyresultvalue' }
               ]
             },
-            "missing variable": {
+            "missing variable discarding original": {
               variables: [
                 { key: 'variable2', value: 'key$variable' }
               ],
@@ -485,6 +522,19 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
                 { key: 'variable3', value: 'key_$variable2_value2' }
               ]
             },
+            "variable value referencing password with special characters": {
+              variables: [
+                { key: 'VAR', value: '$PASSWORD' },
+                { key: 'PASSWORD', value: 'my_password$$_%%_$A' },
+                { key: 'A', value: 'value' }
+              ],
+              keep_undefined: false,
+              result: [
+                { key: 'VAR', value: 'my_password$$_%%_value' },
+                { key: 'PASSWORD', value: 'my_password$$_%%_value' },
+                { key: 'A', value: 'value' }
+              ]
+            },
             "cyclic dependency causes original array to be returned": {
               variables: [
                 { key: 'variable', value: '$variable2' },