diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-05-26 14:45:03 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-05-26 14:45:03 +0000 |
commit | 4cc9d3e28ab73ad593b2abb9f43831865f040e22 (patch) | |
tree | 85b65a5e56534a7b6bee92c8b76cac4789b53c3f | |
parent | e81a7b71c45d6c66f11ac6c1eda11ff8c73a1843 (diff) | |
download | gitlab-ce-4cc9d3e28ab73ad593b2abb9f43831865f040e22.tar.gz |
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 |