diff options
author | Robert Speicher <robert@gitlab.com> | 2018-03-16 18:43:44 +0000 |
---|---|---|
committer | Robert Speicher <robert@gitlab.com> | 2018-03-16 18:43:44 +0000 |
commit | 9d9393d50e326e3360398908ce41d04bbbc9fbff (patch) | |
tree | f19e32048db3f42f6cbe2cab80135cfc2e6f7e15 | |
parent | a99baf5e1fc8d28ca0637c0cd5b13a54832f866b (diff) | |
parent | 1e21892f753aee9e6ec0ef612b53db07763bf03c (diff) | |
download | gitlab-ce-9d9393d50e326e3360398908ce41d04bbbc9fbff.tar.gz |
Merge branch '10-6-stable-rc6-critical' into '10-6-stable'
Preparation MR for 10.6RC6
See merge request gitlab/gitlabhq!2358
59 files changed, 666 insertions, 300 deletions
diff --git a/.rubocop.yml b/.rubocop.yml index 293f61fb725..90dac884538 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -46,6 +46,9 @@ Gitlab/ModuleWithInstanceVariables: - spec/support/**/*.rb - features/steps/**/*.rb +Gitlab/HTTParty: + Enabled: true + GitlabSecurity/PublicSend: Enabled: true Exclude: @@ -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' diff --git a/Gemfile.lock b/Gemfile.lock index fa99ec3febe..8957f1f3867 100644 --- a/Gemfile.lock +++ b/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) diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 6d1b2f452c0..c640003d958 100644 --- a/app/assets/javascripts/notes.js +++ b/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'); diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 8440945ab43..fff249577a2 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/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" diff --git a/app/controllers/projects/discussions_controller.rb b/app/controllers/projects/discussions_controller.rb index cba9a53dc4b..ee507009e50 100644 --- a/app/controllers/projects/discussions_controller.rb +++ b/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 diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 4c4d7cca8a5..63e4a5dc45c 100644 --- a/app/helpers/application_settings_helper.rb +++ b/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 diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 0dee6df525d..4281d7d267f 100644 --- a/app/models/application_setting.rb +++ b/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 diff --git a/app/models/project.rb b/app/models/project.rb index 5f9d9785d64..43e006ad3ee 100644 --- a/app/models/project.rb +++ b/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?, diff --git a/app/models/project_services/assembla_service.rb b/app/models/project_services/assembla_service.rb index ae6af732ed4..4234b8044e5 100644 --- a/app/models/project_services/assembla_service.rb +++ b/app/models/project_services/assembla_service.rb @@ -1,6 +1,4 @@ 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 diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb index 42939ea0ec8..54e4b3278db 100644 --- a/app/models/project_services/bamboo_service.rb +++ b/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 diff --git a/app/models/project_services/buildkite_service.rb b/app/models/project_services/buildkite_service.rb index fc30f6e3365..d2aaff8817a 100644 --- a/app/models/project_services/buildkite_service.rb +++ b/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'] diff --git a/app/models/project_services/campfire_service.rb b/app/models/project_services/campfire_service.rb index 8d7a4fceb08..cb4af73807b 100644 --- a/app/models/project_services/campfire_service.rb +++ b/app/models/project_services/campfire_service.rb @@ -1,6 +1,4 @@ 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 diff --git a/app/models/project_services/drone_ci_service.rb b/app/models/project_services/drone_ci_service.rb index c93f1632652..71b10fc6bc1 100644 --- a/app/models/project_services/drone_ci_service.rb +++ b/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'] diff --git a/app/models/project_services/external_wiki_service.rb b/app/models/project_services/external_wiki_service.rb index 720ad61162e..1553f169827 100644 --- a/app/models/project_services/external_wiki_service.rb +++ b/app/models/project_services/external_wiki_service.rb @@ -1,6 +1,4 @@ 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 diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb index 5fb15c383ca..df6dcd90985 100644 --- a/app/models/project_services/issue_tracker_service.rb +++ b/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) diff --git a/app/models/project_services/mock_ci_service.rb b/app/models/project_services/mock_ci_service.rb index 72ddf9a4be3..2221459c90b 100644 --- a/app/models/project_services/mock_ci_service.rb +++ b/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 diff --git a/app/models/project_services/packagist_service.rb b/app/models/project_services/packagist_service.rb index f68a0c1a3c3..ba62a5b7ac0 100644 --- a/app/models/project_services/packagist_service.rb +++ b/app/models/project_services/packagist_service.rb @@ -1,6 +1,4 @@ class PackagistService < Service - include HTTParty - prop_accessor :username, :token, :server validates :username, presence: true, if: :activated? diff --git a/app/models/project_services/pivotaltracker_service.rb b/app/models/project_services/pivotaltracker_service.rb index f9dfa2e91c3..3476e7d2283 100644 --- a/app/models/project_services/pivotaltracker_service.rb +++ b/app/models/project_services/pivotaltracker_service.rb @@ -1,6 +1,4 @@ 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: { diff --git a/app/models/project_services/pushover_service.rb b/app/models/project_services/pushover_service.rb index e3a1ca2d45f..8777a44b72f 100644 --- a/app/models/project_services/pushover_service.rb +++ b/app/models/project_services/pushover_service.rb @@ -1,6 +1,5 @@ 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 diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb index cbe137452bd..145313b8e71 100644 --- a/app/models/project_services/teamcity_service.rb +++ b/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 diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index f2d676af5c3..a34024f4f80 100644 --- a/app/services/projects/import_service.rb +++ b/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, diff --git a/app/services/submit_usage_ping_service.rb b/app/services/submit_usage_ping_service.rb index 2623f253d98..ac029fad7ea 100644 --- a/app/services/submit_usage_ping_service.rb +++ b/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 diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 36e589d5aa8..809ce1303d8 100644 --- a/app/services/web_hook_service.rb +++ b/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 diff --git a/app/validators/importable_url_validator.rb b/app/validators/importable_url_validator.rb index 37a314adee6..3ec1594e202 100644 --- a/app/validators/importable_url_validator.rb +++ b/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 diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 81d7db04a3c..54b39df8cf3 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/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' diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 8680ec2e298..f9bfc01f213 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/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 } diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml index e9589213f80..8b9fa3d6b05 100644 --- a/app/views/discussions/_discussion.html.haml +++ b/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 diff --git a/changelogs/unreleased/35475-lazy-diff.yml b/changelogs/unreleased/35475-lazy-diff.yml deleted file mode 100644 index bafa66ebe39..00000000000 --- a/changelogs/unreleased/35475-lazy-diff.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: lazy load diffs on merge request discussions -merge_request: -author: -type: performance diff --git a/changelogs/unreleased/fix-auth0-unsafe-login.yml b/changelogs/unreleased/fix-auth0-unsafe-login.yml new file mode 100644 index 00000000000..01c6ea69dcc --- /dev/null +++ b/changelogs/unreleased/fix-auth0-unsafe-login.yml @@ -0,0 +1,5 @@ +--- +title: Fix GitLab Auth0 integration signing in the wrong user +merge_request: +author: +type: security diff --git a/changelogs/unreleased/fj-15329-services-callbacks-ssrf.yml b/changelogs/unreleased/fj-15329-services-callbacks-ssrf.yml new file mode 100644 index 00000000000..7fa6f6a5874 --- /dev/null +++ b/changelogs/unreleased/fj-15329-services-callbacks-ssrf.yml @@ -0,0 +1,5 @@ +--- +title: Fixed some SSRF vulnerabilities in services, hooks and integrations +merge_request: 2337 +author: +type: security diff --git a/changelogs/unreleased/remove-unnecessary-validate-project.yml b/changelogs/unreleased/remove-unnecessary-validate-project.yml deleted file mode 100644 index ebc8da03dd8..00000000000 --- a/changelogs/unreleased/remove-unnecessary-validate-project.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: 'Remove unecessary validate: true from belongs_to :project' -merge_request: -author: -type: fixed diff --git a/config/routes/project.rb b/config/routes/project.rb index 24a87b44fc7..710fe0ec325 100644 --- a/config/routes/project.rb +++ b/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 diff --git a/db/migrate/20180223120443_create_user_interacted_projects_table.rb b/db/migrate/20180223120443_create_user_interacted_projects_table.rb index 20749940b1e..8da8cf68088 100644 --- a/db/migrate/20180223120443_create_user_interacted_projects_table.rb +++ b/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 diff --git a/db/migrate/20180223144945_add_allow_local_requests_from_hooks_and_services_to_application_settings.rb b/db/migrate/20180223144945_add_allow_local_requests_from_hooks_and_services_to_application_settings.rb new file mode 100644 index 00000000000..c994a54698b --- /dev/null +++ b/db/migrate/20180223144945_add_allow_local_requests_from_hooks_and_services_to_application_settings.rb @@ -0,0 +1,18 @@ +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 diff --git a/db/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb b/db/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb new file mode 100644 index 00000000000..2d5a8617169 --- /dev/null +++ b/db/post_migrate/20180220150310_remove_empty_extern_uid_auth0_identities.rb @@ -0,0 +1,25 @@ +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 diff --git a/db/post_migrate/20180223124427_build_user_interacted_projects_table.rb b/db/post_migrate/20180223124427_build_user_interacted_projects_table.rb index 5e729b1aa53..d1a29a5c71b 100644 --- a/db/post_migrate/20180223124427_build_user_interacted_projects_table.rb +++ b/db/post_migrate/20180223124427_build_user_interacted_projects_table.rb @@ -1,9 +1,14 @@ +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 diff --git a/db/schema.rb b/db/schema.rb index ab4370e2754..70d4a3b6de9 100644 --- a/db/schema.rb +++ b/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| diff --git a/doc/integration/auth0.md b/doc/integration/auth0.md index c39d7ab57c6..a75836a915a 100644 --- a/doc/integration/auth0.md +++ b/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' } } ``` diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index dbe6259fce7..21287a8efd0 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/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 diff --git a/lib/gitlab/http.rb b/lib/gitlab/http.rb new file mode 100644 index 00000000000..96558872a37 --- /dev/null +++ b/lib/gitlab/http.rb @@ -0,0 +1,11 @@ +# 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 diff --git a/lib/gitlab/proxy_http_connection_adapter.rb b/lib/gitlab/proxy_http_connection_adapter.rb new file mode 100644 index 00000000000..c70d6f4cd84 --- /dev/null +++ b/lib/gitlab/proxy_http_connection_adapter.rb @@ -0,0 +1,34 @@ +# 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 diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 13150ddab67..0f9f939e204 100644 --- a/lib/gitlab/url_blocker.rb +++ b/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 diff --git a/lib/mattermost/session.rb b/lib/mattermost/session.rb index ef08bd46e17..00e461ed61a 100644 --- a/lib/mattermost/session.rb +++ b/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 diff --git a/lib/microsoft_teams/notifier.rb b/lib/microsoft_teams/notifier.rb index 3bef68a1bcb..c08d3e933a8 100644 --- a/lib/microsoft_teams/notifier.rb +++ b/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 diff --git a/rubocop/cop/gitlab/httparty.rb b/rubocop/cop/gitlab/httparty.rb new file mode 100644 index 00000000000..215f18b6993 --- /dev/null +++ b/rubocop/cop/gitlab/httparty.rb @@ -0,0 +1,62 @@ +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 diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 9110237c538..530fe3409fb 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -1,5 +1,6 @@ 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' diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index c639ad32ec6..9fd129e4ee9 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/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 diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb index b4ad4b64d8e..3e83a549682 100644 --- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb +++ b/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 diff --git a/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb b/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb index 565e375600b..8a834adbf17 100644 --- a/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb new file mode 100644 index 00000000000..b0bc081a3c8 --- /dev/null +++ b/spec/lib/gitlab/http_spec.rb @@ -0,0 +1,49 @@ +require 'spec_helper' + +describe Gitlab::HTTP do + describe 'allow_local_requests_from_hooks_and_services is' do + before do + WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success') + end + + context 'disabled' do + before do + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false) + end + + it 'deny requests to localhost' do + expect { described_class.get('http://localhost:3003') }.to raise_error(URI::InvalidURIError) + end + + it 'deny requests to private network' do + expect { described_class.get('http://192.168.1.2:3003') }.to raise_error(URI::InvalidURIError) + end + + context 'if allow_local_requests set to true' do + it 'override the global value and allow requests to localhost or private network' do + expect { described_class.get('http://localhost:3003', allow_local_requests: true) }.not_to raise_error + end + end + end + + context 'enabled' do + before do + allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true) + end + + it 'allow requests to localhost' do + expect { described_class.get('http://localhost:3003') }.not_to raise_error + end + + it 'allow requests to private network' do + expect { described_class.get('http://192.168.1.2:3003') }.not_to raise_error + end + + context 'if allow_local_requests set to false' do + it 'override the global value and ban requests to localhost or private network' do + expect { described_class.get('http://localhost:3003', allow_local_requests: false) }.to raise_error(URI::InvalidURIError) + end + end + end + end +end diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index d9b3c2350b1..2d35b026485 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -2,6 +2,8 @@ require 'spec_helper' describe Gitlab::UrlBlocker do describe '#blocked_url?' do + let(:valid_ports) { Project::VALID_IMPORT_PORTS } + it 'allows imports from configured web host and port' do import_url = "http://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git" expect(described_class.blocked_url?(import_url)).to be false @@ -17,7 +19,7 @@ describe Gitlab::UrlBlocker do end it 'returns true for bad port' do - expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git')).to be true + expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git', valid_ports: valid_ports)).to be true end it 'returns true for alternative version of 127.0.0.1 (0177.1)' do @@ -71,6 +73,47 @@ describe Gitlab::UrlBlocker do it 'returns false for legitimate URL' do expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false end + + context 'when allow_private_networks is' do + let(:private_networks) { ['192.168.1.2', '10.0.0.2', '172.16.0.2'] } + let(:fake_domain) { 'www.fakedomain.fake' } + + context 'true (default)' do + it 'does not block urls from private networks' do + private_networks.each do |ip| + stub_domain_resolv(fake_domain, ip) + + expect(described_class).not_to be_blocked_url("http://#{fake_domain}") + + unstub_domain_resolv + + expect(described_class).not_to be_blocked_url("http://#{ip}") + end + end + end + + context 'false' do + it 'blocks urls from private networks' do + private_networks.each do |ip| + stub_domain_resolv(fake_domain, ip) + + expect(described_class).to be_blocked_url("http://#{fake_domain}", allow_private_networks: false) + + unstub_domain_resolv + + expect(described_class).to be_blocked_url("http://#{ip}", allow_private_networks: false) + end + end + end + + def stub_domain_resolv(domain, ip) + allow(Addrinfo).to receive(:getaddrinfo).with(domain, any_args).and_return([double(ip_address: ip, ipv4_private?: true)]) + end + + def unstub_domain_resolv + allow(Addrinfo).to receive(:getaddrinfo).and_call_original + end + end end # Resolv does not support resolving UTF-8 domain names diff --git a/spec/lib/mattermost/command_spec.rb b/spec/lib/mattermost/command_spec.rb index 369e7b181b9..8ba15ae0f38 100644 --- a/spec/lib/mattermost/command_spec.rb +++ b/spec/lib/mattermost/command_spec.rb @@ -4,10 +4,11 @@ describe Mattermost::Command do let(:params) { { 'token' => 'token', team_id: 'abc' } } before do - Mattermost::Session.base_uri('http://mattermost.example.com') + session = Mattermost::Session.new(nil) + session.base_uri = 'http://mattermost.example.com' allow_any_instance_of(Mattermost::Client).to receive(:with_session) - .and_yield(Mattermost::Session.new(nil)) + .and_yield(session) end describe '#create' do diff --git a/spec/lib/mattermost/session_spec.rb b/spec/lib/mattermost/session_spec.rb index 3db19d06305..c855643c4d8 100644 --- a/spec/lib/mattermost/session_spec.rb +++ b/spec/lib/mattermost/session_spec.rb @@ -15,7 +15,7 @@ describe Mattermost::Session, type: :request do it { is_expected.to respond_to(:strategy) } before do - described_class.base_uri(mattermost_url) + subject.base_uri = mattermost_url end describe '#with session' do diff --git a/spec/lib/mattermost/team_spec.rb b/spec/lib/mattermost/team_spec.rb index e638ad7a2c9..9795a0c3e39 100644 --- a/spec/lib/mattermost/team_spec.rb +++ b/spec/lib/mattermost/team_spec.rb @@ -2,10 +2,11 @@ require 'spec_helper' describe Mattermost::Team do before do - Mattermost::Session.base_uri('http://mattermost.example.com') + session = Mattermost::Session.new(nil) + session.base_uri = 'http://mattermost.example.com' allow_any_instance_of(Mattermost::Client).to receive(:with_session) - .and_yield(Mattermost::Session.new(nil)) + .and_yield(session) end describe '#all' do diff --git a/spec/migrations/remove_empty_extern_uid_auth0_identities_spec.rb b/spec/migrations/remove_empty_extern_uid_auth0_identities_spec.rb new file mode 100644 index 00000000000..441c4295a40 --- /dev/null +++ b/spec/migrations/remove_empty_extern_uid_auth0_identities_spec.rb @@ -0,0 +1,22 @@ +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 diff --git a/spec/models/project_services/mattermost_slash_commands_service_spec.rb b/spec/models/project_services/mattermost_slash_commands_service_spec.rb index a5bdf9a9337..05d33cd3874 100644 --- a/spec/models/project_services/mattermost_slash_commands_service_spec.rb +++ b/spec/models/project_services/mattermost_slash_commands_service_spec.rb @@ -9,10 +9,11 @@ describe MattermostSlashCommandsService do let(:user) { create(:user) } before do - Mattermost::Session.base_uri("http://mattermost.example.com") + session = Mattermost::Session.new(nil) + session.base_uri = 'http://mattermost.example.com' allow_any_instance_of(Mattermost::Client).to receive(:with_session) - .and_yield(Mattermost::Session.new(nil)) + .and_yield(session) end describe '#configure' do diff --git a/spec/rubocop/cop/gitlab/httparty_spec.rb b/spec/rubocop/cop/gitlab/httparty_spec.rb new file mode 100644 index 00000000000..510839a21d7 --- /dev/null +++ b/spec/rubocop/cop/gitlab/httparty_spec.rb @@ -0,0 +1,74 @@ +require 'spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/gitlab/httparty' + +describe RuboCop::Cop::Gitlab::HTTParty do # rubocop:disable RSpec/FilePath + include CopHelper + + subject(:cop) { described_class.new } + + shared_examples('registering include offense') do |options| + let(:offending_lines) { options[:offending_lines] } + + it 'registers an offense when the class includes HTTParty' do + inspect_source(source) + + aggregate_failures do + expect(cop.offenses.size).to eq(offending_lines.size) + expect(cop.offenses.map(&:line)).to eq(offending_lines) + end + end + end + + shared_examples('registering call offense') do |options| + let(:offending_lines) { options[:offending_lines] } + + it 'registers an offense when the class calls HTTParty' do + inspect_source(source) + + aggregate_failures do + expect(cop.offenses.size).to eq(offending_lines.size) + expect(cop.offenses.map(&:line)).to eq(offending_lines) + end + end + end + + context 'when source is a regular module' do + it_behaves_like 'registering include offense', offending_lines: [2] do + let(:source) do + <<~RUBY + module M + include HTTParty + end + RUBY + end + end + end + + context 'when source is a regular class' do + it_behaves_like 'registering include offense', offending_lines: [2] do + let(:source) do + <<~RUBY + class Foo + include HTTParty + end + RUBY + end + end + end + + context 'when HTTParty is called' do + it_behaves_like 'registering call offense', offending_lines: [3] do + let(:source) do + <<~RUBY + class Foo + def bar + HTTParty.get('http://example.com') + end + end + RUBY + end + end + end +end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 21910e69d2e..2ef2e61babc 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -14,6 +14,20 @@ describe WebHookService do end let(:service_instance) { described_class.new(project_hook, data, :push_hooks) } + describe '#initialize' do + it 'allow_local_requests is true if hook is a SystemHook' do + instance = described_class.new(build(:system_hook), data, :system_hook) + expect(instance.request_options[:allow_local_requests]).to be_truthy + end + + it 'allow_local_requests is false if hook is not a SystemHook' do + %i(project_hook service_hook web_hook_log).each do |hook| + instance = described_class.new(build(hook), data, hook) + expect(instance.request_options[:allow_local_requests]).to be_falsey + end + end + end + describe '#execute' do before do project.hooks << [project_hook] |