diff options
25 files changed, 201 insertions, 21 deletions
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 050f83edacb..e19b09e1324 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -5,6 +5,7 @@ class Admin::UsersController < Admin::ApplicationController before_action :user, except: [:index, :new, :create] before_action :check_impersonation_availability, only: :impersonate + before_action :ensure_destroy_prerequisites_met, only: [:destroy] def index @users = User.filter_items(params[:filter]).order_name_asc @@ -173,7 +174,7 @@ class Admin::UsersController < Admin::ApplicationController end def destroy - user.delete_async(deleted_by: current_user, params: params.permit(:hard_delete)) + user.delete_async(deleted_by: current_user, params: destroy_params) respond_to do |format| format.html { redirect_to admin_users_path, status: :found, notice: _("The user is being deleted.") } @@ -202,6 +203,24 @@ class Admin::UsersController < Admin::ApplicationController user != current_user end + def destroy_params + params.permit(:hard_delete) + end + + def ensure_destroy_prerequisites_met + return if hard_delete? + + if user.solo_owned_groups.present? + message = s_('AdminUsers|You must transfer ownership or delete the groups owned by this user before you can delete their account') + + redirect_to admin_user_path(user), status: :see_other, alert: message + end + end + + def hard_delete? + destroy_params[:hard_delete] + end + def user @user ||= find_routable!(User, params[:id]) end diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb index f666a1150a6..da553e34ef6 100644 --- a/app/controllers/profiles/emails_controller.rb +++ b/app/controllers/profiles/emails_controller.rb @@ -2,6 +2,8 @@ class Profiles::EmailsController < Profiles::ApplicationController before_action :find_email, only: [:destroy, :resend_confirmation_instructions] + before_action -> { rate_limit!(:profile_add_new_email) }, only: [:create] + before_action -> { rate_limit!(:profile_resend_email_confirmation) }, only: [:resend_confirmation_instructions] def index @primary_email = current_user.email @@ -38,6 +40,16 @@ class Profiles::EmailsController < Profiles::ApplicationController private + def rate_limit!(action) + rate_limiter = ::Gitlab::ApplicationRateLimiter + + if rate_limiter.throttled?(action, scope: current_user) + rate_limiter.log_request(request, action, current_user) + + redirect_back_or_default(options: { alert: _('This action has been performed too many times. Try again later.') }) + end + end + def email_params params.require(:email).permit(:email) end diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb index 69a3898af55..29f1e4bfd44 100644 --- a/app/controllers/projects/raw_controller.rb +++ b/app/controllers/projects/raw_controller.rb @@ -12,6 +12,7 @@ class Projects::RawController < Projects::ApplicationController before_action :authorize_download_code! before_action :show_rate_limit, only: [:show], unless: :external_storage_request? before_action :assign_ref_vars + before_action :no_cache_headers, only: [:show] before_action :redirect_to_external_storage, only: :show, if: :static_objects_external_storage_enabled? def show diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index a1252c68403..204520a3e71 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -10,7 +10,7 @@ class RegistrationsController < Devise::RegistrationsController skip_before_action :required_signup_info, :check_two_factor_requirement, only: [:welcome, :update_registration] prepend_before_action :check_captcha, only: :create - before_action :whitelist_query_limiting, only: [:destroy] + before_action :whitelist_query_limiting, :ensure_destroy_prerequisites_met, only: [:destroy] before_action :ensure_terms_accepted, if: -> { action_name == 'create' && Gitlab::CurrentSettings.current_application_settings.enforce_terms? } before_action :load_recaptcha, only: :new @@ -124,6 +124,14 @@ class RegistrationsController < Devise::RegistrationsController private + def ensure_destroy_prerequisites_met + if current_user.solo_owned_groups.present? + redirect_to profile_account_path, + status: :see_other, + alert: s_('Profiles|You must transfer ownership or delete groups you are an owner of before you can delete your account') + end + end + def user_created_message(confirmed: false) "User Created: username=#{resource.username} email=#{resource.email} ip=#{request.remote_ip} confirmed:#{confirmed}" end diff --git a/app/helpers/safe_params_helper.rb b/app/helpers/safe_params_helper.rb index 72bf1377b02..e9f0d82bb27 100644 --- a/app/helpers/safe_params_helper.rb +++ b/app/helpers/safe_params_helper.rb @@ -5,7 +5,7 @@ module SafeParamsHelper # Use this helper when generating links with `params.merge(...)` def safe_params if params.respond_to?(:permit!) - params.except(:host, :port, :protocol).permit! + params.except(*ActionDispatch::Routing::RouteSet::RESERVED_OPTIONS).permit! else params end diff --git a/app/models/clusters/applications/runner.rb b/app/models/clusters/applications/runner.rb index 4983de83800..f0b3c11ba1d 100644 --- a/app/models/clusters/applications/runner.rb +++ b/app/models/clusters/applications/runner.rb @@ -3,7 +3,7 @@ module Clusters module Applications class Runner < ApplicationRecord - VERSION = '0.20.1' + VERSION = '0.20.2' self.table_name = 'clusters_applications_runners' diff --git a/changelogs/unreleased/security-44-stored-xss-via-svg-file-preview.yml b/changelogs/unreleased/security-44-stored-xss-via-svg-file-preview.yml new file mode 100644 index 00000000000..89a1eedb753 --- /dev/null +++ b/changelogs/unreleased/security-44-stored-xss-via-svg-file-preview.yml @@ -0,0 +1,5 @@ +--- +title: Prevent SVG XSS via Web IDE +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-ensure-prerequisites-are-met-before-account-deletion.yml b/changelogs/unreleased/security-ensure-prerequisites-are-met-before-account-deletion.yml new file mode 100644 index 00000000000..4b8f1c64ec7 --- /dev/null +++ b/changelogs/unreleased/security-ensure-prerequisites-are-met-before-account-deletion.yml @@ -0,0 +1,5 @@ +--- +title: Ensure user has no solo owned groups before triggering account deletion +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fix-safe-params-helper.yml b/changelogs/unreleased/security-fix-safe-params-helper.yml new file mode 100644 index 00000000000..ac7d2b60ff2 --- /dev/null +++ b/changelogs/unreleased/security-fix-safe-params-helper.yml @@ -0,0 +1,4 @@ +--- +title: Security fix safe params helper +author: +type: security diff --git a/changelogs/unreleased/security-rate-limit-email-confirmation.yml b/changelogs/unreleased/security-rate-limit-email-confirmation.yml new file mode 100644 index 00000000000..4fa34a3739d --- /dev/null +++ b/changelogs/unreleased/security-rate-limit-email-confirmation.yml @@ -0,0 +1,5 @@ +--- +title: Rate limit adding new email and re-sending email confirmation +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-update-runner-version-13-4-stable.yml b/changelogs/unreleased/security-update-runner-version-13-4-stable.yml new file mode 100644 index 00000000000..ddf3eb7e267 --- /dev/null +++ b/changelogs/unreleased/security-update-runner-version-13-4-stable.yml @@ -0,0 +1,5 @@ +--- +title: Update GitLab Runner Helm Chart to 0.20.2 +merge_request: +author: +type: security diff --git a/doc/user/admin_area/custom_project_templates.md b/doc/user/admin_area/custom_project_templates.md index f39e5b198e7..e148c5f063e 100644 --- a/doc/user/admin_area/custom_project_templates.md +++ b/doc/user/admin_area/custom_project_templates.md @@ -15,7 +15,8 @@ templates are sourced. Every project directly under the group namespace will be available to the user if they have access to them. For example: -- Public project in the group will be available to every logged in user. +- Public projects, in the group will be available to every signed-in user, if all enabled [project features](../project/settings/index.md#sharing-and-permissions) + are set to **Everyone With Access**. - Private projects will be available only if the user is a member of the project. Repository and database information that are copied over to each new project are diff --git a/doc/user/group/custom_project_templates.md b/doc/user/group/custom_project_templates.md index fd8d966fbe1..bbff82811bf 100644 --- a/doc/user/group/custom_project_templates.md +++ b/doc/user/group/custom_project_templates.md @@ -61,9 +61,8 @@ GitLab administrators can [set project templates for an entire GitLab instance](../admin_area/custom_project_templates.md). Within this section, you can configure the group where all the custom project -templates are sourced. Every project directly under the group namespace will be -available to the user if they have access to them. For example, every public -project in the group will be available to every logged in user. +templates are sourced. Every project _template_ directly under the group namespace is +available to every signed-in user, if all enabled [project features](../project/settings/index.md#sharing-and-permissions) are set to **Everyone With Access**. However, private projects will be available only if the user is a member of the project. diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index 4eeec534fc0..6173918b453 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -31,7 +31,9 @@ module Gitlab group_export: { threshold: -> { application_settings.group_export_limit }, interval: 1.minute }, group_download_export: { threshold: -> { application_settings.group_download_export_limit }, interval: 1.minute }, group_import: { threshold: -> { application_settings.group_import_limit }, interval: 1.minute }, - group_testing_hook: { threshold: 5, interval: 1.minute } + group_testing_hook: { threshold: 5, interval: 1.minute }, + profile_add_new_email: { threshold: 5, interval: 1.minute }, + profile_resend_email_confirmation: { threshold: 5, interval: 1.minute } }.freeze end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 439c26a0e99..f301315746f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2132,6 +2132,9 @@ msgstr "" msgid "AdminUsers|You cannot remove your own admin rights." msgstr "" +msgid "AdminUsers|You must transfer ownership or delete the groups owned by this user before you can delete their account" +msgstr "" + msgid "Administration" msgstr "" @@ -19391,6 +19394,9 @@ msgstr "" msgid "Profiles|You don't have access to delete this user." msgstr "" +msgid "Profiles|You must transfer ownership or delete groups you are an owner of before you can delete your account" +msgstr "" + msgid "Profiles|You must transfer ownership or delete these groups before you can delete your account." msgstr "" @@ -25734,6 +25740,9 @@ msgstr "" msgid "This action cannot be undone. You will lose the project's repository and all content: issues, merge requests, etc." msgstr "" +msgid "This action has been performed too many times. Try again later." +msgstr "" + msgid "This action will %{strongOpen}permanently delete%{strongClose} %{codeOpen}%{project}%{codeClose} %{strongOpen}immediately%{strongClose}, including its repositories and all content: issues, merge requests, etc." msgstr "" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index e4cdcda756b..6301da74f4a 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -36,7 +36,7 @@ RSpec.describe Admin::UsersController do end end - describe 'DELETE #user with projects', :sidekiq_might_not_need_inline do + describe 'DELETE #destroy', :sidekiq_might_not_need_inline do let(:project) { create(:project, namespace: user.namespace) } let!(:issue) { create(:issue, author: user) } @@ -59,6 +59,41 @@ RSpec.describe Admin::UsersController do expect(User.exists?(user.id)).to be_falsy expect(Issue.exists?(issue.id)).to be_falsy end + + context 'prerequisites for account deletion' do + context 'solo-owned groups' do + let(:group) { create(:group) } + + context 'if the user is the sole owner of at least one group' do + before do + create(:group_member, :owner, group: group, user: user) + end + + context 'soft-delete' do + it 'fails' do + delete :destroy, params: { id: user.username } + + message = s_('AdminUsers|You must transfer ownership or delete the groups owned by this user before you can delete their account') + + expect(flash[:alert]).to eq(message) + expect(response).to have_gitlab_http_status(:see_other) + expect(response).to redirect_to admin_user_path(user) + expect(User.exists?(user.id)).to be_truthy + end + end + + context 'hard-delete' do + it 'succeeds' do + delete :destroy, params: { id: user.username, hard_delete: true } + + expect(response).to redirect_to(admin_users_path) + expect(flash[:notice]).to eq(_('The user is being deleted.')) + expect(User.exists?(user.id)).to be_falsy + end + end + end + end + end end describe 'PUT #activate' do diff --git a/spec/controllers/profiles/emails_controller_spec.rb b/spec/controllers/profiles/emails_controller_spec.rb index 08552cc28fa..950120ae564 100644 --- a/spec/controllers/profiles/emails_controller_spec.rb +++ b/spec/controllers/profiles/emails_controller_spec.rb @@ -15,6 +15,29 @@ RSpec.describe Profiles::EmailsController do end end + shared_examples_for 'respects the rate limit' do + context 'after the rate limit is exceeded' do + before do + allowed_threshold = Gitlab::ApplicationRateLimiter.rate_limits[action][:threshold] + + allow(Gitlab::ApplicationRateLimiter) + .to receive(:increment) + .and_return(allowed_threshold + 1) + end + + it 'does not send any email' do + expect { subject }.not_to change { ActionMailer::Base.deliveries.size } + end + + it 'displays an alert' do + subject + + expect(response).to have_gitlab_http_status(:redirect) + expect(flash[:alert]).to eq(_('This action has been performed too many times. Try again later.')) + end + end + end + describe '#create' do let(:email) { 'add_email@example.com' } let(:params) { { email: { email: email } } } @@ -32,6 +55,10 @@ RSpec.describe Profiles::EmailsController do expect { subject }.not_to change { ActionMailer::Base.deliveries.size } end end + + it_behaves_like 'respects the rate limit' do + let(:action) { :profile_add_new_email } + end end describe '#resend_confirmation_instructions' do @@ -54,5 +81,9 @@ RSpec.describe Profiles::EmailsController do expect { subject }.not_to change { ActionMailer::Base.deliveries.size } end end + + it_behaves_like 'respects the rate limit' do + let(:action) { :profile_resend_email_confirmation } + end end end diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb index 5f10343eb76..b3921164c81 100644 --- a/spec/controllers/projects/raw_controller_spec.rb +++ b/spec/controllers/projects/raw_controller_spec.rb @@ -33,6 +33,11 @@ RSpec.describe Projects::RawController do it_behaves_like 'project cache control headers' it_behaves_like 'content disposition headers' + it_behaves_like 'uncached response' do + before do + subject + end + end end context 'image header' do diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index f80e18df22e..60957dc72e6 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -459,6 +459,24 @@ RSpec.describe RegistrationsController do expect_success end end + + context 'prerequisites for account deletion' do + context 'solo-owned groups' do + let(:group) { create(:group) } + + context 'if the user is the sole owner of at least one group' do + before do + create(:group_member, :owner, group: group, user: user) + end + + it 'fails' do + delete :destroy, params: { password: '12345678' } + + expect_failure(s_('Profiles|You must transfer ownership or delete groups you are an owner of before you can delete your account')) + end + end + end + end end describe '#welcome' do diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index e3411e4f925..147413557d6 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -29,6 +29,7 @@ FactoryBot.define do pages_access_level do visibility_level == Gitlab::VisibilityLevel::PUBLIC ? ProjectFeature::ENABLED : ProjectFeature::PRIVATE end + metrics_dashboard_access_level { ProjectFeature::PRIVATE } # we can't assign the delegated `#ci_cd_settings` attributes directly, as the # `#ci_cd_settings` relation needs to be created first @@ -53,7 +54,8 @@ FactoryBot.define do forking_access_level: evaluator.forking_access_level, merge_requests_access_level: merge_requests_access_level, repository_access_level: evaluator.repository_access_level, - pages_access_level: evaluator.pages_access_level + pages_access_level: evaluator.pages_access_level, + metrics_dashboard_access_level: evaluator.metrics_dashboard_access_level } project.build_project_feature(hash) @@ -309,6 +311,9 @@ FactoryBot.define do trait(:pages_enabled) { pages_access_level { ProjectFeature::ENABLED } } trait(:pages_disabled) { pages_access_level { ProjectFeature::DISABLED } } trait(:pages_private) { pages_access_level { ProjectFeature::PRIVATE } } + trait(:metrics_dashboard_enabled) { metrics_dashboard_access_level { ProjectFeature::ENABLED } } + trait(:metrics_dashboard_disabled) { metrics_dashboard_access_level { ProjectFeature::DISABLED } } + trait(:metrics_dashboard_private) { metrics_dashboard_access_level { ProjectFeature::PRIVATE } } trait :auto_devops do association :auto_devops, factory: :project_auto_devops diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index d7571ad4bff..bb4e88f97f8 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -532,16 +532,13 @@ RSpec.describe API::Files do expect(response).to have_gitlab_http_status(:ok) end - it 'sets no-cache headers' do - url = route('.gitignore') + "/raw" - expect(Gitlab::Workhorse).to receive(:send_git_blob) - - get api(url, current_user), params: params + it_behaves_like 'uncached response' do + before do + url = route('.gitignore') + "/raw" + expect(Gitlab::Workhorse).to receive(:send_git_blob) - expect(response.headers["Cache-Control"]).to include("no-store") - expect(response.headers["Cache-Control"]).to include("no-cache") - expect(response.headers["Pragma"]).to eq("no-cache") - expect(response.headers["Expires"]).to eq("Fri, 01 Jan 1990 00:00:00 GMT") + get api(url, current_user), params: params + end end context 'when mandatory params are not given' do diff --git a/spec/requests/projects/metrics_dashboard_spec.rb b/spec/requests/projects/metrics_dashboard_spec.rb index f0e0e6a02ee..0a4100f2bf5 100644 --- a/spec/requests/projects/metrics_dashboard_spec.rb +++ b/spec/requests/projects/metrics_dashboard_spec.rb @@ -39,7 +39,9 @@ RSpec.describe 'Projects::MetricsDashboardController' do context 'with anonymous user and public dashboard visibility' do let(:anonymous_user) { create(:user) } - let(:project) { create(:project, :public) } + let(:project) do + create(:project, :public, :metrics_dashboard_enabled) + end before do project.update!(metrics_dashboard_access_level: 'enabled') diff --git a/spec/support/shared_examples/cached_response_shared_examples.rb b/spec/support/shared_examples/cached_response_shared_examples.rb new file mode 100644 index 00000000000..34e5f741b4e --- /dev/null +++ b/spec/support/shared_examples/cached_response_shared_examples.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true +# +# Negates lib/gitlab/no_cache_headers.rb +# + +RSpec.shared_examples 'cached response' do + it 'defines a cached header response' do + expect(response.headers["Cache-Control"]).not_to include("no-store", "no-cache") + expect(response.headers["Pragma"]).not_to eq("no-cache") + expect(response.headers["Expires"]).not_to eq("Fri, 01 Jan 1990 00:00:00 GMT") + end +end diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore index 259148fa18f..259148fa18f 100644..100755 --- a/vendor/gitignore/C++.gitignore +++ b/vendor/gitignore/C++.gitignore diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore index a1c2a238a96..a1c2a238a96 100644..100755 --- a/vendor/gitignore/Java.gitignore +++ b/vendor/gitignore/Java.gitignore |