Add latest changes from gitlab-org/security/gitlab@12-10-stable-ee

4 files changed, 121 insertions, 134 deletions

diff --git a/app/services/prometheus/proxy_variable_substitution_service.rb b/app/services/prometheus/proxy_variable_substitution_service.rb
index 240586c8419..41ac2987804 100644
--- a/app/services/prometheus/proxy_variable_substitution_service.rb
+++ b/app/services/prometheus/proxy_variable_substitution_service.rb
@@ -4,6 +4,16 @@ module Prometheus
   class ProxyVariableSubstitutionService < BaseService
     include Stepable
 
+    VARIABLE_INTERPOLATION_REGEX = /
+      %{                  # Variable needs to be wrapped in these chars.
+        \s*               # Allow whitespace before and after the variable name.
+          (?<variable>    # Named capture.
+            \w+           # Match one or more word characters.
+          )
+        \s*
+      }
+    /x.freeze
+
     steps :validate_variables,
       :add_params_to_result,
       :substitute_params,
@@ -46,6 +56,14 @@ module Prometheus
       success(result)
     end
 
+    def substitute_ruby_variables(result)
+      return success(result) unless query(result)
+
+      result[:params][:query] = gsub(query(result), full_context)
+
+      success(result)
+    end
+
     def substitute_liquid_variables(result)
       return success(result) unless query(result)
 
@@ -57,26 +75,20 @@ module Prometheus
       error(e.message)
     end
 
-    def substitute_ruby_variables(result)
-      return success(result) unless query(result)
-
-      # The % operator doesn't replace variables if the hash contains string
-      # keys.
-      result[:params][:query] = query(result) % predefined_context.symbolize_keys
-
-      success(result)
-    rescue TypeError, ArgumentError => exception
-      log_error(exception.message)
-      Gitlab::ErrorTracking.track_exception(exception, {
-        template_string: query(result),
-        variables: predefined_context
-      })
-
-      error(_('Malformed string'))
+    def gsub(string, context)
+      # Search for variables of the form `%{variable}` in the string and replace
+      # them with their value.
+      string.gsub(VARIABLE_INTERPOLATION_REGEX) do |match|
+        # Replace with the value of the variable, or if there is no such variable,
+        # replace the invalid variable with itself. So,
+        # `up{instance="%{invalid_variable}"}` will remain
+        # `up{instance="%{invalid_variable}"}` after substitution.
+        context.fetch($~[:variable], match)
+      end
     end
 
     def predefined_context
-      @predefined_context ||= Gitlab::Prometheus::QueryVariables.call(@environment)
+      Gitlab::Prometheus::QueryVariables.call(@environment).stringify_keys
     end
 
     def full_context
diff --git a/changelogs/unreleased/security-use-gsub-variable-substitution.yml b/changelogs/unreleased/security-use-gsub-variable-substitution.yml
new file mode 100644
index 00000000000..83fb61ae47a
--- /dev/null
+++ b/changelogs/unreleased/security-use-gsub-variable-substitution.yml
@@ -0,0 +1,5 @@
+---
+title: Use `gsub` instead of the Ruby `%` operator to perform variable substitution in Prometheus proxy API
+merge_request:
+author:
+type: security
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 4726f0e941d..e12263908ab 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -12434,9 +12434,6 @@ msgstr ""
 msgid "Makes this issue confidential."
 msgstr ""
 
-msgid "Malformed string"
-msgstr ""
-
 msgid "Manage"
 msgstr ""
 
diff --git a/spec/services/prometheus/proxy_variable_substitution_service_spec.rb b/spec/services/prometheus/proxy_variable_substitution_service_spec.rb
index 9978c631366..7faac1b4fc6 100644
--- a/spec/services/prometheus/proxy_variable_substitution_service_spec.rb
+++ b/spec/services/prometheus/proxy_variable_substitution_service_spec.rb
@@ -6,19 +6,46 @@ describe Prometheus::ProxyVariableSubstitutionService do
   describe '#execute' do
     let_it_be(:environment) { create(:environment) }
 
-    let(:params_keys) { { query: 'up{environment="%{ci_environment_slug}"}' } }
+    let(:params_keys) { { query: "up{environment=\"#{w('ci_environment_slug')}\"}" } }
     let(:params) { ActionController::Parameters.new(params_keys).permit! }
     let(:result) { subject.execute }
 
     subject { described_class.new(environment, params) }
 
-    shared_examples 'success' do
+    # Default implementation of the w method. The `success` shared example overrides
+    # this implementation to test the Ruby and Liquid syntaxes.
+    # This method wraps the given variable name in the variable interpolation
+    # syntax. Using this method along with the `success` shared example allows
+    # a spec to test both syntaxes.
+    def w(variable_name)
+      "%{#{variable_name}}"
+    end
+
+    shared_examples 'replaces variables with values' do
       it 'replaces variables with values' do
         expect(result[:status]).to eq(:success)
         expect(result[:params][:query]).to eq(expected_query)
       end
     end
 
+    shared_examples 'success' do
+      context 'with Ruby syntax `${}`' do
+        it_behaves_like 'replaces variables with values'
+
+        def w(variable_name)
+          "%{#{variable_name}}"
+        end
+      end
+
+      context 'with Liquid syntax `{{}}`' do
+        it_behaves_like 'replaces variables with values'
+
+        def w(variable_name)
+          "{{#{variable_name}}}"
+        end
+      end
+    end
+
     shared_examples 'error' do |message|
       it 'returns error' do
         expect(result[:status]).to eq(:error)
@@ -39,11 +66,13 @@ describe Prometheus::ProxyVariableSubstitutionService do
     end
 
     context 'with predefined variables' do
-      let(:params_keys) { { query: 'up{%{environment_filter}}' } }
+      # Liquid replaces the opening brace of the query as well, if there is no space
+      # between `up{` and the rest of the string.
+      let(:params_keys) { { query: "up{ #{w('environment_filter')}}" } }
 
       it_behaves_like 'success' do
         let(:expected_query) do
-          %Q[up{container_name!="POD",environment="#{environment.slug}"}]
+          %Q[up{ container_name!="POD",environment="#{environment.slug}"}]
         end
       end
 
@@ -54,26 +83,16 @@ describe Prometheus::ProxyVariableSubstitutionService do
           let(:expected_query) { nil }
         end
       end
+    end
 
-      context 'with liquid format' do
-        let(:params_keys) do
-          { query: 'up{environment="{{ci_environment_slug}}"}' }
-        end
-
-        it_behaves_like 'success' do
-          let(:expected_query) { %Q[up{environment="#{environment.slug}"}] }
-        end
+    context 'with ruby and liquid formats' do
+      let(:params_keys) do
+        { query: 'up{%{environment_filter},env2="{{ci_environment_slug}}"}' }
       end
 
-      context 'with ruby and liquid formats' do
-        let(:params_keys) do
-          { query: 'up{%{environment_filter},env2="{{ci_environment_slug}}"}' }
-        end
-
-        it_behaves_like 'success' do
-          let(:expected_query) do
-            %Q[up{container_name!="POD",environment="#{environment.slug}",env2="#{environment.slug}"}]
-          end
+      it_behaves_like 'success' do
+        let(:expected_query) do
+          %Q[up{container_name!="POD",environment="#{environment.slug}",env2="#{environment.slug}"}]
         end
       end
     end
@@ -83,7 +102,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
 
       let(:params_keys) do
         {
-          query: 'up{pod_name="{{pod_name}}"}',
+          query: "up{pod_name=\"#{w('pod_name')}\"}",
           variables: ['pod_name', pod_name]
         }
       end
@@ -92,24 +111,10 @@ describe Prometheus::ProxyVariableSubstitutionService do
         let(:expected_query) { %q[up{pod_name="pod1"}] }
       end
 
-      context 'with ruby variable interpolation format' do
-        let(:params_keys) do
-          {
-            query: 'up{pod_name="%{pod_name}"}',
-            variables: ['pod_name', pod_name]
-          }
-        end
-
-        it_behaves_like 'success' do
-          # Custom variables cannot be used with the Ruby interpolation format.
-          let(:expected_query) { "up{pod_name=\"%{pod_name}\"}" }
-        end
-      end
-
       context 'with predefined variables in variables parameter' do
         let(:params_keys) do
           {
-            query: 'up{pod_name="{{pod_name}}",env="{{ci_environment_slug}}"}',
+            query: "up{pod_name=\"#{w('pod_name')}\",env=\"#{w('ci_environment_slug')}\"}",
             variables: ['pod_name', pod_name, 'ci_environment_slug', 'custom_value']
           }
         end
@@ -124,7 +129,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
       context 'with invalid variables parameter' do
         let(:params_keys) do
           {
-            query: 'up{pod_name="{{pod_name}}"}',
+            query: "up{pod_name=\"#{w('pod_name')}\"}",
             variables: ['a']
           }
         end
@@ -136,68 +141,66 @@ describe Prometheus::ProxyVariableSubstitutionService do
       context 'with nil variables' do
         let(:params_keys) do
           {
-            query: 'up{pod_name="{{pod_name}}"}',
+            query: "up{pod_name=\"%{pod_name}\"}",
             variables: nil
           }
         end
 
-        it_behaves_like 'success' do
-          let(:expected_query) { 'up{pod_name=""}' }
+        it_behaves_like 'replaces variables with values' do
+          let(:expected_query) { "up{pod_name=\"%{pod_name}\"}" }
         end
       end
+    end
 
-      context 'with ruby and liquid variables' do
+    context 'gsub variable substitution tolerance for weirdness' do
+      context 'with whitespace around variable' do
         let(:params_keys) do
           {
-            query: 'up{env1="%{ruby_variable}",env2="{{ liquid_variable }}"}',
-            variables: %w(ruby_variable value liquid_variable env_slug)
+            query: 'up{' \
+                "env1=#{w(' ci_environment_slug')}," \
+                "env2=#{w('ci_environment_slug ')}," \
+                "#{w('  environment_filter ')}" \
+              '}'
           }
         end
 
         it_behaves_like 'success' do
-          # It should replace only liquid variables with their values
-          let(:expected_query) { %q[up{env1="%{ruby_variable}",env2="env_slug"}] }
+          let(:expected_query) do
+            'up{' \
+              "env1=#{environment.slug}," \
+              "env2=#{environment.slug}," \
+              "container_name!=\"POD\",environment=\"#{environment.slug}\"" \
+            '}'
+          end
         end
       end
-    end
-
-    context 'with liquid tags and ruby format variables' do
-      let(:params_keys) do
-        {
-          query: 'up{ {% if true %}env1="%{ci_environment_slug}",' \
-            'env2="{{ci_environment_slug}}"{% endif %} }'
-        }
-      end
 
-      # The following spec will fail and should be changed to a 'success' spec
-      # once we remove support for the Ruby interpolation format.
-      # https://gitlab.com/gitlab-org/gitlab/issues/37990
-      #
-      # Liquid tags `{% %}` cannot be used currently because the Ruby `%`
-      # operator raises an error when it encounters a Liquid `{% %}` tag in the
-      # string.
-      #
-      # Once we remove support for the Ruby format, users can start using
-      # Liquid tags.
-
-      it_behaves_like 'error', 'Malformed string'
-    end
+      context 'with liquid tags and ruby format variables' do
+        let(:params_keys) do
+          {
+            query: 'up{ {% if true %}env1="%{ci_environment_slug}",' \
+              'env2="{{ci_environment_slug}}"{% endif %} }'
+          }
+        end
 
-    context 'ruby template rendering' do
-      let(:params_keys) do
-        { query: 'up{env=%{ci_environment_slug},%{environment_filter}}' }
+        it_behaves_like 'replaces variables with values' do
+          let(:expected_query) { "up{ env1=\"#{environment.slug}\",env2=\"#{environment.slug}\" }" }
+        end
       end
 
-      it_behaves_like 'success' do
-        let(:expected_query) do
-          "up{env=#{environment.slug},container_name!=\"POD\"," \
-          "environment=\"#{environment.slug}\"}"
+      context 'with empty variables' do
+        let(:params_keys) do
+          { query: "up{env1=%{},env2=%{  }}" }
+        end
+
+        it_behaves_like 'replaces variables with values' do
+          let(:expected_query) { "up{env1=%{},env2=%{  }}" }
         end
       end
 
       context 'with multiple occurrences of variable in string' do
         let(:params_keys) do
-          { query: 'up{env1=%{ci_environment_slug},env2=%{ci_environment_slug}}' }
+          { query: "up{env1=#{w('ci_environment_slug')},env2=#{w('ci_environment_slug')}}" }
         end
 
         it_behaves_like 'success' do
@@ -207,7 +210,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
 
       context 'with multiple variables in string' do
         let(:params_keys) do
-          { query: 'up{env=%{ci_environment_slug},%{environment_filter}}' }
+          { query: "up{env=#{w('ci_environment_slug')},#{w('environment_filter')}}" }
         end
 
         it_behaves_like 'success' do
@@ -219,54 +222,24 @@ describe Prometheus::ProxyVariableSubstitutionService do
       end
 
       context 'with unknown variables in string' do
-        let(:params_keys) { { query: 'up{env=%{env_slug}}' } }
+        let(:params_keys) { { query: "up{env=#{w('env_slug')}}" } }
 
-        it_behaves_like 'success' do
-          let(:expected_query) { 'up{env=%{env_slug}}' }
-        end
-      end
-
-      # This spec is needed if there are multiple keys in the context provided
-      # by `Gitlab::Prometheus::QueryVariables.call(environment)` which is
-      # passed to the Ruby `%` operator.
-      # If the number of keys in the context is one, there is no need for
-      # this spec.
-      context 'with extra variables in context' do
-        let(:params_keys) { { query: 'up{env=%{ci_environment_slug}}' } }
-
-        it_behaves_like 'success' do
-          let(:expected_query) { "up{env=#{environment.slug}}" }
-        end
-
-        it 'has more than one variable in context' do
-          expect(Gitlab::Prometheus::QueryVariables.call(environment).size).to be > 1
+        it_behaves_like 'replaces variables with values' do
+          let(:expected_query) { "up{env=%{env_slug}}" }
         end
       end
 
       # The ruby % operator will not replace known variables if there are unknown
       # variables also in the string. It doesn't raise an error
       # (though the `sprintf` and `format` methods do).
+      # Fortunately, we do not use the % operator anymore.
       context 'with unknown and known variables in string' do
         let(:params_keys) do
-          { query: 'up{env=%{ci_environment_slug},other_env=%{env_slug}}' }
+          { query: "up{env=%{ci_environment_slug},other_env=%{env_slug}}" }
         end
 
-        it_behaves_like 'success' do
-          let(:expected_query) { 'up{env=%{ci_environment_slug},other_env=%{env_slug}}' }
-        end
-      end
-
-      context 'when rendering raises error' do
-        context 'when TypeError is raised' do
-          let(:params_keys) { { query: '{% a %}' } }
-
-          it_behaves_like 'error', 'Malformed string'
-        end
-
-        context 'when ArgumentError is raised' do
-          let(:params_keys) { { query: '%<' } }
-
-          it_behaves_like 'error', 'Malformed string'
+        it_behaves_like 'replaces variables with values' do
+          let(:expected_query) { "up{env=#{environment.slug},other_env=#{w('env_slug')}}" }
         end
       end
     end