From f5897da89ca63facbef54c23cff894f2bbe8e644 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 28 Sep 2022 22:05:40 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-4-stable-ee --- .rubocop_todo/style/string_concatenation.yml | 1 + app/models/concerns/integrations/has_web_hook.rb | 11 ++++++++- app/models/concerns/safe_url.rb | 9 ++++--- app/models/integrations/buildkite.rb | 6 ++++- app/models/integrations/datadog.rb | 8 +++++-- app/models/integrations/drone_ci.rb | 6 ++++- app/models/integrations/jenkins.rb | 4 ++++ app/models/integrations/packagist.rb | 6 ++++- .../concerns/integrations/has_web_hook_spec.rb | 28 ++++++++++++++++++++++ spec/models/integrations/buildkite_spec.rb | 4 ++-- spec/models/integrations/datadog_spec.rb | 15 ++++++------ spec/models/integrations/drone_ci_spec.rb | 2 +- spec/models/integrations/packagist_spec.rb | 2 +- ...ntegrations_hook_log_actions_shared_examples.rb | 2 +- .../integrations/has_web_hook_shared_examples.rb | 6 +++++ 15 files changed, 89 insertions(+), 21 deletions(-) create mode 100644 spec/models/concerns/integrations/has_web_hook_spec.rb diff --git a/.rubocop_todo/style/string_concatenation.yml b/.rubocop_todo/style/string_concatenation.yml index 3dd708d2c49..315ce3701a8 100644 --- a/.rubocop_todo/style/string_concatenation.yml +++ b/.rubocop_todo/style/string_concatenation.yml @@ -265,6 +265,7 @@ Style/StringConcatenation: - 'spec/models/integrations/campfire_spec.rb' - 'spec/models/integrations/chat_message/pipeline_message_spec.rb' - 'spec/models/integrations/chat_message/push_message_spec.rb' + - 'spec/models/integrations/datadog_spec.rb' - 'spec/models/integrations/jenkins_spec.rb' - 'spec/models/merge_request_diff_spec.rb' - 'spec/models/merge_request_spec.rb' diff --git a/app/models/concerns/integrations/has_web_hook.rb b/app/models/concerns/integrations/has_web_hook.rb index e6ca6cc7938..5fd71f3d72f 100644 --- a/app/models/concerns/integrations/has_web_hook.rb +++ b/app/models/concerns/integrations/has_web_hook.rb @@ -14,6 +14,11 @@ module Integrations raise NotImplementedError end + # Return the url variables to be used for the webhook. + def url_variables + raise NotImplementedError + end + # Return whether the webhook should use SSL verification. def hook_ssl_verification if respond_to?(:enable_ssl_verification) @@ -26,7 +31,11 @@ module Integrations # Create or update the webhook, raising an exception if it cannot be saved. def update_web_hook! hook = service_hook || build_service_hook - hook.url = hook_url if hook.url != hook_url # avoid reencryption + + # Avoid reencryption + hook.url = hook_url if hook.url != hook_url + hook.url_variables = url_variables if hook.url_variables != url_variables + hook.enable_ssl_verification = hook_ssl_verification hook.save! if hook.changed? hook diff --git a/app/models/concerns/safe_url.rb b/app/models/concerns/safe_url.rb index 7dce05bddba..d6378e6ac6f 100644 --- a/app/models/concerns/safe_url.rb +++ b/app/models/concerns/safe_url.rb @@ -3,13 +3,16 @@ module SafeUrl extend ActiveSupport::Concern + # Return the URL with obfuscated userinfo + # and keeping it intact def safe_url(allowed_usernames: []) return if url.nil? - uri = URI.parse(url) + escaped = Addressable::URI.escape(url) + uri = URI.parse(escaped) uri.password = '*****' if uri.password uri.user = '*****' if uri.user && allowed_usernames.exclude?(uri.user) - uri.to_s - rescue URI::Error + Addressable::URI.unescape(uri.to_s) + rescue URI::Error, TypeError end end diff --git a/app/models/integrations/buildkite.rb b/app/models/integrations/buildkite.rb index 7a48e71b934..f2d2aca3ffe 100644 --- a/app/models/integrations/buildkite.rb +++ b/app/models/integrations/buildkite.rb @@ -50,7 +50,11 @@ module Integrations override :hook_url def hook_url - "#{buildkite_endpoint('webhook')}/deliver/#{webhook_token}" + "#{buildkite_endpoint('webhook')}/deliver/{webhook_token}" + end + + def url_variables + { 'webhook_token' => webhook_token } end def execute(data) diff --git a/app/models/integrations/datadog.rb b/app/models/integrations/datadog.rb index 4479725a33b..c9407aa738e 100644 --- a/app/models/integrations/datadog.rb +++ b/app/models/integrations/datadog.rb @@ -154,13 +154,17 @@ module Integrations url = api_url.presence || sprintf(URL_TEMPLATE, datadog_domain: datadog_domain) url = URI.parse(url) query = { - "dd-api-key" => api_key, + "dd-api-key" => 'THIS_VALUE_WILL_BE_REPLACED', service: datadog_service.presence, env: datadog_env.presence, tags: datadog_tags_query_param.presence }.compact url.query = query.to_query - url.to_s + url.to_s.gsub('THIS_VALUE_WILL_BE_REPLACED', '{api_key}') + end + + def url_variables + { 'api_key' => api_key } end def execute(data) diff --git a/app/models/integrations/drone_ci.rb b/app/models/integrations/drone_ci.rb index de69afeba6a..d1a64aa96d4 100644 --- a/app/models/integrations/drone_ci.rb +++ b/app/models/integrations/drone_ci.rb @@ -106,7 +106,11 @@ module Integrations override :hook_url def hook_url - [drone_url, "/hook", "?owner=#{project.namespace.full_path}", "&name=#{project.path}", "&access_token=#{token}"].join + [drone_url, "/hook", "?owner=#{project.namespace.full_path}", "&name=#{project.path}", "&access_token={token}"].join + end + + def url_variables + { 'token' => token } end override :update_web_hook! diff --git a/app/models/integrations/jenkins.rb b/app/models/integrations/jenkins.rb index c68b5fd2a96..74a6449f4f9 100644 --- a/app/models/integrations/jenkins.rb +++ b/app/models/integrations/jenkins.rb @@ -69,6 +69,10 @@ module Integrations url.to_s end + def url_variables + {} + end + def self.supported_events %w(push merge_request tag_push) end diff --git a/app/models/integrations/packagist.rb b/app/models/integrations/packagist.rb index f91404dab23..7177c82a167 100644 --- a/app/models/integrations/packagist.rb +++ b/app/models/integrations/packagist.rb @@ -66,7 +66,11 @@ module Integrations override :hook_url def hook_url base_url = server.presence || 'https://packagist.org' - "#{base_url}/api/update-package?username=#{username}&apiToken=#{token}" + "#{base_url}/api/update-package?username={username}&apiToken={token}" + end + + def url_variables + { 'username' => username, 'token' => token } end end end diff --git a/spec/models/concerns/integrations/has_web_hook_spec.rb b/spec/models/concerns/integrations/has_web_hook_spec.rb new file mode 100644 index 00000000000..9061cb90f90 --- /dev/null +++ b/spec/models/concerns/integrations/has_web_hook_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Integrations::HasWebHook do + let(:integration_class) do + Class.new(Integration) do + include Integrations::HasWebHook + end + end + + let(:integration) { integration_class.new } + + context 'when hook_url and url_variables are not implemented' do + it { expect { integration.hook_url }.to raise_error(NotImplementedError) } + it { expect { integration.url_variables }.to raise_error(NotImplementedError) } + end + + context 'when integration does not respond to enable_ssl_verification' do + it { expect(integration.hook_ssl_verification).to eq true } + end + + context 'when integration responds to enable_ssl_verification' do + let(:integration) { build(:drone_ci_integration) } + + it { expect(integration.hook_ssl_verification).to eq true } + end +end diff --git a/spec/models/integrations/buildkite_spec.rb b/spec/models/integrations/buildkite_spec.rb index 38ceb5db49c..c720dc6d418 100644 --- a/spec/models/integrations/buildkite_spec.rb +++ b/spec/models/integrations/buildkite_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do end it_behaves_like Integrations::HasWebHook do - let(:hook_url) { 'https://webhook.buildkite.com/deliver/secret-sauce-webhook-token' } + let(:hook_url) { 'https://webhook.buildkite.com/deliver/{webhook_token}' } end describe 'Validations' do @@ -68,7 +68,7 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do describe '#hook_url' do it 'returns the webhook url' do expect(integration.hook_url).to eq( - 'https://webhook.buildkite.com/deliver/secret-sauce-webhook-token' + 'https://webhook.buildkite.com/deliver/{webhook_token}' ) end end diff --git a/spec/models/integrations/datadog_spec.rb b/spec/models/integrations/datadog_spec.rb index 4ac684e8ff0..b7da6a79e44 100644 --- a/spec/models/integrations/datadog_spec.rb +++ b/spec/models/integrations/datadog_spec.rb @@ -19,6 +19,7 @@ RSpec.describe Integrations::Datadog do let(:dd_tags) { '' } let(:expected_hook_url) { default_url + "?dd-api-key=#{api_key}&env=#{dd_env}&service=#{dd_service}" } + let(:hook_url) { default_url + "?dd-api-key={api_key}&env=#{dd_env}&service=#{dd_service}" } let(:instance) do described_class.new( @@ -48,7 +49,7 @@ RSpec.describe Integrations::Datadog do it_behaves_like Integrations::HasWebHook do let(:integration) { instance } - let(:hook_url) { "#{described_class::URL_TEMPLATE % { datadog_domain: dd_site }}?dd-api-key=#{api_key}&env=#{dd_env}&service=#{dd_service}" } + let(:hook_url) { "#{described_class::URL_TEMPLATE % { datadog_domain: dd_site }}?dd-api-key={api_key}&env=#{dd_env}&service=#{dd_service}" } end describe 'validations' do @@ -132,18 +133,18 @@ RSpec.describe Integrations::Datadog do subject { instance.hook_url } context 'with standard site URL' do - it { is_expected.to eq(expected_hook_url) } + it { is_expected.to eq(hook_url) } end context 'with custom URL' do let(:api_url) { 'https://webhook-intake.datad0g.com/api/v2/webhook' } - it { is_expected.to eq(api_url + "?dd-api-key=#{api_key}&env=#{dd_env}&service=#{dd_service}") } + it { is_expected.to eq(api_url + "?dd-api-key={api_key}&env=#{dd_env}&service=#{dd_service}") } context 'blank' do let(:api_url) { '' } - it { is_expected.to eq(expected_hook_url) } + it { is_expected.to eq(hook_url) } end end @@ -152,19 +153,19 @@ RSpec.describe Integrations::Datadog do let(:dd_env) { '' } let(:dd_tags) { '' } - it { is_expected.to eq(default_url + "?dd-api-key=#{api_key}") } + it { is_expected.to eq(default_url + "?dd-api-key={api_key}") } end context 'with custom tags' do let(:dd_tags) { "key:value\nkey2:value, 2" } let(:escaped_tags) { CGI.escape("key:value,\"key2:value, 2\"") } - it { is_expected.to eq(expected_hook_url + "&tags=#{escaped_tags}") } + it { is_expected.to eq(hook_url + "&tags=#{escaped_tags}") } context 'and empty lines' do let(:dd_tags) { "key:value\r\n\n\n\nkey2:value, 2\n" } - it { is_expected.to eq(expected_hook_url + "&tags=#{escaped_tags}") } + it { is_expected.to eq(hook_url + "&tags=#{escaped_tags}") } end end end diff --git a/spec/models/integrations/drone_ci_spec.rb b/spec/models/integrations/drone_ci_spec.rb index 905fee075ad..f3203a6e69d 100644 --- a/spec/models/integrations/drone_ci_spec.rb +++ b/spec/models/integrations/drone_ci_spec.rb @@ -116,7 +116,7 @@ RSpec.describe Integrations::DroneCi, :use_clean_rails_memory_store_caching do include_context :drone_ci_integration let(:integration) { drone } - let(:hook_url) { "#{drone_url}/hook?owner=#{project.namespace.full_path}&name=#{project.path}&access_token=#{token}" } + let(:hook_url) { "#{drone_url}/hook?owner=#{project.namespace.full_path}&name=#{project.path}&access_token={token}" } it 'does not create a hook if project is not present' do integration.project = nil diff --git a/spec/models/integrations/packagist_spec.rb b/spec/models/integrations/packagist_spec.rb index d1976e73e2e..e078debd126 100644 --- a/spec/models/integrations/packagist_spec.rb +++ b/spec/models/integrations/packagist_spec.rb @@ -26,7 +26,7 @@ RSpec.describe Integrations::Packagist do it_behaves_like Integrations::HasWebHook do let(:integration) { described_class.new(packagist_params) } - let(:hook_url) { "#{packagist_server}/api/update-package?username=#{packagist_username}&apiToken=#{packagist_token}" } + let(:hook_url) { "#{packagist_server}/api/update-package?username={username}&apiToken={token}" } end it_behaves_like Integrations::ResetSecretFields do diff --git a/spec/support/shared_examples/controllers/concerns/web_hooks/integrations_hook_log_actions_shared_examples.rb b/spec/support/shared_examples/controllers/concerns/web_hooks/integrations_hook_log_actions_shared_examples.rb index 62c9c3508a8..56a5dcb10b2 100644 --- a/spec/support/shared_examples/controllers/concerns/web_hooks/integrations_hook_log_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/concerns/web_hooks/integrations_hook_log_actions_shared_examples.rb @@ -26,7 +26,7 @@ RSpec.shared_examples WebHooks::HookLogActions do describe 'POST #retry' do it 'executes the hook and redirects to the service form' do - stub_request(:post, web_hook.url) + stub_request(:post, web_hook.interpolated_url) expect_next_found_instance_of(web_hook.class) do |hook| expect(hook).to receive(:execute).and_call_original diff --git a/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb b/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb index 2f693edeb53..e309aa50c6e 100644 --- a/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb +++ b/spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb @@ -37,6 +37,12 @@ RSpec.shared_examples Integrations::HasWebHook do end end + describe '#url_variables' do + it 'returns a string' do + expect(integration.url_variables).to be_a(Hash) + end + end + describe '#hook_ssl_verification' do it 'returns a boolean' do expect(integration.hook_ssl_verification).to be_in([true, false]) -- cgit v1.2.3