Commit dd7a0992 authored by Stan Hu's avatar Stan Hu

Fix Service Side Request Forgery in JenkinsDeprecatedService

We noticed requests on some ReactiveCache jobs going to localhost, and
it appears that JenkinsDeprecatedService may have had legacy code to
allow localhost requests instead of using the setting defaults. This
likely happened because all the changes were made in CE and not EE.

To fix this, we now remove the explicit argument and allow `UrlBlocker`
to use the default settings.
parent 2abc63f1
...@@ -83,6 +83,8 @@ class JenkinsDeprecatedService < CiService ...@@ -83,6 +83,8 @@ class JenkinsDeprecatedService < CiService
def calculate_reactive_cache(sha, ref) def calculate_reactive_cache(sha, ref)
{ commit_status: read_commit_status(sha, ref) } { commit_status: read_commit_status(sha, ref) }
rescue Gitlab::HTTP::BlockedUrlError => ex
Gitlab::ErrorTracking.track_exception(ex)
end end
private private
...@@ -91,14 +93,14 @@ class JenkinsDeprecatedService < CiService ...@@ -91,14 +93,14 @@ class JenkinsDeprecatedService < CiService
parsed_url = URI.parse(build_page(sha, ref)) parsed_url = URI.parse(build_page(sha, ref))
if parsed_url.userinfo.blank? if parsed_url.userinfo.blank?
response = Gitlab::HTTP.get(build_page(sha, ref), verify: false, allow_local_requests: true) response = Gitlab::HTTP.get(build_page(sha, ref), verify: false)
else else
get_url = build_page(sha, ref).gsub("#{parsed_url.userinfo}@", "") get_url = build_page(sha, ref).gsub("#{parsed_url.userinfo}@", "")
auth = { auth = {
username: CGI.unescape(parsed_url.user), username: CGI.unescape(parsed_url.user),
password: CGI.unescape(parsed_url.password) password: CGI.unescape(parsed_url.password)
} }
response = Gitlab::HTTP.get(get_url, verify: false, basic_auth: auth, allow_local_requests: true) response = Gitlab::HTTP.get(get_url, verify: false, basic_auth: auth)
end end
if response.code == 200 if response.code == 200
......
---
title: Fix Service Side Request Forgery in JenkinsDeprecatedService
merge_request:
author:
type: security
...@@ -60,6 +60,35 @@ ICON_STATUS_HTML ...@@ -60,6 +60,35 @@ ICON_STATUS_HTML
expect(@service.calculate_reactive_cache('2ab7834c', 'master')).to eq(commit_status: :error) expect(@service.calculate_reactive_cache('2ab7834c', 'master')).to eq(commit_status: :error)
end end
end end
describe 'respects outbound network setting' do
let(:url) { 'http://127.0.0.1:8080/job/test' }
before do
stub_application_setting(allow_local_requests_from_web_hooks_and_services: setting)
allow(@service).to receive(:project_url).and_return(url)
end
context 'when local requests are allowed' do
let(:setting) { true }
it "makes an outbound request" do
stub_request(:get, 'http://127.0.0.1:8080/job/test/scm/bySHA1/2ab7834c').to_return(status: 200, body: status_body_for_icon('blue.png'), headers: {})
expect(@service.calculate_reactive_cache('2ab7834c', 'master')).to eq(commit_status: 'success')
end
end
context 'when local requests are not allowed' do
let(:setting) { false }
it 'raises an exception' do
expect(Gitlab::ErrorTracking).to receive(:track_exception).with(an_instance_of(Gitlab::HTTP::BlockedUrlError)).and_call_original
expect { @service.calculate_reactive_cache('2ab7834c', 'master') }.not_to raise_error
end
end
end
end end
describe '#commit_status' do describe '#commit_status' do
......
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