Commit 4cc9d3e2 authored by GitLab Bot's avatar GitLab Bot

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

parent e81a7b71
...@@ -4,6 +4,16 @@ module Prometheus ...@@ -4,6 +4,16 @@ module Prometheus
class ProxyVariableSubstitutionService < BaseService class ProxyVariableSubstitutionService < BaseService
include Stepable 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, steps :validate_variables,
:add_params_to_result, :add_params_to_result,
:substitute_params, :substitute_params,
...@@ -46,6 +56,14 @@ module Prometheus ...@@ -46,6 +56,14 @@ module Prometheus
success(result) success(result)
end 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) def substitute_liquid_variables(result)
return success(result) unless query(result) return success(result) unless query(result)
...@@ -57,26 +75,20 @@ module Prometheus ...@@ -57,26 +75,20 @@ module Prometheus
error(e.message) error(e.message)
end end
def substitute_ruby_variables(result) def gsub(string, context)
return success(result) unless query(result) # Search for variables of the form `%{variable}` in the string and replace
# them with their value.
# The % operator doesn't replace variables if the hash contains string string.gsub(VARIABLE_INTERPOLATION_REGEX) do |match|
# keys. # Replace with the value of the variable, or if there is no such variable,
result[:params][:query] = query(result) % predefined_context.symbolize_keys # replace the invalid variable with itself. So,
# `up{instance="%{invalid_variable}"}` will remain
success(result) # `up{instance="%{invalid_variable}"}` after substitution.
rescue TypeError, ArgumentError => exception context.fetch($~[:variable], match)
log_error(exception.message) end
Gitlab::ErrorTracking.track_exception(exception, {
template_string: query(result),
variables: predefined_context
})
error(_('Malformed string'))
end end
def predefined_context def predefined_context
@predefined_context ||= Gitlab::Prometheus::QueryVariables.call(@environment) Gitlab::Prometheus::QueryVariables.call(@environment).stringify_keys
end end
def full_context def full_context
......
---
title: Use `gsub` instead of the Ruby `%` operator to perform variable substitution in Prometheus proxy API
merge_request:
author:
type: security
...@@ -12434,9 +12434,6 @@ msgstr "" ...@@ -12434,9 +12434,6 @@ msgstr ""
msgid "Makes this issue confidential." msgid "Makes this issue confidential."
msgstr "" msgstr ""
msgid "Malformed string"
msgstr ""
msgid "Manage" msgid "Manage"
msgstr "" msgstr ""
......
...@@ -6,19 +6,46 @@ describe Prometheus::ProxyVariableSubstitutionService do ...@@ -6,19 +6,46 @@ describe Prometheus::ProxyVariableSubstitutionService do
describe '#execute' do describe '#execute' do
let_it_be(:environment) { create(:environment) } 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(:params) { ActionController::Parameters.new(params_keys).permit! }
let(:result) { subject.execute } let(:result) { subject.execute }
subject { described_class.new(environment, params) } 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 it 'replaces variables with values' do
expect(result[:status]).to eq(:success) expect(result[:status]).to eq(:success)
expect(result[:params][:query]).to eq(expected_query) expect(result[:params][:query]).to eq(expected_query)
end end
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| shared_examples 'error' do |message|
it 'returns error' do it 'returns error' do
expect(result[:status]).to eq(:error) expect(result[:status]).to eq(:error)
...@@ -39,11 +66,13 @@ describe Prometheus::ProxyVariableSubstitutionService do ...@@ -39,11 +66,13 @@ describe Prometheus::ProxyVariableSubstitutionService do
end end
context 'with predefined variables' do 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 it_behaves_like 'success' do
let(:expected_query) do let(:expected_query) do
%Q[up{container_name!="POD",environment="#{environment.slug}"}] %Q[up{ container_name!="POD",environment="#{environment.slug}"}]
end end
end end
...@@ -54,26 +83,16 @@ describe Prometheus::ProxyVariableSubstitutionService do ...@@ -54,26 +83,16 @@ describe Prometheus::ProxyVariableSubstitutionService do
let(:expected_query) { nil } let(:expected_query) { nil }
end end
end end
end
context 'with liquid format' do context 'with ruby and liquid formats' do
let(:params_keys) do let(:params_keys) do
{ query: 'up{environment="{{ci_environment_slug}}"}' } { query: 'up{%{environment_filter},env2="{{ci_environment_slug}}"}' }
end
it_behaves_like 'success' do
let(:expected_query) { %Q[up{environment="#{environment.slug}"}] }
end
end end
context 'with ruby and liquid formats' do it_behaves_like 'success' do
let(:params_keys) do let(:expected_query) do
{ query: 'up{%{environment_filter},env2="{{ci_environment_slug}}"}' } %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
end end
end end
...@@ -83,7 +102,7 @@ describe Prometheus::ProxyVariableSubstitutionService do ...@@ -83,7 +102,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
let(:params_keys) do let(:params_keys) do
{ {
query: 'up{pod_name="{{pod_name}}"}', query: "up{pod_name=\"#{w('pod_name')}\"}",
variables: ['pod_name', pod_name] variables: ['pod_name', pod_name]
} }
end end
...@@ -92,24 +111,10 @@ describe Prometheus::ProxyVariableSubstitutionService do ...@@ -92,24 +111,10 @@ describe Prometheus::ProxyVariableSubstitutionService do
let(:expected_query) { %q[up{pod_name="pod1"}] } let(:expected_query) { %q[up{pod_name="pod1"}] }
end 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 context 'with predefined variables in variables parameter' do
let(:params_keys) 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'] variables: ['pod_name', pod_name, 'ci_environment_slug', 'custom_value']
} }
end end
...@@ -124,7 +129,7 @@ describe Prometheus::ProxyVariableSubstitutionService do ...@@ -124,7 +129,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
context 'with invalid variables parameter' do context 'with invalid variables parameter' do
let(:params_keys) do let(:params_keys) do
{ {
query: 'up{pod_name="{{pod_name}}"}', query: "up{pod_name=\"#{w('pod_name')}\"}",
variables: ['a'] variables: ['a']
} }
end end
...@@ -136,68 +141,66 @@ describe Prometheus::ProxyVariableSubstitutionService do ...@@ -136,68 +141,66 @@ describe Prometheus::ProxyVariableSubstitutionService do
context 'with nil variables' do context 'with nil variables' do
let(:params_keys) do let(:params_keys) do
{ {
query: 'up{pod_name="{{pod_name}}"}', query: "up{pod_name=\"%{pod_name}\"}",
variables: nil variables: nil
} }
end end
it_behaves_like 'success' do it_behaves_like 'replaces variables with values' do
let(:expected_query) { 'up{pod_name=""}' } let(:expected_query) { "up{pod_name=\"%{pod_name}\"}" }
end end
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 let(:params_keys) do
{ {
query: 'up{env1="%{ruby_variable}",env2="{{ liquid_variable }}"}', query: 'up{' \
variables: %w(ruby_variable value liquid_variable env_slug) "env1=#{w(' ci_environment_slug')}," \
"env2=#{w('ci_environment_slug ')}," \
"#{w(' environment_filter ')}" \
'}'
} }
end end
it_behaves_like 'success' do it_behaves_like 'success' do
# It should replace only liquid variables with their values let(:expected_query) do
let(:expected_query) { %q[up{env1="%{ruby_variable}",env2="env_slug"}] } 'up{' \
"env1=#{environment.slug}," \
"env2=#{environment.slug}," \
"container_name!=\"POD\",environment=\"#{environment.slug}\"" \
'}'
end
end 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 context 'with liquid tags and ruby format variables' do
# once we remove support for the Ruby interpolation format. let(:params_keys) do
# https://gitlab.com/gitlab-org/gitlab/issues/37990 {
# query: 'up{ {% if true %}env1="%{ci_environment_slug}",' \
# Liquid tags `{% %}` cannot be used currently because the Ruby `%` 'env2="{{ci_environment_slug}}"{% endif %} }'
# operator raises an error when it encounters a Liquid `{% %}` tag in the }
# string. end
#
# Once we remove support for the Ruby format, users can start using
# Liquid tags.
it_behaves_like 'error', 'Malformed string'
end
context 'ruby template rendering' do it_behaves_like 'replaces variables with values' do
let(:params_keys) do let(:expected_query) { "up{ env1=\"#{environment.slug}\",env2=\"#{environment.slug}\" }" }
{ query: 'up{env=%{ci_environment_slug},%{environment_filter}}' } end
end end
it_behaves_like 'success' do context 'with empty variables' do
let(:expected_query) do let(:params_keys) do
"up{env=#{environment.slug},container_name!=\"POD\"," \ { query: "up{env1=%{},env2=%{ }}" }
"environment=\"#{environment.slug}\"}" end
it_behaves_like 'replaces variables with values' do
let(:expected_query) { "up{env1=%{},env2=%{ }}" }
end end
end end
context 'with multiple occurrences of variable in string' do context 'with multiple occurrences of variable in string' do
let(:params_keys) 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 end
it_behaves_like 'success' do it_behaves_like 'success' do
...@@ -207,7 +210,7 @@ describe Prometheus::ProxyVariableSubstitutionService do ...@@ -207,7 +210,7 @@ describe Prometheus::ProxyVariableSubstitutionService do
context 'with multiple variables in string' do context 'with multiple variables in string' do
let(:params_keys) do let(:params_keys) do
{ query: 'up{env=%{ci_environment_slug},%{environment_filter}}' } { query: "up{env=#{w('ci_environment_slug')},#{w('environment_filter')}}" }
end end
it_behaves_like 'success' do it_behaves_like 'success' do
...@@ -219,54 +222,24 @@ describe Prometheus::ProxyVariableSubstitutionService do ...@@ -219,54 +222,24 @@ describe Prometheus::ProxyVariableSubstitutionService do
end end
context 'with unknown variables in string' do 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 it_behaves_like 'replaces variables with values' do
let(:expected_query) { 'up{env=%{env_slug}}' } 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
end end
end end
# The ruby % operator will not replace known variables if there are unknown # The ruby % operator will not replace known variables if there are unknown
# variables also in the string. It doesn't raise an error # variables also in the string. It doesn't raise an error
# (though the `sprintf` and `format` methods do). # (though the `sprintf` and `format` methods do).
# Fortunately, we do not use the % operator anymore.
context 'with unknown and known variables in string' do context 'with unknown and known variables in string' do
let(:params_keys) 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 end
it_behaves_like 'success' do it_behaves_like 'replaces variables with values' do
let(:expected_query) { 'up{env=%{ci_environment_slug},other_env=%{env_slug}}' } let(:expected_query) { "up{env=#{environment.slug},other_env=#{w('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'
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment