kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Use Gitlab::HTTP for all chat notifications

Browse source
This commit is contained in:
Hordur Freyr Yngvason 2019-09-27 13:35:37 +02:00
parent b5ad06174b
commit 729040717e
12 changed files with 294 additions and 181 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
app/models/project_services/chat_notification_service.rb
View file

@ -113,12 +113,9 @@ class ChatNotificationService < Service
private
# every notifier must implement this independently
def notify(message, opts)
Slack::Notifier.new(webhook, opts).ping(
message.pretext,
attachments: message.attachments,
fallback: message.fallback
)
raise NotImplementedError
end
def custom_data(data)

2
app/models/project_services/mattermost_service.rb
View file

@ -1,6 +1,8 @@
# frozen_string_literal: true
class MattermostService < ChatNotificationService
include ::SlackService::Notifier
def title
'Mattermost notifications'
end

24
app/models/project_services/slack_service.rb
View file

@ -30,4 +30,28 @@ class SlackService < ChatNotificationService
def webhook_placeholder
'https://hooks.slack.com/services/…'
end
module Notifier
private
def notify(message, opts)
# See https://github.com/stevenosloan/slack-notifier#custom-http-client
notifier = Slack::Notifier.new(webhook, opts.merge(http_client: HTTPClient))
notifier.ping(
message.pretext,
attachments: message.attachments,
fallback: message.fallback
)
end
class HTTPClient
def self.post(uri, params = {})
params.delete(:http_options) # these are internal to the client and we do not want them
Gitlab::HTTP.post(uri, body: params)
end
end
end
include Notifier
end

5
changelogs/unreleased/security-dns-rebind-ssrf-in-slack-notifications.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Limit potential for DNS rebind SSRF in chat notifications
merge_request:
author:
type: security

29
config/initializers/hangouts_chat_http_override.rb Normal file
View file

@ -0,0 +1,29 @@
# frozen_string_literal: true
module HangoutsChat
class Sender
class HTTP
module GitlabHTTPOverride
extend ::Gitlab::Utils::Override
attr_reader :uri
# see https://github.com/enzinia/hangouts-chat/blob/6a509f61a56e757f8f417578b393b94423831ff7/lib/hangouts_chat/http.rb
override :post
def post(payload)
httparty_response = Gitlab::HTTP.post(
uri,
body: payload.to_json,
headers: { 'Content-Type' => 'application/json' },
parse: nil # disables automatic response parsing
)
net_http_response = httparty_response.response
# The rest of the integration expects a Net::HTTP response
net_http_response
end
end
prepend GitlabHTTPOverride
end
end
end

1
lib/microsoft_teams/notifier.rb
View file

@ -14,7 +14,6 @@ module MicrosoftTeams
response = Gitlab::HTTP.post(
@webhook.to_str,
headers: @header,
allow_local_requests: true,
body: body(options)
)

34
spec/initializers/hangouts_chat_http_override_spec.rb Normal file
View file

@ -0,0 +1,34 @@
# frozen_string_literal: true
require 'spec_helper'
describe 'HangoutsChat::Sender Gitlab::HTTP override' do
describe 'HangoutsChat::Sender::HTTP#post' do
it 'calls Gitlab::HTTP.post with default protection settings' do
webhook_url = 'https://example.gitlab.com'
payload = { key: 'value' }
http = HangoutsChat::Sender::HTTP.new(webhook_url)
mock_response = double(response: 'the response')
expect(Gitlab::HTTP).to receive(:post)
.with(
URI.parse(webhook_url),
body: payload.to_json,
headers: { 'Content-Type' => 'application/json' },
parse: nil
)
.and_return(mock_response)
expect(http.post(payload)).to eq(mock_response.response)
end
it_behaves_like 'a request using Gitlab::UrlBlocker' do
let(:http_method) { :post }
let(:url_blocked_error_class) { Gitlab::HTTP::BlockedUrlError }
def make_request(uri)
HangoutsChat::Sender::HTTP.new(uri).post({})
end
end
end
end

145
spec/initializers/rest-client-hostname_override_spec.rb
View file

@ -3,147 +3,12 @@
require 'spec_helper'
describe 'rest-client dns rebinding protection' do
include StubRequests
it_behaves_like 'a request using Gitlab::UrlBlocker' do
let(:http_method) { :get }
let(:url_blocked_error_class) { ArgumentError }
context 'when local requests are not allowed' do
it 'allows an external request with http' do
request_stub = stub_full_request('http://example.com', ip_address: '93.184.216.34')
RestClient.get('http://example.com/')
expect(request_stub).to have_been_requested
end
it 'allows an external request with https' do
request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34')
RestClient.get('https://example.com/')
expect(request_stub).to have_been_requested
end
it 'raises error when it is a request that resolves to a local address' do
stub_full_request('https://example.com', ip_address: '172.16.0.0')
expect { RestClient.get('https://example.com') }
.to raise_error(ArgumentError,
"URL 'https://example.com' is blocked: Requests to the local network are not allowed")
end
it 'raises error when it is a request that resolves to a localhost address' do
stub_full_request('https://example.com', ip_address: '127.0.0.1')
expect { RestClient.get('https://example.com') }
.to raise_error(ArgumentError,
"URL 'https://example.com' is blocked: Requests to localhost are not allowed")
end
it 'raises error when it is a request to local address' do
expect { RestClient.get('http://172.16.0.0') }
.to raise_error(ArgumentError,
"URL 'http://172.16.0.0' is blocked: Requests to the local network are not allowed")
end
it 'raises error when it is a request to localhost address' do
expect { RestClient.get('http://127.0.0.1') }
.to raise_error(ArgumentError,
"URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed")
end
end
context 'when port different from URL scheme is used' do
it 'allows the request' do
request_stub = stub_full_request('https://example.com:8080', ip_address: '93.184.216.34')
RestClient.get('https://example.com:8080/')
expect(request_stub).to have_been_requested
end
it 'raises error when it is a request to local address' do
expect { RestClient.get('https://172.16.0.0:8080') }
.to raise_error(ArgumentError,
"URL 'https://172.16.0.0:8080' is blocked: Requests to the local network are not allowed")
end
it 'raises error when it is a request to localhost address' do
expect { RestClient.get('https://127.0.0.1:8080') }
.to raise_error(ArgumentError,
"URL 'https://127.0.0.1:8080' is blocked: Requests to localhost are not allowed")
end
end
context 'when DNS rebinding protection is disabled' do
before do
stub_application_setting(dns_rebinding_protection_enabled: false)
end
it 'allows the request' do
request_stub = stub_request(:get, 'https://example.com')
RestClient.get('https://example.com/')
expect(request_stub).to have_been_requested
end
end
context 'when http(s) proxy environment variable is set' do
before do
stub_env('https_proxy' => 'https://my.proxy')
end
it 'allows the request' do
request_stub = stub_request(:get, 'https://example.com')
RestClient.get('https://example.com/')
expect(request_stub).to have_been_requested
end
end
context 'when local requests are allowed' do
before do
stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
end
it 'allows an external request' do
request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34')
RestClient.get('https://example.com/')
expect(request_stub).to have_been_requested
end
it 'allows an external request that resolves to a local address' do
request_stub = stub_full_request('https://example.com', ip_address: '172.16.0.0')
RestClient.get('https://example.com/')
expect(request_stub).to have_been_requested
end
it 'allows an external request that resolves to a localhost address' do
request_stub = stub_full_request('https://example.com', ip_address: '127.0.0.1')
RestClient.get('https://example.com/')
expect(request_stub).to have_been_requested
end
it 'allows a local address request' do
request_stub = stub_request(:get, 'http://172.16.0.0')
RestClient.get('http://172.16.0.0')
expect(request_stub).to have_been_requested
end
it 'allows a localhost address request' do
request_stub = stub_request(:get, 'http://127.0.0.1')
RestClient.get('http://127.0.0.1')
expect(request_stub).to have_been_requested
def make_request(uri)
RestClient.get(uri)
end
end
end

17
spec/models/project_services/chat_notification_service_spec.rb
View file

@ -30,7 +30,8 @@ describe ChatNotificationService do
end
describe '#execute' do
let(:chat_service) { described_class.new }
subject(:chat_service) { described_class.new }
let(:user) { create(:user) }
let(:project) { create(:project, :repository) }
let(:webhook_url) { 'https://example.gitlab.com/' }
@ -53,12 +54,7 @@ describe ChatNotificationService do
subject.project = project
data = Gitlab::DataBuilder::Push.build_sample(project, user)
expect(Slack::Notifier).to receive(:new)
.with(webhook_url, {})
.and_return(
double(:slack_service).as_null_object
)
expect(chat_service).to receive(:notify).and_return(true)
expect(chat_service.execute(data)).to be true
end
end
@ -68,12 +64,7 @@ describe ChatNotificationService do
subject.project = create(:project, :empty_repo)
data = Gitlab::DataBuilder::Push.build_sample(subject.project, user)
expect(Slack::Notifier).to receive(:new)
.with(webhook_url, {})
.and_return(
double(:slack_service).as_null_object
)
expect(chat_service).to receive(:notify).and_return(true)
expect(chat_service.execute(data)).to be true
end
end

2
spec/models/project_services/slack_service_spec.rb
View file

@ -3,5 +3,5 @@
require 'spec_helper'
describe SlackService do
it_behaves_like "slack or mattermost notifications", "Slack"
it_behaves_like "slack or mattermost notifications", 'Slack'
end

54
spec/support/shared_examples/slack_mattermost_notifications_shared_examples.rb
View file

@ -1,11 +1,13 @@
# frozen_string_literal: true
RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
include StubRequests
let(:chat_service) { described_class.new }
let(:webhook_url) { 'https://example.gitlab.com/' }
let(:webhook_url) { 'https://example.gitlab.com' }
def execute_with_options(options)
receive(:new).with(webhook_url, options)
receive(:new).with(webhook_url, options.merge(http_client: SlackService::Notifier::HTTPClient))
.and_return(double(:slack_service).as_null_object)
end
@ -38,9 +40,13 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
chat_service.branches_to_be_notified = branches_to_be_notified if branches_to_be_notified
end
let!(:stubbed_resolved_hostname) do
stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s
end
it "notifies about #{event_type} events" do
chat_service.execute(data)
expect(WebMock).to have_requested(:post, webhook_url)
expect(WebMock).to have_requested(:post, stubbed_resolved_hostname)
end
end
@ -49,9 +55,13 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
chat_service.branches_to_be_notified = branches_to_be_notified if branches_to_be_notified
end
let!(:stubbed_resolved_hostname) do
stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s
end
it "notifies about #{event_type} events" do
chat_service.execute(data)
expect(WebMock).not_to have_requested(:post, webhook_url)
expect(WebMock).not_to have_requested(:post, stubbed_resolved_hostname)
end
end
@ -66,6 +76,10 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
Gitlab::DataBuilder::Push.build_sample(project, user)
end
let!(:stubbed_resolved_hostname) do
stub_full_request(webhook_url, method: :post).request_pattern.uri_pattern.to_s
end
before do
allow(chat_service).to receive_messages(
project: project,
@ -74,8 +88,6 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
webhook: webhook_url
)
WebMock.stub_request(:post, webhook_url)
issue_service = Issues::CreateService.new(project, user, issue_service_options)
@issue = issue_service.execute
@issues_sample_data = issue_service.hook_data(@issue, 'open')
@ -107,25 +119,25 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
it "calls #{service_name} API for push events" do
chat_service.execute(data)
expect(WebMock).to have_requested(:post, webhook_url).once
expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once
end
it "calls #{service_name} API for issue events" do
chat_service.execute(@issues_sample_data)
expect(WebMock).to have_requested(:post, webhook_url).once
expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once
end
it "calls #{service_name} API for merge requests events" do
chat_service.execute(@merge_sample_data)
expect(WebMock).to have_requested(:post, webhook_url).once
expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once
end
it "calls #{service_name} API for wiki page events" do
chat_service.execute(@wiki_page_sample_data)
expect(WebMock).to have_requested(:post, webhook_url).once
expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once
end
it "calls #{service_name} API for deployment events" do
@ -133,14 +145,14 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
chat_service.execute(deployment_event_data)
expect(WebMock).to have_requested(:post, webhook_url).once
expect(WebMock).to have_requested(:post, stubbed_resolved_hostname).once
end
it 'uses the username as an option for slack when configured' do
allow(chat_service).to receive(:username).and_return(username)
expect(Slack::Notifier).to receive(:new)
.with(webhook_url, username: username)
.with(webhook_url, username: username, http_client: SlackService::Notifier::HTTPClient)
.and_return(
double(:slack_service).as_null_object
)
@ -151,7 +163,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
it 'uses the channel as an option when it is configured' do
allow(chat_service).to receive(:channel).and_return(channel)
expect(Slack::Notifier).to receive(:new)
.with(webhook_url, channel: channel)
.with(webhook_url, channel: channel, http_client: SlackService::Notifier::HTTPClient)
.and_return(
double(:slack_service).as_null_object
)
@ -163,7 +175,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
chat_service.update(push_channel: "random")
expect(Slack::Notifier).to receive(:new)
.with(webhook_url, channel: "random")
.with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient)
.and_return(
double(:slack_service).as_null_object
)
@ -175,7 +187,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
chat_service.update(merge_request_channel: "random")
expect(Slack::Notifier).to receive(:new)
.with(webhook_url, channel: "random")
.with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient)
.and_return(
double(:slack_service).as_null_object
)
@ -187,7 +199,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
chat_service.update(issue_channel: "random")
expect(Slack::Notifier).to receive(:new)
.with(webhook_url, channel: "random")
.with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient)
.and_return(
double(:slack_service).as_null_object
)
@ -219,7 +231,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
chat_service.update(wiki_page_channel: "random")
expect(Slack::Notifier).to receive(:new)
.with(webhook_url, channel: "random")
.with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient)
.and_return(
double(:slack_service).as_null_object
)
@ -238,7 +250,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
note_data = Gitlab::DataBuilder::Note.build(issue_note, user)
expect(Slack::Notifier).to receive(:new)
.with(webhook_url, channel: "random")
.with(webhook_url, channel: "random", http_client: SlackService::Notifier::HTTPClient)
.and_return(
double(:slack_service).as_null_object
)
@ -286,7 +298,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
webhook: webhook_url
)
WebMock.stub_request(:post, webhook_url)
stub_full_request(webhook_url, method: :post)
end
context 'on default branch' do
@ -461,7 +473,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
webhook: webhook_url
)
WebMock.stub_request(:post, webhook_url)
stub_full_request(webhook_url, method: :post)
end
context 'when commit comment event executed' do
@ -535,7 +547,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do |service_name|
webhook: webhook_url
)
WebMock.stub_request(:post, webhook_url)
stub_full_request(webhook_url, method: :post)
end
context 'with succeeded pipeline' do

155
spec/support/shared_examples/uses_gitlab_url_blocker_examples.rb Normal file
View file

@ -0,0 +1,155 @@
# frozen_string_literal: true
shared_examples 'a request using Gitlab::UrlBlocker' do
# Written to test internal patches against 3rd party libraries
#
# Expects the following to be available in the example contexts:
#
# make_request(uri): Wraps the request we want to test goes through Gitlab::HTTP
# http_method: :get, :post etc
# url_blocked_error_class: Probably just Gitlab::HTTP::BlockedUrlError
include StubRequests
context 'when local requests are not allowed' do
it 'allows an external request with http' do
request_stub = stub_full_request('http://example.com', method: http_method, ip_address: '93.184.216.34')
make_request('http://example.com/')
expect(request_stub).to have_been_requested
end
it 'allows an external request with https' do
request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '93.184.216.34')
make_request('https://example.com/')
expect(request_stub).to have_been_requested
end
it 'raises error when it is a request that resolves to a local address' do
stub_full_request('https://example.com', method: http_method, ip_address: '172.16.0.0')
expect { make_request('https://example.com') }
.to raise_error(url_blocked_error_class,
"URL 'https://example.com' is blocked: Requests to the local network are not allowed")
end
it 'raises error when it is a request that resolves to a localhost address' do
stub_full_request('https://example.com', method: http_method, ip_address: '127.0.0.1')
expect { make_request('https://example.com') }
.to raise_error(url_blocked_error_class,
"URL 'https://example.com' is blocked: Requests to localhost are not allowed")
end
it 'raises error when it is a request to local address' do
expect { make_request('http://172.16.0.0') }
.to raise_error(url_blocked_error_class,
"URL 'http://172.16.0.0' is blocked: Requests to the local network are not allowed")
end
it 'raises error when it is a request to localhost address' do
expect { make_request('http://127.0.0.1') }
.to raise_error(url_blocked_error_class,
"URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed")
end
end
context 'when port different from URL scheme is used' do
it 'allows the request' do
request_stub = stub_full_request('https://example.com:8080', method: http_method, ip_address: '93.184.216.34')
make_request('https://example.com:8080/')
expect(request_stub).to have_been_requested
end
it 'raises error when it is a request to local address' do
expect { make_request('https://172.16.0.0:8080') }
.to raise_error(url_blocked_error_class,
"URL 'https://172.16.0.0:8080' is blocked: Requests to the local network are not allowed")
end
it 'raises error when it is a request to localhost address' do
expect { make_request('https://127.0.0.1:8080') }
.to raise_error(url_blocked_error_class,
"URL 'https://127.0.0.1:8080' is blocked: Requests to localhost are not allowed")
end
end
context 'when DNS rebinding protection is disabled' do
before do
stub_application_setting(dns_rebinding_protection_enabled: false)
end
it 'allows the request' do
request_stub = stub_request(http_method, 'https://example.com')
make_request('https://example.com/')
expect(request_stub).to have_been_requested
end
end
context 'when http(s) proxy environment variable is set' do
before do
stub_env('https_proxy' => 'https://my.proxy')
end
it 'allows the request' do
request_stub = stub_request(http_method, 'https://example.com')
make_request('https://example.com/')
expect(request_stub).to have_been_requested
end
end
context 'when local requests are allowed' do
before do
stub_application_setting(allow_local_requests_from_web_hooks_and_services: true)
end
it 'allows an external request' do
request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '93.184.216.34')
make_request('https://example.com/')
expect(request_stub).to have_been_requested
end
it 'allows an external request that resolves to a local address' do
request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '172.16.0.0')
make_request('https://example.com/')
expect(request_stub).to have_been_requested
end
it 'allows an external request that resolves to a localhost address' do
request_stub = stub_full_request('https://example.com', method: http_method, ip_address: '127.0.0.1')
make_request('https://example.com/')
expect(request_stub).to have_been_requested
end
it 'allows a local address request' do
request_stub = stub_request(http_method, 'http://172.16.0.0')
make_request('http://172.16.0.0')
expect(request_stub).to have_been_requested
end
it 'allows a localhost address request' do
request_stub = stub_request(http_method, 'http://127.0.0.1')
make_request('http://127.0.0.1')
expect(request_stub).to have_been_requested
end
end
end