Merge branch '10-6-stable-rc6-critical' into '10-6-stable'

.rubocop.yml

@@ -46,6 +46,9 @@ Gitlab/ModuleWithInstanceVariables:
     - spec/support/**/*.rb
     - features/steps/**/*.rb
 
+Gitlab/HTTParty:
+  Enabled: true
+
 GitlabSecurity/PublicSend:
   Enabled: true
   Exclude:

Gemfile

@@ -25,7 +25,7 @@ gem 'devise', '~> 4.2'
 gem 'doorkeeper', '~> 4.2.0'
 gem 'doorkeeper-openid_connect', '~> 1.2.0'
 gem 'omniauth', '~> 1.4.2'
-gem 'omniauth-auth0', '~> 1.4.1'
+gem 'omniauth-auth0', '~> 2.0.0'
 gem 'omniauth-azure-oauth2', '~> 0.0.9'
 gem 'omniauth-cas3', '~> 1.1.4'
 gem 'omniauth-facebook', '~> 4.0.0'

Gemfile.lock

@@ -531,8 +531,8 @@ GEM
     omniauth (1.4.2)
       hashie (>= 1.2, < 4)
       rack (>= 1.0, < 3)
-    omniauth-auth0 (1.4.1)
-      omniauth-oauth2 (~> 1.1)
+    omniauth-auth0 (2.0.0)
+      omniauth-oauth2 (~> 1.4)
     omniauth-authentiq (0.3.1)
       omniauth-oauth2 (~> 1.3, >= 1.3.1)
     omniauth-azure-oauth2 (0.0.9)
@@ -1113,7 +1113,7 @@ DEPENDENCIES
   octokit (~> 4.6.2)
   oj (~> 2.17.4)
   omniauth (~> 1.4.2)
-  omniauth-auth0 (~> 1.4.1)
+  omniauth-auth0 (~> 2.0.0)
   omniauth-authentiq (~> 0.3.1)
   omniauth-azure-oauth2 (~> 0.0.9)
   omniauth-cas3 (~> 1.1.4)

app/assets/javascripts/notes.js

@@ -16,10 +16,6 @@ import Autosize from 'autosize';
 import 'vendor/jquery.caret'; // required by jquery.atwho
 import 'vendor/jquery.atwho';
 import AjaxCache from '~/lib/utils/ajax_cache';
-import Vue from 'vue';
-import syntaxHighlight from '~/syntax_highlight';
-import SkeletonLoadingContainer from '~/vue_shared/components/skeleton_loading_container.vue';
-import { __ } from '~/locale';
 import axios from './lib/utils/axios_utils';
 import { getLocationHash } from './lib/utils/url_utility';
 import Flash from './flash';
@@ -103,13 +99,6 @@ export default class Notes {
       $('.note-edit-form').clone()
         .addClass('mr-note-edit-form').insertAfter('.note-edit-form');
     }
-
-    const hash = getLocationHash();
-    const $anchor = hash && document.getElementById(hash);
-
-    if ($anchor) {
-      this.loadLazyDiff({ currentTarget: $anchor });
-    }
   }
 
   setViewType(view) {
@@ -146,8 +135,6 @@ export default class Notes {
     this.$wrapperEl.on('click', '.js-close-discussion-note-form', this.cancelDiscussionForm);
     // toggle commit list
     this.$wrapperEl.on('click', '.system-note-commit-list-toggler', this.toggleCommitList);
-
-    this.$wrapperEl.on('click', '.js-toggle-lazy-diff', this.loadLazyDiff);
     // fetch notes when tab becomes visible
     this.$wrapperEl.on('visibilitychange', this.visibilityChange);
     // when issue status changes, we need to refresh data
@@ -186,7 +173,6 @@ export default class Notes {
     this.$wrapperEl.off('keydown', '.js-note-text');
     this.$wrapperEl.off('click', '.js-comment-resolve-button');
     this.$wrapperEl.off('click', '.system-note-commit-list-toggler');
-    this.$wrapperEl.off('click', '.js-toggle-lazy-diff');
     this.$wrapperEl.off('ajax:success', '.js-main-target-form');
     this.$wrapperEl.off('ajax:success', '.js-discussion-note-form');
     this.$wrapperEl.off('ajax:complete', '.js-main-target-form');
@@ -1221,60 +1207,6 @@ export default class Notes {
     return this.notesCountBadge.text(parseInt(this.notesCountBadge.text(), 10) + updateCount);
   }
 
-  static renderPlaceholderComponent($container) {
-    const el = $container.find('.js-code-placeholder').get(0);
-    new Vue({ // eslint-disable-line no-new
-      el,
-      components: {
-        SkeletonLoadingContainer,
-      },
-      render(createElement) {
-        return createElement('skeleton-loading-container');
-      },
-    });
-  }
-
-  static renderDiffContent($container, data) {
-    const { discussion_html } = data;
-    const lines = $(discussion_html).find('.line_holder');
-    lines.addClass('fade-in');
-    $container.find('tbody').prepend(lines);
-    const fileHolder = $container.find('.file-holder');
-    $container.find('.line-holder-placeholder').remove();
-    syntaxHighlight(fileHolder);
-  }
-
-  static renderDiffError($container) {
-    $container.find('.line_content').html(
-      $(`
-        <div class="nothing-here-block">
-          ${__('Unable to load the diff.')} <a class="js-toggle-lazy-diff" href="javascript:void(0)">Try again</a>?
-        </div>
-      `),
-    );
-  }
-
-  loadLazyDiff(e) {
-    const $container = $(e.currentTarget).closest('.js-toggle-container');
-    Notes.renderPlaceholderComponent($container);
-
-    $container.find('.js-toggle-lazy-diff').removeClass('js-toggle-lazy-diff');
-
-    const tableEl = $container.find('tbody');
-    if (tableEl.length === 0) return;
-
-    const fileHolder = $container.find('.file-holder');
-    const url = fileHolder.data('linesPath');
-
-    axios.get(url)
-      .then(({ data }) => {
-        Notes.renderDiffContent($container, data);
-      })
-      .catch(() => {
-        Notes.renderDiffError($container);
-      });
-  }
-
   toggleCommitList(e) {
     const $element = $(e.currentTarget);
     const $closestSystemCommitList = $element.siblings('.system-note-commit-list');

app/controllers/omniauth_callbacks_controller.rb

@@ -95,6 +95,14 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     handle_omniauth
   end
 
+  def auth0
+    if oauth['uid'].blank?
+      fail_auth0_login
+    else
+      handle_omniauth
+    end
+  end
+
   private
 
   def handle_omniauth
@@ -170,6 +178,12 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     redirect_to new_user_session_path
   end
 
+  def fail_auth0_login
+    flash[:alert] = 'Wrong extern UID provided. Make sure Auth0 is configured correctly.'
+
+    redirect_to new_user_session_path
+  end
+
   def handle_disabled_provider
     label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider'])
     flash[:alert] = "Signing in using #{label} has been disabled"

app/controllers/projects/discussions_controller.rb

@@ -19,12 +19,6 @@ class Projects::DiscussionsController < Projects::ApplicationController
     render_discussion
   end
 
-  def show
-    render json: {
-      discussion_html: view_to_html_string('discussions/_diff_with_notes', discussion: discussion, expanded: true)
-    }
-  end
-
   private
 
   def render_discussion

app/helpers/application_settings_helper.rb

@@ -245,7 +245,8 @@ module ApplicationSettingsHelper
       :usage_ping_enabled,
       :user_default_external,
       :user_oauth_applications,
-      :version_check_enabled
+      :version_check_enabled,
+      :allow_local_requests_from_hooks_and_services
     ]
   end
 end

app/models/application_setting.rb

@@ -330,7 +330,8 @@ class ApplicationSetting < ActiveRecord::Base
       usage_ping_enabled: Settings.gitlab['usage_ping_enabled'],
       gitaly_timeout_fast: 10,
       gitaly_timeout_medium: 30,
-      gitaly_timeout_default: 55
+      gitaly_timeout_default: 55,
+      allow_local_requests_from_hooks_and_services: false
     }
   end
 

app/models/project.rb

@@ -38,6 +38,9 @@ class Project < ActiveRecord::Base
     attachments: 2
   }.freeze
 
+  # Valids ports to import from
+  VALID_IMPORT_PORTS = [22, 80, 443].freeze
+
   cache_markdown_field :description, pipeline: :description
 
   delegate :feature_available?, :builds_enabled?, :wiki_enabled?,

app/models/project_services/assembla_service.rb

 class AssemblaService < Service
-  include HTTParty
-
   prop_accessor :token, :subdomain
   validates :token, presence: true, if: :activated?
 
@@ -31,6 +29,6 @@ class AssemblaService < Service
     return unless supported_events.include?(data[:object_kind])
 
     url = "https://atlas.assembla.com/spaces/#{subdomain}/github_tool?secret_key=#{token}"
-    AssemblaService.post(url, body: { payload: data }.to_json, headers: { 'Content-Type' => 'application/json' })
+    Gitlab::HTTP.post(url, body: { payload: data }.to_json, headers: { 'Content-Type' => 'application/json' })
   end
 end

app/models/project_services/bamboo_service.rb

@@ -117,14 +117,14 @@ class BambooService < CiService
     url = build_url(path)
 
     if username.blank? && password.blank?
-      HTTParty.get(url, verify: false)
+      Gitlab::HTTP.get(url, verify: false)
     else
       url << '&os_authType=basic'
-      HTTParty.get(url, verify: false,
-                        basic_auth: {
-                          username: username,
-                          password: password
-                        })
+      Gitlab::HTTP.get(url, verify: false,
+                            basic_auth: {
+                              username: username,
+                              password: password
+                            })
     end
   end
 end

app/models/project_services/buildkite_service.rb

@@ -71,7 +71,7 @@ class BuildkiteService < CiService
   end
 
   def calculate_reactive_cache(sha, ref)
-    response = HTTParty.get(commit_status_path(sha), verify: false)
+    response = Gitlab::HTTP.get(commit_status_path(sha), verify: false)
 
     status =
       if response.code == 200 && response['status']

app/models/project_services/campfire_service.rb

 class CampfireService < Service
-  include HTTParty
-
   prop_accessor :token, :subdomain, :room
   validates :token, presence: true, if: :activated?
 
@@ -31,7 +29,6 @@ class CampfireService < Service
   def execute(data)
     return unless supported_events.include?(data[:object_kind])
 
-    self.class.base_uri base_uri
     message = build_message(data)
     speak(self.room, message, auth)
   end
@@ -69,14 +66,14 @@ class CampfireService < Service
         }
       }
     }
-    res = self.class.post(path, auth.merge(body))
+    res = Gitlab::HTTP.post(path, base_uri: base_uri, **auth.merge(body))
     res.code == 201 ? res : nil
   end
 
   # Returns a list of rooms, or [].
   # https://github.com/basecamp/campfire-api/blob/master/sections/rooms.md#get-rooms
   def rooms(auth)
-    res = self.class.get("/rooms.json", auth)
+    res = Gitlab::HTTP.get("/rooms.json", base_uri: base_uri, **auth)
     res.code == 200 ? res["rooms"] : []
   end
 

app/models/project_services/drone_ci_service.rb

@@ -49,7 +49,7 @@ class DroneCiService < CiService
   end
 
   def calculate_reactive_cache(sha, ref)
-    response = HTTParty.get(commit_status_path(sha, ref), verify: enable_ssl_verification)
+    response = Gitlab::HTTP.get(commit_status_path(sha, ref), verify: enable_ssl_verification)
 
     status =
       if response.code == 200 && response['status']

app/models/project_services/external_wiki_service.rb

 class ExternalWikiService < Service
-  include HTTParty
-
   prop_accessor :external_wiki_url
 
   validates :external_wiki_url, presence: true, url: true, if: :activated?
@@ -24,7 +22,7 @@ class ExternalWikiService < Service
   end
 
   def execute(_data)
-    @response = HTTParty.get(properties['external_wiki_url'], verify: true) rescue nil
+    @response = Gitlab::HTTP.get(properties['external_wiki_url'], verify: true) rescue nil
     if @response != 200
       nil
     end

app/models/project_services/issue_tracker_service.rb

@@ -77,13 +77,13 @@ class IssueTrackerService < Service
     result = false
 
     begin
-      response = HTTParty.head(self.project_url, verify: true)
+      response = Gitlab::HTTP.head(self.project_url, verify: true)
 
       if response
         message = "#{self.type} received response #{response.code} when attempting to connect to #{self.project_url}"
         result = true
       end
-    rescue HTTParty::Error, Timeout::Error, SocketError, Errno::ECONNRESET, Errno::ECONNREFUSED, OpenSSL::SSL::SSLError => error
+    rescue Gitlab::HTTP::Error, Timeout::Error, SocketError, Errno::ECONNRESET, Errno::ECONNREFUSED, OpenSSL::SSL::SSLError => error
       message = "#{self.type} had an error when trying to connect to #{self.project_url}: #{error.message}"
     end
     Rails.logger.info(message)

app/models/project_services/mock_ci_service.rb

@@ -52,7 +52,7 @@ class MockCiService < CiService
   #
   #
   def commit_status(sha, ref)
-    response = HTTParty.get(commit_status_path(sha), verify: false)
+    response = Gitlab::HTTP.get(commit_status_path(sha), verify: false)
     read_commit_status(response)
   rescue Errno::ECONNREFUSED
     :error

app/models/project_services/packagist_service.rb

 class PackagistService < Service
-  include HTTParty
-
   prop_accessor :username, :token, :server
 
   validates :username, presence: true, if: :activated?

app/models/project_services/pivotaltracker_service.rb

 class PivotaltrackerService < Service
-  include HTTParty
-
   API_ENDPOINT = 'https://www.pivotaltracker.com/services/v5/source_commits'.freeze
 
   prop_accessor :token, :restrict_to_branch
@@ -52,7 +50,7 @@ class PivotaltrackerService < Service
           'message' => commit[:message]
         }
       }
-      PivotaltrackerService.post(
+      Gitlab::HTTP.post(
         API_ENDPOINT,
         body: message.to_json,
         headers: {

app/models/project_services/pushover_service.rb

 class PushoverService < Service
-  include HTTParty
-  base_uri 'https://api.pushover.net/1'
+  BASE_URI = 'https://api.pushover.net/1'.freeze
 
   prop_accessor :api_key, :user_key, :device, :priority, :sound
   validates :api_key, :user_key, :priority, presence: true, if: :activated?
@@ -99,6 +98,6 @@ class PushoverService < Service
       pushover_data[:sound] = sound
     end
 
-    PushoverService.post('/messages.json', body: pushover_data)
+    Gitlab::HTTP.post('/messages.json', base_uri: BASE_URI, body: pushover_data)
   end
 end

app/models/project_services/teamcity_service.rb

@@ -83,7 +83,7 @@ class TeamcityService < CiService
 
     branch = Gitlab::Git.ref_name(data[:ref])
 
-    HTTParty.post(
+    Gitlab::HTTP.post(
       build_url('httpAuth/app/rest/buildQueue'),
       body: "<build branchName=\"#{branch}\">"\
             "<buildType id=\"#{build_type}\"/>"\
@@ -134,10 +134,10 @@ class TeamcityService < CiService
   end
 
   def get_path(path)
-    HTTParty.get(build_url(path), verify: false,
-                                  basic_auth: {
-                                    username: username,
-                                    password: password
-                                  })
+    Gitlab::HTTP.get(build_url(path), verify: false,
+                                      basic_auth: {
+                                        username: username,
+                                        password: password
+                                      })
   end
 end

app/services/projects/import_service.rb

@@ -28,7 +28,7 @@ module Projects
 
     def add_repository_to_project
       if project.external_import? && !unknown_url?
-        raise Error, 'Blocked import URL.' if Gitlab::UrlBlocker.blocked_url?(project.import_url)
+        raise Error, 'Blocked import URL.' if Gitlab::UrlBlocker.blocked_url?(project.import_url, valid_ports: Project::VALID_IMPORT_PORTS)
       end
 
       # We should skip the repository for a GitHub import or GitLab project import,

app/services/submit_usage_ping_service.rb

@@ -14,16 +14,17 @@ class SubmitUsagePingService
   def execute
     return false unless Gitlab::CurrentSettings.usage_ping_enabled?
 
-    response = HTTParty.post(
+    response = Gitlab::HTTP.post(
       URL,
       body: Gitlab::UsageData.to_json(force_refresh: true),
+      allow_local_requests: true,
       headers: { 'Content-type' => 'application/json' }
     )
 
     store_metrics(response)
 
     true
-  rescue HTTParty::Error => e
+  rescue Gitlab::HTTP::Error => e
     Rails.logger.info "Unable to contact GitLab, Inc.: #{e}"
 
     false

app/services/web_hook_service.rb

@@ -3,23 +3,20 @@ class WebHookService
     attr_reader :body, :headers, :code
 
     def initialize
-      @headers = HTTParty::Response::Headers.new({})
+      @headers = Gitlab::HTTP::Response::Headers.new({})
       @body = ''
       @code = 'internal error'
     end
   end
 
-  include HTTParty
-
-  # HTTParty timeout
-  default_timeout Gitlab.config.gitlab.webhook_timeout
-
-  attr_accessor :hook, :data, :hook_name
+  attr_accessor :hook, :data, :hook_name, :request_options
 
   def initialize(hook, data, hook_name)
     @hook = hook
     @data = data
     @hook_name = hook_name.to_s
+    @request_options = { timeout: Gitlab.config.gitlab.webhook_timeout }
+    @request_options.merge!(allow_local_requests: true) if @hook.is_a?(SystemHook)
   end
 
   def execute
@@ -73,11 +70,12 @@ class WebHookService
   end
 
   def make_request(url, basic_auth = false)
-    self.class.post(url,
+    Gitlab::HTTP.post(url,
       body: data.to_json,
       headers: build_headers(hook_name),
       verify: hook.enable_ssl_verification,
-      basic_auth: basic_auth)
+      basic_auth: basic_auth,
+      **request_options)
   end
 
   def make_request_with_auth

app/validators/importable_url_validator.rb

@@ -4,7 +4,7 @@
 # protect against Server-side Request Forgery (SSRF).
 class ImportableUrlValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
-    if Gitlab::UrlBlocker.blocked_url?(value)
+    if Gitlab::UrlBlocker.blocked_url?(value, valid_ports: Project::VALID_IMPORT_PORTS)
       record.errors.add(attribute, "imports are not allowed from that URL")
     end
   end

app/views/admin/application_settings/_form.html.haml

@@ -860,5 +860,14 @@
       .col-sm-10
         = f.number_field :throttle_authenticated_web_period_in_seconds, class: 'form-control'
 
+  %fieldset
+    %legend Outbound requests
+    .form-group
+      .col-sm-offset-2.col-sm-10
+        .checkbox
+          = f.label :allow_local_requests_from_hooks_and_services do
+            = f.check_box :allow_local_requests_from_hooks_and_services
+            Allow requests to the local network from hooks and services
+
   .form-actions
     = f.submit 'Save', class: 'btn btn-save'

app/views/discussions/_diff_with_notes.html.haml

@@ -2,12 +2,8 @@
 - blob = discussion.blob
 - discussions = { discussion.original_line_code => [discussion] }
 - diff_file_class = diff_file.text? ? 'text-file' : 'js-image-file'
-- diff_data = {}
-- expanded = discussion.expanded? || local_assigns.fetch(:expanded, nil)
-- unless expanded
-  - diff_data = { lines_path: project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion) }
 
-.diff-file.file-holder{ class: diff_file_class, data: diff_data }
+.diff-file.file-holder{ class: diff_file_class }
   .js-file-title.file-title.file-title-flex-parent
     .file-header-content
       = render "projects/diffs/file_header", diff_file: diff_file, url: discussion_path(discussion), show_toggle: false
@@ -15,24 +11,17 @@
   - if diff_file.text?
     .diff-content.code.js-syntax-highlight
       %table
-        - if expanded
-          - discussions = { discussion.original_line_code => [discussion] }
-          = render partial: "projects/diffs/line",
-            collection: discussion.truncated_diff_lines,
-            as: :line,
-            locals: { diff_file: diff_file,
-              discussions: discussions,
-              discussion_expanded: true,
-              plain: true }
-        - else
-          %tr.line_holder.line-holder-placeholder
-            %td.old_line.diff-line-num
-            %td.new_line.diff-line-num
-            %td.line_content
-              .js-code-placeholder
-          = render "discussions/diff_discussion", discussions: [discussion], expanded: true
+        = render partial: "projects/diffs/line",
+          collection: discussion.truncated_diff_lines,
+          as: :line,
+          locals: { diff_file: diff_file,
+            discussions: discussions,
+            discussion_expanded: true,
+            plain: true }
   - else
     - partial = (diff_file.new_file? || diff_file.deleted_file?) ? 'single_image_diff' : 'replaced_image_diff'
+
     = render partial: "projects/diffs/#{partial}", locals: { diff_file: diff_file, position: discussion.position.to_json, click_to_comment: false }
+
     .note-container
       = render partial: "discussions/notes", locals: { discussion: discussion, show_toggle: false, show_image_comment_badge: true, disable_collapse_class: true }

app/views/discussions/_discussion.html.haml

@@ -8,7 +8,7 @@
       .discussion.js-toggle-container{ data: { discussion_id: discussion.id } }
         .discussion-header
           .discussion-actions
-            %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button", class: ("js-toggle-lazy-diff" unless expanded) }
+            %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button" }
               - if expanded
                 = icon("chevron-up")
               - else

changelogs/unreleased/35475-lazy-diff.yml → changelogs/unreleased/fix-auth0-unsafe-login.yml

 ---
-title: lazy load diffs on merge request discussions
+title: Fix GitLab Auth0 integration signing in the wrong user
 merge_request:
 author:
-type: performance
+type: security

changelogs/unreleased/fj-15329-services-callbacks-ssrf.yml

+---
+title: Fixed some SSRF vulnerabilities in services, hooks and integrations
+merge_request: 2337
+author:
+type: security

changelogs/unreleased/remove-unnecessary-validate-project.yml

----
-title: 'Remove unecessary validate: true from belongs_to :project'
-merge_request:
-author:
-type: fixed

config/routes/project.rb

@@ -132,7 +132,7 @@ constraints(ProjectUrlConstrainer.new) do
           post :bulk_update
         end
 
-        resources :discussions, only: [:show], constraints: { id: /\h{40}/ } do
+        resources :discussions, only: [], constraints: { id: /\h{40}/ } do
           member do
             post :resolve
             delete :resolve, action: :unresolve

db/migrate/20180223120443_create_user_interacted_projects_table.rb

@@ -3,13 +3,15 @@ class CreateUserInteractedProjectsTable < ActiveRecord::Migration
 
   DOWNTIME = false
 
-  disable_ddl_transaction!
+  INDEX_NAME = 'user_interacted_projects_non_unique_index'
 
   def up
     create_table :user_interacted_projects, id: false do |t|
       t.references :user, null: false
       t.references :project, null: false
     end
+
+    add_index :user_interacted_projects, [:project_id, :user_id], name: INDEX_NAME
   end
 
   def down

db/migrate/20180223144945_add_allow_local_requests_from_hooks_and_services_to_application_settings.rb

+class AddAllowLocalRequestsFromHooksAndServicesToApplicationSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:application_settings, :allow_local_requests_from_hooks_and_services,
+                            :boolean,
+                            default: false,
+                            allow_null: false)
+  end
+
+  def down
+    remove_column(:application_settings, :allow_local_requests_from_hooks_and_services)
+  end
+end

db/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb

+class RemoveEmptyExternUidAuth0Identities < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  class Identity < ActiveRecord::Base
+    self.table_name = 'identities'
+    include EachBatch
+  end
+
+  def up
+    broken_auth0_identities.each_batch do |identity|
+      identity.delete_all
+    end
+  end
+
+  def broken_auth0_identities
+    Identity.where(provider: 'auth0', extern_uid: [nil, ''])
+  end
+
+  def down
+  end
+end

db/post_migrate/20180223124427_build_user_interacted_projects_table.rb

+require_relative '../migrate/20180223120443_create_user_interacted_projects_table.rb'
+# rubocop:disable AddIndex
+# rubocop:disable AddConcurrentForeignKey
 class BuildUserInteractedProjectsTable < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
   # Set this constant to true if this migration requires downtime.
   DOWNTIME = false
 
+  UNIQUE_INDEX_NAME = 'index_user_interacted_projects_on_project_id_and_user_id'
+
   disable_ddl_transaction!
 
   def up
@@ -13,16 +18,8 @@ class BuildUserInteractedProjectsTable < ActiveRecord::Migration
       MysqlStrategy.new
     end.up
 
-    unless index_exists?(:user_interacted_projects, [:project_id, :user_id])
-      add_concurrent_index :user_interacted_projects, [:project_id, :user_id], unique: true
-    end
-
-    unless foreign_key_exists?(:user_interacted_projects, :user_id)
-      add_concurrent_foreign_key :user_interacted_projects, :users, column: :user_id, on_delete: :cascade
-    end
-
-    unless foreign_key_exists?(:user_interacted_projects, :project_id)
-      add_concurrent_foreign_key :user_interacted_projects, :projects, column: :project_id, on_delete: :cascade
+    if index_exists_by_name?(:user_interacted_projects, CreateUserInteractedProjectsTable::INDEX_NAME)
+      remove_concurrent_index_by_name :user_interacted_projects, CreateUserInteractedProjectsTable::INDEX_NAME
     end
   end
 
@@ -37,31 +34,16 @@ class BuildUserInteractedProjectsTable < ActiveRecord::Migration
       remove_foreign_key :user_interacted_projects, :projects
     end
 
-    if index_exists_by_name?(:user_interacted_projects, 'index_user_interacted_projects_on_project_id_and_user_id')
-      remove_concurrent_index_by_name :user_interacted_projects, 'index_user_interacted_projects_on_project_id_and_user_id'
+    if index_exists_by_name?(:user_interacted_projects, UNIQUE_INDEX_NAME)
+      remove_concurrent_index_by_name :user_interacted_projects, UNIQUE_INDEX_NAME
     end
-  end
 
-  private
-
-  # Rails' index_exists? doesn't work when you only give it a table and index
-  # name. As such we have to use some extra code to check if an index exists for
-  # a given name.
-  def index_exists_by_name?(table, index)
-    indexes_for_table[table].include?(index)
-  end
-
-  def indexes_for_table
-    @indexes_for_table ||= Hash.new do |hash, table_name|
-      hash[table_name] = indexes(table_name).map(&:name)
+    unless index_exists_by_name?(:user_interacted_projects, CreateUserInteractedProjectsTable::INDEX_NAME)
+      add_concurrent_index :user_interacted_projects, [:project_id, :user_id], name: CreateUserInteractedProjectsTable::INDEX_NAME
     end
   end
 
-  def foreign_key_exists?(table, column)
-    foreign_keys(table).any? do |key|
-      key.options[:column] == column.to_s
-    end
-  end
+  private
 
   class PostgresStrategy < ActiveRecord::Migration
     include Gitlab::Database::MigrationHelpers
@@ -71,33 +53,86 @@ class BuildUserInteractedProjectsTable < ActiveRecord::Migration
 
     def up
       with_index(:events, [:author_id, :project_id], name: 'events_user_interactions_temp', where: 'project_id IS NOT NULL') do
-        iteration = 0
-        records = 0
-        begin
-          Rails.logger.info "Building user_interacted_projects table, batch ##{iteration}"
-          result = execute <<~SQL
+        insert_missing_records
+
+        # Do this once without lock to speed up the second invocation
+        remove_duplicates
+        with_table_lock(:user_interacted_projects) do
+          remove_duplicates
+          create_unique_index
+        end
+
+        remove_without_project
+        with_table_lock(:user_interacted_projects, :projects) do
+          remove_without_project
+          create_fk :user_interacted_projects, :projects, :project_id
+        end
+
+        remove_without_user
+        with_table_lock(:user_interacted_projects, :users) do
+          remove_without_user
+          create_fk :user_interacted_projects, :users, :user_id
+        end
+      end
+
+      execute "ANALYZE user_interacted_projects"
+    end
+
+    private
+    def insert_missing_records
+      iteration = 0
+      records = 0
+      begin
+        Rails.logger.info "Building user_interacted_projects table, batch ##{iteration}"
+        result = execute <<~SQL
             INSERT INTO user_interacted_projects (user_id, project_id)
             SELECT e.user_id, e.project_id
             FROM (SELECT DISTINCT author_id AS user_id, project_id FROM events WHERE project_id IS NOT NULL) AS e
             LEFT JOIN user_interacted_projects ucp USING (user_id, project_id)
             WHERE ucp.user_id IS NULL
             LIMIT #{BATCH_SIZE}
-          SQL
-          iteration += 1
-          records += result.cmd_tuples
-          Rails.logger.info "Building user_interacted_projects table, batch ##{iteration} complete, created #{records} overall"
-          Kernel.sleep(SLEEP_TIME) if result.cmd_tuples > 0
-        rescue ActiveRecord::InvalidForeignKey => e
-          Rails.logger.info "Retry on InvalidForeignKey: #{e}"
-          retry
-        end while result.cmd_tuples > 0
-      end
+        SQL
+        iteration += 1
+        records += result.cmd_tuples
+        Rails.logger.info "Building user_interacted_projects table, batch ##{iteration} complete, created #{records} overall"
+        Kernel.sleep(SLEEP_TIME) if result.cmd_tuples > 0
+      end while result.cmd_tuples > 0
+    end
 
-      execute "ANALYZE user_interacted_projects"
+    def remove_duplicates
+      execute <<~SQL
+        WITH numbered AS (select ctid, ROW_NUMBER() OVER (PARTITION BY (user_id, project_id)) as row_number, user_id, project_id from user_interacted_projects)
+        DELETE FROM user_interacted_projects WHERE ctid IN (SELECT ctid FROM numbered WHERE row_number > 1);
+      SQL
+    end
 
+    def remove_without_project
+      execute "DELETE FROM user_interacted_projects WHERE NOT EXISTS (SELECT 1 FROM projects WHERE id = user_interacted_projects.project_id)"
     end
 
-    private
+    def remove_without_user
+      execute "DELETE FROM user_interacted_projects WHERE NOT EXISTS (SELECT 1 FROM users WHERE id = user_interacted_projects.user_id)"
+    end
+
+    def create_fk(table, target, column)
+      return if foreign_key_exists?(table, column)
+
+      add_foreign_key table, target, column: column, on_delete: :cascade
+    end
+
+    def create_unique_index
+      return if index_exists_by_name?(:user_interacted_projects, UNIQUE_INDEX_NAME)
+
+      add_index :user_interacted_projects, [:project_id, :user_id], unique: true, name: UNIQUE_INDEX_NAME
+    end
+
+    # Protect table against concurrent data changes while still allowing reads
+    def with_table_lock(*tables)
+      ActiveRecord::Base.connection.transaction do
+        execute "LOCK TABLE #{tables.join(", ")} IN SHARE MODE"
+        yield
+      end
+    end
 
     def with_index(*args)
       add_concurrent_index(*args) unless index_exists?(*args)
@@ -118,7 +153,18 @@ class BuildUserInteractedProjectsTable < ActiveRecord::Migration
         LEFT JOIN user_interacted_projects ucp USING (user_id, project_id)
         WHERE ucp.user_id IS NULL
       SQL
+
+      unless index_exists?(:user_interacted_projects, [:project_id, :user_id])
+        add_concurrent_index :user_interacted_projects, [:project_id, :user_id], unique: true, name: UNIQUE_INDEX_NAME
+      end
+
+      unless foreign_key_exists?(:user_interacted_projects, :user_id)
+        add_concurrent_foreign_key :user_interacted_projects, :users, column: :user_id, on_delete: :cascade
+      end
+
+      unless foreign_key_exists?(:user_interacted_projects, :project_id)
+        add_concurrent_foreign_key :user_interacted_projects, :projects, column: :project_id, on_delete: :cascade
+      end
     end
   end
-
 end

db/schema.rb

@@ -157,6 +157,7 @@ ActiveRecord::Schema.define(version: 20180309160427) do
     t.boolean "authorized_keys_enabled", default: true, null: false
     t.string "auto_devops_domain"
     t.boolean "pages_domain_verification_enabled", default: true, null: false
+    t.boolean "allow_local_requests_from_hooks_and_services", default: false, null: false
   end
 
   create_table "audit_events", force: :cascade do |t|

doc/integration/auth0.md

@@ -56,7 +56,8 @@ for initial settings.
           "name" => "auth0",
           "args" => { client_id: 'YOUR_AUTH0_CLIENT_ID',
                       client_secret: 'YOUR_AUTH0_CLIENT_SECRET',
-                      namespace: 'YOUR_AUTH0_DOMAIN'
+                      domain: 'YOUR_AUTH0_DOMAIN',
+                      scope: 'openid profile email'
                     }
         }
       ]
@@ -69,8 +70,8 @@ for initial settings.
           args: {
             client_id: 'YOUR_AUTH0_CLIENT_ID',
             client_secret: 'YOUR_AUTH0_CLIENT_SECRET',
-            namespace: 'YOUR_AUTH0_DOMAIN'
-            }
+            domain: 'YOUR_AUTH0_DOMAIN',
+            scope: 'openid profile email' }
         }
     ```
 

lib/gitlab/database/migration_helpers.rb

@@ -859,6 +859,19 @@ into similar problems in the future (e.g. when new tables are created).
           BackgroundMigrationWorker.perform_in(delay_interval * index, job_class_name, [start_id, end_id])
         end
       end
+
+      def foreign_key_exists?(table, column)
+        foreign_keys(table).any? do |key|
+          key.options[:column] == column.to_s
+        end
+      end
+
+      # Rails' index_exists? doesn't work when you only give it a table and index
+      # name. As such we have to use some extra code to check if an index exists for
+      # a given name.
+      def index_exists_by_name?(table, index)
+        indexes(table).map(&:name).include?(index)
+      end
     end
   end
 end

lib/gitlab/http.rb

+# This class is used as a proxy for all outbounding http connection
+# coming from callbacks, services and hooks. The direct use of the HTTParty
+# is discouraged because it can lead to several security problems, like SSRF
+# calling internal IP or services.
+module Gitlab
+  class HTTP
+    include HTTParty # rubocop:disable Gitlab/HTTParty
+
+    connection_adapter ProxyHTTPConnectionAdapter
+  end
+end

lib/gitlab/proxy_http_connection_adapter.rb

+# This class is part of the Gitlab::HTTP wrapper. Depending on the value
+# of the global setting allow_local_requests_from_hooks_and_services this adapter
+# will allow/block connection to internal IPs and/or urls.
+#
+# This functionality can be overriden by providing the setting the option
+# allow_local_requests = true in the request. For example:
+# Gitlab::HTTP.get('http://www.gitlab.com', allow_local_requests: true)
+#
+# This option will take precedence over the global setting.
+module Gitlab
+  class ProxyHTTPConnectionAdapter < HTTParty::ConnectionAdapter
+    def connection
+      if !allow_local_requests? && blocked_url?
+        raise URI::InvalidURIError
+      end
+
+      super
+    end
+
+    private
+
+    def blocked_url?
+      Gitlab::UrlBlocker.blocked_url?(uri, allow_private_networks: false)
+    end
+
+    def allow_local_requests?
+      options.fetch(:allow_local_requests, allow_settings_local_requests?)
+    end
+
+    def allow_settings_local_requests?
+      Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services?
+    end
+  end
+end

lib/gitlab/url_blocker.rb

@@ -3,11 +3,7 @@ require 'resolv'
 module Gitlab
   class UrlBlocker
     class << self
-      # Used to specify what hosts and port numbers should be prohibited for project
-      # imports.
-      VALID_PORTS = [22, 80, 443].freeze
-
-      def blocked_url?(url)
+      def blocked_url?(url, allow_private_networks: true, valid_ports: [])
         return false if url.nil?
 
         blocked_ips = ["127.0.0.1", "::1", "0.0.0.0"]
@@ -18,12 +14,15 @@ module Gitlab
           # Allow imports from the GitLab instance itself but only from the configured ports
           return false if internal?(uri)
 
-          return true if blocked_port?(uri.port)
+          return true if blocked_port?(uri.port, valid_ports)
           return true if blocked_user_or_hostname?(uri.user)
           return true if blocked_user_or_hostname?(uri.hostname)
 
-          server_ips = Addrinfo.getaddrinfo(uri.hostname, 80, nil, :STREAM).map(&:ip_address)
+          addrs_info = Addrinfo.getaddrinfo(uri.hostname, 80, nil, :STREAM)
+          server_ips = addrs_info.map(&:ip_address)
+
           return true if (blocked_ips & server_ips).any?
+          return true if !allow_private_networks && private_network?(addrs_info)
         rescue Addressable::URI::InvalidURIError
           return true
         rescue SocketError
@@ -35,10 +34,10 @@ module Gitlab
 
       private
 
-      def blocked_port?(port)
-        return false if port.blank?
+      def blocked_port?(port, valid_ports)
+        return false if port.blank? || valid_ports.blank?
 
-        port < 1024 && !VALID_PORTS.include?(port)
+        port < 1024 && !valid_ports.include?(port)
       end
 
       def blocked_user_or_hostname?(value)
@@ -61,6 +60,10 @@ module Gitlab
           (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port)
       end
 
+      def private_network?(addrs_info)
+        addrs_info.any? { |addr| addr.ipv4_private? || addr.ipv6_sitelocal? }
+      end
+
       def config
         Gitlab.config
       end

lib/mattermost/session.rb

@@ -22,16 +22,14 @@ module Mattermost
   # going.
   class Session
     include Doorkeeper::Helpers::Controller
-    include HTTParty
 
     LEASE_TIMEOUT = 60
 
-    base_uri Settings.mattermost.host
-
-    attr_accessor :current_resource_owner, :token
+    attr_accessor :current_resource_owner, :token, :base_uri
 
     def initialize(current_user)
       @current_resource_owner = current_user
+      @base_uri = Settings.mattermost.host
     end
 
     def with_session
@@ -73,18 +71,26 @@ module Mattermost
 
     def get(path, options = {})
       handle_exceptions do
-        self.class.get(path, options.merge(headers: @headers))
+        Gitlab::HTTP.get(path, build_options(options))
       end
     end
 
     def post(path, options = {})
       handle_exceptions do
-        self.class.post(path, options.merge(headers: @headers))
+        Gitlab::HTTP.post(path, build_options(options))
       end
     end
 
     private
 
+    def build_options(options)
+      options.tap do |hash|
+        hash[:headers] = @headers
+        hash[:allow_local_requests] = true
+        hash[:base_uri] = base_uri if base_uri.presence
+      end
+    end
+
     def create
       raise Mattermost::NoSessionError unless oauth_uri
       raise Mattermost::NoSessionError unless token_uri
@@ -159,14 +165,14 @@ module Mattermost
 
     def handle_exceptions
       yield
-    rescue HTTParty::Error => e
+    rescue Gitlab::HTTP::Error => e
       raise Mattermost::ConnectionError.new(e.message)
     rescue Errno::ECONNREFUSED => e
       raise Mattermost::ConnectionError.new(e.message)
     end
 
     def parse_cookie(response)
-      cookie_hash = CookieHash.new
+      cookie_hash = Gitlab::HTTP::CookieHash.new
       response.get_fields('Set-Cookie').each { |c| cookie_hash.add_cookies(c) }
       cookie_hash
     end

lib/microsoft_teams/notifier.rb

@@ -9,14 +9,15 @@ module MicrosoftTeams
       result = false
 
       begin
-        response = HTTParty.post(
+        response = Gitlab::HTTP.post(
           @webhook.to_str,
           headers: @header,
+          allow_local_requests: true,
           body: body(options)
         )
 
         result = true if response
-      rescue HTTParty::Error, StandardError => error
+      rescue Gitlab::HTTP::Error, StandardError => error
         Rails.logger.info("#{self.class.name}: Error while connecting to #{@webhook}: #{error.message}")
       end
 

rubocop/cop/gitlab/httparty.rb

+require_relative '../../spec_helpers'
+
+module RuboCop
+  module Cop
+    module Gitlab
+      class HTTParty < RuboCop::Cop::Cop
+        include SpecHelpers
+
+        MSG_SEND = <<~EOL.freeze
+          Avoid calling `HTTParty` directly. Instead, use the Gitlab::HTTP
+          wrapper. To allow request to localhost or the private network set
+          the option :allow_local_requests in the request call.
+        EOL
+
+        MSG_INCLUDE = <<~EOL.freeze
+          Avoid including `HTTParty` directly. Instead, use the Gitlab::HTTP
+          wrapper. To allow request to localhost or the private network set
+          the option :allow_local_requests in the request call.
+        EOL
+
+        def_node_matcher :includes_httparty?, <<~PATTERN
+          (send nil? :include (const nil? :HTTParty))
+        PATTERN
+
+        def_node_matcher :httparty_node?, <<~PATTERN
+          (send (const nil? :HTTParty)...)
+        PATTERN
+
+        def on_send(node)
+          return if in_spec?(node)
+
+          add_offense(node, location: :expression, message: MSG_SEND) if httparty_node?(node)
+          add_offense(node, location: :expression, message: MSG_INCLUDE) if includes_httparty?(node)
+        end
+
+        def autocorrect(node)
+          if includes_httparty?(node)
+            autocorrect_includes_httparty(node)
+          else
+            autocorrect_httparty_node(node)
+          end
+        end
+
+        def autocorrect_includes_httparty(node)
+          lambda do |corrector|
+            corrector.remove(node.source_range)
+          end
+        end
+
+        def autocorrect_httparty_node(node)
+          _, method_name, *arg_nodes = *node
+
+          replacement = "Gitlab::HTTP.#{method_name}(#{arg_nodes.map(&:source).join(', ')})"
+
+          lambda do |corrector|
+            corrector.replace(node.source_range, replacement)
+          end
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

 require_relative 'cop/gitlab/module_with_instance_variables'
 require_relative 'cop/gitlab/predicate_memoization'
+require_relative 'cop/gitlab/httparty'
 require_relative 'cop/include_sidekiq_worker'
 require_relative 'cop/line_break_around_conditional_block'
 require_relative 'cop/migration/add_column'

spec/controllers/omniauth_callbacks_controller_spec.rb

@@ -3,73 +3,90 @@ require 'spec_helper'
 describe OmniauthCallbacksController do
   include LoginHelpers
 
-  let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: provider) }
-  let(:provider) { :github }
+  let(:user) { create(:omniauth_user, extern_uid: extern_uid, provider: provider) }
 
   before do
-    mock_auth_hash(provider.to_s, 'my-uid', user.email)
+    mock_auth_hash(provider.to_s, extern_uid, user.email)
     stub_omniauth_provider(provider, context: request)
   end
 
-  it 'allows sign in' do
-    post provider
+  context 'github' do
+    let(:extern_uid) { 'my-uid' }
+    let(:provider) { :github }
 
-    expect(request.env['warden']).to be_authenticated
-  end
+    it 'allows sign in' do
+      post provider
+
+      expect(request.env['warden']).to be_authenticated
+    end
 
-  shared_context 'sign_up' do
-    let(:user) { double(email: 'new@example.com') }
+    shared_context 'sign_up' do
+      let(:user) { double(email: 'new@example.com') }
 
-    before do
-      stub_omniauth_setting(block_auto_created_users: false)
+      before do
+        stub_omniauth_setting(block_auto_created_users: false)
+      end
     end
-  end
 
-  context 'sign up' do
-    include_context 'sign_up'
+    context 'sign up' do
+      include_context 'sign_up'
 
-    it 'is allowed' do
-      post provider
+      it 'is allowed' do
+        post provider
 
-      expect(request.env['warden']).to be_authenticated
+        expect(request.env['warden']).to be_authenticated
+      end
     end
-  end
 
-  context 'when OAuth is disabled' do
-    before do
-      stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
-      settings = Gitlab::CurrentSettings.current_application_settings
-      settings.update(disabled_oauth_sign_in_sources: [provider.to_s])
-    end
+    context 'when OAuth is disabled' do
+      before do
+        stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+        settings = Gitlab::CurrentSettings.current_application_settings
+        settings.update(disabled_oauth_sign_in_sources: [provider.to_s])
+      end
 
-    it 'prevents login via POST' do
-      post provider
+      it 'prevents login via POST' do
+        post provider
 
-      expect(request.env['warden']).not_to be_authenticated
-    end
+        expect(request.env['warden']).not_to be_authenticated
+      end
 
-    it 'shows warning when attempting login' do
-      post provider
+      it 'shows warning when attempting login' do
+        post provider
 
-      expect(response).to redirect_to new_user_session_path
-      expect(flash[:alert]).to eq('Signing in using GitHub has been disabled')
-    end
+        expect(response).to redirect_to new_user_session_path
+        expect(flash[:alert]).to eq('Signing in using GitHub has been disabled')
+      end
 
-    it 'allows linking the disabled provider' do
-      user.identities.destroy_all
-      sign_in(user)
+      it 'allows linking the disabled provider' do
+        user.identities.destroy_all
+        sign_in(user)
 
-      expect { post provider }.to change { user.reload.identities.count }.by(1)
-    end
+        expect { post provider }.to change { user.reload.identities.count }.by(1)
+      end
 
-    context 'sign up' do
-      include_context 'sign_up'
+      context 'sign up' do
+        include_context 'sign_up'
 
-      it 'is prevented' do
-        post provider
+        it 'is prevented' do
+          post provider
 
-        expect(request.env['warden']).not_to be_authenticated
+          expect(request.env['warden']).not_to be_authenticated
+        end
       end
     end
   end
+
+  context 'auth0' do
+    let(:extern_uid) { '' }
+    let(:provider) { :auth0 }
+
+    it 'does not allow sign in without extern_uid' do
+      post 'auth0'
+
+      expect(request.env['warden']).not_to be_authenticated
+      expect(response.status).to eq(302)
+      expect(controller).to set_flash[:alert].to('Wrong extern UID provided. Make sure Auth0 is configured correctly.')
+    end
+  end
 end

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -108,7 +108,6 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
           it 'shows resolved discussion when toggled' do
             find(".timeline-content .discussion[data-discussion-id='#{note.discussion_id}'] .discussion-toggle-button").click
 
-            expect(page.find(".line-holder-placeholder")).to be_visible
             expect(page.find(".timeline-content #note_#{note.id}")).to be_visible
           end
         end

spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb

@@ -5,18 +5,15 @@ describe 'Merge request > User scrolls to note on load', :js do
   let(:user) { project.creator }
   let(:merge_request) { create(:merge_request, source_project: project, author: user) }
   let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
-  let(:resolved_note) { create(:diff_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
   let(:fragment_id) { "#note_#{note.id}" }
-  let(:collapsed_fragment_id) { "#note_#{resolved_note.id}" }
 
   before do
     sign_in(user)
     page.current_window.resize_to(1000, 300)
-  end
-
-  it 'scrolls note into view' do
     visit "#{project_merge_request_path(project, merge_request)}#{fragment_id}"
+  end
 
+  it 'scrolls down to fragment' do
     page_height = page.current_window.size[1]
     page_scroll_y = page.evaluate_script("window.scrollY")
     fragment_position_top = page.evaluate_script("Math.round($('#{fragment_id}').offset().top)")
@@ -26,13 +23,4 @@ describe 'Merge request > User scrolls to note on load', :js do
     expect(fragment_position_top).to be >= page_scroll_y
     expect(fragment_position_top).to be < (page_scroll_y + page_height)
   end
-
-  it 'expands collapsed notes' do
-    visit "#{project_merge_request_path(project, merge_request)}#{collapsed_fragment_id}"
-    note_element = find(collapsed_fragment_id)
-    note_container = note_element.ancestor('.js-toggle-container')
-
-    expect(note_element.visible?).to eq true
-    expect(note_container.find('.line_content.noteable_line.old', match: :first).visible?).to eq true
-  end
 end

spec/lib/gitlab/http_spec.rb

+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

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

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

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

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

spec/migrations/remove_empty_extern_uid_auth0_identities_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20180220150310_remove_empty_extern_uid_auth0_identities.rb')
+
+describe RemoveEmptyExternUidAuth0Identities, :migration do
+  let(:identities) { table(:identities) }
+
+  before do
+    identities.create(provider: 'auth0', extern_uid: '')
+    identities.create(provider: 'auth0', extern_uid: 'valid')
+    identities.create(provider: 'github', extern_uid: '')
+
+    migrate!
+  end
+
+  it 'leaves the correct auth0 identity' do
+    expect(identities.where(provider: 'auth0').pluck(:extern_uid)).to eq(['valid'])
+  end
+
+  it 'leaves the correct github identity' do
+    expect(identities.where(provider: 'github').count).to eq(1)
+  end
+end

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

spec/rubocop/cop/gitlab/httparty_spec.rb

+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

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]