HTTP connection adapter with proxy settings

This updates the HTTPConnectionAdapter to prevent any SSRF protection
bypass when using the proxy settings.

lib/gitlab/http_connection_adapter.rb

@@ -11,13 +11,18 @@
 # This option will take precedence over the global setting.
 module Gitlab
   class HTTPConnectionAdapter < HTTParty::ConnectionAdapter
+    extend ::Gitlab::Utils::Override
+
+    override :connection
     def connection
-      begin
-        @uri, hostname = Gitlab::UrlBlocker.validate!(uri, allow_local_network: allow_local_requests?,
-                                                           allow_localhost: allow_local_requests?,
-                                                           dns_rebind_protection: dns_rebind_protection?)
-      rescue Gitlab::UrlBlocker::BlockedUrlError => e
-        raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}"
+      @uri, hostname = validate_url!(uri)
+
+      if options.key?(:http_proxyaddr)
+        proxy_uri_with_port = uri_with_port(options[:http_proxyaddr], options[:http_proxyport])
+        proxy_uri_validated = validate_url!(proxy_uri_with_port).first
+
+        @options[:http_proxyaddr] = proxy_uri_validated.omit(:port).to_s
+        @options[:http_proxyport] = proxy_uri_validated.port
       end
 
       super.tap do |http|
@@ -27,6 +32,14 @@ module Gitlab
 
     private
 
+    def validate_url!(url)
+      Gitlab::UrlBlocker.validate!(url, allow_local_network: allow_local_requests?,
+                                        allow_localhost: allow_local_requests?,
+                                        dns_rebind_protection: dns_rebind_protection?)
+    rescue Gitlab::UrlBlocker::BlockedUrlError => e
+      raise Gitlab::HTTP::BlockedUrlError, "URL '#{url}' is blocked: #{e.message}"
+    end
+
     def allow_local_requests?
       options.fetch(:allow_local_requests, allow_settings_local_requests?)
     end
@@ -40,5 +53,11 @@ module Gitlab
     def allow_settings_local_requests?
       Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?
     end
+
+    def uri_with_port(address, port)
+      uri = Addressable::URI.parse(address)
+      uri.port = port if port.present?
+      uri
+    end
   end
 end

spec/lib/gitlab/http_connection_adapter_spec.rb

@@ -5,17 +5,32 @@ require 'spec_helper'
 RSpec.describe Gitlab::HTTPConnectionAdapter do
   include StubRequests
 
+  let(:uri) { URI('https://example.org') }
+  let(:options) { {} }
+
+  subject(:connection) { described_class.new(uri, options).connection }
+
   describe '#connection' do
     before do
       stub_all_dns('https://example.org', ip_address: '93.184.216.34')
     end
 
-    context 'when local requests are not allowed' do
+    context 'when local requests are allowed' do
+      let(:options) { { allow_local_requests: true } }
+
       it 'sets up the connection' do
-        uri = URI('https://example.org')
+        expect(connection).to be_a(Net::HTTP)
+        expect(connection.address).to eq('93.184.216.34')
+        expect(connection.hostname_override).to eq('example.org')
+        expect(connection.addr_port).to eq('example.org')
+        expect(connection.port).to eq(443)
+      end
+    end
 
-        connection = described_class.new(uri).connection
+    context 'when local requests are not allowed' do
+      let(:options) { { allow_local_requests: false } }
 
+      it 'sets up the connection' do
         expect(connection).to be_a(Net::HTTP)
         expect(connection.address).to eq('93.184.216.34')
         expect(connection.hostname_override).to eq('example.org')
@@ -23,28 +38,57 @@ RSpec.describe Gitlab::HTTPConnectionAdapter do
         expect(connection.port).to eq(443)
       end
 
-      it 'raises error when it is a request to local address' do
-        uri = URI('http://172.16.0.0/12')
+      context 'when it is a request to local network' do
+        let(:uri) { URI('http://172.16.0.0/12') }
+
+        it 'raises error' do
+          expect { subject }.to raise_error(
+            Gitlab::HTTP::BlockedUrlError,
+            "URL 'http://172.16.0.0/12' is blocked: Requests to the local network are not allowed"
+          )
+        end
+
+        context 'when local request allowed' do
+          let(:options) { { allow_local_requests: true } }
 
-        expect { described_class.new(uri).connection }
-          .to raise_error(Gitlab::HTTP::BlockedUrlError,
-                          "URL 'http://172.16.0.0/12' is blocked: Requests to the local network are not allowed")
+          it 'sets up the connection' do
+            expect(connection).to be_a(Net::HTTP)
+            expect(connection.address).to eq('172.16.0.0')
+            expect(connection.hostname_override).to be(nil)
+            expect(connection.addr_port).to eq('172.16.0.0')
+            expect(connection.port).to eq(80)
+          end
+        end
       end
 
-      it 'raises error when it is a request to localhost address' do
-        uri = URI('http://127.0.0.1')
+      context 'when it is a request to local address' do
+        let(:uri) { URI('http://127.0.0.1') }
+
+        it 'raises error' do
+          expect { subject }.to raise_error(
+            Gitlab::HTTP::BlockedUrlError,
+            "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed"
+          )
+        end
 
-        expect { described_class.new(uri).connection }
-          .to raise_error(Gitlab::HTTP::BlockedUrlError,
-                          "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed")
+        context 'when local request allowed' do
+          let(:options) { { allow_local_requests: true } }
+
+          it 'sets up the connection' do
+            expect(connection).to be_a(Net::HTTP)
+            expect(connection.address).to eq('127.0.0.1')
+            expect(connection.hostname_override).to be(nil)
+            expect(connection.addr_port).to eq('127.0.0.1')
+            expect(connection.port).to eq(80)
+          end
+        end
       end
 
       context 'when port different from URL scheme is used' do
-        it 'sets up the addr_port accordingly' do
-          uri = URI('https://example.org:8080')
-
-          connection = described_class.new(uri).connection
+        let(:uri) { URI('https://example.org:8080') }
 
+        it 'sets up the addr_port accordingly' do
+          expect(connection).to be_a(Net::HTTP)
           expect(connection.address).to eq('93.184.216.34')
           expect(connection.hostname_override).to eq('example.org')
           expect(connection.addr_port).to eq('example.org:8080')
@@ -54,13 +98,11 @@ RSpec.describe Gitlab::HTTPConnectionAdapter do
     end
 
     context 'when DNS rebinding protection is disabled' do
-      it 'sets up the connection' do
+      before do
         stub_application_setting(dns_rebinding_protection_enabled: false)
+      end
 
-        uri = URI('https://example.org')
-
-        connection = described_class.new(uri).connection
-
+      it 'sets up the connection' do
         expect(connection).to be_a(Net::HTTP)
         expect(connection.address).to eq('example.org')
         expect(connection.hostname_override).to eq(nil)
@@ -70,13 +112,11 @@ RSpec.describe Gitlab::HTTPConnectionAdapter do
     end
 
     context 'when http(s) environment variable is set' do
-      it 'sets up the connection' do
+      before do
         stub_env('https_proxy' => 'https://my.proxy')
+      end
 
-        uri = URI('https://example.org')
-
-        connection = described_class.new(uri).connection
-
+      it 'sets up the connection' do
         expect(connection).to be_a(Net::HTTP)
         expect(connection.address).to eq('example.org')
         expect(connection.hostname_override).to eq(nil)
@@ -85,41 +125,128 @@ RSpec.describe Gitlab::HTTPConnectionAdapter do
       end
     end
 
-    context 'when local requests are allowed' do
-      it 'sets up the connection' do
-        uri = URI('https://example.org')
+    context 'when proxy settings are configured' do
+      let(:options) do
+        {
+          http_proxyaddr: 'https://proxy.org',
+          http_proxyport: 1557,
+          http_proxyuser: 'user',
+          http_proxypass: 'pass'
+        }
+      end
 
-        connection = described_class.new(uri, allow_local_requests: true).connection
+      before do
+        stub_all_dns('https://proxy.org', ip_address: '166.84.12.54')
+      end
 
-        expect(connection).to be_a(Net::HTTP)
-        expect(connection.address).to eq('93.184.216.34')
-        expect(connection.hostname_override).to eq('example.org')
-        expect(connection.addr_port).to eq('example.org')
-        expect(connection.port).to eq(443)
+      it 'sets up the proxy settings' do
+        expect(connection.proxy_address).to eq('https://166.84.12.54')
+        expect(connection.proxy_port).to eq(1557)
+        expect(connection.proxy_user).to eq('user')
+        expect(connection.proxy_pass).to eq('pass')
       end
 
-      it 'sets up the connection when it is a local network' do
-        uri = URI('http://172.16.0.0/12')
+      context 'when the address has path' do
+        before do
+          options[:http_proxyaddr] = 'https://proxy.org/path'
+        end
 
-        connection = described_class.new(uri, allow_local_requests: true).connection
+        it 'sets up the proxy settings' do
+          expect(connection.proxy_address).to eq('https://166.84.12.54/path')
+          expect(connection.proxy_port).to eq(1557)
+        end
+      end
 
-        expect(connection).to be_a(Net::HTTP)
-        expect(connection.address).to eq('172.16.0.0')
-        expect(connection.hostname_override).to be(nil)
-        expect(connection.addr_port).to eq('172.16.0.0')
-        expect(connection.port).to eq(80)
+      context 'when the port is in the address and port' do
+        before do
+          options[:http_proxyaddr] = 'https://proxy.org:1422'
+        end
+
+        it 'sets up the proxy settings' do
+          expect(connection.proxy_address).to eq('https://166.84.12.54')
+          expect(connection.proxy_port).to eq(1557)
+        end
+
+        context 'when the port is only in the address' do
+          before do
+            options[:http_proxyport] = nil
+          end
+
+          it 'sets up the proxy settings' do
+            expect(connection.proxy_address).to eq('https://166.84.12.54')
+            expect(connection.proxy_port).to eq(1422)
+          end
+        end
       end
 
-      it 'sets up the connection when it is localhost' do
-        uri = URI('http://127.0.0.1')
+      context 'when it is a request to local network' do
+        before do
+          options[:http_proxyaddr] = 'http://172.16.0.0/12'
+        end
+
+        it 'raises error' do
+          expect { subject }.to raise_error(
+            Gitlab::HTTP::BlockedUrlError,
+            "URL 'http://172.16.0.0:1557/12' is blocked: Requests to the local network are not allowed"
+          )
+        end
 
-        connection = described_class.new(uri, allow_local_requests: true).connection
+        context 'when local request allowed' do
+          before do
+            options[:allow_local_requests] = true
+          end
 
-        expect(connection).to be_a(Net::HTTP)
-        expect(connection.address).to eq('127.0.0.1')
-        expect(connection.hostname_override).to be(nil)
-        expect(connection.addr_port).to eq('127.0.0.1')
-        expect(connection.port).to eq(80)
+          it 'sets up the connection' do
+            expect(connection.proxy_address).to eq('http://172.16.0.0/12')
+            expect(connection.proxy_port).to eq(1557)
+          end
+        end
+      end
+
+      context 'when it is a request to local address' do
+        before do
+          options[:http_proxyaddr] = 'http://127.0.0.1'
+        end
+
+        it 'raises error' do
+          expect { subject }.to raise_error(
+            Gitlab::HTTP::BlockedUrlError,
+            "URL 'http://127.0.0.1:1557' is blocked: Requests to localhost are not allowed"
+          )
+        end
+
+        context 'when local request allowed' do
+          before do
+            options[:allow_local_requests] = true
+          end
+
+          it 'sets up the connection' do
+            expect(connection.proxy_address).to eq('http://127.0.0.1')
+            expect(connection.proxy_port).to eq(1557)
+          end
+        end
+      end
+
+      context 'when http(s) environment variable is set' do
+        before do
+          stub_env('https_proxy' => 'https://my.proxy')
+        end
+
+        it 'sets up the connection' do
+          expect(connection.proxy_address).to eq('https://proxy.org')
+          expect(connection.proxy_port).to eq(1557)
+        end
+      end
+
+      context 'when DNS rebinding protection is disabled' do
+        before do
+          stub_application_setting(dns_rebinding_protection_enabled: false)
+        end
+
+        it 'sets up the connection' do
+          expect(connection.proxy_address).to eq('https://proxy.org')
+          expect(connection.proxy_port).to eq(1557)
+        end
       end
     end
   end