From d00d60a66deeacb19ccbd39501946ed646db64b6 Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Tue, 16 Jul 2019 14:20:52 +1000 Subject: Allow UsageData.count to use count_by: --- lib/gitlab/usage_data.rb | 4 ++-- spec/lib/gitlab/usage_data_spec.rb | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb index 7572c0bdbfd..f02d9f7d7e7 100644 --- a/lib/gitlab/usage_data.rb +++ b/lib/gitlab/usage_data.rb @@ -176,8 +176,8 @@ module Gitlab {} # augmented in EE end - def count(relation, fallback: -1) - relation.count + def count(relation, count_by: nil, fallback: -1) + count_by ? relation.count(count_by) : relation.count rescue ActiveRecord::StatementInvalid fallback end diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb index 90a534de202..7fe60fd214c 100644 --- a/spec/lib/gitlab/usage_data_spec.rb +++ b/spec/lib/gitlab/usage_data_spec.rb @@ -234,6 +234,12 @@ describe Gitlab::UsageData do expect(described_class.count(relation)).to eq(1) end + it 'returns the count for count_by when provided' do + allow(relation).to receive(:count).with(:creator_id).and_return(2) + + expect(described_class.count(relation, count_by: :creator_id)).to eq(2) + end + it 'returns the fallback value when counting fails' do allow(relation).to receive(:count).and_raise(ActiveRecord::StatementInvalid.new('')) -- cgit v1.2.1 From f53112de5931144ac74819f87c227f06e115ba58 Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Thu, 18 Jul 2019 12:09:30 +1000 Subject: New GroupMember.of_ldap_type scope --- app/models/members/group_member.rb | 2 +- spec/factories/group_members.rb | 4 ++++ spec/models/members/group_member_spec.rb | 36 ++++++++++++++++++++------------ 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 4cba69069bb..341d2fe2149 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -13,8 +13,8 @@ class GroupMember < Member default_scope { where(source_type: SOURCE_TYPE) } scope :of_groups, ->(groups) { where(source_id: groups.select(:id)) } - scope :count_users_by_group_id, -> { joins(:user).group(:source_id).count } + scope :of_ldap_type, -> { where(ldap: true) } after_create :update_two_factor_requirement, unless: :invite? after_destroy :update_two_factor_requirement, unless: :invite? diff --git a/spec/factories/group_members.rb b/spec/factories/group_members.rb index 077c6ddc5ae..0cf99a31d20 100644 --- a/spec/factories/group_members.rb +++ b/spec/factories/group_members.rb @@ -16,5 +16,9 @@ FactoryBot.define do invite_token 'xxx' invite_email 'email@email.com' end + + trait(:ldap) do + ldap true + end end end diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index f227abd3dae..a6928ec31f4 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -3,19 +3,29 @@ require 'spec_helper' describe GroupMember do - describe '.count_users_by_group_id' do - it 'counts users by group ID' do - user_1 = create(:user) - user_2 = create(:user) - group_1 = create(:group) - group_2 = create(:group) - - group_1.add_owner(user_1) - group_1.add_owner(user_2) - group_2.add_owner(user_1) - - expect(described_class.count_users_by_group_id).to eq(group_1.id => 2, - group_2.id => 1) + context 'scopes' do + describe '.count_users_by_group_id' do + it 'counts users by group ID' do + user_1 = create(:user) + user_2 = create(:user) + group_1 = create(:group) + group_2 = create(:group) + + group_1.add_owner(user_1) + group_1.add_owner(user_2) + group_2.add_owner(user_1) + + expect(described_class.count_users_by_group_id).to eq(group_1.id => 2, + group_2.id => 1) + end + end + + describe '.of_ldap_type' do + it 'returns ldap type users' do + group_member = create(:group_member, :ldap) + + expect(described_class.of_ldap_type).to eq([group_member]) + end end end -- cgit v1.2.1 From dfcf4cf5f1e87a29f0d9fcc5ff2bba47258893bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Thu, 18 Jul 2019 10:27:02 +0200 Subject: Add captcha if there are multiple failed login attempts Add method to store session ids by ip Add new specs for storing session ids Add cleaning up records after login Add retrieving anonymous sessions Add login recaptcha setting Add new setting to sessions controller Add conditions for showing captcha Add sessions controller specs Add admin settings specs for login protection Add new settings to api Add stub to devise spec Add new translation key Add cr remarks Rename class call Add cr remarks Change if-clause for consistency Add cr remarks Add code review remarks Refactor AnonymousSession class Add changelog entry Move AnonymousSession class to lib Move store unauthenticated sessions to sessions controller Move link to recaptcha info Regenerate text file Improve copy on the spam page Change action filter for storing anonymous sessions Fix rubocop offences Add code review remarks --- app/controllers/sessions_controller.rb | 44 ++++++++- app/helpers/application_settings_helper.rb | 1 + app/models/application_setting.rb | 8 +- app/models/application_setting_implementation.rb | 1 + .../admin/application_settings/_spam.html.haml | 13 ++- .../admin/application_settings/reporting.html.haml | 4 +- app/views/devise/sessions/_new_base.html.haml | 2 +- ...security-59549-add-capcha-for-failed-logins.yml | 5 + config/initializers/warden.rb | 1 + ...a_protection_enabled_to_application_settings.rb | 12 +++ db/schema.rb | 1 + lib/api/settings.rb | 5 + lib/gitlab/anonymous_session.rb | 39 ++++++++ lib/gitlab/recaptcha.rb | 6 +- lib/gitlab/redis/shared_state.rb | 1 + locale/gitlab.pot | 7 +- spec/controllers/sessions_controller_spec.rb | 108 +++++++++++++++++---- spec/features/admin/admin_settings_spec.rb | 2 + spec/lib/gitlab/anonymous_session_spec.rb | 78 +++++++++++++++ .../devise/shared/_signin_box.html.haml_spec.rb | 1 + 20 files changed, 307 insertions(+), 32 deletions(-) create mode 100644 changelogs/unreleased/security-59549-add-capcha-for-failed-logins.yml create mode 100644 db/migrate/20190719122333_add_login_recaptcha_protection_enabled_to_application_settings.rb create mode 100644 lib/gitlab/anonymous_session.rb create mode 100644 spec/lib/gitlab/anonymous_session_spec.rb diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 1880bead3ee..7b682cc0cc5 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -21,10 +21,13 @@ class SessionsController < Devise::SessionsController prepend_before_action :ensure_password_authentication_enabled!, if: -> { action_name == 'create' && password_based_login? } before_action :auto_sign_in_with_provider, only: [:new] + before_action :store_unauthenticated_sessions, only: [:new] + before_action :save_failed_login, if: :action_new_and_failed_login? before_action :load_recaptcha - after_action :log_failed_login, if: -> { action_name == 'new' && failed_login? } - helper_method :captcha_enabled? + after_action :log_failed_login, if: :action_new_and_failed_login? + + helper_method :captcha_enabled?, :captcha_on_login_required? # protect_from_forgery is already prepended in ApplicationController but # authenticate_with_two_factor which signs in the user is prepended before @@ -38,6 +41,7 @@ class SessionsController < Devise::SessionsController protect_from_forgery with: :exception, prepend: true CAPTCHA_HEADER = 'X-GitLab-Show-Login-Captcha'.freeze + MAX_FAILED_LOGIN_ATTEMPTS = 5 def new set_minimum_password_length @@ -81,10 +85,14 @@ class SessionsController < Devise::SessionsController request.headers[CAPTCHA_HEADER] && Gitlab::Recaptcha.enabled? end + def captcha_on_login_required? + Gitlab::Recaptcha.enabled_on_login? && unverified_anonymous_user? + end + # From https://github.com/plataformatec/devise/wiki/How-To:-Use-Recaptcha-with-Devise#devisepasswordscontroller def check_captcha return unless user_params[:password].present? - return unless captcha_enabled? + return unless captcha_enabled? || captcha_on_login_required? return unless Gitlab::Recaptcha.load_configurations! if verify_recaptcha @@ -126,10 +134,28 @@ class SessionsController < Devise::SessionsController Gitlab::AppLogger.info("Failed Login: username=#{user_params[:login]} ip=#{request.remote_ip}") end + def action_new_and_failed_login? + action_name == 'new' && failed_login? + end + + def save_failed_login + session[:failed_login_attempts] ||= 0 + session[:failed_login_attempts] += 1 + end + def failed_login? (options = request.env["warden.options"]) && options[:action] == "unauthenticated" end + # storing sessions per IP lets us check if there are associated multiple + # anonymous sessions with one IP and prevent situations when there are + # multiple attempts of logging in + def store_unauthenticated_sessions + return if current_user + + Gitlab::AnonymousSession.new(request.remote_ip, session_id: request.session.id).store_session_id_per_ip + end + # Handle an "initial setup" state, where there's only one user, it's an admin, # and they require a password change. # rubocop: disable CodeReuse/ActiveRecord @@ -240,6 +266,18 @@ class SessionsController < Devise::SessionsController @ldap_servers ||= Gitlab::Auth::LDAP::Config.available_servers end + def unverified_anonymous_user? + exceeded_failed_login_attempts? || exceeded_anonymous_sessions? + end + + def exceeded_failed_login_attempts? + session.fetch(:failed_login_attempts, 0) > MAX_FAILED_LOGIN_ATTEMPTS + end + + def exceeded_anonymous_sessions? + Gitlab::AnonymousSession.new(request.remote_ip).stored_sessions >= MAX_FAILED_LOGIN_ATTEMPTS + end + def authentication_method if user_params[:otp_attempt] "two-factor" diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 3847a35fbab..f13d69d1c37 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -229,6 +229,7 @@ module ApplicationSettingsHelper :recaptcha_enabled, :recaptcha_private_key, :recaptcha_site_key, + :login_recaptcha_protection_enabled, :receive_max_input_size, :repository_checks_enabled, :repository_storages, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index a769a8f07fd..16e35c1be67 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -73,11 +73,11 @@ class ApplicationSetting < ApplicationRecord validates :recaptcha_site_key, presence: true, - if: :recaptcha_enabled + if: :recaptcha_or_login_protection_enabled validates :recaptcha_private_key, presence: true, - if: :recaptcha_enabled + if: :recaptcha_or_login_protection_enabled validates :akismet_api_key, presence: true, @@ -285,4 +285,8 @@ class ApplicationSetting < ApplicationRecord def self.cache_backend Gitlab::ThreadMemoryCache.cache_backend end + + def recaptcha_or_login_protection_enabled + recaptcha_enabled || login_recaptcha_protection_enabled + end end diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index 1e612bd0e78..b8e6156d231 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -63,6 +63,7 @@ module ApplicationSettingImplementation polling_interval_multiplier: 1, project_export_enabled: true, recaptcha_enabled: false, + login_recaptcha_protection_enabled: false, repository_checks_enabled: true, repository_storages: ['default'], require_two_factor_authentication: false, diff --git a/app/views/admin/application_settings/_spam.html.haml b/app/views/admin/application_settings/_spam.html.haml index d24e46b2815..f0a19075115 100644 --- a/app/views/admin/application_settings/_spam.html.haml +++ b/app/views/admin/application_settings/_spam.html.haml @@ -7,11 +7,15 @@ = f.check_box :recaptcha_enabled, class: 'form-check-input' = f.label :recaptcha_enabled, class: 'form-check-label' do Enable reCAPTCHA - - recaptcha_v2_link_url = 'https://developers.google.com/recaptcha/docs/versions' - - recaptcha_v2_link_start = ''.html_safe % { url: recaptcha_v2_link_url } %span.form-text.text-muted#recaptcha_help_block - = _('Helps prevent bots from creating accounts. We currently only support %{recaptcha_v2_link_start}reCAPTCHA v2%{recaptcha_v2_link_end}').html_safe % { recaptcha_v2_link_start: recaptcha_v2_link_start, recaptcha_v2_link_end: ''.html_safe } - + = _('Helps prevent bots from creating accounts.') + .form-group + .form-check + = f.check_box :login_recaptcha_protection_enabled, class: 'form-check-input' + = f.label :login_recaptcha_protection_enabled, class: 'form-check-label' do + Enable reCAPTCHA for login + %span.form-text.text-muted#recaptcha_help_block + = _('Helps prevent bots from brute-force attacks.') .form-group = f.label :recaptcha_site_key, 'reCAPTCHA Site Key', class: 'label-bold' = f.text_field :recaptcha_site_key, class: 'form-control' @@ -21,6 +25,7 @@ .form-group = f.label :recaptcha_private_key, 'reCAPTCHA Private Key', class: 'label-bold' + .form-group = f.text_field :recaptcha_private_key, class: 'form-control' .form-group diff --git a/app/views/admin/application_settings/reporting.html.haml b/app/views/admin/application_settings/reporting.html.haml index 46e3d1c4570..c60e44b3864 100644 --- a/app/views/admin/application_settings/reporting.html.haml +++ b/app/views/admin/application_settings/reporting.html.haml @@ -9,7 +9,9 @@ %button.btn.btn-default.js-settings-toggle{ type: 'button' } = expanded_by_default? ? _('Collapse') : _('Expand') %p - = _('Enable reCAPTCHA or Akismet and set IP limits.') + - recaptcha_v2_link_url = 'https://developers.google.com/recaptcha/docs/versions' + - recaptcha_v2_link_start = ''.html_safe % { url: recaptcha_v2_link_url } + = _('Enable reCAPTCHA or Akismet and set IP limits. For reCAPTCHA, we currently only support %{recaptcha_v2_link_start}v2%{recaptcha_v2_link_end}').html_safe % { recaptcha_v2_link_start: recaptcha_v2_link_start, recaptcha_v2_link_end: ''.html_safe } .settings-content = render 'spam' diff --git a/app/views/devise/sessions/_new_base.html.haml b/app/views/devise/sessions/_new_base.html.haml index 2f10f08c839..6382849de9c 100644 --- a/app/views/devise/sessions/_new_base.html.haml +++ b/app/views/devise/sessions/_new_base.html.haml @@ -13,7 +13,7 @@ .float-right.forgot-password = link_to "Forgot your password?", new_password_path(:user) %div - - if captcha_enabled? + - if captcha_enabled? || captcha_on_login_required? = recaptcha_tags .submit-container.move-submit-down diff --git a/changelogs/unreleased/security-59549-add-capcha-for-failed-logins.yml b/changelogs/unreleased/security-59549-add-capcha-for-failed-logins.yml new file mode 100644 index 00000000000..55f9e36c39c --- /dev/null +++ b/changelogs/unreleased/security-59549-add-capcha-for-failed-logins.yml @@ -0,0 +1,5 @@ +--- +title: Add :login_recaptcha_protection_enabled setting to prevent bots from brute-force attacks. +merge_request: +author: +type: security diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb index 1d2bb2bce0a..d8a4da8cdf9 100644 --- a/config/initializers/warden.rb +++ b/config/initializers/warden.rb @@ -19,6 +19,7 @@ Rails.application.configure do |config| Warden::Manager.after_authentication(scope: :user) do |user, auth, opts| ActiveSession.cleanup(user) + Gitlab::AnonymousSession.new(auth.request.remote_ip, session_id: auth.request.session.id).cleanup_session_per_ip_entries end Warden::Manager.after_set_user(scope: :user, only: :fetch) do |user, auth, opts| diff --git a/db/migrate/20190719122333_add_login_recaptcha_protection_enabled_to_application_settings.rb b/db/migrate/20190719122333_add_login_recaptcha_protection_enabled_to_application_settings.rb new file mode 100644 index 00000000000..4561e1e8aa9 --- /dev/null +++ b/db/migrate/20190719122333_add_login_recaptcha_protection_enabled_to_application_settings.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddLoginRecaptchaProtectionEnabledToApplicationSettings < ActiveRecord::Migration[5.1] + DOWNTIME = false + + def change + add_column :application_settings, :login_recaptcha_protection_enabled, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 6f5fc6c65eb..8acc81c26ab 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -228,6 +228,7 @@ ActiveRecord::Schema.define(version: 2019_07_29_090456) do t.boolean "lock_memberships_to_ldap", default: false, null: false t.boolean "time_tracking_limit_to_hours", default: false, null: false t.string "grafana_url", default: "/-/grafana", null: false + t.boolean "login_recaptcha_protection_enabled", default: false, null: false t.string "outbound_local_requests_whitelist", limit: 255, default: [], null: false, array: true t.integer "raw_blob_request_limit", default: 300, null: false t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id" diff --git a/lib/api/settings.rb b/lib/api/settings.rb index aa9e879160d..5ac6f5bf61a 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -104,6 +104,11 @@ module API requires :recaptcha_site_key, type: String, desc: 'Generate site key at http://www.google.com/recaptcha' requires :recaptcha_private_key, type: String, desc: 'Generate private key at http://www.google.com/recaptcha' end + optional :login_recaptcha_protection_enabled, type: Boolean, desc: 'Helps prevent brute-force attacks' + given login_recaptcha_protection_enabled: ->(val) { val } do + requires :recaptcha_site_key, type: String, desc: 'Generate site key at http://www.google.com/recaptcha' + requires :recaptcha_private_key, type: String, desc: 'Generate private key at http://www.google.com/recaptcha' + end optional :repository_checks_enabled, type: Boolean, desc: "GitLab will periodically run 'git fsck' in all project and wiki repositories to look for silent disk corruption issues." optional :repository_storages, type: Array[String], desc: 'Storage paths for new projects' optional :require_two_factor_authentication, type: Boolean, desc: 'Require all users to set up Two-factor authentication' diff --git a/lib/gitlab/anonymous_session.rb b/lib/gitlab/anonymous_session.rb new file mode 100644 index 00000000000..148b6d3310d --- /dev/null +++ b/lib/gitlab/anonymous_session.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Gitlab + class AnonymousSession + def initialize(remote_ip, session_id: nil) + @remote_ip = remote_ip + @session_id = session_id + end + + def store_session_id_per_ip + Gitlab::Redis::SharedState.with do |redis| + redis.pipelined do + redis.sadd(session_lookup_name, session_id) + redis.expire(session_lookup_name, 24.hours) + end + end + end + + def stored_sessions + Gitlab::Redis::SharedState.with do |redis| + redis.scard(session_lookup_name) + end + end + + def cleanup_session_per_ip_entries + Gitlab::Redis::SharedState.with do |redis| + redis.srem(session_lookup_name, session_id) + end + end + + private + + attr_reader :remote_ip, :session_id + + def session_lookup_name + @session_lookup_name ||= "#{Gitlab::Redis::SharedState::IP_SESSIONS_LOOKUP_NAMESPACE}:#{remote_ip}" + end + end +end diff --git a/lib/gitlab/recaptcha.rb b/lib/gitlab/recaptcha.rb index 772d743c9b0..f3cbe1db901 100644 --- a/lib/gitlab/recaptcha.rb +++ b/lib/gitlab/recaptcha.rb @@ -3,7 +3,7 @@ module Gitlab module Recaptcha def self.load_configurations! - if Gitlab::CurrentSettings.recaptcha_enabled + if Gitlab::CurrentSettings.recaptcha_enabled || enabled_on_login? ::Recaptcha.configure do |config| config.site_key = Gitlab::CurrentSettings.recaptcha_site_key config.secret_key = Gitlab::CurrentSettings.recaptcha_private_key @@ -16,5 +16,9 @@ module Gitlab def self.enabled? Gitlab::CurrentSettings.recaptcha_enabled end + + def self.enabled_on_login? + Gitlab::CurrentSettings.login_recaptcha_protection_enabled + end end end diff --git a/lib/gitlab/redis/shared_state.rb b/lib/gitlab/redis/shared_state.rb index 9066606ca21..270a19e780c 100644 --- a/lib/gitlab/redis/shared_state.rb +++ b/lib/gitlab/redis/shared_state.rb @@ -9,6 +9,7 @@ module Gitlab SESSION_NAMESPACE = 'session:gitlab'.freeze USER_SESSIONS_NAMESPACE = 'session:user:gitlab'.freeze USER_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:user:gitlab'.freeze + IP_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:ip:gitlab'.freeze DEFAULT_REDIS_SHARED_STATE_URL = 'redis://localhost:6382'.freeze REDIS_SHARED_STATE_CONFIG_ENV_VAR_NAME = 'GITLAB_REDIS_SHARED_STATE_CONFIG_FILE'.freeze diff --git a/locale/gitlab.pot b/locale/gitlab.pot index feae99fb95a..e6fc76bd28d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4175,7 +4175,7 @@ msgstr "" msgid "Enable or disable version check and usage ping." msgstr "" -msgid "Enable reCAPTCHA or Akismet and set IP limits." +msgid "Enable reCAPTCHA or Akismet and set IP limits. For reCAPTCHA, we currently only support %{recaptcha_v2_link_start}v2%{recaptcha_v2_link_end}" msgstr "" msgid "Enable shared Runners" @@ -5416,7 +5416,10 @@ msgstr "" msgid "Help page text and support page url." msgstr "" -msgid "Helps prevent bots from creating accounts. We currently only support %{recaptcha_v2_link_start}reCAPTCHA v2%{recaptcha_v2_link_end}" +msgid "Helps prevent bots from brute-force attacks." +msgstr "" + +msgid "Helps prevent bots from creating accounts." msgstr "" msgid "Hide archived projects" diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 9c4ddce5409..68b7bf61231 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -100,16 +100,8 @@ describe SessionsController do end end - context 'when reCAPTCHA is enabled' do - let(:user) { create(:user) } - let(:user_params) { { login: user.username, password: user.password } } - - before do - stub_application_setting(recaptcha_enabled: true) - request.headers[described_class::CAPTCHA_HEADER] = 1 - end - - it 'displays an error when the reCAPTCHA is not solved' do + context 'with reCAPTCHA' do + def unsuccesful_login(user_params, sesion_params: {}) # Without this, `verify_recaptcha` arbitrarily returns true in test env Recaptcha.configuration.skip_verify_env.delete('test') counter = double(:counter) @@ -119,14 +111,10 @@ describe SessionsController do .with(:failed_login_captcha_total, anything) .and_return(counter) - post(:create, params: { user: user_params }) - - expect(response).to render_template(:new) - expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' - expect(subject.current_user).to be_nil + post(:create, params: { user: user_params }, session: sesion_params) end - it 'successfully logs in a user when reCAPTCHA is solved' do + def succesful_login(user_params, sesion_params: {}) # Avoid test ordering issue and ensure `verify_recaptcha` returns true Recaptcha.configuration.skip_verify_env << 'test' counter = double(:counter) @@ -137,9 +125,80 @@ describe SessionsController do .and_return(counter) expect(Gitlab::Metrics).to receive(:counter).and_call_original - post(:create, params: { user: user_params }) + post(:create, params: { user: user_params }, session: sesion_params) + end - expect(subject.current_user).to eq user + context 'when reCAPTCHA is enabled' do + let(:user) { create(:user) } + let(:user_params) { { login: user.username, password: user.password } } + + before do + stub_application_setting(recaptcha_enabled: true) + request.headers[described_class::CAPTCHA_HEADER] = 1 + end + + it 'displays an error when the reCAPTCHA is not solved' do + # Without this, `verify_recaptcha` arbitrarily returns true in test env + + unsuccesful_login(user_params) + + expect(response).to render_template(:new) + expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' + expect(subject.current_user).to be_nil + end + + it 'successfully logs in a user when reCAPTCHA is solved' do + succesful_login(user_params) + + expect(subject.current_user).to eq user + end + end + + context 'when reCAPTCHA login protection is enabled' do + let(:user) { create(:user) } + let(:user_params) { { login: user.username, password: user.password } } + + before do + stub_application_setting(login_recaptcha_protection_enabled: true) + end + + context 'when user tried to login 5 times' do + it 'displays an error when the reCAPTCHA is not solved' do + unsuccesful_login(user_params, sesion_params: { failed_login_attempts: 6 }) + + expect(response).to render_template(:new) + expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' + expect(subject.current_user).to be_nil + end + + it 'successfully logs in a user when reCAPTCHA is solved' do + succesful_login(user_params, sesion_params: { failed_login_attempts: 6 }) + + expect(subject.current_user).to eq user + end + end + + context 'when there are more than 5 anonymous session with the same IP' do + before do + allow(Gitlab::AnonymousSession).to receive_message_chain(:new, :stored_sessions).and_return(6) + end + + it 'displays an error when the reCAPTCHA is not solved' do + unsuccesful_login(user_params) + + expect(response).to render_template(:new) + expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' + expect(subject.current_user).to be_nil + end + + it 'successfully logs in a user when reCAPTCHA is solved' do + expect(Gitlab::AnonymousSession).to receive_message_chain(:new, :cleanup_session_per_ip_entries) + + succesful_login(user_params) + + expect(subject.current_user).to eq user + end + end end end end @@ -348,4 +407,17 @@ describe SessionsController do expect(controller.stored_location_for(:redirect)).to eq(search_path) end end + + context 'when login fails' do + before do + set_devise_mapping(context: @request) + @request.env["warden.options"] = { action: 'unauthenticated' } + end + + it 'does increment failed login counts for session' do + get(:new, params: { user: { login: 'failed' } }) + + expect(session[:failed_login_attempts]).to eq(1) + end + end end diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index c77605f3869..4d3cf052744 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -263,6 +263,7 @@ describe 'Admin updates settings' do page.within('.as-spam') do check 'Enable reCAPTCHA' + check 'Enable reCAPTCHA for login' fill_in 'reCAPTCHA Site Key', with: 'key' fill_in 'reCAPTCHA Private Key', with: 'key' fill_in 'IPs per user', with: 15 @@ -271,6 +272,7 @@ describe 'Admin updates settings' do expect(page).to have_content "Application settings saved successfully" expect(current_settings.recaptcha_enabled).to be true + expect(current_settings.login_recaptcha_protection_enabled).to be true expect(current_settings.unique_ips_limit_per_user).to eq(15) end end diff --git a/spec/lib/gitlab/anonymous_session_spec.rb b/spec/lib/gitlab/anonymous_session_spec.rb new file mode 100644 index 00000000000..628aae81ada --- /dev/null +++ b/spec/lib/gitlab/anonymous_session_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Gitlab::AnonymousSession, :clean_gitlab_redis_shared_state do + let(:default_session_id) { '6919a6f1bb119dd7396fadc38fd18d0d' } + let(:additional_session_id) { '7919a6f1bb119dd7396fadc38fd18d0d' } + + subject { new_anonymous_session } + + def new_anonymous_session(session_id = default_session_id) + described_class.new('127.0.0.1', session_id: session_id) + end + + describe '#store_session_id_per_ip' do + it 'adds session id to proper key' do + subject.store_session_id_per_ip + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [default_session_id] + end + end + + it 'adds expiration time to key' do + Timecop.freeze do + subject.store_session_id_per_ip + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.ttl("session:lookup:ip:gitlab:127.0.0.1")).to eq(24.hours.to_i) + end + end + end + + it 'adds id only once' do + subject.store_session_id_per_ip + subject.store_session_id_per_ip + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [default_session_id] + end + end + + context 'when there is already one session' do + it 'adds session id to proper key' do + subject.store_session_id_per_ip + new_anonymous_session(additional_session_id).store_session_id_per_ip + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to contain_exactly(default_session_id, additional_session_id) + end + end + end + end + + describe '#stored_sessions' do + it 'returns all anonymous sessions per ip' do + Gitlab::Redis::SharedState.with do |redis| + redis.sadd("session:lookup:ip:gitlab:127.0.0.1", default_session_id) + redis.sadd("session:lookup:ip:gitlab:127.0.0.1", additional_session_id) + end + + expect(subject.stored_sessions).to eq(2) + end + end + + it 'removes obsolete lookup through ip entries' do + Gitlab::Redis::SharedState.with do |redis| + redis.sadd("session:lookup:ip:gitlab:127.0.0.1", default_session_id) + redis.sadd("session:lookup:ip:gitlab:127.0.0.1", additional_session_id) + end + + subject.cleanup_session_per_ip_entries + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [additional_session_id] + end + end +end diff --git a/spec/views/devise/shared/_signin_box.html.haml_spec.rb b/spec/views/devise/shared/_signin_box.html.haml_spec.rb index 66c064e3fba..5d521d18c70 100644 --- a/spec/views/devise/shared/_signin_box.html.haml_spec.rb +++ b/spec/views/devise/shared/_signin_box.html.haml_spec.rb @@ -7,6 +7,7 @@ describe 'devise/shared/_signin_box' do assign(:ldap_servers, []) allow(view).to receive(:current_application_settings).and_return(Gitlab::CurrentSettings.current_application_settings) allow(view).to receive(:captcha_enabled?).and_return(false) + allow(view).to receive(:captcha_on_login_required?).and_return(false) end it 'is shown when Crowd is enabled' do -- cgit v1.2.1 From 6391eeec3078f97fac6f6ef92ca0d8c8f0068667 Mon Sep 17 00:00:00 2001 From: "David H. Wilkins" Date: Fri, 2 Aug 2019 18:18:09 -0500 Subject: git-user-related local test failures Some of the tests fail locally due to the git user being different than it is on the test runners. I'd really like to be able to run all of the tests locally. --- spec/helpers/projects_helper_spec.rb | 2 +- spec/lib/gitlab/middleware/go_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 3716879c458..d4cf15ad5f8 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -478,7 +478,7 @@ describe ProjectsHelper do it 'returns the command to push to create project over SSH' do allow(Gitlab::CurrentSettings.current_application_settings).to receive(:enabled_git_access_protocol) { 'ssh' } - expect(helper.push_to_create_project_command(user)).to eq('git push --set-upstream git@localhost:john/$(git rev-parse --show-toplevel | xargs basename).git $(git rev-parse --abbrev-ref HEAD)') + expect(helper.push_to_create_project_command(user)).to eq("git push --set-upstream #{Gitlab.config.gitlab.user}@localhost:john/$(git rev-parse --show-toplevel | xargs basename).git $(git rev-parse --abbrev-ref HEAD)") end end diff --git a/spec/lib/gitlab/middleware/go_spec.rb b/spec/lib/gitlab/middleware/go_spec.rb index f52095bf633..16595102375 100644 --- a/spec/lib/gitlab/middleware/go_spec.rb +++ b/spec/lib/gitlab/middleware/go_spec.rb @@ -202,7 +202,7 @@ describe Gitlab::Middleware::Go do def expect_response_with_path(response, protocol, path) repository_url = case protocol when :ssh - "ssh://git@#{Gitlab.config.gitlab.host}/#{path}.git" + "ssh://#{Gitlab.config.gitlab.user}@#{Gitlab.config.gitlab.host}/#{path}.git" when :http, nil "http://#{Gitlab.config.gitlab.host}/#{path}.git" end -- cgit v1.2.1 From 927f608f2c4905e430d2df1c455cec793ef41aa9 Mon Sep 17 00:00:00 2001 From: Patrick Derichs Date: Mon, 15 Jul 2019 13:29:56 +0200 Subject: Fix HTML injection for label description Add changelog entry Add spec --- app/helpers/labels_helper.rb | 2 +- app/models/label.rb | 8 ++++++-- ...rity-fix-html-injection-for-label-description-ce-master.yml | 5 +++++ spec/helpers/labels_helper_spec.rb | 10 ++++++++++ spec/models/label_spec.rb | 7 +++++++ 5 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/security-fix-html-injection-for-label-description-ce-master.yml diff --git a/app/helpers/labels_helper.rb b/app/helpers/labels_helper.rb index 2ed016beea4..c5a3507637e 100644 --- a/app/helpers/labels_helper.rb +++ b/app/helpers/labels_helper.rb @@ -71,7 +71,7 @@ module LabelsHelper end def label_tooltip_title(label) - label.description + Sanitize.clean(label.description) end def suggested_colors diff --git a/app/models/label.rb b/app/models/label.rb index 25de26b8384..19f684c32af 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -197,7 +197,11 @@ class Label < ApplicationRecord end def title=(value) - write_attribute(:title, sanitize_title(value)) if value.present? + write_attribute(:title, sanitize_value(value)) if value.present? + end + + def description=(value) + write_attribute(:description, sanitize_value(value)) if value.present? end ## @@ -258,7 +262,7 @@ class Label < ApplicationRecord end end - def sanitize_title(value) + def sanitize_value(value) CGI.unescapeHTML(Sanitize.clean(value.to_s)) end diff --git a/changelogs/unreleased/security-fix-html-injection-for-label-description-ce-master.yml b/changelogs/unreleased/security-fix-html-injection-for-label-description-ce-master.yml new file mode 100644 index 00000000000..07124ac399b --- /dev/null +++ b/changelogs/unreleased/security-fix-html-injection-for-label-description-ce-master.yml @@ -0,0 +1,5 @@ +--- +title: Fix HTML injection for label description +merge_request: +author: +type: security diff --git a/spec/helpers/labels_helper_spec.rb b/spec/helpers/labels_helper_spec.rb index 4f1cab38f34..1d57aaa0da5 100644 --- a/spec/helpers/labels_helper_spec.rb +++ b/spec/helpers/labels_helper_spec.rb @@ -278,4 +278,14 @@ describe LabelsHelper do it { is_expected.to eq('Subscribe at group level') } end end + + describe '#label_tooltip_title' do + let(:html) { 'This is an image' } + let(:label_with_html_content) { create(:label, title: 'test', description: html) } + + it 'removes HTML' do + tooltip = label_tooltip_title(label_with_html_content) + expect(tooltip).to eq('This is an image') + end + end end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index c2e2298823e..baf2cfeab0c 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -84,6 +84,13 @@ describe Label do end end + describe '#description' do + it 'sanitizes description' do + label = described_class.new(description: 'foo & bar?') + expect(label.description).to eq('foo & bar?') + end + end + describe 'priorization' do subject(:label) { create(:label) } -- cgit v1.2.1 From 20e8c92410d93c39f4a800941d0c633f949e7790 Mon Sep 17 00:00:00 2001 From: Martin Hanzel Date: Wed, 24 Jul 2019 14:19:25 +0200 Subject: Enforce max chars and max render time in markdown math KaTeX math will now render progressivly and asynchronously. There are upper limits on the character count of each formula, and on cumulative render time. --- .../javascripts/behaviors/markdown/render_math.js | 146 ++++++++++++++++++--- .../unreleased/security-katex-dos-master.yml | 5 + locale/gitlab.pot | 9 +- spec/features/markdown/math_spec.rb | 6 +- 4 files changed, 143 insertions(+), 23 deletions(-) create mode 100644 changelogs/unreleased/security-katex-dos-master.yml diff --git a/app/assets/javascripts/behaviors/markdown/render_math.js b/app/assets/javascripts/behaviors/markdown/render_math.js index a68936d79e2..53867b3096b 100644 --- a/app/assets/javascripts/behaviors/markdown/render_math.js +++ b/app/assets/javascripts/behaviors/markdown/render_math.js @@ -1,6 +1,5 @@ -import $ from 'jquery'; -import { __ } from '~/locale'; import flash from '~/flash'; +import { s__, sprintf } from '~/locale'; // Renders math using KaTeX in any element with the // `js-render-math` class @@ -10,21 +9,131 @@ import flash from '~/flash'; // // -// Loop over all math elements and render math -function renderWithKaTeX(elements, katex) { - elements.each(function katexElementsLoop() { - const mathNode = $(''); - const $this = $(this); - - const display = $this.attr('data-math-style') === 'display'; - try { - katex.render($this.text(), mathNode.get(0), { displayMode: display, throwOnError: false }); - mathNode.insertAfter($this); - $this.remove(); - } catch (err) { - throw err; +const MAX_MATH_CHARS = 1000; +const MAX_RENDER_TIME_MS = 2000; + +// These messages might be used with inline errors in the future. Keep them around. For now, we will +// display a single error message using flash(). + +// const CHAR_LIMIT_EXCEEDED_MSG = sprintf( +// s__( +// 'math|The following math is too long. For performance reasons, math blocks are limited to %{maxChars} characters. Try splitting up this block, or include an image instead.', +// ), +// { maxChars: MAX_MATH_CHARS }, +// ); +// const RENDER_TIME_EXCEEDED_MSG = s__( +// "math|The math in this entry is taking too long to render. Any math below this point won't be shown. Consider splitting it among multiple entries.", +// ); + +const RENDER_FLASH_MSG = sprintf( + s__( + 'math|The math in this entry is taking too long to render and may not be displayed as expected. For performance reasons, math blocks are also limited to %{maxChars} characters. Consider splitting up large formulae, splitting math blocks among multiple entries, or using an image instead.', + ), + { maxChars: MAX_MATH_CHARS }, +); + +// Wait for the browser to reflow the layout. Reflowing SVG takes time. +// This has to wrap the inner function, otherwise IE/Edge throw "invalid calling object". +const waitForReflow = fn => { + window.requestAnimationFrame(fn); +}; + +/** + * Renders math blocks sequentially while protecting against DoS attacks. Math blocks have a maximum character limit of MAX_MATH_CHARS. If rendering math takes longer than MAX_RENDER_TIME_MS, all subsequent math blocks are skipped and an error message is shown. + */ +class SafeMathRenderer { + /* + How this works: + + The performance bottleneck in rendering math is in the browser trying to reflow the generated SVG. + During this time, the JS is blocked and the page becomes unresponsive. + We want to render math blocks one by one until a certain time is exceeded, after which we stop + rendering subsequent math blocks, to protect against DoS. However, browsers do reflowing in an + asynchronous task, so we can't time it synchronously. + + SafeMathRenderer essentially does the following: + 1. Replaces all math blocks with placeholders so that they're not mistakenly rendered twice. + 2. Places each placeholder element in a queue. + 3. Renders the element at the head of the queue and waits for reflow. + 4. After reflow, gets the elapsed time since step 3 and repeats step 3 until the queue is empty. + */ + queue = []; + totalMS = 0; + + constructor(elements, katex) { + this.elements = elements; + this.katex = katex; + + this.renderElement = this.renderElement.bind(this); + this.render = this.render.bind(this); + } + + renderElement() { + if (!this.queue.length) { + return; } - }); + + const el = this.queue.shift(); + const text = el.textContent; + + el.removeAttribute('style'); + + if (this.totalMS >= MAX_RENDER_TIME_MS || text.length > MAX_MATH_CHARS) { + if (!this.flashShown) { + flash(RENDER_FLASH_MSG); + this.flashShown = true; + } + + // Show unrendered math code + const codeElement = document.createElement('pre'); + codeElement.className = 'code'; + codeElement.textContent = el.textContent; + el.parentNode.replaceChild(codeElement, el); + + // Render the next math + this.renderElement(); + } else { + this.startTime = Date.now(); + + try { + el.innerHTML = this.katex.renderToString(text, { + displayMode: el.getAttribute('data-math-style') === 'display', + throwOnError: true, + maxSize: 20, + maxExpand: 20, + }); + } catch { + // Don't show a flash for now because it would override an existing flash message + el.textContent = s__('math|There was an error rendering this math block'); + // el.style.color = '#d00'; + el.className = 'katex-error'; + } + + // Give the browser time to reflow the svg + waitForReflow(() => { + const deltaTime = Date.now() - this.startTime; + this.totalMS += deltaTime; + + this.renderElement(); + }); + } + } + + render() { + // Replace math blocks with a placeholder so they aren't rendered twice + this.elements.forEach(el => { + const placeholder = document.createElement('span'); + placeholder.style.display = 'none'; + placeholder.setAttribute('data-math-style', el.getAttribute('data-math-style')); + placeholder.textContent = el.textContent; + el.parentNode.replaceChild(placeholder, el); + this.queue.push(placeholder); + }); + + // If we wait for the browser thread to settle down a bit, math rendering becomes 5-10x faster + // and less prone to timeouts. + setTimeout(this.renderElement, 400); + } } export default function renderMath($els) { @@ -34,7 +143,8 @@ export default function renderMath($els) { import(/* webpackChunkName: 'katex' */ 'katex/dist/katex.min.css'), ]) .then(([katex]) => { - renderWithKaTeX($els, katex); + const renderer = new SafeMathRenderer($els.get(), katex); + renderer.render(); }) - .catch(() => flash(__('An error occurred while rendering KaTeX'))); + .catch(() => {}); } diff --git a/changelogs/unreleased/security-katex-dos-master.yml b/changelogs/unreleased/security-katex-dos-master.yml new file mode 100644 index 00000000000..df803a5eafd --- /dev/null +++ b/changelogs/unreleased/security-katex-dos-master.yml @@ -0,0 +1,5 @@ +--- +title: Enforce max chars and max render time in markdown math +merge_request: +author: +type: security diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8cf70014256..8ecd7ccd045 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1083,9 +1083,6 @@ msgstr "" msgid "An error occurred while parsing recent searches" msgstr "" -msgid "An error occurred while rendering KaTeX" -msgstr "" - msgid "An error occurred while rendering preview broadcast message" msgstr "" @@ -13272,6 +13269,12 @@ msgstr "" msgid "manual" msgstr "" +msgid "math|The math in this entry is taking too long to render and may not be displayed as expected. For performance reasons, math blocks are also limited to %{maxChars} characters. Consider splitting up large formulae, splitting math blocks among multiple entries, or using an image instead." +msgstr "" + +msgid "math|There was an error rendering this math block" +msgstr "" + msgid "merge request" msgid_plural "merge requests" msgstr[0] "" diff --git a/spec/features/markdown/math_spec.rb b/spec/features/markdown/math_spec.rb index 68d99b4241a..76eef66c517 100644 --- a/spec/features/markdown/math_spec.rb +++ b/spec/features/markdown/math_spec.rb @@ -34,7 +34,9 @@ describe 'Math rendering', :js do visit project_issue_path(project, issue) - expect(page).to have_selector('.katex-error', text: "\href{javascript:alert('xss');}{xss}") - expect(page).to have_selector('.katex-html a', text: 'Gitlab') + page.within '.description > .md' do + expect(page).to have_selector('.katex-error') + expect(page).to have_selector('.katex-html a', text: 'Gitlab') + end end end -- cgit v1.2.1 From d30a90a354f3dc015093d80f9de9dc15b38ff2a0 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Tue, 4 Jun 2019 23:30:54 -0400 Subject: Prevent unauthorised comments on merge requests * Prevent creating notes on inaccessible MRs This applies the notes rules at the MR scope. Rather than adding extra rules to the Project level policy, preventing :create_note here is better since it only prevents creating notes on MRs. * Prevent creating notes in inaccessible Issues without this policy, non-team-members are allowed to comment on issues even when the project has the private-issues policy set. This means that without this change, users are allowed to comment on issues that they cannot read. * Add CHANGELOG entry --- app/policies/issue_policy.rb | 9 +- app/policies/merge_request_policy.rb | 6 + .../ce-60465-prevent-comments-on-private-mrs.yml | 3 + spec/controllers/projects/notes_controller_spec.rb | 311 ++++++++++++++++----- spec/policies/issue_policy_spec.rb | 28 ++ spec/policies/merge_request_policy_spec.rb | 89 ++++++ 6 files changed, 371 insertions(+), 75 deletions(-) create mode 100644 changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index dd8c5d49cf4..fa252af55e4 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -5,6 +5,8 @@ class IssuePolicy < IssuablePolicy # Make sure to sync this class checks with issue.rb to avoid security problems. # Check commit 002ad215818450d2cbbc5fa065850a953dc7ada8 for more information. + extend ProjectPolicy::ClassMethods + desc "User can read confidential issues" condition(:can_read_confidential) do @user && IssueCollection.new([@subject]).visible_to(@user).any? @@ -14,13 +16,12 @@ class IssuePolicy < IssuablePolicy condition(:confidential, scope: :subject) { @subject.confidential? } rule { confidential & ~can_read_confidential }.policy do - prevent :read_issue + prevent(*create_read_update_admin_destroy(:issue)) prevent :read_issue_iid - prevent :update_issue - prevent :admin_issue - prevent :create_note end + rule { ~can?(:read_issue) }.prevent :create_note + rule { locked }.policy do prevent :reopen_issue end diff --git a/app/policies/merge_request_policy.rb b/app/policies/merge_request_policy.rb index a3692857ff4..5ad7bdabdff 100644 --- a/app/policies/merge_request_policy.rb +++ b/app/policies/merge_request_policy.rb @@ -4,4 +4,10 @@ class MergeRequestPolicy < IssuablePolicy rule { locked }.policy do prevent :reopen_merge_request end + + # Only users who can read the merge request can comment. + # Although :read_merge_request is computed in the policy context, + # it would not be safe to prevent :create_note there, since + # note permissions are shared, and this would apply too broadly. + rule { ~can?(:read_merge_request) }.prevent :create_note end diff --git a/changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml b/changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml new file mode 100644 index 00000000000..ba970162447 --- /dev/null +++ b/changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml @@ -0,0 +1,3 @@ +--- +title: Ensure only authorised users can create notes on Merge Requests and Issues +type: security diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 9ab565dc2e8..99808dce016 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -212,40 +212,232 @@ describe Projects::NotesController do describe 'POST create' do let(:merge_request) { create(:merge_request) } let(:project) { merge_request.source_project } + let(:note_text) { 'some note' } let(:request_params) do { - note: { note: 'some note', noteable_id: merge_request.id, noteable_type: 'MergeRequest' }, + note: { note: note_text, noteable_id: merge_request.id, noteable_type: 'MergeRequest' }, namespace_id: project.namespace, project_id: project, merge_request_diff_head_sha: 'sha', target_type: 'merge_request', target_id: merge_request.id - } + }.merge(extra_request_params) + end + let(:extra_request_params) { {} } + + let(:project_visibility) { Gitlab::VisibilityLevel::PUBLIC } + let(:merge_requests_access_level) { ProjectFeature::ENABLED } + + def create! + post :create, params: request_params end before do + project.update_attribute(:visibility_level, project_visibility) + project.project_feature.update(merge_requests_access_level: merge_requests_access_level) sign_in(user) - project.add_developer(user) end - it "returns status 302 for html" do - post :create, params: request_params + describe 'making the creation request' do + before do + create! + end + + context 'the project is publically available' do + context 'for HTML' do + it "returns status 302" do + expect(response).to have_gitlab_http_status(302) + end + end + + context 'for JSON' do + let(:extra_request_params) { { format: :json } } + + it "returns status 200 for json" do + expect(response).to have_gitlab_http_status(200) + end + end + end - expect(response).to have_gitlab_http_status(302) + context 'the project is a private project' do + let(:project_visibility) { Gitlab::VisibilityLevel::PRIVATE } + + [{}, { format: :json }].each do |extra| + context "format is #{extra[:format]}" do + let(:extra_request_params) { extra } + + it "returns status 404" do + expect(response).to have_gitlab_http_status(404) + end + end + end + end end - it "returns status 200 for json" do - post :create, params: request_params.merge(format: :json) + context 'the user is a developer on a private project' do + let(:project_visibility) { Gitlab::VisibilityLevel::PRIVATE } - expect(response).to have_gitlab_http_status(200) + before do + project.add_developer(user) + end + + context 'HTML requests' do + it "returns status 302 (redirect)" do + create! + + expect(response).to have_gitlab_http_status(302) + end + end + + context 'JSON requests' do + let(:extra_request_params) { { format: :json } } + + it "returns status 200" do + create! + + expect(response).to have_gitlab_http_status(200) + end + end + + context 'the return_discussion param is set' do + let(:extra_request_params) { { format: :json, return_discussion: 'true' } } + + it 'returns discussion JSON when the return_discussion param is set' do + create! + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to have_key 'discussion' + expect(json_response.dig('discussion', 'notes', 0, 'note')).to eq(request_params[:note][:note]) + end + end + + context 'when creating a note with quick actions' do + context 'with commands that return changes' do + let(:note_text) { "/award :thumbsup:\n/estimate 1d\n/spend 3h" } + let(:extra_request_params) { { format: :json } } + + it 'includes changes in commands_changes ' do + create! + + expect(response).to have_gitlab_http_status(200) + expect(json_response['commands_changes']).to include('emoji_award', 'time_estimate', 'spend_time') + expect(json_response['commands_changes']).not_to include('target_project', 'title') + end + end + + context 'with commands that do not return changes' do + let(:issue) { create(:issue, project: project) } + let(:other_project) { create(:project) } + let(:note_text) { "/move #{other_project.full_path}\n/title AAA" } + let(:extra_request_params) { { format: :json, target_id: issue.id, target_type: 'issue' } } + + before do + other_project.add_developer(user) + end + + it 'does not include changes in commands_changes' do + create! + + expect(response).to have_gitlab_http_status(200) + expect(json_response['commands_changes']).not_to include('target_project', 'title') + end + end + end end - it 'returns discussion JSON when the return_discussion param is set' do - post :create, params: request_params.merge(format: :json, return_discussion: 'true') + context 'when the internal project prohibits non-members from accessing merge requests' do + let(:project_visibility) { Gitlab::VisibilityLevel::INTERNAL } + let(:merge_requests_access_level) { ProjectFeature::PRIVATE } - expect(response).to have_gitlab_http_status(200) - expect(json_response).to have_key 'discussion' - expect(json_response['discussion']['notes'][0]['note']).to eq(request_params[:note][:note]) + it "prevents a non-member user from creating a note on one of the project's merge requests" do + create! + + expect(response).to have_gitlab_http_status(404) + end + + context 'when the user is a team member' do + before do + project.add_developer(user) + end + + it 'can add comments' do + expect { create! }.to change { project.notes.count }.by(1) + end + end + + # Illustration of the attack vector for posting comments to discussions that should + # be inaccessible. + # + # This relies on posting a note to a commit that is not necessarily even in the + # merge request, with a value of :in_reply_to_discussion_id that points to a + # discussion on a merge_request that should not be accessible. + context 'when the request includes a :in_reply_to_discussion_id designed to fool us' do + let(:commit) { create(:commit, project: project) } + + let(:existing_comment) do + create(:note_on_commit, + note: 'first', + project: project, + commit_id: merge_request.commit_shas.first) + end + + let(:discussion) { existing_comment.discussion } + + # see !60465 for details of the structure of this request + let(:request_params) do + { "utf8" => "✓", + "authenticity_token" => "1", + "view" => "inline", + "line_type" => "", + "merge_request_diff_head_sha" => "", + "in_reply_to_discussion_id" => discussion.id, + "note_project_id" => project.id, + "project_id" => project.id, + "namespace_id" => project.namespace, + "target_type" => "commit", + "target_id" => commit.id, + "note" => { + "noteable_type" => "", + "noteable_id" => "", + "commit_id" => "", + "type" => "", + "line_code" => "", + "position" => "", + "note" => "ThisReplyWillGoToMergeRequest" + } } + end + + it 'prevents the request from adding notes to the spoofed discussion' do + expect { create! }.not_to change { discussion.notes.count } + end + + it 'returns an error to the user' do + create! + expect(response).to have_gitlab_http_status(404) + end + end + end + + context 'when the public project prohibits non-members from accessing merge requests' do + let(:project_visibility) { Gitlab::VisibilityLevel::PUBLIC } + let(:merge_requests_access_level) { ProjectFeature::PRIVATE } + + it "prevents a non-member user from creating a note on one of the project's merge requests" do + create! + + expect(response).to have_gitlab_http_status(404) + end + + context 'when the user is a team member' do + before do + project.add_developer(user) + create! + end + + it 'can add comments' do + expect(response).to be_redirect + end + end end context 'when merge_request_diff_head_sha present' do @@ -262,7 +454,7 @@ describe Projects::NotesController do end it "returns status 302 for html" do - post :create, params: request_params + create! expect(response).to have_gitlab_http_status(302) end @@ -285,7 +477,7 @@ describe Projects::NotesController do end context 'when creating a commit comment from an MR fork' do - let(:project) { create(:project, :repository) } + let(:project) { create(:project, :repository, :public) } let(:forked_project) do fork_project(project, nil, repository: true) @@ -299,45 +491,59 @@ describe Projects::NotesController do create(:note_on_commit, note: 'a note', project: forked_project, commit_id: merge_request.commit_shas.first) end - def post_create(extra_params = {}) - post :create, params: { + let(:note_project_id) do + forked_project.id + end + + let(:request_params) do + { note: { note: 'some other note', noteable_id: merge_request.id }, namespace_id: project.namespace, project_id: project, target_type: 'merge_request', target_id: merge_request.id, - note_project_id: forked_project.id, + note_project_id: note_project_id, in_reply_to_discussion_id: existing_comment.discussion_id - }.merge(extra_params) + } + end + + let(:fork_visibility) { Gitlab::VisibilityLevel::PUBLIC } + + before do + forked_project.update_attribute(:visibility_level, fork_visibility) end context 'when the note_project_id is not correct' do - it 'returns a 404' do - post_create(note_project_id: Project.maximum(:id).succ) + let(:note_project_id) do + project.id && Project.maximum(:id).succ + end + it 'returns a 404' do + create! expect(response).to have_gitlab_http_status(404) end end context 'when the user has no access to the fork' do - it 'returns a 404' do - post_create + let(:fork_visibility) { Gitlab::VisibilityLevel::PRIVATE } + it 'returns a 404' do + create! expect(response).to have_gitlab_http_status(404) end end context 'when the user has access to the fork' do - let(:discussion) { forked_project.notes.find_discussion(existing_comment.discussion_id) } + let!(:discussion) { forked_project.notes.find_discussion(existing_comment.discussion_id) } + let(:fork_visibility) { Gitlab::VisibilityLevel::PUBLIC } - before do - forked_project.add_developer(user) - - existing_comment + it 'is successful' do + create! + expect(response).to have_gitlab_http_status(302) end it 'creates the note' do - expect { post_create }.to change { forked_project.notes.count }.by(1) + expect { create! }.to change { forked_project.notes.count }.by(1) end end end @@ -346,11 +552,6 @@ describe Projects::NotesController do let(:locked_issue) { create(:issue, :locked, project: project) } let(:issue) {create(:issue, project: project)} - before do - project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) - project.project_member(user).destroy - end - it 'uses target_id and ignores noteable_id' do request_params = { note: { note: 'some note', noteable_type: 'Issue', noteable_id: locked_issue.id }, @@ -368,7 +569,6 @@ describe Projects::NotesController do context 'when the merge request discussion is locked' do before do - project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) merge_request.update_attribute(:discussion_locked, true) end @@ -382,6 +582,10 @@ describe Projects::NotesController do end context 'when a user is a team member' do + before do + project.add_developer(user) + end + it 'returns 302 status for html' do post :create, params: request_params @@ -400,10 +604,6 @@ describe Projects::NotesController do end context 'when a user is not a team member' do - before do - project.project_member(user).destroy - end - it 'returns 404 status' do post :create, params: request_params @@ -415,37 +615,6 @@ describe Projects::NotesController do end end end - - context 'when creating a note with quick actions' do - context 'with commands that return changes' do - let(:note_text) { "/award :thumbsup:\n/estimate 1d\n/spend 3h" } - - it 'includes changes in commands_changes ' do - post :create, params: request_params.merge(note: { note: note_text }, format: :json) - - expect(response).to have_gitlab_http_status(200) - expect(json_response['commands_changes']).to include('emoji_award', 'time_estimate', 'spend_time') - expect(json_response['commands_changes']).not_to include('target_project', 'title') - end - end - - context 'with commands that do not return changes' do - let(:issue) { create(:issue, project: project) } - let(:other_project) { create(:project) } - let(:note_text) { "/move #{other_project.full_path}\n/title AAA" } - - before do - other_project.add_developer(user) - end - - it 'does not include changes in commands_changes' do - post :create, params: request_params.merge(note: { note: note_text }, target_type: 'issue', target_id: issue.id, format: :json) - - expect(response).to have_gitlab_http_status(200) - expect(json_response['commands_changes']).not_to include('target_project', 'title') - end - end - end end describe 'PUT update' do diff --git a/spec/policies/issue_policy_spec.rb b/spec/policies/issue_policy_spec.rb index b149dbcf871..25267d36ab8 100644 --- a/spec/policies/issue_policy_spec.rb +++ b/spec/policies/issue_policy_spec.rb @@ -172,6 +172,34 @@ describe IssuePolicy do expect(permissions(assignee, issue_locked)).to be_disallowed(:admin_issue, :reopen_issue) end + context 'when issues are private' do + before do + project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE) + end + let(:issue) { create(:issue, project: project, author: author) } + let(:visitor) { create(:user) } + let(:admin) { create(:user, :admin) } + + it 'forbids visitors from viewing issues' do + expect(permissions(visitor, issue)).to be_disallowed(:read_issue) + end + it 'forbids visitors from commenting' do + expect(permissions(visitor, issue)).to be_disallowed(:create_note) + end + it 'allows guests to view' do + expect(permissions(guest, issue)).to be_allowed(:read_issue) + end + it 'allows guests to comment' do + expect(permissions(guest, issue)).to be_allowed(:create_note) + end + it 'allows admins to view' do + expect(permissions(admin, issue)).to be_allowed(:read_issue) + end + it 'allows admins to comment' do + expect(permissions(admin, issue)).to be_allowed(:create_note) + end + end + context 'with confidential issues' do let(:confidential_issue) { create(:issue, :confidential, project: project, assignees: [assignee], author: author) } let(:confidential_issue_no_assignee) { create(:issue, :confidential, project: project) } diff --git a/spec/policies/merge_request_policy_spec.rb b/spec/policies/merge_request_policy_spec.rb index 81279225d61..87205f56589 100644 --- a/spec/policies/merge_request_policy_spec.rb +++ b/spec/policies/merge_request_policy_spec.rb @@ -6,6 +6,7 @@ describe MergeRequestPolicy do let(:guest) { create(:user) } let(:author) { create(:user) } let(:developer) { create(:user) } + let(:non_team_member) { create(:user) } let(:project) { create(:project, :public) } def permissions(user, merge_request) @@ -18,6 +19,78 @@ describe MergeRequestPolicy do project.add_developer(developer) end + MR_PERMS = %i[create_merge_request_in + create_merge_request_from + read_merge_request + create_note].freeze + + shared_examples_for 'a denied user' do + let(:perms) { permissions(subject, merge_request) } + + MR_PERMS.each do |thing| + it "cannot #{thing}" do + expect(perms).to be_disallowed(thing) + end + end + end + + shared_examples_for 'a user with access' do + let(:perms) { permissions(subject, merge_request) } + + MR_PERMS.each do |thing| + it "can #{thing}" do + expect(perms).to be_allowed(thing) + end + end + end + + context 'when merge requests have been disabled' do + let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) } + + before do + project.project_feature.update(merge_requests_access_level: ProjectFeature::DISABLED) + end + + describe 'the author' do + subject { author } + it_behaves_like 'a denied user' + end + + describe 'a guest' do + subject { guest } + it_behaves_like 'a denied user' + end + + describe 'a developer' do + subject { developer } + it_behaves_like 'a denied user' + end + + describe 'any other user' do + subject { non_team_member } + it_behaves_like 'a denied user' + end + end + + context 'when merge requests are private' do + let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) } + + before do + project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update(merge_requests_access_level: ProjectFeature::PRIVATE) + end + + describe 'a non-team-member' do + subject { non_team_member } + it_behaves_like 'a denied user' + end + + describe 'a developer' do + subject { developer } + it_behaves_like 'a user with access' + end + end + context 'when merge request is unlocked' do let(:merge_request) { create(:merge_request, :closed, source_project: project, target_project: project, author: author) } @@ -48,6 +121,22 @@ describe MergeRequestPolicy do it 'prevents guests from reopening merge request' do expect(permissions(guest, merge_request_locked)).to be_disallowed(:reopen_merge_request) end + + context 'when the user is not a project member' do + let(:user) { create(:user) } + + it 'cannot create a note' do + expect(permissions(user, merge_request_locked)).to be_disallowed(:create_note) + end + end + + context 'when the user is project member, with at least guest access' do + let(:user) { guest } + + it 'can create a note' do + expect(permissions(user, merge_request_locked)).to be_allowed(:create_note) + end + end end context 'with external authorization enabled' do -- cgit v1.2.1 From 7dd545b130cb0cc2196ac15d7968736f679e8072 Mon Sep 17 00:00:00 2001 From: Elliot Rushton Date: Thu, 8 Aug 2019 01:00:23 +0000 Subject: Fix required runner permissions --- doc/ci/runners/README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/ci/runners/README.md b/doc/ci/runners/README.md index 03a219e03ca..f5f8e04755e 100644 --- a/doc/ci/runners/README.md +++ b/doc/ci/runners/README.md @@ -88,7 +88,7 @@ visit the project you want to make the Runner work for in GitLab: ## Registering a group Runner -Creating a group Runner requires Maintainer permissions for the group. To create a +Creating a group Runner requires Owner permissions for the group. To create a group Runner visit the group you want to make the Runner work for in GitLab: 1. Go to **Settings > CI/CD** to obtain the token @@ -124,9 +124,9 @@ To lock/unlock a Runner: ## Assigning a Runner to another project -If you are Maintainer on a project where a specific Runner is assigned to, and the +If you are an Owner on a project where a specific Runner is assigned to, and the Runner is not [locked only to that project](#locking-a-specific-runner-from-being-enabled-for-other-projects), -you can enable the Runner also on any other project where you have Maintainer permissions. +you can enable the Runner also on any other project where you have Owner permissions. To enable/disable a Runner in your project: @@ -250,7 +250,7 @@ When you [register a Runner][register], its default behavior is to **only pick** [tagged jobs](../yaml/README.md#tags). NOTE: **Note:** -Maintainer [permissions](../../user/permissions.md) are required to change the +Owner [permissions](../../user/permissions.md) are required to change the Runner settings. To make a Runner pick untagged jobs: -- cgit v1.2.1 From ae1f0aea0c11c5a64d4c7cb8d5f12a54346bc385 Mon Sep 17 00:00:00 2001 From: Nourdin el Bacha Date: Thu, 8 Aug 2019 10:45:47 +0000 Subject: improve grammar in Gitaly configuration --- doc/install/installation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/install/installation.md b/doc/install/installation.md index 72a3514e2d5..c11f803b819 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -611,7 +611,7 @@ You can specify a different Git repository by providing it as an extra parameter sudo -u git -H bundle exec rake "gitlab:gitaly:install[/home/git/gitaly,/home/git/repositories,https://example.com/gitaly.git]" RAILS_ENV=production ``` -Next, make sure gitaly configured: +Next, make sure that Gitaly is configured: ```sh # Restrict Gitaly socket access -- cgit v1.2.1 From 492a7e753d0ef06458163aecc5ca43892a5acc73 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Tue, 16 Jul 2019 16:49:47 -0300 Subject: Fix DNS rebind vulnerability for JIRA integration Uses Gitlab::HTTP for JIRA requests instead of Net::Http. Gitlab::Http comes with some built in SSRF protections. --- app/models/project_services/jira_service.rb | 7 ++- .../security-fix_jira_ssrf_vulnerability.yml | 5 ++ lib/gitlab/jira/http_client.rb | 66 ++++++++++++++++++++++ .../projects/services_controller_spec.rb | 5 ++ 4 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-fix_jira_ssrf_vulnerability.yml create mode 100644 lib/gitlab/jira/http_client.rb diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb index d08fcd8954d..0728c83005e 100644 --- a/app/models/project_services/jira_service.rb +++ b/app/models/project_services/jira_service.rb @@ -64,7 +64,12 @@ class JiraService < IssueTrackerService end def client - @client ||= JIRA::Client.new(options) + @client ||= begin + JIRA::Client.new(options).tap do |client| + # Replaces JIRA default http client with our implementation + client.request_client = Gitlab::Jira::HttpClient.new(client.options) + end + end end def help diff --git a/changelogs/unreleased/security-fix_jira_ssrf_vulnerability.yml b/changelogs/unreleased/security-fix_jira_ssrf_vulnerability.yml new file mode 100644 index 00000000000..25518dd2d05 --- /dev/null +++ b/changelogs/unreleased/security-fix_jira_ssrf_vulnerability.yml @@ -0,0 +1,5 @@ +--- +title: Prevent DNS rebind on JIRA service integration +merge_request: +author: +type: security diff --git a/lib/gitlab/jira/http_client.rb b/lib/gitlab/jira/http_client.rb new file mode 100644 index 00000000000..11a33a7b358 --- /dev/null +++ b/lib/gitlab/jira/http_client.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +module Gitlab + module Jira + # Gitlab JIRA HTTP client to be used with jira-ruby gem, this subclasses JIRA::HTTPClient. + # Uses Gitlab::HTTP to make requests to JIRA REST API. + # The parent class implementation can be found at: https://github.com/sumoheavy/jira-ruby/blob/v1.4.0/lib/jira/http_client.rb + class HttpClient < JIRA::HttpClient + extend ::Gitlab::Utils::Override + + override :request + def request(*args) + result = make_request(*args) + + raise JIRA::HTTPError.new(result) unless result.response.is_a?(Net::HTTPSuccess) + + result + end + + override :make_cookie_auth_request + def make_cookie_auth_request + body = { + username: @options.delete(:username), + password: @options.delete(:password) + }.to_json + + make_request(:post, @options[:context_path] + '/rest/auth/1/session', body, { 'Content-Type' => 'application/json' }) + end + + override :make_request + def make_request(http_method, path, body = '', headers = {}) + request_params = { headers: headers } + request_params[:body] = body if body.present? + request_params[:headers][:Cookie] = get_cookies if options[:use_cookies] + request_params[:timeout] = options[:read_timeout] if options[:read_timeout] + request_params[:base_uri] = uri.to_s + request_params.merge!(auth_params) + + result = Gitlab::HTTP.public_send(http_method, path, **request_params) # rubocop:disable GitlabSecurity/PublicSend + @authenticated = result.response.is_a?(Net::HTTPOK) + store_cookies(result) if options[:use_cookies] + + result + end + + def auth_params + return {} unless @options[:username] && @options[:password] + + { + basic_auth: { + username: @options[:username], + password: @options[:password] + } + } + end + + private + + def get_cookies + cookie_array = @cookies.values.map { |cookie| "#{cookie.name}=#{cookie.value[0]}" } + cookie_array += Array(@options[:additional_cookies]) if @options.key?(:additional_cookies) + cookie_array.join('; ') if cookie_array.any? + end + end + end +end diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb index 68eabce8513..22ae65ea2fb 100644 --- a/spec/controllers/projects/services_controller_spec.rb +++ b/spec/controllers/projects/services_controller_spec.rb @@ -11,6 +11,7 @@ describe Projects::ServicesController do before do sign_in(user) project.add_maintainer(user) + allow(Gitlab::UrlBlocker).to receive(:validate!).and_return([URI.parse('http://example.com'), nil]) end describe '#test' do @@ -56,6 +57,8 @@ describe Projects::ServicesController do stub_request(:get, 'http://example.com/rest/api/2/serverInfo') .to_return(status: 200, body: '{}') + expect(Gitlab::HTTP).to receive(:get).with("/rest/api/2/serverInfo", any_args).and_call_original + put :test, params: { namespace_id: project.namespace, project_id: project, id: service.to_param, service: service_params } expect(response.status).to eq(200) @@ -66,6 +69,8 @@ describe Projects::ServicesController do stub_request(:get, 'http://example.com/rest/api/2/serverInfo') .to_return(status: 200, body: '{}') + expect(Gitlab::HTTP).to receive(:get).with("/rest/api/2/serverInfo", any_args).and_call_original + put :test, params: { namespace_id: project.namespace, project_id: project, id: service.to_param, service: service_params } expect(response.status).to eq(200) -- cgit v1.2.1 From 1a7280220503a4bdcbed6e0b641a22be2368a626 Mon Sep 17 00:00:00 2001 From: Alexander Oleynikov Date: Thu, 8 Aug 2019 18:14:21 +0000 Subject: Remove inoperative >/dev/null --- doc/ci/ssh_keys/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/ci/ssh_keys/README.md b/doc/ci/ssh_keys/README.md index d9f022a7125..b6aebd3bd78 100644 --- a/doc/ci/ssh_keys/README.md +++ b/doc/ci/ssh_keys/README.md @@ -76,7 +76,7 @@ to access it. This is where an SSH key pair comes in handy. ## without extra base64 encoding. ## https://gitlab.com/gitlab-examples/ssh-private-key/issues/1#note_48526556 ## - - echo "$SSH_PRIVATE_KEY" | tr -d '\r' | ssh-add - > /dev/null + - echo "$SSH_PRIVATE_KEY" | tr -d '\r' | ssh-add - ## ## Create the SSH directory and give it the right permissions -- cgit v1.2.1 From 4882303760c9f22112985f78746f2416b701a47b Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Mon, 12 Aug 2019 18:13:12 +0200 Subject: Fixed deletion of directories in Web IDE --- app/assets/javascripts/ide/stores/utils.js | 2 +- .../unreleased/64677-delete-directory-webide.yml | 5 ++++ spec/javascripts/ide/stores/utils_spec.js | 35 ++++++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/64677-delete-directory-webide.yml diff --git a/app/assets/javascripts/ide/stores/utils.js b/app/assets/javascripts/ide/stores/utils.js index 04e86afb268..52200ce7847 100644 --- a/app/assets/javascripts/ide/stores/utils.js +++ b/app/assets/javascripts/ide/stores/utils.js @@ -129,7 +129,7 @@ export const commitActionForFile = file => { export const getCommitFiles = stagedFiles => stagedFiles.reduce((acc, file) => { - if (file.moved) return acc; + if (file.moved || file.type === 'tree') return acc; return acc.concat({ ...file, diff --git a/changelogs/unreleased/64677-delete-directory-webide.yml b/changelogs/unreleased/64677-delete-directory-webide.yml new file mode 100644 index 00000000000..27d596b6b19 --- /dev/null +++ b/changelogs/unreleased/64677-delete-directory-webide.yml @@ -0,0 +1,5 @@ +--- +title: Fixed removing directories in Web IDE +merge_request: 31727 +author: +type: fixed diff --git a/spec/javascripts/ide/stores/utils_spec.js b/spec/javascripts/ide/stores/utils_spec.js index bceb3a8db91..0fc9519a6bf 100644 --- a/spec/javascripts/ide/stores/utils_spec.js +++ b/spec/javascripts/ide/stores/utils_spec.js @@ -261,6 +261,41 @@ describe('Multi-file store utils', () => { }, ]); }); + + it('filters out folders from the list', () => { + const files = [ + { + path: 'a', + type: 'blob', + deleted: true, + }, + { + path: 'c', + type: 'tree', + deleted: true, + }, + { + path: 'c/d', + type: 'blob', + deleted: true, + }, + ]; + + const flattendFiles = utils.getCommitFiles(files); + + expect(flattendFiles).toEqual([ + { + path: 'a', + type: 'blob', + deleted: true, + }, + { + path: 'c/d', + type: 'blob', + deleted: true, + }, + ]); + }); }); describe('mergeTrees', () => { -- cgit v1.2.1 From 1c7c91806d4b9866f512f50f36c9c74b48cb8229 Mon Sep 17 00:00:00 2001 From: drew cimino Date: Mon, 22 Jul 2019 14:16:15 -0400 Subject: Permission fix for MergeRequestsController#pipeline_status - Use set_pipeline_variables to filter for visible pipelines - Mimic response of nonexistent pipeline if not found - Provide set_pipeline_variables as a before_filter for other actions --- .../projects/merge_requests_controller.rb | 9 ++++++- .../unreleased/security-mr-head-pipeline-leak.yml | 5 ++++ .../projects/merge_requests_controller_spec.rb | 30 +++++++++++++++++++--- 3 files changed, 39 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/security-mr-head-pipeline-leak.yml diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index f4d381244d9..ee755d68cf1 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -188,7 +188,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo def pipeline_status render json: PipelineSerializer .new(project: @project, current_user: @current_user) - .represent_status(@merge_request.head_pipeline) + .represent_status(head_pipeline) end def ci_environments_status @@ -238,6 +238,13 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo private + def head_pipeline + strong_memoize(:head_pipeline) do + pipeline = @merge_request.head_pipeline + pipeline if can?(current_user, :read_pipeline, pipeline) + end + end + def ci_environments_status_on_merge_result? params[:environment_target] == 'merge_commit' end diff --git a/changelogs/unreleased/security-mr-head-pipeline-leak.yml b/changelogs/unreleased/security-mr-head-pipeline-leak.yml new file mode 100644 index 00000000000..b15b353ff41 --- /dev/null +++ b/changelogs/unreleased/security-mr-head-pipeline-leak.yml @@ -0,0 +1,5 @@ +--- +title: Check permissions before responding in MergeController#pipeline_status +merge_request: +author: +type: security diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index b1dc6a65dd4..8a5fdbc98f4 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -1052,17 +1052,39 @@ describe Projects::MergeRequestsController do let(:status) { pipeline.detailed_status(double('user')) } - before do + it 'returns a detailed head_pipeline status in json' do get_pipeline_status - end - it 'return a detailed head_pipeline status in json' do expect(response).to have_gitlab_http_status(:ok) expect(json_response['text']).to eq status.text expect(json_response['label']).to eq status.label expect(json_response['icon']).to eq status.icon expect(json_response['favicon']).to match_asset_path "/assets/ci_favicons/#{status.favicon}.png" end + + context 'with project member visibility on a public project' do + let(:user) { create(:user) } + let(:project) { create(:project, :repository, :public, :builds_private) } + + it 'returns pipeline data to project members' do + project.add_developer(user) + + get_pipeline_status + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['text']).to eq status.text + expect(json_response['label']).to eq status.label + expect(json_response['icon']).to eq status.icon + expect(json_response['favicon']).to match_asset_path "/assets/ci_favicons/#{status.favicon}.png" + end + + it 'returns blank OK response to non-project-members' do + get_pipeline_status + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_empty + end + end end context 'when head_pipeline does not exist' do @@ -1070,7 +1092,7 @@ describe Projects::MergeRequestsController do get_pipeline_status end - it 'return empty' do + it 'returns blank OK response' do expect(response).to have_gitlab_http_status(:ok) expect(json_response).to be_empty end -- cgit v1.2.1 From 139df74253214fbab6aa703ed8ce381d0871aa46 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Mon, 12 Aug 2019 15:07:15 +0300 Subject: Add merge note type as cross reference --- app/models/system_note_metadata.rb | 2 +- .../unreleased/security-id-filter-timeline-activities-for-guests.yml | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/security-id-filter-timeline-activities-for-guests.yml diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 9a2640db9ca..a19755d286a 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -9,7 +9,7 @@ class SystemNoteMetadata < ApplicationRecord TYPES_WITH_CROSS_REFERENCES = %w[ commit cross_reference close duplicate - moved + moved merge ].freeze ICON_TYPES = %w[ diff --git a/changelogs/unreleased/security-id-filter-timeline-activities-for-guests.yml b/changelogs/unreleased/security-id-filter-timeline-activities-for-guests.yml new file mode 100644 index 00000000000..0fa5f89e2c0 --- /dev/null +++ b/changelogs/unreleased/security-id-filter-timeline-activities-for-guests.yml @@ -0,0 +1,5 @@ +--- +title: Show cross-referenced MR-id in issues' activities only to authorized users +merge_request: +author: +type: security -- cgit v1.2.1 From 8c2c4e4d75ec60182e37ddaf637bc25095bdd57f Mon Sep 17 00:00:00 2001 From: Takuya Noguchi Date: Wed, 14 Aug 2019 17:48:00 +0100 Subject: Upgrade babel to 7.5.5 Signed-off-by: Takuya Noguchi --- changelogs/unreleased/update-babel-to-7-5-5.yml | 5 ++++ package.json | 8 +++--- yarn.lock | 36 ++++++++++++------------- 3 files changed, 27 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/update-babel-to-7-5-5.yml diff --git a/changelogs/unreleased/update-babel-to-7-5-5.yml b/changelogs/unreleased/update-babel-to-7-5-5.yml new file mode 100644 index 00000000000..c498e2adfe8 --- /dev/null +++ b/changelogs/unreleased/update-babel-to-7-5-5.yml @@ -0,0 +1,5 @@ +--- +title: Upgrade babel to 7.5.5 +merge_request: 31819 +author: Takuya Noguchi +type: other diff --git a/package.json b/package.json index 803aebcb5fd..9c9a7687fc2 100644 --- a/package.json +++ b/package.json @@ -30,13 +30,13 @@ "webpack-vrt": "NODE_OPTIONS=\"--max-old-space-size=3584\" NODE_ENV=production webpack --config config/webpack.config.review_toolbar.js" }, "dependencies": { - "@babel/core": "^7.4.4", - "@babel/plugin-proposal-class-properties": "^7.4.4", + "@babel/core": "^7.5.5", + "@babel/plugin-proposal-class-properties": "^7.5.5", "@babel/plugin-proposal-json-strings": "^7.2.0", "@babel/plugin-proposal-private-methods": "^7.4.4", "@babel/plugin-syntax-dynamic-import": "^7.2.0", "@babel/plugin-syntax-import-meta": "^7.2.0", - "@babel/preset-env": "^7.4.4", + "@babel/preset-env": "^7.5.5", "@gitlab/csslab": "^1.9.0", "@gitlab/svgs": "^1.67.0", "@gitlab/ui": "5.15.0", @@ -145,7 +145,7 @@ "xterm": "^3.5.0" }, "devDependencies": { - "@babel/plugin-transform-modules-commonjs": "^7.2.0", + "@babel/plugin-transform-modules-commonjs": "^7.5.0", "@gitlab/eslint-config": "^1.6.0", "@gitlab/eslint-plugin-i18n": "^1.1.0", "@gitlab/eslint-plugin-vue-i18n": "^1.2.0", diff --git a/yarn.lock b/yarn.lock index ed1f06523c0..5a59f149e8a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9,7 +9,7 @@ dependencies: "@babel/highlight" "^7.0.0" -"@babel/core@>=7.2.2", "@babel/core@^7.1.0", "@babel/core@^7.1.2", "@babel/core@^7.4.4": +"@babel/core@>=7.2.2", "@babel/core@^7.1.0", "@babel/core@^7.1.2", "@babel/core@^7.5.5": version "7.5.5" resolved "https://registry.yarnpkg.com/@babel/core/-/core-7.5.5.tgz#17b2686ef0d6bc58f963dddd68ab669755582c30" integrity sha512-i4qoSr2KTtce0DmkuuQBV4AuQgGPUcPXMr9L5MyYAtk06z068lQ10a4O009fe5OB/DfNV+h+qqT7ddNV8UnRjg== @@ -139,16 +139,16 @@ "@babel/types" "^7.0.0" "@babel/helper-module-transforms@^7.1.0", "@babel/helper-module-transforms@^7.4.4": - version "7.4.4" - resolved "https://registry.yarnpkg.com/@babel/helper-module-transforms/-/helper-module-transforms-7.4.4.tgz#96115ea42a2f139e619e98ed46df6019b94414b8" - integrity sha512-3Z1yp8TVQf+B4ynN7WoHPKS8EkdTbgAEy0nU0rs/1Kw4pDgmvYH3rz3aI11KgxKCba2cn7N+tqzV1mY2HMN96w== + version "7.5.5" + resolved "https://registry.yarnpkg.com/@babel/helper-module-transforms/-/helper-module-transforms-7.5.5.tgz#f84ff8a09038dcbca1fd4355661a500937165b4a" + integrity sha512-jBeCvETKuJqeiaCdyaheF40aXnnU1+wkSiUs/IQg3tB85up1LyL8x77ClY8qJpuRJUcXQo+ZtdNESmZl4j56Pw== dependencies: "@babel/helper-module-imports" "^7.0.0" "@babel/helper-simple-access" "^7.1.0" "@babel/helper-split-export-declaration" "^7.4.4" "@babel/template" "^7.4.4" - "@babel/types" "^7.4.4" - lodash "^4.17.11" + "@babel/types" "^7.5.5" + lodash "^4.17.13" "@babel/helper-optimise-call-expression@^7.0.0": version "7.0.0" @@ -163,11 +163,11 @@ integrity sha512-CYAOUCARwExnEixLdB6sDm2dIJ/YgEAKDM1MOeMeZu9Ld/bDgVo8aiWrXwcY7OBh+1Ea2uUcVRcxKk0GJvW7QA== "@babel/helper-regex@^7.0.0", "@babel/helper-regex@^7.4.4": - version "7.4.4" - resolved "https://registry.yarnpkg.com/@babel/helper-regex/-/helper-regex-7.4.4.tgz#a47e02bc91fb259d2e6727c2a30013e3ac13c4a2" - integrity sha512-Y5nuB/kESmR3tKjU8Nkn1wMGEx1tjJX076HBMeL3XLQCu6vA/YRzuTW0bbb+qRnXvQGn+d6Rx953yffl8vEy7Q== + version "7.5.5" + resolved "https://registry.yarnpkg.com/@babel/helper-regex/-/helper-regex-7.5.5.tgz#0aa6824f7100a2e0e89c1527c23936c152cab351" + integrity sha512-CkCYQLkfkiugbRDO8eZn6lRuR8kzZoGXCg3149iTk5se7g6qykSpy3+hELSwquhu+TgHn8nkLiBwHvNX8Hofcw== dependencies: - lodash "^4.17.11" + lodash "^4.17.13" "@babel/helper-remap-async-to-generator@^7.1.0": version "7.1.0" @@ -225,9 +225,9 @@ "@babel/types" "^7.5.5" "@babel/highlight@^7.0.0": - version "7.0.0" - resolved "https://registry.yarnpkg.com/@babel/highlight/-/highlight-7.0.0.tgz#f710c38c8d458e6dd9a201afb637fcb781ce99e4" - integrity sha512-UFMC4ZeFC48Tpvj7C8UgLvtkaUuovQX+5xNWrsIoMG8o2z+XFKjKaN9iVmS84dPwVN00W4wPmqvYoZF3EGAsfw== + version "7.5.0" + resolved "https://registry.yarnpkg.com/@babel/highlight/-/highlight-7.5.0.tgz#56d11312bd9248fa619591d02472be6e8cb32540" + integrity sha512-7dV4eu9gBxoM0dAnj/BCFDW9LFU0zvTrkq0ugM7pnHEgguOEeOz1so2ZghEdzviYzQEED0r4EAgpsBChKy1TRQ== dependencies: chalk "^2.0.0" esutils "^2.0.2" @@ -252,7 +252,7 @@ "@babel/helper-remap-async-to-generator" "^7.1.0" "@babel/plugin-syntax-async-generators" "^7.2.0" -"@babel/plugin-proposal-class-properties@^7.1.0", "@babel/plugin-proposal-class-properties@^7.4.4": +"@babel/plugin-proposal-class-properties@^7.1.0", "@babel/plugin-proposal-class-properties@^7.5.5": version "7.5.5" resolved "https://registry.yarnpkg.com/@babel/plugin-proposal-class-properties/-/plugin-proposal-class-properties-7.5.5.tgz#a974cfae1e37c3110e71f3c6a2e48b8e71958cd4" integrity sha512-AF79FsnWFxjlaosgdi421vmYG6/jg79bVD0dpD44QdgobzHKuLZ6S3vl8la9qIeSwGi8i1fS0O1mfuDAAdo1/A== @@ -833,7 +833,7 @@ "@babel/helper-regex" "^7.4.4" regexpu-core "^4.5.4" -"@babel/preset-env@^7.1.0", "@babel/preset-env@^7.4.4": +"@babel/preset-env@^7.1.0", "@babel/preset-env@^7.5.5": version "7.5.5" resolved "https://registry.yarnpkg.com/@babel/preset-env/-/preset-env-7.5.5.tgz#bc470b53acaa48df4b8db24a570d6da1fef53c9a" integrity sha512-GMZQka/+INwsMz1A5UEql8tG015h5j/qjptpKY2gJ7giy8ohzU710YciJB5rcKsWGWHiW3RUnHib0E5/m3Tp3A== @@ -914,9 +914,9 @@ integrity sha512-FBMd0IiARPtH5aaOFUVki6evHiJQiY0pFy7fizyRF7dtwc+el3nwpzvhb9qBNzceG1OIJModG1xpE0DDFjPXwA== "@babel/standalone@^7.0.0": - version "7.3.4" - resolved "https://registry.yarnpkg.com/@babel/standalone/-/standalone-7.3.4.tgz#b622c1e522acef91b2a14f22bdcdd4f935a1a474" - integrity sha512-4L9c5i4WlGqbrjOVX0Yp8TIR5cEiw1/tPYYZENW/iuO2uI6viY38U7zALidzNfGdZIwNc+A/AWqMEWKeScWkBg== + version "7.5.5" + resolved "https://registry.yarnpkg.com/@babel/standalone/-/standalone-7.5.5.tgz#9d3143f6078ff408db694a4254bd6f03c5c33962" + integrity sha512-YIp5taErC4uvp4d5urJtWMui3cpvZt83x57l4oVJNvFtDzumf3pMgRmoTSpGuEzh1yzo7jHhg3mbQmMhmKPbjA== "@babel/template@^7.1.0", "@babel/template@^7.4.0", "@babel/template@^7.4.4": version "7.4.4" -- cgit v1.2.1 From 50956e5c85e29d0d1a7634068b782ffce73be479 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 12 Aug 2019 10:17:32 +1200 Subject: Link more issues in Design Management Limitations --- doc/user/project/issues/design_management.md | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/doc/user/project/issues/design_management.md b/doc/user/project/issues/design_management.md index bffbcb544e3..1324a90e00b 100644 --- a/doc/user/project/issues/design_management.md +++ b/doc/user/project/issues/design_management.md @@ -35,9 +35,19 @@ to be enabled: ## Limitations -- Files uploaded must have a file extension of either `png`, `jpg`, `jpeg`, `gif`, `bmp`, `tiff` or `ico`. The [`svg` extension is not yet supported](https://gitlab.com/gitlab-org/gitlab-ee/issues/12771). +- Files uploaded must have a file extension of either `png`, `jpg`, `jpeg`, `gif`, `bmp`, `tiff` or `ico`. + The [`svg` extension is not yet supported](https://gitlab.com/gitlab-org/gitlab-ee/issues/12771). +- Design uploads are limited to 10 files at a time. - [Designs cannot yet be deleted](https://gitlab.com/gitlab-org/gitlab-ee/issues/11089). -- Design Management is [not yet supported in the project export](https://gitlab.com/gitlab-org/gitlab-ee/issues/11090). +- Design Management is + [not yet supported in the project export](https://gitlab.com/gitlab-org/gitlab-ee/issues/11090). +- Design Management data + [isn't deleted when a project is destroyed](https://gitlab.com/gitlab-org/gitlab-ee/issues/13429) yet. +- Design Management data [won't be moved](https://gitlab.com/gitlab-org/gitlab-ee/issues/13426) + when an issue is moved, nor [deleted](https://gitlab.com/gitlab-org/gitlab-ee/issues/13427) + when an issue is deleted. +- Design Management + [isn't supported by Geo](https://gitlab.com/groups/gitlab-org/-/epics/1633) yet. ## The Design Management page -- cgit v1.2.1 From 06eddc3edfa739c0c685d461a58621b43c4ecd5d Mon Sep 17 00:00:00 2001 From: George Koltsov Date: Tue, 6 Aug 2019 14:05:18 +0100 Subject: Fix project import restricted visibility bypass Add Gitlab::VisibilityLevelChecker that verifies selected project visibility level (or overridden param) is not restricted when creating or importing a project --- app/services/projects/create_service.rb | 27 ++++--- .../unreleased/security-project-import-bypass.yml | 5 ++ lib/gitlab/visibility_level_checker.rb | 88 ++++++++++++++++++++++ spec/lib/gitlab/visibility_level_checker_spec.rb | 82 ++++++++++++++++++++ spec/services/projects/create_service_spec.rb | 68 +++++++++++++---- 5 files changed, 244 insertions(+), 26 deletions(-) create mode 100644 changelogs/unreleased/security-project-import-bypass.yml create mode 100644 lib/gitlab/visibility_level_checker.rb create mode 100644 spec/lib/gitlab/visibility_level_checker_spec.rb diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 89dc4375c63..942a45286b2 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -5,9 +5,11 @@ module Projects include ValidatesClassificationLabel def initialize(user, params) - @current_user, @params = user, params.dup - @skip_wiki = @params.delete(:skip_wiki) + @current_user, @params = user, params.dup + @skip_wiki = @params.delete(:skip_wiki) @initialize_with_readme = Gitlab::Utils.to_boolean(@params.delete(:initialize_with_readme)) + @import_data = @params.delete(:import_data) + @relations_block = @params.delete(:relations_block) end def execute @@ -15,14 +17,11 @@ module Projects return ::Projects::CreateFromTemplateService.new(current_user, params).execute end - import_data = params.delete(:import_data) - relations_block = params.delete(:relations_block) - @project = Project.new(params) # Make sure that the user is allowed to use the specified visibility level - unless Gitlab::VisibilityLevel.allowed_for?(current_user, @project.visibility_level) - deny_visibility_level(@project) + if project_visibility.restricted? + deny_visibility_level(@project, project_visibility.visibility_level) return @project end @@ -44,7 +43,7 @@ module Projects @project.namespace_id = current_user.namespace_id end - relations_block&.call(@project) + @relations_block&.call(@project) yield(@project) if block_given? validate_classification_label(@project, :external_authorization_classification_label) @@ -54,7 +53,7 @@ module Projects @project.creator = current_user - save_project_and_import_data(import_data) + save_project_and_import_data after_create_actions if @project.persisted? @@ -129,9 +128,9 @@ module Projects !@project.feature_available?(:wiki, current_user) || @skip_wiki end - def save_project_and_import_data(import_data) + def save_project_and_import_data Project.transaction do - @project.create_or_update_import_data(data: import_data[:data], credentials: import_data[:credentials]) if import_data + @project.create_or_update_import_data(data: @import_data[:data], credentials: @import_data[:credentials]) if @import_data if @project.save unless @project.gitlab_project_import? @@ -192,5 +191,11 @@ module Projects fail(error: @project.errors.full_messages.join(', ')) end end + + def project_visibility + @project_visibility ||= Gitlab::VisibilityLevelChecker + .new(current_user, @project, project_params: { import_data: @import_data }) + .level_restricted? + end end end diff --git a/changelogs/unreleased/security-project-import-bypass.yml b/changelogs/unreleased/security-project-import-bypass.yml new file mode 100644 index 00000000000..fc7b823509c --- /dev/null +++ b/changelogs/unreleased/security-project-import-bypass.yml @@ -0,0 +1,5 @@ +--- +title: Fix project import restricted visibility bypass via API +merge_request: +author: +type: security diff --git a/lib/gitlab/visibility_level_checker.rb b/lib/gitlab/visibility_level_checker.rb new file mode 100644 index 00000000000..f15f1486a4e --- /dev/null +++ b/lib/gitlab/visibility_level_checker.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +# Gitlab::VisibilityLevelChecker verifies that: +# - Current @project.visibility_level is not restricted +# - Override visibility param is not restricted +# - @see https://docs.gitlab.com/ce/api/project_import_export.html#import-a-file +# +# @param current_user [User] Current user object to verify visibility level against +# @param project [Project] Current project that is being created/imported +# @param project_params [Hash] Supplementary project params (e.g. import +# params containing visibility override) +# +# @example +# user = User.find(2) +# project = Project.last +# project_params = {:import_data=>{:data=>{:override_params=>{"visibility"=>"public"}}}} +# level_checker = Gitlab::VisibilityLevelChecker.new(user, project, project_params: project_params) +# +# project_visibility = level_checker.level_restricted? +# => # +# +# if project_visibility.restricted? +# deny_visibility_level(project, project_visibility.visibility_level) +# end +# +# @return [VisibilityEvaluationResult] Visibility evaluation result. Responds to: +# #restricted - boolean indicating if level is restricted +# #visibility_level - integer of restricted visibility level +# +module Gitlab + class VisibilityLevelChecker + def initialize(current_user, project, project_params: {}) + @current_user = current_user + @project = project + @project_params = project_params + end + + def level_restricted? + return VisibilityEvaluationResult.new(true, override_visibility_level_value) if override_visibility_restricted? + return VisibilityEvaluationResult.new(true, project.visibility_level) if project_visibility_restricted? + + VisibilityEvaluationResult.new(false, nil) + end + + private + + attr_reader :current_user, :project, :project_params + + def override_visibility_restricted? + return unless import_data + return unless override_visibility_level + return if Gitlab::VisibilityLevel.allowed_for?(current_user, override_visibility_level_value) + + true + end + + def project_visibility_restricted? + return if Gitlab::VisibilityLevel.allowed_for?(current_user, project.visibility_level) + + true + end + + def import_data + @import_data ||= project_params[:import_data] + end + + def override_visibility_level + @override_visibility_level ||= import_data.deep_symbolize_keys.dig(:data, :override_params, :visibility) + end + + def override_visibility_level_value + @override_visibility_level_value ||= Gitlab::VisibilityLevel.level_value(override_visibility_level) + end + end + + class VisibilityEvaluationResult + attr_reader :visibility_level + + def initialize(restricted, visibility_level) + @restricted = restricted + @visibility_level = visibility_level + end + + def restricted? + @restricted + end + end +end diff --git a/spec/lib/gitlab/visibility_level_checker_spec.rb b/spec/lib/gitlab/visibility_level_checker_spec.rb new file mode 100644 index 00000000000..325ac3c6f31 --- /dev/null +++ b/spec/lib/gitlab/visibility_level_checker_spec.rb @@ -0,0 +1,82 @@ +require 'spec_helper' + +describe Gitlab::VisibilityLevelChecker do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:visibility_level_checker) { } + let(:override_params) { {} } + + subject { described_class.new(user, project, project_params: override_params) } + + describe '#level_restricted?' do + context 'when visibility level is allowed' do + it 'returns false with nil for visibility level' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(false) + expect(result.visibility_level).to be_nil + end + end + + context 'when visibility level is restricted' do + before do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + end + + it 'returns true and visibility name' do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + result = subject.level_restricted? + + expect(result.restricted?).to eq(true) + expect(result.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) + end + + context 'overridden visibility' do + let(:override_params) do + { + import_data: { + data: { + override_params: { + visibility: override_visibility + } + } + } + } + end + + context 'when restricted' do + let(:override_visibility) { 'public' } + + it 'returns true and visibility name' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(true) + expect(result.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) + end + end + + context 'when misspelled' do + let(:override_visibility) { 'publik' } + + it 'returns false with nil for visibility level' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(false) + expect(result.visibility_level).to be_nil + end + end + + context 'when import_data is missing' do + let(:override_params) { {} } + + it 'returns false with nil for visibility level' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(false) + expect(result.visibility_level).to be_nil + end + end + end + end + end +end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index b0b74407812..a87ae7bfd37 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -182,27 +182,65 @@ describe Projects::CreateService, '#execute' do context 'restricted visibility level' do before do stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + end - opts.merge!( - visibility_level: Gitlab::VisibilityLevel::PUBLIC - ) + shared_examples 'restricted visibility' do + it 'does not allow a restricted visibility level for non-admins' do + project = create_project(user, opts) + + expect(project).to respond_to(:errors) + expect(project.errors.messages).to have_key(:visibility_level) + expect(project.errors.messages[:visibility_level].first).to( + match('restricted by your GitLab administrator') + ) + end + + it 'allows a restricted visibility level for admins' do + admin = create(:admin) + project = create_project(admin, opts) + + expect(project.errors.any?).to be(false) + expect(project.saved?).to be(true) + end end - it 'does not allow a restricted visibility level for non-admins' do - project = create_project(user, opts) - expect(project).to respond_to(:errors) - expect(project.errors.messages).to have_key(:visibility_level) - expect(project.errors.messages[:visibility_level].first).to( - match('restricted by your GitLab administrator') - ) + context 'when visibility is project based' do + before do + opts.merge!( + visibility_level: Gitlab::VisibilityLevel::PUBLIC + ) + end + + include_examples 'restricted visibility' end - it 'allows a restricted visibility level for admins' do - admin = create(:admin) - project = create_project(admin, opts) + context 'when visibility is overridden' do + let(:visibility) { 'public' } - expect(project.errors.any?).to be(false) - expect(project.saved?).to be(true) + before do + opts.merge!( + import_data: { + data: { + override_params: { + visibility: visibility + } + } + } + ) + end + + include_examples 'restricted visibility' + + context 'when visibility is misspelled' do + let(:visibility) { 'publik' } + + it 'does not restrict project creation' do + project = create_project(user, opts) + + expect(project.errors.any?).to be(false) + expect(project.saved?).to be(true) + end + end end end -- cgit v1.2.1 From b179b752e8024679e6ce6fbd2a5fe1da17fef3f6 Mon Sep 17 00:00:00 2001 From: Arun Kumar Mohan Date: Tue, 13 Aug 2019 01:04:10 -0500 Subject: Refactor nextUnresolvedDiscussionId and previousUnresolvedDiscussionId getters --- app/assets/javascripts/notes/stores/getters.js | 39 +++++--- spec/javascripts/notes/stores/getters_spec.js | 130 +++++++++++++++++-------- 2 files changed, 112 insertions(+), 57 deletions(-) diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 52410f18d4a..3d0ec8cd3a7 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -171,26 +171,33 @@ export const isLastUnresolvedDiscussion = (state, getters) => (discussionId, dif return lastDiscussionId === discussionId; }; -// Gets the ID of the discussion following the one provided, respecting order (diff or date) -// @param {Boolean} discussionId - id of the current discussion -// @param {Boolean} diffOrder - is ordered by diff? -export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => { - const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); - const currentIndex = idsOrdered.indexOf(discussionId); - const slicedIds = idsOrdered.slice(currentIndex + 1, currentIndex + 2); +export const findUnresolvedDiscussionIdNeighbor = (state, getters) => ({ + discussionId, + diffOrder, + step, +}) => { + const ids = getters.unresolvedDiscussionsIdsOrdered(diffOrder); + const index = ids.indexOf(discussionId) + step; + + if (index < 0 && step < 0) { + return ids[ids.length - 1]; + } + + if (index === ids.length && step > 0) { + return ids[0]; + } - // Get the first ID if there is none after the currentIndex - return slicedIds.length ? idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0] : idsOrdered[0]; + return ids[index]; }; -export const previousUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => { - const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); - const currentIndex = idsOrdered.indexOf(discussionId); - const slicedIds = idsOrdered.slice(currentIndex - 1, currentIndex); +// Gets the ID of the discussion following the one provided, respecting order (diff or date) +// @param {Boolean} discussionId - id of the current discussion +// @param {Boolean} diffOrder - is ordered by diff? +export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => + getters.findUnresolvedDiscussionIdNeighbor({ discussionId, diffOrder, step: 1 }); - // Get the last ID if there is none after the currentIndex - return slicedIds.length ? slicedIds[0] : idsOrdered[idsOrdered.length - 1]; -}; +export const previousUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => + getters.findUnresolvedDiscussionIdNeighbor({ discussionId, diffOrder, step: -1 }); // @param {Boolean} diffOrder - is ordered by diff? export const firstUnresolvedDiscussionId = (state, getters) => diffOrder => { diff --git a/spec/javascripts/notes/stores/getters_spec.js b/spec/javascripts/notes/stores/getters_spec.js index 71dcba114a9..d69f469c7c7 100644 --- a/spec/javascripts/notes/stores/getters_spec.js +++ b/spec/javascripts/notes/stores/getters_spec.js @@ -14,6 +14,13 @@ import { const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json'; +// Helper function to ensure that we're using the same schema across tests. +const createDiscussionNeighborParams = (discussionId, diffOrder, step) => ({ + discussionId, + diffOrder, + step, +}); + describe('Getters Notes Store', () => { let state; @@ -25,7 +32,6 @@ describe('Getters Notes Store', () => { targetNoteHash: 'hash', lastFetchedAt: 'timestamp', isNotesFetched: false, - notesData: notesDataMock, userData: userDataMock, noteableData: noteableDataMock, @@ -244,62 +250,104 @@ describe('Getters Notes Store', () => { }); }); - describe('nextUnresolvedDiscussionId', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], - }; + describe('findUnresolvedDiscussionIdNeighbor', () => { + let localGetters; + beforeEach(() => { + localGetters = { + unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], + }; + }); - it('should return the ID of the discussion after the ID provided', () => { - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('123')).toBe('456'); - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('456')).toBe('789'); - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('789')).toBe('123'); + [ + { step: 1, id: '123', expected: '456' }, + { step: 1, id: '456', expected: '789' }, + { step: 1, id: '789', expected: '123' }, + { step: -1, id: '123', expected: '789' }, + { step: -1, id: '456', expected: '123' }, + { step: -1, id: '789', expected: '456' }, + ].forEach(({ step, id, expected }) => { + it(`with step ${step} and id ${id}, returns next value`, () => { + const params = createDiscussionNeighborParams(id, true, step); + + expect(getters.findUnresolvedDiscussionIdNeighbor(state, localGetters)(params)).toBe( + expected, + ); + }); }); - }); - describe('previousUnresolvedDiscussionId', () => { - describe('with unresolved discussions', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], - }; + describe('with 1 unresolved discussion', () => { + beforeEach(() => { + localGetters = { + unresolvedDiscussionsIdsOrdered: () => ['123'], + }; + }); + + [{ step: 1, id: '123', expected: '123' }, { step: -1, id: '123', expected: '123' }].forEach( + ({ step, id, expected }) => { + it(`with step ${step} and match, returns only value`, () => { + const params = createDiscussionNeighborParams(id, true, step); - it('with bogus returns falsey', () => { - expect(getters.previousUnresolvedDiscussionId(state, localGetters)('bogus')).toBe('456'); + expect(getters.findUnresolvedDiscussionIdNeighbor(state, localGetters)(params)).toBe( + expected, + ); + }); + }, + ); + + it('with no match, returns only value', () => { + const params = createDiscussionNeighborParams('bogus', true, 1); + + expect(getters.findUnresolvedDiscussionIdNeighbor(state, localGetters)(params)).toBe('123'); }); + }); - [ - { id: '123', expected: '789' }, - { id: '456', expected: '123' }, - { id: '789', expected: '456' }, - ].forEach(({ id, expected }) => { - it(`with ${id}, returns previous value`, () => { - expect(getters.previousUnresolvedDiscussionId(state, localGetters)(id)).toBe(expected); + describe('with 0 unresolved discussions', () => { + beforeEach(() => { + localGetters = { + unresolvedDiscussionsIdsOrdered: () => [], + }; + }); + + [{ step: 1 }, { step: -1 }].forEach(({ step }) => { + it(`with step ${step}, returns undefined`, () => { + const params = createDiscussionNeighborParams('bogus', true, step); + + expect( + getters.findUnresolvedDiscussionIdNeighbor(state, localGetters)(params), + ).toBeUndefined(); }); }); }); + }); - describe('with 1 unresolved discussion', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => ['123'], - }; + describe('findUnresolvedDiscussionIdNeighbor aliases', () => { + let neighbor; + let findUnresolvedDiscussionIdNeighbor; + let localGetters; - it('with bogus returns id', () => { - expect(getters.previousUnresolvedDiscussionId(state, localGetters)('bogus')).toBe('123'); - }); + beforeEach(() => { + neighbor = {}; + findUnresolvedDiscussionIdNeighbor = jasmine.createSpy().and.returnValue(neighbor); + localGetters = { findUnresolvedDiscussionIdNeighbor }; + }); - it('with match, returns value', () => { - expect(getters.previousUnresolvedDiscussionId(state, localGetters)('123')).toEqual('123'); + describe('nextUnresolvedDiscussionId', () => { + it('should return result of find neighbor', () => { + const expectedParams = createDiscussionNeighborParams('123', true, 1); + const result = getters.nextUnresolvedDiscussionId(state, localGetters)('123', true); + + expect(findUnresolvedDiscussionIdNeighbor).toHaveBeenCalledWith(expectedParams); + expect(result).toBe(neighbor); }); }); - describe('with 0 unresolved discussions', () => { - const localGetters = { - unresolvedDiscussionsIdsOrdered: () => [], - }; + describe('previosuUnresolvedDiscussionId', () => { + it('should return result of find neighbor', () => { + const expectedParams = createDiscussionNeighborParams('123', true, -1); + const result = getters.previousUnresolvedDiscussionId(state, localGetters)('123', true); - it('returns undefined', () => { - expect( - getters.previousUnresolvedDiscussionId(state, localGetters)('bogus'), - ).toBeUndefined(); + expect(findUnresolvedDiscussionIdNeighbor).toHaveBeenCalledWith(expectedParams); + expect(result).toBe(neighbor); }); }); }); -- cgit v1.2.1 From 790a1cd911d6394eeb3865ec51fcb944fcd26b21 Mon Sep 17 00:00:00 2001 From: ddavison Date: Thu, 15 Aug 2019 13:36:38 -0700 Subject: Ensure autodevops is enabled before running Smoke test Extract #enable_autodevops to new page object Change Smoke test naming to only running pipeline Generate AutoDevOps project with SecureRandom suffix --- qa/qa.rb | 1 + qa/qa/page/project/settings/auto_devops.rb | 21 +++++++++++++++++++++ qa/qa/page/project/settings/ci_cd.rb | 10 ++-------- .../create_project_with_auto_devops_spec.rb | 16 ++++++++++++---- 4 files changed, 36 insertions(+), 12 deletions(-) create mode 100644 qa/qa/page/project/settings/auto_devops.rb diff --git a/qa/qa.rb b/qa/qa.rb index 18fb4509dce..b6fc7f5071e 100644 --- a/qa/qa.rb +++ b/qa/qa.rb @@ -204,6 +204,7 @@ module QA autoload :Main, 'qa/page/project/settings/main' autoload :Repository, 'qa/page/project/settings/repository' autoload :CICD, 'qa/page/project/settings/ci_cd' + autoload :AutoDevops, 'qa/page/project/settings/auto_devops' autoload :DeployKeys, 'qa/page/project/settings/deploy_keys' autoload :DeployTokens, 'qa/page/project/settings/deploy_tokens' autoload :ProtectedBranches, 'qa/page/project/settings/protected_branches' diff --git a/qa/qa/page/project/settings/auto_devops.rb b/qa/qa/page/project/settings/auto_devops.rb new file mode 100644 index 00000000000..827d5b072c3 --- /dev/null +++ b/qa/qa/page/project/settings/auto_devops.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module QA + module Page + module Project + module Settings + class AutoDevops < Page::Base + view 'app/views/projects/settings/ci_cd/_autodevops_form.html.haml' do + element :enable_autodevops_checkbox + element :save_changes_button + end + + def enable_autodevops + check_element :enable_autodevops_checkbox + click_element :save_changes_button + end + end + end + end + end +end diff --git a/qa/qa/page/project/settings/ci_cd.rb b/qa/qa/page/project/settings/ci_cd.rb index ae826fb3a32..45040cf4660 100644 --- a/qa/qa/page/project/settings/ci_cd.rb +++ b/qa/qa/page/project/settings/ci_cd.rb @@ -13,11 +13,6 @@ module QA element :variables_settings_content end - view 'app/views/projects/settings/ci_cd/_autodevops_form.html.haml' do - element :enable_autodevops_checkbox - element :save_changes_button - end - def expand_runners_settings(&block) expand_section(:runners_settings_content) do Settings::Runners.perform(&block) @@ -30,10 +25,9 @@ module QA end end - def enable_auto_devops + def expand_auto_devops(&block) expand_section(:autodevops_settings_content) do - check_element :enable_autodevops_checkbox - click_element :save_changes_button + Settings::AutoDevops.perform(&block) end end end diff --git a/qa/qa/specs/features/browser_ui/7_configure/auto_devops/create_project_with_auto_devops_spec.rb b/qa/qa/specs/features/browser_ui/7_configure/auto_devops/create_project_with_auto_devops_spec.rb index 60c1e105ae6..3f99ae644c7 100644 --- a/qa/qa/specs/features/browser_ui/7_configure/auto_devops/create_project_with_auto_devops_spec.rb +++ b/qa/qa/specs/features/browser_ui/7_configure/auto_devops/create_project_with_auto_devops_spec.rb @@ -147,23 +147,31 @@ module QA end describe 'Auto DevOps', :smoke do - it 'enables AutoDevOps by default' do + before do login - project = Resource::Project.fabricate! do |p| - p.name = Runtime::Env.auto_devops_project_name || 'project-with-autodevops' + @project = Resource::Project.fabricate_via_browser_ui! do |p| + p.name = "project-with-autodevops-#{SecureRandom.hex(8)}" p.description = 'Project with AutoDevOps' end + Page::Project::Menu.perform(&:go_to_ci_cd_settings) + Page::Project::Settings::CICD.perform(&:expand_auto_devops) + Page::Project::Settings::AutoDevops.perform(&:enable_autodevops) + + @project.visit! + # Create AutoDevOps repo Resource::Repository::ProjectPush.fabricate! do |push| - push.project = project + push.project = @project push.directory = Pathname .new(__dir__) .join('../../../../../fixtures/auto_devops_rack') push.commit_message = 'Create AutoDevOps compatible Project' end + end + it 'runs an AutoDevOps pipeline' do Page::Project::Menu.perform(&:click_ci_cd_pipelines) Page::Project::Pipeline::Index.perform(&:click_on_latest_pipeline) -- cgit v1.2.1 From a5a316baf3146916a1eb3b90b5b9906318d22ddb Mon Sep 17 00:00:00 2001 From: Zeff Morgan Date: Thu, 15 Aug 2019 23:10:37 +0000 Subject: Correct case of GitLab in section header --- doc/user/project/integrations/prometheus.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/project/integrations/prometheus.md b/doc/user/project/integrations/prometheus.md index aa7db97c413..787ac3fce5c 100644 --- a/doc/user/project/integrations/prometheus.md +++ b/doc/user/project/integrations/prometheus.md @@ -354,7 +354,7 @@ Prometheus server. ![Merge Request with Performance Impact](img/merge_request_performance.png) -## Embedding metric charts within Gitlab Flavored Markdown +## Embedding metric charts within GitLab Flavored Markdown > [Introduced][ce-29691] in GitLab 12.2. > Requires [Kubernetes](prometheus_library/kubernetes.md) metrics. -- cgit v1.2.1 From 880c9a25a8738a4f4be26b147655b4cb6ac89d4e Mon Sep 17 00:00:00 2001 From: Steve Azzopardi Date: Thu, 25 Jul 2019 14:14:31 +0200 Subject: Update srmX configuration --- doc/user/gitlab_com/index.md | 83 ++++++++++---------------------------------- 1 file changed, 19 insertions(+), 64 deletions(-) diff --git a/doc/user/gitlab_com/index.md b/doc/user/gitlab_com/index.md index c9fbd7effa0..d21a325d401 100644 --- a/doc/user/gitlab_com/index.md +++ b/doc/user/gitlab_com/index.md @@ -112,57 +112,6 @@ Below are the shared Runners settings. The full contents of our `config.toml` are: -**DigitalOcean** - -```toml -concurrent = X -check_interval = 1 -metrics_server = "X" -sentry_dsn = "X" - -[[runners]] - name = "docker-auto-scale" - request_concurrency = X - url = "https://gitlab.com/" - token = "SHARED_RUNNER_TOKEN" - executor = "docker+machine" - environment = [ - "DOCKER_DRIVER=overlay2" - ] - limit = X - [runners.docker] - image = "ruby:2.5" - privileged = true - [runners.machine] - IdleCount = 20 - IdleTime = 1800 - OffPeakPeriods = ["* * * * * sat,sun *"] - OffPeakTimezone = "UTC" - OffPeakIdleCount = 5 - OffPeakIdleTime = 1800 - MaxBuilds = 1 - MachineName = "srm-%s" - MachineDriver = "digitalocean" - MachineOptions = [ - "digitalocean-image=X", - "digitalocean-ssh-user=core", - "digitalocean-region=nyc1", - "digitalocean-size=s-2vcpu-2gb", - "digitalocean-private-networking", - "digitalocean-tags=shared_runners,gitlab_com", - "engine-registry-mirror=http://INTERNAL_IP_OF_OUR_REGISTRY_MIRROR", - "digitalocean-access-token=DIGITAL_OCEAN_ACCESS_TOKEN", - ] - [runners.cache] - Type = "s3" - BucketName = "runner" - Insecure = true - Shared = true - ServerAddress = "INTERNAL_IP_OF_OUR_CACHE_SERVER" - AccessKey = "ACCESS_KEY" - SecretKey = "ACCESS_SECRET_KEY" -``` - **Google Cloud Platform** ```toml @@ -178,20 +127,25 @@ sentry_dsn = "X" token = "SHARED_RUNNER_TOKEN" executor = "docker+machine" environment = [ - "DOCKER_DRIVER=overlay2" + "DOCKER_DRIVER=overlay2", + "DOCKER_TLS_CERTDIR=" ] limit = X [runners.docker] image = "ruby:2.5" privileged = true + volumes = [ + "/certs/client", + "/dummy-sys-class-dmi-id:/sys/class/dmi/id:ro" # Make kaniko builds work on GCP. + ] [runners.machine] - IdleCount = 20 - IdleTime = 1800 + IdleCount = 50 + IdleTime = 3600 OffPeakPeriods = ["* * * * * sat,sun *"] OffPeakTimezone = "UTC" - OffPeakIdleCount = 5 - OffPeakIdleTime = 1800 - MaxBuilds = 1 + OffPeakIdleCount = 15 + OffPeakIdleTime = 3600 + MaxBuilds = 1 # For security reasons we delete the VM after job has finished so it's not reused. MachineName = "srm-%s" MachineDriver = "google" MachineOptions = [ @@ -202,17 +156,18 @@ sentry_dsn = "X" "google-tags=gitlab-com,srm", "google-use-internal-ip", "google-zone=us-east1-d", + "engine-opt=mtu=1460", # Set MTU for container interface, for more information check https://gitlab.com/gitlab-org/gitlab-runner/issues/3214#note_82892928 "google-machine-image=PROJECT/global/images/IMAGE", - "engine-registry-mirror=http://INTERNAL_IP_OF_OUR_REGISTRY_MIRROR" + "engine-opt=ipv6", # This will create IPv6 interfaces in the containers. + "engine-opt=fixed-cidr-v6=fc00::/7", + "google-operation-backoff-initial-interval=2" # Custom flag from forked docker-machine, for more information check https://github.com/docker/machine/pull/4600 ] [runners.cache] - Type = "s3" - BucketName = "runner" - Insecure = true + Type = "gcs" Shared = true - ServerAddress = "INTERNAL_IP_OF_OUR_CACHE_SERVER" - AccessKey = "ACCESS_KEY" - SecretKey = "ACCESS_SECRET_KEY" + [runners.cache.gcs] + CredentialsFile = "/path/to/file" + BucketName = "bucket-name" ``` ## Sidekiq -- cgit v1.2.1 From 5a574883f95373e13f663568eb4710c9d69d00d6 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Mon, 12 Aug 2019 11:29:10 +0100 Subject: Remove MySQL references from development docs I noticed the doc/development/testing_guide/best_practices.md still referenced the `[run mysql]` tags, etc. They no longer work, so I removed them, then realised I had better clean up the rest of doc/development ! --- doc/development/architecture.md | 22 ++++++++-------- doc/development/hash_indexes.md | 2 +- doc/development/rake_tasks.md | 2 +- doc/development/sha1_as_binary.md | 2 +- doc/development/sql.md | 30 +++++++--------------- doc/development/testing_guide/best_practices.md | 10 -------- doc/development/testing_guide/ci.md | 1 - doc/development/testing_guide/flaky_tests.md | 4 +-- doc/development/verifying_database_capabilities.md | 8 +++--- doc/development/what_requires_downtime.md | 25 ++++-------------- 10 files changed, 34 insertions(+), 72 deletions(-) diff --git a/doc/development/architecture.md b/doc/development/architecture.md index 87405bc2fec..5cb2ddf6e52 100644 --- a/doc/development/architecture.md +++ b/doc/development/architecture.md @@ -20,7 +20,7 @@ A typical install of GitLab will be on GNU/Linux. It uses Nginx or Apache as a w We also support deploying GitLab on Kubernetes using our [gitlab Helm chart](https://docs.gitlab.com/charts/). -The GitLab web app uses MySQL or PostgreSQL for persistent database information (e.g. users, permissions, issues, other meta data). GitLab stores the bare git repositories it serves in `/home/git/repositories` by default. It also keeps default branch and hook information with the bare repository. +The GitLab web app uses PostgreSQL for persistent database information (e.g. users, permissions, issues, other meta data). GitLab stores the bare git repositories it serves in `/home/git/repositories` by default. It also keeps default branch and hook information with the bare repository. When serving repositories over HTTP/HTTPS GitLab utilizes the GitLab API to resolve authorization and access as well as serving git objects. @@ -511,7 +511,15 @@ To summarize here's the [directory structure of the `git` user home directory](. ps aux | grep '^git' ``` -GitLab has several components to operate. As a system user (i.e. any user that is not the `git` user) it requires a persistent database (MySQL/PostreSQL) and redis database. It also uses Apache httpd or Nginx to proxypass Unicorn. As the `git` user it starts Sidekiq and Unicorn (a simple ruby HTTP server running on port `8080` by default). Under the GitLab user there are normally 4 processes: `unicorn_rails master` (1 process), `unicorn_rails worker` (2 processes), `sidekiq` (1 process). +GitLab has several components to operate. It requires a persistent database +(PostgreSQL) and redis database, and uses Apache httpd or Nginx to proxypass +Unicorn. All these components should run as different system users to GitLab +(e.g., `postgres`, `redis` and `www-data`, instead of `git`). + +As the `git` user it starts Sidekiq and Unicorn (a simple ruby HTTP server +running on port `8080` by default). Under the GitLab user there are normally 4 +processes: `unicorn_rails master` (1 process), `unicorn_rails worker` +(2 processes), `sidekiq` (1 process). ### Repository access @@ -554,12 +562,9 @@ $ /etc/init.d/nginx Usage: nginx {start|stop|restart|reload|force-reload|status|configtest} ``` -Persistent database (one of the following) +Persistent database ``` -/etc/init.d/mysqld -Usage: /etc/init.d/mysqld {start|stop|status|restart|condrestart|try-restart|reload|force-reload} - $ /etc/init.d/postgresql Usage: /etc/init.d/postgresql {start|stop|restart|reload|force-reload|status} [version ..] ``` @@ -597,11 +602,6 @@ PostgreSQL - `/var/log/postgresql/*` -MySQL - -- `/var/log/mysql/*` -- `/var/log/mysql.*` - ### GitLab specific config files GitLab has configuration files located in `/home/git/gitlab/config/*`. Commonly referenced config files include: diff --git a/doc/development/hash_indexes.md b/doc/development/hash_indexes.md index e6c1b3590b1..417ea18e22f 100644 --- a/doc/development/hash_indexes.md +++ b/doc/development/hash_indexes.md @@ -1,6 +1,6 @@ # Hash Indexes -Both PostgreSQL and MySQL support hash indexes besides the regular btree +PostgreSQL supports hash indexes besides the regular btree indexes. Hash indexes however are to be avoided at all costs. While they may _sometimes_ provide better performance the cost of rehashing can be very high. More importantly: at least until PostgreSQL 10.0 hash indexes are not diff --git a/doc/development/rake_tasks.md b/doc/development/rake_tasks.md index c97e179910b..e9d6cfe00b2 100644 --- a/doc/development/rake_tasks.md +++ b/doc/development/rake_tasks.md @@ -9,7 +9,7 @@ bundle exec rake setup ``` The `setup` task is an alias for `gitlab:setup`. -This tasks calls `db:reset` to create the database, calls `add_limits_mysql` that adds limits to the database schema in case of a MySQL database and finally it calls `db:seed_fu` to seed the database. +This tasks calls `db:reset` to create the database, and calls `db:seed_fu` to seed the database. Note: `db:setup` calls `db:seed` but this does nothing. ### Seeding issues for all or a given project diff --git a/doc/development/sha1_as_binary.md b/doc/development/sha1_as_binary.md index 3151cc29bbc..6c4252ec634 100644 --- a/doc/development/sha1_as_binary.md +++ b/doc/development/sha1_as_binary.md @@ -2,7 +2,7 @@ Storing SHA1 hashes as strings is not very space efficient. A SHA1 as a string requires at least 40 bytes, an additional byte to store the encoding, and -perhaps more space depending on the internals of PostgreSQL and MySQL. +perhaps more space depending on the internals of PostgreSQL. On the other hand, if one were to store a SHA1 as binary one would only need 20 bytes for the actual SHA1, and 1 or 4 bytes of additional space (again depending diff --git a/doc/development/sql.md b/doc/development/sql.md index a256fd46c09..2584dcfb4ca 100644 --- a/doc/development/sql.md +++ b/doc/development/sql.md @@ -15,14 +15,11 @@ FROM issues WHERE title LIKE 'WIP:%'; ``` -On PostgreSQL the `LIKE` statement is case-sensitive. On MySQL this depends on -the case-sensitivity of the collation, which is usually case-insensitive. To -perform a case-insensitive `LIKE` on PostgreSQL you have to use `ILIKE` instead. -This statement in turn isn't supported on MySQL. +On PostgreSQL the `LIKE` statement is case-sensitive. To perform a case-insensitive +`LIKE` you have to use `ILIKE` instead. -To work around this problem you should write `LIKE` queries using Arel instead -of raw SQL fragments as Arel automatically uses `ILIKE` on PostgreSQL and `LIKE` -on MySQL. This means that instead of this: +To handle this automatically you should use `LIKE` queries using Arel instead +of raw SQL fragments, as Arel automatically uses `ILIKE` on PostgreSQL. ```ruby Issue.where('title LIKE ?', 'WIP:%') @@ -45,7 +42,7 @@ table = Issue.arel_table Issue.where(table[:title].matches('WIP:%').or(table[:foo].matches('WIP:%'))) ``` -For PostgreSQL this produces: +On PostgreSQL, this produces: ```sql SELECT * @@ -53,18 +50,10 @@ FROM issues WHERE (title ILIKE 'WIP:%' OR foo ILIKE 'WIP:%') ``` -In turn for MySQL this produces: - -```sql -SELECT * -FROM issues -WHERE (title LIKE 'WIP:%' OR foo LIKE 'WIP:%') -``` - ## LIKE & Indexes -Neither PostgreSQL nor MySQL use any indexes when using `LIKE` / `ILIKE` with a -wildcard at the start. For example, this will not use any indexes: +PostgreSQL won't use any indexes when using `LIKE` / `ILIKE` with a wildcard at +the start. For example, this will not use any indexes: ```sql SELECT * @@ -75,9 +64,8 @@ WHERE title ILIKE '%WIP:%'; Because the value for `ILIKE` starts with a wildcard the database is not able to use an index as it doesn't know where to start scanning the indexes. -MySQL provides no known solution to this problem. Luckily PostgreSQL _does_ -provide a solution: trigram GIN indexes. These indexes can be created as -follows: +Luckily, PostgreSQL _does_ provide a solution: trigram GIN indexes. These +indexes can be created as follows: ```sql CREATE INDEX [CONCURRENTLY] index_name_here diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md index 448d9fd01c4..a505d3c79a2 100644 --- a/doc/development/testing_guide/best_practices.md +++ b/doc/development/testing_guide/best_practices.md @@ -15,16 +15,6 @@ manifest themselves within our code. When designing our tests, take time to revi our test design. We can find some helpful heuristics documented in the Handbook in the [Test Design](https://about.gitlab.com/handbook/engineering/quality/guidelines/test-engineering/test-design/) section. -## Run tests against MySQL - -By default, tests are only run against PostgreSQL, but you can run them on -demand against MySQL by following one of the following conventions: - -| Convention | Valid example | -|:----------------------|:-----------------------------| -| Include `mysql` in your branch name | `enhance-mysql-support` | -| Include `[run mysql]` in your commit message | `Fix MySQL support

[run mysql]` | - ## Test speed GitLab has a massive test suite that, without [parallelization], can take hours diff --git a/doc/development/testing_guide/ci.md b/doc/development/testing_guide/ci.md index 87d48726268..d9f66a827de 100644 --- a/doc/development/testing_guide/ci.md +++ b/doc/development/testing_guide/ci.md @@ -39,7 +39,6 @@ slowest test files and try to improve them. ## CI setup -- On CE and EE, the test suite runs both PostgreSQL and MySQL. - Rails logging to `log/test.log` is disabled by default in CI [for performance reasons][logging]. To override this setting, provide the `RAILS_ENABLE_TEST_LOG` environment variable. diff --git a/doc/development/testing_guide/flaky_tests.md b/doc/development/testing_guide/flaky_tests.md index 931cbc51cae..eb0bf6fc563 100644 --- a/doc/development/testing_guide/flaky_tests.md +++ b/doc/development/testing_guide/flaky_tests.md @@ -35,8 +35,8 @@ Once a test is in quarantine, there are 3 choices: Quarantined tests are run on the CI in dedicated jobs that are allowed to fail: -- `rspec-pg-quarantine` and `rspec-mysql-quarantine` (CE & EE) -- `rspec-pg-quarantine-ee` and `rspec-mysql-quarantine-ee` (EE only) +- `rspec-pg-quarantine` (CE & EE) +- `rspec-pg-quarantine-ee` (EE only) ## Automatic retries and flaky tests detection diff --git a/doc/development/verifying_database_capabilities.md b/doc/development/verifying_database_capabilities.md index ccec6f7d719..6b4995aebe2 100644 --- a/doc/development/verifying_database_capabilities.md +++ b/doc/development/verifying_database_capabilities.md @@ -1,15 +1,15 @@ # Verifying Database Capabilities -Sometimes certain bits of code may only work on a certain database and/or +Sometimes certain bits of code may only work on a certain database version. While we try to avoid such code as much as possible sometimes it is necessary to add database (version) specific behaviour. To facilitate this we have the following methods that you can use: -- `Gitlab::Database.postgresql?`: returns `true` if PostgreSQL is being used -- `Gitlab::Database.mysql?`: returns `true` if MySQL is being used +- `Gitlab::Database.postgresql?`: returns `true` if PostgreSQL is being used. + You can normally just assume this is the case. - `Gitlab::Database.version`: returns the PostgreSQL version number as a string - in the format `X.Y.Z`. This method does not work for MySQL + in the format `X.Y.Z`. This allows you to write code such as: diff --git a/doc/development/what_requires_downtime.md b/doc/development/what_requires_downtime.md index f0da1cc2ddc..944bf5900c5 100644 --- a/doc/development/what_requires_downtime.md +++ b/doc/development/what_requires_downtime.md @@ -7,9 +7,8 @@ downtime. ## Adding Columns -On PostgreSQL you can safely add a new column to an existing table as long as it -does **not** have a default value. For example, this query would not require -downtime: +You can safely add a new column to an existing table as long as it does **not** +have a default value. For example, this query would not require downtime: ```sql ALTER TABLE projects ADD COLUMN random_value int; @@ -27,11 +26,6 @@ This requires updating every single row in the `projects` table so that indexes in a table. This in turn acquires enough locks on the table for it to effectively block any other queries. -As of MySQL 5.6 adding a column to a table is still quite an expensive -operation, even when using `ALGORITHM=INPLACE` and `LOCK=NONE`. This means -downtime _may_ be required when modifying large tables as otherwise the -operation could potentially take hours to complete. - Adding a column with a default value _can_ be done without requiring downtime when using the migration helper method `Gitlab::Database::MigrationHelpers#add_column_with_default`. This method works @@ -311,8 +305,7 @@ migrations](background_migrations.md#cleaning-up). ## Adding Indexes Adding indexes is an expensive process that blocks INSERT and UPDATE queries for -the duration. When using PostgreSQL one can work around this by using the -`CONCURRENTLY` option: +the duration. You can work around this by using the `CONCURRENTLY` option: ```sql CREATE INDEX CONCURRENTLY index_name ON projects (column_name); @@ -336,17 +329,9 @@ end Note that `add_concurrent_index` can not be reversed automatically, thus you need to manually define `up` and `down`. -When running this on PostgreSQL the `CONCURRENTLY` option mentioned above is -used. On MySQL this method produces a regular `CREATE INDEX` query. - -MySQL doesn't really have a workaround for this. Supposedly it _can_ create -indexes without the need for downtime but only for variable width columns. The -details on this are a bit sketchy. Since it's better to be safe than sorry one -should assume that adding indexes requires downtime on MySQL. - ## Dropping Indexes -Dropping an index does not require downtime on both PostgreSQL and MySQL. +Dropping an index does not require downtime. ## Adding Tables @@ -370,7 +355,7 @@ transaction this means this approach would require downtime. GitLab allows you to work around this by using `Gitlab::Database::MigrationHelpers#add_concurrent_foreign_key`. This method -ensures that when PostgreSQL is used no downtime is needed. +ensures that no downtime is needed. ## Removing Foreign Keys -- cgit v1.2.1 From 3488da59363a5edfbf431a37ba59247b836bc50a Mon Sep 17 00:00:00 2001 From: Jarek Ostrowski Date: Fri, 16 Aug 2019 10:27:11 -0400 Subject: Add expand up and expand down icons Add changelog Add MR to changelog Remove stray file --- app/assets/javascripts/diffs/components/diff_expansion_cell.vue | 6 ++---- changelogs/unreleased/66161-replace-expand-icons.yml | 5 +++++ 2 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/66161-replace-expand-icons.yml diff --git a/app/assets/javascripts/diffs/components/diff_expansion_cell.vue b/app/assets/javascripts/diffs/components/diff_expansion_cell.vue index 925385fa98a..43edc116ed1 100644 --- a/app/assets/javascripts/diffs/components/diff_expansion_cell.vue +++ b/app/assets/javascripts/diffs/components/diff_expansion_cell.vue @@ -223,8 +223,7 @@ export default { :title="__('Expand up')" @click="handleExpandLines(EXPAND_UP)" > - -