Protect Gitlab::HTTP against DNS rebinding attack

Gitlab::HTTP now resolves the hostname only once, verifies the IP is not
blocked, and then uses the same IP to perform the actual request, while
passing the original hostname in the `Host` header and SSL SNI field.

changelogs/unreleased/dm-http-hostname-override.yml

+---
+title: Protect Gitlab::HTTP against DNS rebinding attack
+merge_request:
+author:
+type: security

config/initializers/hipchat_client_patch.rb

@@ -2,14 +2,14 @@
 # This monkey patches the HTTParty used in https://github.com/hipchat/hipchat-rb.
 module HipChat
   class Client
-    connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter
+    connection_adapter ::Gitlab::HTTPConnectionAdapter
   end
 
   class Room
-    connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter
+    connection_adapter ::Gitlab::HTTPConnectionAdapter
   end
 
   class User
-    connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter
+    connection_adapter ::Gitlab::HTTPConnectionAdapter
   end
 end

config/initializers/http_hostname_override.rb

+# This override allows passing `@hostname_override` to the SNI protocol,
+# which is used to lookup the correct SSL certificate in the
+# request handshake process.
+#
+# Given we've forced the HTTP request to be sent to the resolved
+# IP address in a few scenarios (e.g.: `Gitlab::HTTP` through
+# `Gitlab::UrlBlocker.validate!`), we need to provide the _original_
+# hostname via SNI in order to have a clean connection setup.
+#
+# This is ultimately needed in order to avoid DNS rebinding attacks
+# through HTTP requests.
+#
+class OpenSSL::SSL::SSLContext
+  attr_accessor :hostname_override
+end
+
+class OpenSSL::SSL::SSLSocket
+  module HostnameOverride
+    # rubocop: disable Gitlab/ModuleWithInstanceVariables
+    def hostname=(hostname)
+      super(@context.hostname_override || hostname)
+    end
+
+    def post_connection_check(hostname)
+      super(@context.hostname_override || hostname)
+    end
+    # rubocop: enable Gitlab/ModuleWithInstanceVariables
+  end
+
+  prepend HostnameOverride
+end
+
+class Net::HTTP
+  attr_accessor :hostname_override
+  SSL_IVNAMES << :@hostname_override
+  SSL_ATTRIBUTES << :hostname_override
+
+  module HostnameOverride
+    def addr_port
+      return super unless hostname_override
+
+      addr = hostname_override
+      default_port = use_ssl? ? Net::HTTP.https_default_port : Net::HTTP.http_default_port
+      default_port == port ? addr : "#{addr}:#{port}"
+    end
+  end
+
+  prepend HostnameOverride
+end

lib/gitlab/http.rb

@@ -11,7 +11,7 @@ module Gitlab
 
     include HTTParty # rubocop:disable Gitlab/HTTParty
 
-    connection_adapter ProxyHTTPConnectionAdapter
+    connection_adapter HTTPConnectionAdapter
 
     def self.perform_request(http_method, path, options, &block)
       super

lib/gitlab/proxy_http_connection_adapter.rb → lib/gitlab/http_connection_adapter.rb

@@ -10,17 +10,18 @@
 #
 # This option will take precedence over the global setting.
 module Gitlab
-  class ProxyHTTPConnectionAdapter < HTTParty::ConnectionAdapter
+  class HTTPConnectionAdapter < HTTParty::ConnectionAdapter
     def connection
-      unless allow_local_requests?
       begin
-          Gitlab::UrlBlocker.validate!(uri, allow_local_network: false)
+        @uri, hostname = Gitlab::UrlBlocker.validate!(uri, allow_local_network: allow_local_requests?,
+                                                           allow_localhost: allow_local_requests?)
       rescue Gitlab::UrlBlocker::BlockedUrlError => e
         raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}"
       end
-      end
 
-      super
+      super.tap do |http|
+        http.hostname_override = hostname if hostname
+      end
     end
 
     private

lib/gitlab/url_blocker.rb

@@ -8,38 +8,52 @@ module Gitlab
     BlockedUrlError = Class.new(StandardError)
 
     class << self
-      def validate!(url, ports: [], schemes: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false)
-        return true if url.nil?
+      # Validates the given url according to the constraints specified by arguments.
+      #
+      # ports - Raises error if the given URL port does is not between given ports.
+      # allow_localhost - Raises error if URL resolves to a localhost IP address and argument is true.
+      # allow_local_network - Raises error if URL resolves to a link-local address and argument is true.
+      # ascii_only - Raises error if URL has unicode characters and argument is true.
+      # enforce_user - Raises error if URL user doesn't start with alphanumeric characters and argument is true.
+      # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true.
+      #
+      # Returns an array with [<uri>, <original-hostname>].
+      def validate!(url, ports: [], schemes: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false) # rubocop:disable Metrics/CyclomaticComplexity
+        return [nil, nil] if url.nil?
 
         # Param url can be a string, URI or Addressable::URI
         uri = parse_url(url)
 
         validate_html_tags!(uri) if enforce_sanitization
 
-        # Allow imports from the GitLab instance itself but only from the configured ports
-        return true if internal?(uri)
-
+        hostname = uri.hostname
         port = get_port(uri)
+
+        unless internal?(uri)
           validate_scheme!(uri.scheme, schemes)
           validate_port!(port, ports) if ports.any?
           validate_user!(uri.user) if enforce_user
-        validate_hostname!(uri.hostname)
+          validate_hostname!(hostname)
           validate_unicode_restriction!(uri) if ascii_only
+        end
 
         begin
-          addrs_info = Addrinfo.getaddrinfo(uri.hostname, port, nil, :STREAM).map do |addr|
+          addrs_info = Addrinfo.getaddrinfo(hostname, port, nil, :STREAM).map do |addr|
             addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr
           end
         rescue SocketError
-          return true
+          return [uri, nil]
         end
 
+        # Allow url from the GitLab instance itself but only for the configured hostname and ports
+        return enforce_uri_hostname(addrs_info, uri, hostname) if internal?(uri)
+
         validate_localhost!(addrs_info) unless allow_localhost
         validate_loopback!(addrs_info) unless allow_localhost
         validate_local_network!(addrs_info) unless allow_local_network
         validate_link_local!(addrs_info) unless allow_local_network
 
-        true
+        enforce_uri_hostname(addrs_info, uri, hostname)
       end
 
       def blocked_url?(*args)
@@ -52,6 +66,27 @@ module Gitlab
 
       private
 
+      # Returns the given URI with IP address as hostname and the original hostname respectively
+      # in an Array.
+      #
+      # It checks whether the resolved IP address matches with the hostname. If not, it changes
+      # the hostname to the resolved IP address.
+      #
+      # The original hostname is used to validate the SSL, given in that scenario
+      # we'll be making the request to the IP address, instead of using the hostname.
+      def enforce_uri_hostname(addrs_info, uri, hostname)
+        address = addrs_info.first
+        ip_address = address&.ip_address
+
+        if ip_address && ip_address != hostname
+          uri = uri.dup
+          uri.hostname = ip_address
+          return [uri, hostname]
+        end
+
+        [uri, nil]
+      end
+
       def get_port(uri)
         uri.port || uri.default_port
       end

spec/controllers/projects/ci/lints_controller_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Projects::Ci::LintsController do
+  include StubRequests
+
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
 
@@ -70,7 +72,7 @@ describe Projects::Ci::LintsController do
 
     context 'with a valid gitlab-ci.yml' do
       before do
-        WebMock.stub_request(:get, remote_file_path).to_return(body: remote_file_content)
+        stub_full_request(remote_file_path).to_return(body: remote_file_content)
         project.add_developer(user)
 
         post :create, params: { namespace_id: project.namespace, project_id: project, content: content }

spec/lib/gitlab/ci/config/external/file/remote_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::External::File::Remote do
+  include StubRequests
+
   let(:context) { described_class::Context.new(nil, '12345', nil, Set.new) }
   let(:params) { { remote: location } }
   let(:remote_file) { described_class.new(params, context) }
@@ -46,7 +48,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
   describe "#valid?" do
     context 'when is a valid remote url' do
       before do
-        WebMock.stub_request(:get, location).to_return(body: remote_file_content)
+        stub_full_request(location).to_return(body: remote_file_content)
       end
 
       it 'returns true' do
@@ -92,7 +94,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
   describe "#content" do
     context 'with a valid remote file' do
       before do
-        WebMock.stub_request(:get, location).to_return(body: remote_file_content)
+        stub_full_request(location).to_return(body: remote_file_content)
       end
 
       it 'returns the content of the file' do
@@ -114,7 +116,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
       let(:location) { 'https://asdasdasdaj48ggerexample.com' }
 
       before do
-        WebMock.stub_request(:get, location).to_raise(SocketError.new('Some HTTP error'))
+        stub_full_request(location).to_raise(SocketError.new('Some HTTP error'))
       end
 
       it 'is nil' do
@@ -144,7 +146,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
 
     context 'when timeout error has been raised' do
       before do
-        WebMock.stub_request(:get, location).to_timeout
+        stub_full_request(location).to_timeout
       end
 
       it 'returns error message about a timeout' do
@@ -154,7 +156,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
 
     context 'when HTTP error has been raised' do
       before do
-        WebMock.stub_request(:get, location).to_raise(Gitlab::HTTP::Error)
+        stub_full_request(location).to_raise(Gitlab::HTTP::Error)
       end
 
       it 'returns error message about a HTTP error' do
@@ -164,7 +166,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
 
     context 'when response has 404 status' do
       before do
-        WebMock.stub_request(:get, location).to_return(body: remote_file_content, status: 404)
+        stub_full_request(location).to_return(body: remote_file_content, status: 404)
       end
 
       it 'returns error message about a timeout' do

spec/lib/gitlab/ci/config/external/mapper_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::External::Mapper do
+  include StubRequests
+
   set(:project) { create(:project, :repository) }
   set(:user) { create(:user) }
 
@@ -18,7 +20,7 @@ describe Gitlab::Ci::Config::External::Mapper do
   end
 
   before do
-    WebMock.stub_request(:get, remote_url).to_return(body: file_content)
+    stub_full_request(remote_url).to_return(body: file_content)
   end
 
   describe '#process' do

spec/lib/gitlab/ci/config/external/processor_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::External::Processor do
+  include StubRequests
+
   set(:project) { create(:project, :repository) }
   set(:another_project) { create(:project, :repository) }
   set(:user) { create(:user) }
@@ -42,7 +44,7 @@ describe Gitlab::Ci::Config::External::Processor do
       let(:values) { { include: remote_file, image: 'ruby:2.2' } }
 
       before do
-        WebMock.stub_request(:get, remote_file).to_raise(SocketError.new('Some HTTP error'))
+        stub_full_request(remote_file).and_raise(SocketError.new('Some HTTP error'))
       end
 
       it 'raises an error' do
@@ -75,7 +77,7 @@ describe Gitlab::Ci::Config::External::Processor do
       end
 
       before do
-        WebMock.stub_request(:get, remote_file).to_return(body: external_file_content)
+        stub_full_request(remote_file).to_return(body: external_file_content)
       end
 
       it 'appends the file to the values' do
@@ -145,7 +147,7 @@ describe Gitlab::Ci::Config::External::Processor do
         allow_any_instance_of(Gitlab::Ci::Config::External::File::Local)
           .to receive(:fetch_local_content).and_return(local_file_content)
 
-        WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
+        stub_full_request(remote_file).to_return(body: remote_file_content)
       end
 
       it 'appends the files to the values' do
@@ -191,7 +193,8 @@ describe Gitlab::Ci::Config::External::Processor do
       end
 
       it 'takes precedence' do
-        WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
+        stub_full_request(remote_file).to_return(body: remote_file_content)
+
         expect(processor.perform[:image]).to eq('ruby:2.2')
       end
     end
@@ -231,7 +234,8 @@ describe Gitlab::Ci::Config::External::Processor do
           HEREDOC
         end
 
-        WebMock.stub_request(:get, 'http://my.domain.com/config.yml').to_return(body: 'remote_build: { script: echo Hello World }')
+        stub_full_request('http://my.domain.com/config.yml')
+          .to_return(body: 'remote_build: { script: echo Hello World }')
       end
 
       context 'when project is public' do
@@ -273,8 +277,10 @@ describe Gitlab::Ci::Config::External::Processor do
 
     context 'when config includes an external configuration file via SSL web request' do
       before do
-        stub_request(:get, 'https://sha256.badssl.com/fake.yml').to_return(body: 'image: ruby:2.6', status: 200)
-        stub_request(:get, 'https://self-signed.badssl.com/fake.yml')
+        stub_full_request('https://sha256.badssl.com/fake.yml', ip_address: '8.8.8.8')
+          .to_return(body: 'image: ruby:2.6', status: 200)
+
+        stub_full_request('https://self-signed.badssl.com/fake.yml', ip_address: '8.8.8.9')
           .to_raise(OpenSSL::SSL::SSLError.new('SSL_connect returned=1 errno=0 state=error: certificate verify failed (self signed certificate)'))
       end
 

spec/lib/gitlab/ci/config_spec.rb

 require 'spec_helper'
 
 describe Gitlab::Ci::Config do
+  include StubRequests
+
   set(:user) { create(:user) }
 
   let(:config) do
@@ -216,8 +218,7 @@ describe Gitlab::Ci::Config do
     end
 
     before do
-      WebMock.stub_request(:get, remote_location)
-        .to_return(body: remote_file_content)
+      stub_full_request(remote_location).to_return(body: remote_file_content)
 
       allow(project.repository)
         .to receive(:blob_data_at).and_return(local_file_content)

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -3,6 +3,8 @@ require 'spec_helper'
 module Gitlab
   module Ci
     describe YamlProcessor do
+      include StubRequests
+
       subject { described_class.new(config, user: nil) }
 
       describe '#build_attributes' do
@@ -648,7 +650,7 @@ module Gitlab
             end
 
             before do
-              WebMock.stub_request(:get, 'https://gitlab.com/awesome-project/raw/master/.before-script-template.yml')
+              stub_full_request('https://gitlab.com/awesome-project/raw/master/.before-script-template.yml')
                 .to_return(
                   status: 200,
                   headers: { 'Content-Type' => 'application/json' },

spec/lib/gitlab/http_connection_adapter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::HTTPConnectionAdapter do
+  describe '#connection' do
+    context 'when local requests are not allowed' do
+      it 'sets up the connection' do
+        uri = URI('https://example.org')
+
+        connection = described_class.new(uri).connection
+
+        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
+
+      it 'raises error when it is a request to local address' do
+        uri = URI('http://172.16.0.0/12')
+
+        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")
+      end
+
+      it 'raises error when it is a request to localhost address' do
+        uri = URI('http://127.0.0.1')
+
+        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")
+      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
+
+          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')
+          expect(connection.port).to eq(8080)
+        end
+      end
+    end
+
+    context 'when local requests are allowed' do
+      it 'sets up the connection' do
+        uri = URI('https://example.org')
+
+        connection = described_class.new(uri, allow_local_requests: true).connection
+
+        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
+
+      it 'sets up the connection when it is a local network' do
+        uri = URI('http://172.16.0.0/12')
+
+        connection = described_class.new(uri, allow_local_requests: true).connection
+
+        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
+
+      it 'sets up the connection when it is localhost' do
+        uri = URI('http://127.0.0.1')
+
+        connection = described_class.new(uri, allow_local_requests: true).connection
+
+        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
+end

spec/lib/gitlab/http_spec.rb

 require 'spec_helper'
 
 describe Gitlab::HTTP do
+  include StubRequests
+
+  context 'when allow_local_requests' do
+    it 'sends the request to the correct URI' do
+      stub_full_request('https://example.org:8080', ip_address: '8.8.8.8').to_return(status: 200)
+
+      described_class.get('https://example.org:8080', allow_local_requests: false)
+
+      expect(WebMock).to have_requested(:get, 'https://8.8.8.8:8080').once
+    end
+  end
+
+  context 'when not allow_local_requests' do
+    it 'sends the request to the correct URI' do
+      stub_full_request('https://example.org:8080')
+
+      described_class.get('https://example.org:8080', allow_local_requests: true)
+
+      expect(WebMock).to have_requested(:get, 'https://8.8.8.9:8080').once
+    end
+  end
+
   describe 'allow_local_requests_from_hooks_and_services is' do
     before do
       WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success')
@@ -21,6 +43,8 @@ describe Gitlab::HTTP do
 
       context 'if allow_local_requests set to true' do
         it 'override the global value and allow requests to localhost or private network' do
+          stub_full_request('http://localhost:3003')
+
           expect { described_class.get('http://localhost:3003', allow_local_requests: true) }.not_to raise_error
         end
       end
@@ -32,6 +56,8 @@ describe Gitlab::HTTP do
       end
 
       it 'allow requests to localhost' do
+        stub_full_request('http://localhost:3003')
+
         expect { described_class.get('http://localhost:3003') }.not_to raise_error
       end
 
@@ -49,7 +75,7 @@ describe Gitlab::HTTP do
 
   describe 'handle redirect loops' do
     before do
-      WebMock.stub_request(:any, "http://example.org").to_raise(HTTParty::RedirectionTooDeep.new("Redirection Too Deep"))
+      stub_full_request("http://example.org", method: :any).to_raise(HTTParty::RedirectionTooDeep.new("Redirection Too Deep"))
     end
 
     it 'handles GET requests' do

spec/lib/gitlab/import_export/after_export_strategies/web_upload_strategy_spec.rb

 require 'spec_helper'
 
 describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do
+  include StubRequests
+
   let(:example_url) { 'http://www.example.com' }
   let(:strategy) { subject.new(url: example_url, http_method: 'post') }
   let!(:project) { create(:project, :with_export) }
@@ -35,7 +37,7 @@ describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do
 
     context 'when upload fails' do
       it 'stores the export error' do
-        stub_request(:post, example_url).to_return(status: [404, 'Page not found'])
+        stub_full_request(example_url, method: :post).to_return(status: [404, 'Page not found'])
 
         strategy.execute(user, project)
 

spec/lib/gitlab/url_blocker_spec.rb

@@ -2,6 +2,52 @@
 require 'spec_helper'
 
 describe Gitlab::UrlBlocker do
+  describe '#validate!' do
+    context 'when URI is nil' do
+      let(:import_url) { nil }
+
+      it 'returns no URI and hostname' do
+        uri, hostname = described_class.validate!(import_url)
+
+        expect(uri).to be(nil)
+        expect(hostname).to be(nil)
+      end
+    end
+
+    context 'when URI is internal' do
+      let(:import_url) { 'http://localhost' }
+
+      it 'returns URI and no hostname' do
+        uri, hostname = described_class.validate!(import_url)
+
+        expect(uri).to eq(Addressable::URI.parse('http://[::1]'))
+        expect(hostname).to eq('localhost')
+      end
+    end
+
+    context 'when the URL hostname is a domain' do
+      let(:import_url) { 'https://example.org' }
+
+      it 'returns URI and hostname' do
+        uri, hostname = described_class.validate!(import_url)
+
+        expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34'))
+        expect(hostname).to eq('example.org')
+      end
+    end
+
+    context 'when the URL hostname is an IP address' do
+      let(:import_url) { 'https://93.184.216.34' }
+
+      it 'returns URI and no hostname' do
+        uri, hostname = described_class.validate!(import_url)
+
+        expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34'))
+        expect(hostname).to be(nil)
+      end
+    end
+  end
+
   describe '#blocked_url?' do
     let(:ports) { Project::VALID_IMPORT_PORTS }
 
@@ -208,7 +254,7 @@ describe Gitlab::UrlBlocker do
       end
 
       def stub_domain_resolv(domain, ip)
-        address = double(ip_address: ip, ipv4_private?: true, ipv6_link_local?: false, ipv4_loopback?: false, ipv6_loopback?: false)
+        address = double(ip_address: ip, ipv4_private?: true, ipv6_link_local?: false, ipv4_loopback?: false, ipv6_loopback?: false, ipv4?: false)
         allow(Addrinfo).to receive(:getaddrinfo).with(domain, any_args).and_return([address])
         allow(address).to receive(:ipv6_v4mapped?).and_return(false)
       end

spec/lib/mattermost/session_spec.rb

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe Mattermost::Session, type: :request do
   include ExclusiveLeaseHelpers
+  include StubRequests
 
   let(:user) { create(:user) }
 
@@ -24,7 +25,7 @@ describe Mattermost::Session, type: :request do
     let(:location) { 'http://location.tld' }
     let(:cookie_header) {'MMOAUTH=taskik8az7rq8k6rkpuas7htia; Path=/;'}
     let!(:stub) do
-      WebMock.stub_request(:get, "#{mattermost_url}/oauth/gitlab/login")
+      stub_full_request("#{mattermost_url}/oauth/gitlab/login")
         .to_return(headers: { 'location' => location, 'Set-Cookie' => cookie_header }, status: 302)
     end
 
@@ -63,7 +64,7 @@ describe Mattermost::Session, type: :request do
         end
 
         before do
-          WebMock.stub_request(:get, "#{mattermost_url}/signup/gitlab/complete")
+          stub_full_request("#{mattermost_url}/signup/gitlab/complete")
             .with(query: hash_including({ 'state' => state }))
             .to_return do |request|
               post "/oauth/token",
@@ -80,7 +81,7 @@ describe Mattermost::Session, type: :request do
               end
             end
 
-          WebMock.stub_request(:post, "#{mattermost_url}/api/v4/users/logout")
+          stub_full_request("#{mattermost_url}/api/v4/users/logout", method: :post)
             .to_return(headers: { Authorization: 'token thisworksnow' }, status: 200)
         end
 

spec/models/project_services/assembla_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe AssemblaService do
+  include StubRequests
+
   describe "Associations" do
     it { is_expected.to belong_to :project }
     it { is_expected.to have_one :service_hook }
@@ -23,12 +25,12 @@ describe AssemblaService do
       )
       @sample_data = Gitlab::DataBuilder::Push.build_sample(project, user)
       @api_url = 'https://atlas.assembla.com/spaces/project_name/github_tool?secret_key=verySecret'
-      WebMock.stub_request(:post, @api_url)
+      stub_full_request(@api_url, method: :post)
     end
 
     it "calls Assembla API" do
       @assembla_service.execute(@sample_data)
-      expect(WebMock).to have_requested(:post, @api_url).with(
+      expect(WebMock).to have_requested(:post, stubbed_hostname(@api_url)).with(
         body: /#{@sample_data[:before]}.*#{@sample_data[:after]}.*#{project.path}/
       ).once
     end

spec/models/project_services/bamboo_service_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe BambooService, :use_clean_rails_memory_store_caching do
   include ReactiveCachingHelpers
+  include StubRequests
 
   let(:bamboo_url) { 'http://gitlab.com/bamboo' }
 
@@ -257,7 +258,7 @@ describe BambooService, :use_clean_rails_memory_store_caching do
   end
 
   def stub_bamboo_request(url, status, body)
-    WebMock.stub_request(:get, url).to_return(
+    stub_full_request(url).to_return(
       status: status,
       headers: { 'Content-Type' => 'application/json' },
       body: body

spec/models/project_services/buildkite_service_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe BuildkiteService, :use_clean_rails_memory_store_caching do
   include ReactiveCachingHelpers
+  include StubRequests
 
   let(:project) { create(:project) }
 
@@ -110,10 +111,9 @@ describe BuildkiteService, :use_clean_rails_memory_store_caching do
     body ||= %q({"status":"success"})
     buildkite_full_url = 'https://gitlab.buildkite.com/status/secret-sauce-status-token.json?commit=123'
 
-    WebMock.stub_request(:get, buildkite_full_url).to_return(
-      status: status,
+    stub_full_request(buildkite_full_url)
+      .to_return(status: status,
                  headers: { 'Content-Type' => 'application/json' },
-      body: body
-    )
+                 body: body)
   end
 end

spec/models/project_services/campfire_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe CampfireService do
+  include StubRequests
+
   describe 'Associations' do
     it { is_expected.to belong_to :project }
     it { is_expected.to have_one :service_hook }
@@ -49,39 +51,37 @@ describe CampfireService do
     it "calls Campfire API to get a list of rooms and speak in a room" do
       # make sure a valid list of rooms is returned
       body = File.read(Rails.root + 'spec/fixtures/project_services/campfire/rooms.json')
-      WebMock.stub_request(:get, @rooms_url).with(basic_auth: @auth).to_return(
+
+      stub_full_request(@rooms_url).with(basic_auth: @auth).to_return(
         body: body,
         status: 200,
         headers: @headers
       )
+
       # stub the speak request with the room id found in the previous request's response
       speak_url = 'https://project-name.campfirenow.com/room/123/speak.json'
-      WebMock.stub_request(:post, speak_url).with(basic_auth: @auth)
+      stub_full_request(speak_url, method: :post).with(basic_auth: @auth)
 
       @campfire_service.execute(@sample_data)
 
-      expect(WebMock).to have_requested(:get, @rooms_url).once
-      expect(WebMock).to have_requested(:post, speak_url).with(
-        body: /#{project.path}.*#{@sample_data[:before]}.*#{@sample_data[:after]}/
-      ).once
+      expect(WebMock).to have_requested(:get, stubbed_hostname(@rooms_url)).once
+      expect(WebMock).to have_requested(:post, stubbed_hostname(speak_url))
+                           .with(body: /#{project.path}.*#{@sample_data[:before]}.*#{@sample_data[:after]}/).once
     end
 
     it "calls Campfire API to get a list of rooms but shouldn't speak in a room" do
       # return a list of rooms that do not contain a room named 'test-room'
       body = File.read(Rails.root + 'spec/fixtures/project_services/campfire/rooms2.json')
-      WebMock.stub_request(:get, @rooms_url).with(basic_auth: @auth).to_return(
+      stub_full_request(@rooms_url).with(basic_auth: @auth).to_return(
         body: body,
         status: 200,
         headers: @headers
       )
-      # we want to make sure no request is sent to the /speak endpoint, here is a basic
-      # regexp that matches this endpoint
-      speak_url = 'https://verySecret:X@project-name.campfirenow.com/room/.*/speak.json'
 
       @campfire_service.execute(@sample_data)
 
-      expect(WebMock).to have_requested(:get, @rooms_url).once
-      expect(WebMock).not_to have_requested(:post, /#{speak_url}/)
+      expect(WebMock).to have_requested(:get, 'https://8.8.8.9/rooms.json').once
+      expect(WebMock).not_to have_requested(:post, '*/room/.*/speak.json')
     end
   end
 end

spec/models/project_services/pivotaltracker_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe PivotaltrackerService do
+  include StubRequests
+
   describe 'Associations' do
     it { is_expected.to belong_to :project }
     it { is_expected.to have_one :service_hook }
@@ -53,12 +55,12 @@ describe PivotaltrackerService do
     end
 
     before do
-      WebMock.stub_request(:post, url)
+      stub_full_request(url, method: :post)
     end
 
     it 'posts correct message' do
       service.execute(push_data)
-      expect(WebMock).to have_requested(:post, url).with(
+      expect(WebMock).to have_requested(:post, stubbed_hostname(url)).with(
         body: {
           'source_commit' => {
             'commit_id' => '21c12ea',
@@ -85,14 +87,14 @@ describe PivotaltrackerService do
         service.execute(push_data(branch: 'master'))
         service.execute(push_data(branch: 'v10'))
 
-        expect(WebMock).to have_requested(:post, url).twice
+        expect(WebMock).to have_requested(:post, stubbed_hostname(url)).twice
       end
 
       it 'does not post message if branch is not in the list' do
         service.execute(push_data(branch: 'mas'))
         service.execute(push_data(branch: 'v11'))
 
-        expect(WebMock).not_to have_requested(:post, url)
+        expect(WebMock).not_to have_requested(:post, stubbed_hostname(url))
       end
     end
   end

spec/models/project_services/pushover_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe PushoverService do
+  include StubRequests
+
   describe 'Associations' do
     it { is_expected.to belong_to :project }
     it { is_expected.to have_one :service_hook }
@@ -57,13 +59,13 @@ describe PushoverService do
         sound: sound
       )
 
-      WebMock.stub_request(:post, api_url)
+      stub_full_request(api_url, method: :post, ip_address: '8.8.8.8')
     end
 
     it 'calls Pushover API' do
       pushover.execute(sample_data)
 
-      expect(WebMock).to have_requested(:post, api_url).once
+      expect(WebMock).to have_requested(:post, 'https://8.8.8.8/1/messages.json').once
     end
   end
 end

spec/models/project_services/teamcity_service_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe TeamcityService, :use_clean_rails_memory_store_caching do
   include ReactiveCachingHelpers
+  include StubRequests
 
   let(:teamcity_url) { 'http://gitlab.com/teamcity' }
 
@@ -212,7 +213,7 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
 
     body ||= %Q({"build":{"status":"#{build_status}","id":"666"}})
 
-    WebMock.stub_request(:get, teamcity_full_url).with(basic_auth: auth).to_return(
+    stub_full_request(teamcity_full_url).with(basic_auth: auth).to_return(
       status: status,
       headers: { 'Content-Type' => 'application/json' },
       body: body

spec/requests/api/system_hooks_spec.rb

 require 'spec_helper'
 
 describe API::SystemHooks do
+  include StubRequests
+
   let(:user) { create(:user) }
   let(:admin) { create(:admin) }
   let!(:hook) { create(:system_hook, url: "http://example.com") }
 
   before do
-    stub_request(:post, hook.url)
+    stub_full_request(hook.url, method: :post)
   end
 
   describe "GET /hooks" do
@@ -68,6 +70,8 @@ describe API::SystemHooks do
     end
 
     it 'sets default values for events' do
+      stub_full_request('http://mep.mep', method: :post)
+
       post api('/hooks', admin), params: { url: 'http://mep.mep' }
 
       expect(response).to have_gitlab_http_status(201)
@@ -78,6 +82,8 @@ describe API::SystemHooks do
     end
 
     it 'sets explicit values for events' do
+      stub_full_request('http://mep.mep', method: :post)
+
       post api('/hooks', admin),
         params: {
           url: 'http://mep.mep',

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -2,6 +2,8 @@
 require 'spec_helper'
 
 describe Projects::LfsPointers::LfsDownloadService do
+  include StubRequests
+
   let(:project) { create(:project) }
   let(:lfs_content) { SecureRandom.random_bytes(10) }
   let(:oid) { Digest::SHA256.hexdigest(lfs_content) }
@@ -62,7 +64,7 @@ describe Projects::LfsPointers::LfsDownloadService do
   describe '#execute' do
     context 'when file download succeeds' do
       before do
-        WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
+        stub_full_request(download_link).to_return(body: lfs_content)
       end
 
       it_behaves_like 'lfs object is created'
@@ -104,7 +106,7 @@ describe Projects::LfsPointers::LfsDownloadService do
       let(:size) { 1 }
 
       before do
-        WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
+        stub_full_request(download_link).to_return(body: lfs_content)
       end
 
       it_behaves_like 'no lfs object is created'
@@ -118,7 +120,7 @@ describe Projects::LfsPointers::LfsDownloadService do
 
     context 'when downloaded lfs file has a different oid' do
       before do
-        WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
+        stub_full_request(download_link).to_return(body: lfs_content)
         allow_any_instance_of(Digest::SHA256).to receive(:hexdigest).and_return('foobar')
       end
 
@@ -136,7 +138,7 @@ describe Projects::LfsPointers::LfsDownloadService do
       let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials) }
 
       before do
-        WebMock.stub_request(:get, download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content)
+        stub_full_request(download_link).with(headers: { 'Authorization' => 'Basic dXNlcjpwYXNzd29yZA==' }).to_return(body: lfs_content)
       end
 
       it 'the request adds authorization headers' do
@@ -149,7 +151,7 @@ describe Projects::LfsPointers::LfsDownloadService do
       let(:local_request_setting) { true }
 
       before do
-        WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
+        stub_full_request(download_link, ip_address: '192.168.2.120').to_return(body: lfs_content)
       end
 
       it_behaves_like 'lfs object is created'
@@ -173,7 +175,8 @@ describe Projects::LfsPointers::LfsDownloadService do
 
         with_them do
           before do
-            WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
+            stub_full_request(download_link, ip_address: '192.168.2.120')
+              .to_return(status: 301, headers: { 'Location' => redirect_link })
           end
 
           it_behaves_like 'no lfs object is created'
@@ -184,8 +187,8 @@ describe Projects::LfsPointers::LfsDownloadService do
         let(:redirect_link) { "http://example.com/"}
 
         before do
-          WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
-          WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content)
+          stub_full_request(download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
+          stub_full_request(redirect_link).to_return(body: lfs_content)
         end
 
         it_behaves_like 'lfs object is created'

spec/services/submit_usage_ping_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe SubmitUsagePingService do
+  include StubRequests
+
   context 'when usage ping is disabled' do
     before do
       stub_application_setting(usage_ping_enabled: false)
@@ -99,7 +101,7 @@ describe SubmitUsagePingService do
   end
 
   def stub_response(body)
-    stub_request(:post, 'https://version.gitlab.com/usage_data')
+    stub_full_request('https://version.gitlab.com/usage_data', method: :post)
       .to_return(
         headers: { 'Content-Type' => 'application/json' },
         body: body.to_json

spec/services/web_hook_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe WebHookService do
+  include StubRequests
+
   let(:project) { create(:project) }
   let(:project_hook) { create(:project_hook) }
   let(:headers) do
@@ -67,11 +69,11 @@ describe WebHookService do
       let(:project_hook) { create(:project_hook, url: 'https://demo:demo@example.org/') }
 
       it 'uses the credentials' do
-        WebMock.stub_request(:post, url)
+        stub_full_request(url, method: :post)
 
         service_instance.execute
 
-        expect(WebMock).to have_requested(:post, url).with(
+        expect(WebMock).to have_requested(:post, stubbed_hostname(url)).with(
           headers: headers.merge('Authorization' => 'Basic ZGVtbzpkZW1v')
         ).once
       end
@@ -82,11 +84,11 @@ describe WebHookService do
       let(:project_hook) { create(:project_hook, url: 'https://demo@example.org/') }
 
       it 'uses the credentials anyways' do
-        WebMock.stub_request(:post, url)
+        stub_full_request(url, method: :post)
 
         service_instance.execute
 
-        expect(WebMock).to have_requested(:post, url).with(
+        expect(WebMock).to have_requested(:post, stubbed_hostname(url)).with(
           headers: headers.merge('Authorization' => 'Basic ZGVtbzo=')
         ).once
       end

spec/support/helpers/stub_requests.rb

+module StubRequests
+  IP_ADDRESS_STUB = '8.8.8.9'.freeze
+
+  # Fully stubs a request using WebMock class. This class also
+  # stubs the IP address the URL is translated to (DNS lookup).
+  #
+  # It expects the final request to go to the `ip_address` instead the given url.
+  # That's primarily a DNS rebind attack prevention of Gitlab::HTTP
+  # (see: Gitlab::UrlBlocker).
+  #
+  def stub_full_request(url, ip_address: IP_ADDRESS_STUB, port: 80, method: :get)
+    stub_dns(url, ip_address: ip_address, port: port)
+
+    url = stubbed_hostname(url, hostname: ip_address)
+    WebMock.stub_request(method, url)
+  end
+
+  def stub_dns(url, ip_address:, port: 80)
+    url = parse_url(url)
+    socket = Socket.sockaddr_in(port, ip_address)
+    addr = Addrinfo.new(socket)
+
+    # See Gitlab::UrlBlocker
+    allow(Addrinfo).to receive(:getaddrinfo)
+                         .with(url.hostname, url.port, nil, :STREAM)
+                         .and_return([addr])
+  end
+
+  def stubbed_hostname(url, hostname: IP_ADDRESS_STUB)
+    url = parse_url(url)
+    url.hostname = hostname
+    url.to_s
+  end
+
+  private
+
+  def parse_url(url)
+    url.is_a?(URI) ? url : URI(url)
+  end
+end