summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2022-09-28 22:05:40 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2022-09-28 22:05:44 +0000
commitf5897da89ca63facbef54c23cff894f2bbe8e644 (patch)
tree9671f60e71b3cb78705a211977870fb1c5a4b354
parent10d9a3bf50cca85dd857c5306a34d7a6032580e6 (diff)
downloadgitlab-ce-f5897da89ca63facbef54c23cff894f2bbe8e644.tar.gz
Add latest changes from gitlab-org/security/gitlab@15-4-stable-ee
-rw-r--r--.rubocop_todo/style/string_concatenation.yml1
-rw-r--r--app/models/concerns/integrations/has_web_hook.rb11
-rw-r--r--app/models/concerns/safe_url.rb9
-rw-r--r--app/models/integrations/buildkite.rb6
-rw-r--r--app/models/integrations/datadog.rb8
-rw-r--r--app/models/integrations/drone_ci.rb6
-rw-r--r--app/models/integrations/jenkins.rb4
-rw-r--r--app/models/integrations/packagist.rb6
-rw-r--r--spec/models/concerns/integrations/has_web_hook_spec.rb28
-rw-r--r--spec/models/integrations/buildkite_spec.rb4
-rw-r--r--spec/models/integrations/datadog_spec.rb15
-rw-r--r--spec/models/integrations/drone_ci_spec.rb2
-rw-r--r--spec/models/integrations/packagist_spec.rb2
-rw-r--r--spec/support/shared_examples/controllers/concerns/web_hooks/integrations_hook_log_actions_shared_examples.rb2
-rw-r--r--spec/support/shared_examples/models/integrations/has_web_hook_shared_examples.rb6
15 files changed, 89 insertions, 21 deletions
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])