diff options
author | Douwe Maan <douwe@gitlab.com> | 2018-03-14 01:38:27 +0300 |
---|---|---|
committer | Mark Fletcher <mark@gitlab.com> | 2018-03-16 15:57:21 +0300 |
commit | 254529300eeb0a11e50e0b2ebc1abecf9908f13e (patch) | |
tree | 1b2b7a71b41f57af1413ec2dba472fd5ecf5d0b3 /spec | |
parent | c7e4919eaaf385d283fdb8bbb737d898326f7304 (diff) |
Merge branch 'fj-15329-services-callbacks-ssrf-10-5' into 'security-10-5'
[10.5] Server Side Request Forgery in Services and Web Hooks
See merge request gitlab/gitlabhq!2348
Diffstat (limited to 'spec')
-rw-r--r-- | spec/lib/gitlab/http_spec.rb | 49 | ||||
-rw-r--r-- | spec/lib/gitlab/url_blocker_spec.rb | 45 | ||||
-rw-r--r-- | spec/lib/mattermost/command_spec.rb | 5 | ||||
-rw-r--r-- | spec/lib/mattermost/session_spec.rb | 2 | ||||
-rw-r--r-- | spec/lib/mattermost/team_spec.rb | 5 | ||||
-rw-r--r-- | spec/models/project_services/mattermost_slash_commands_service_spec.rb | 5 | ||||
-rw-r--r-- | spec/rubocop/cop/gitlab/httparty_spec.rb | 74 | ||||
-rw-r--r-- | spec/services/web_hook_service_spec.rb | 14 |
8 files changed, 191 insertions, 8 deletions
diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb new file mode 100644 index 00000000000..b0bc081a3c8 --- /dev/null +++ b/spec/lib/gitlab/http_spec.rb @@ -0,0 +1,49 @@ +require 'spec_helper' + +describe Gitlab::HTTP do + describe 'allow_local_requests_from_hooks_and_services is' do + before do + WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success') + end + + context 'disabled' do + before do + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false) + end + + it 'deny requests to localhost' do + expect { described_class.get('http://localhost:3003') }.to raise_error(URI::InvalidURIError) + end + + it 'deny requests to private network' do + expect { described_class.get('http://192.168.1.2:3003') }.to raise_error(URI::InvalidURIError) + end + + context 'if allow_local_requests set to true' do + it 'override the global value and allow requests to localhost or private network' do + expect { described_class.get('http://localhost:3003', allow_local_requests: true) }.not_to raise_error + end + end + end + + context 'enabled' do + before do + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true) + end + + it 'allow requests to localhost' do + expect { described_class.get('http://localhost:3003') }.not_to raise_error + end + + it 'allow requests to private network' do + expect { described_class.get('http://192.168.1.2:3003') }.not_to raise_error + end + + context 'if allow_local_requests set to false' do + it 'override the global value and ban requests to localhost or private network' do + expect { described_class.get('http://localhost:3003', allow_local_requests: false) }.to raise_error(URI::InvalidURIError) + end + end + end + end +end diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index d9b3c2350b1..2d35b026485 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -2,6 +2,8 @@ require 'spec_helper' describe Gitlab::UrlBlocker do describe '#blocked_url?' do + let(:valid_ports) { Project::VALID_IMPORT_PORTS } + it 'allows imports from configured web host and port' do import_url = "http://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git" expect(described_class.blocked_url?(import_url)).to be false @@ -17,7 +19,7 @@ describe Gitlab::UrlBlocker do end it 'returns true for bad port' do - expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git')).to be true + expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git', valid_ports: valid_ports)).to be true end it 'returns true for alternative version of 127.0.0.1 (0177.1)' do @@ -71,6 +73,47 @@ describe Gitlab::UrlBlocker do it 'returns false for legitimate URL' do expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false end + + context 'when allow_private_networks is' do + let(:private_networks) { ['192.168.1.2', '10.0.0.2', '172.16.0.2'] } + let(:fake_domain) { 'www.fakedomain.fake' } + + context 'true (default)' do + it 'does not block urls from private networks' do + private_networks.each do |ip| + stub_domain_resolv(fake_domain, ip) + + expect(described_class).not_to be_blocked_url("http://#{fake_domain}") + + unstub_domain_resolv + + expect(described_class).not_to be_blocked_url("http://#{ip}") + end + end + end + + context 'false' do + it 'blocks urls from private networks' do + private_networks.each do |ip| + stub_domain_resolv(fake_domain, ip) + + expect(described_class).to be_blocked_url("http://#{fake_domain}", allow_private_networks: false) + + unstub_domain_resolv + + expect(described_class).to be_blocked_url("http://#{ip}", allow_private_networks: false) + end + end + end + + def stub_domain_resolv(domain, ip) + allow(Addrinfo).to receive(:getaddrinfo).with(domain, any_args).and_return([double(ip_address: ip, ipv4_private?: true)]) + end + + def unstub_domain_resolv + allow(Addrinfo).to receive(:getaddrinfo).and_call_original + end + end end # Resolv does not support resolving UTF-8 domain names diff --git a/spec/lib/mattermost/command_spec.rb b/spec/lib/mattermost/command_spec.rb index 369e7b181b9..8ba15ae0f38 100644 --- a/spec/lib/mattermost/command_spec.rb +++ b/spec/lib/mattermost/command_spec.rb @@ -4,10 +4,11 @@ describe Mattermost::Command do let(:params) { { 'token' => 'token', team_id: 'abc' } } before do - Mattermost::Session.base_uri('http://mattermost.example.com') + session = Mattermost::Session.new(nil) + session.base_uri = 'http://mattermost.example.com' allow_any_instance_of(Mattermost::Client).to receive(:with_session) - .and_yield(Mattermost::Session.new(nil)) + .and_yield(session) end describe '#create' do diff --git a/spec/lib/mattermost/session_spec.rb b/spec/lib/mattermost/session_spec.rb index 3db19d06305..c855643c4d8 100644 --- a/spec/lib/mattermost/session_spec.rb +++ b/spec/lib/mattermost/session_spec.rb @@ -15,7 +15,7 @@ describe Mattermost::Session, type: :request do it { is_expected.to respond_to(:strategy) } before do - described_class.base_uri(mattermost_url) + subject.base_uri = mattermost_url end describe '#with session' do diff --git a/spec/lib/mattermost/team_spec.rb b/spec/lib/mattermost/team_spec.rb index e638ad7a2c9..9795a0c3e39 100644 --- a/spec/lib/mattermost/team_spec.rb +++ b/spec/lib/mattermost/team_spec.rb @@ -2,10 +2,11 @@ require 'spec_helper' describe Mattermost::Team do before do - Mattermost::Session.base_uri('http://mattermost.example.com') + session = Mattermost::Session.new(nil) + session.base_uri = 'http://mattermost.example.com' allow_any_instance_of(Mattermost::Client).to receive(:with_session) - .and_yield(Mattermost::Session.new(nil)) + .and_yield(session) end describe '#all' do diff --git a/spec/models/project_services/mattermost_slash_commands_service_spec.rb b/spec/models/project_services/mattermost_slash_commands_service_spec.rb index 522cf15f3ba..ee4b4061cb3 100644 --- a/spec/models/project_services/mattermost_slash_commands_service_spec.rb +++ b/spec/models/project_services/mattermost_slash_commands_service_spec.rb @@ -9,10 +9,11 @@ describe MattermostSlashCommandsService do let(:user) { create(:user) } before do - Mattermost::Session.base_uri("http://mattermost.example.com") + session = Mattermost::Session.new(nil) + session.base_uri = 'http://mattermost.example.com' allow_any_instance_of(Mattermost::Client).to receive(:with_session) - .and_yield(Mattermost::Session.new(nil)) + .and_yield(session) end describe '#configure' do diff --git a/spec/rubocop/cop/gitlab/httparty_spec.rb b/spec/rubocop/cop/gitlab/httparty_spec.rb new file mode 100644 index 00000000000..510839a21d7 --- /dev/null +++ b/spec/rubocop/cop/gitlab/httparty_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/gitlab/httparty' + +describe RuboCop::Cop::Gitlab::HTTParty do # rubocop:disable RSpec/FilePath + include CopHelper + + subject(:cop) { described_class.new } + + shared_examples('registering include offense') do |options| + let(:offending_lines) { options[:offending_lines] } + + it 'registers an offense when the class includes HTTParty' do + inspect_source(source) + + aggregate_failures do + expect(cop.offenses.size).to eq(offending_lines.size) + expect(cop.offenses.map(&:line)).to eq(offending_lines) + end + end + end + + shared_examples('registering call offense') do |options| + let(:offending_lines) { options[:offending_lines] } + + it 'registers an offense when the class calls HTTParty' do + inspect_source(source) + + aggregate_failures do + expect(cop.offenses.size).to eq(offending_lines.size) + expect(cop.offenses.map(&:line)).to eq(offending_lines) + end + end + end + + context 'when source is a regular module' do + it_behaves_like 'registering include offense', offending_lines: [2] do + let(:source) do + <<~RUBY + module M + include HTTParty + end + RUBY + end + end + end + + context 'when source is a regular class' do + it_behaves_like 'registering include offense', offending_lines: [2] do + let(:source) do + <<~RUBY + class Foo + include HTTParty + end + RUBY + end + end + end + + context 'when HTTParty is called' do + it_behaves_like 'registering call offense', offending_lines: [3] do + let(:source) do + <<~RUBY + class Foo + def bar + HTTParty.get('http://example.com') + end + end + RUBY + end + end + end +end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 21910e69d2e..2ef2e61babc 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -14,6 +14,20 @@ describe WebHookService do end let(:service_instance) { described_class.new(project_hook, data, :push_hooks) } + describe '#initialize' do + it 'allow_local_requests is true if hook is a SystemHook' do + instance = described_class.new(build(:system_hook), data, :system_hook) + expect(instance.request_options[:allow_local_requests]).to be_truthy + end + + it 'allow_local_requests is false if hook is not a SystemHook' do + %i(project_hook service_hook web_hook_log).each do |hook| + instance = described_class.new(build(hook), data, hook) + expect(instance.request_options[:allow_local_requests]).to be_falsey + end + end + end + describe '#execute' do before do project.hooks << [project_hook] |