From 3c992c78e972b8019ae49015d445524d654e4076 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 29 Sep 2021 12:57:45 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@14-1-stable-ee --- app/assets/javascripts/gfm_auto_complete.js | 33 ++++++--- app/models/user.rb | 3 +- app/services/projects/destroy_service.rb | 11 +++ app/views/projects/_export.html.haml | 1 + ...4095310_cleanup_orphan_project_access_tokens.rb | 56 +++++++++++++++ db/schema_migrations/20210914095310 | 1 + doc/user/project/settings/import_export.md | 1 + lib/api/import_bitbucket_server.rb | 4 ++ lib/gitlab/auth.rb | 6 +- lib/gitlab/auth/two_factor_auth_verifier.rb | 4 ++ lib/gitlab/import_export/project/import_export.yml | 1 - .../lib/gitlab/import_export/complex/project.json | 17 ----- .../complex/tree/project/triggers.ndjson | 2 +- spec/frontend/gfm_auto_complete_spec.js | 37 +++++++++- .../gitlab/auth/two_factor_auth_verifier_spec.rb | 61 +++++++++------- spec/lib/gitlab/auth_spec.rb | 55 ++++++++++++--- spec/lib/gitlab/git_access_spec.rb | 35 ++++----- .../import_export/project/tree_restorer_spec.rb | 4 +- .../gitlab/import_export/safe_model_attributes.yml | 9 --- spec/lib/gitlab/lfs_token_spec.rb | 14 ++-- ...10_cleanup_orphan_project_access_tokens_spec.rb | 47 +++++++++++++ spec/models/user_spec.rb | 82 +++++----------------- spec/policies/global_policy_spec.rb | 18 ++--- spec/requests/api/import_bitbucket_server_spec.rb | 14 ++++ spec/requests/git_http_spec.rb | 16 ++--- spec/requests/lfs_http_spec.rb | 6 +- spec/services/projects/destroy_service_spec.rb | 10 +++ 27 files changed, 360 insertions(+), 188 deletions(-) create mode 100644 db/post_migrate/20210914095310_cleanup_orphan_project_access_tokens.rb create mode 100644 db/schema_migrations/20210914095310 create mode 100644 spec/migrations/20210914095310_cleanup_orphan_project_access_tokens_spec.rb diff --git a/app/assets/javascripts/gfm_auto_complete.js b/app/assets/javascripts/gfm_auto_complete.js index 470c785f7e4..cb63c86a4fa 100644 --- a/app/assets/javascripts/gfm_auto_complete.js +++ b/app/assets/javascripts/gfm_auto_complete.js @@ -1,6 +1,6 @@ import $ from 'jquery'; import '~/lib/utils/jquery_at_who'; -import { escape, sortBy, template } from 'lodash'; +import { escape as lodashEscape, sortBy, template } from 'lodash'; import * as Emoji from '~/emoji'; import axios from '~/lib/utils/axios_utils'; import { s__, __, sprintf } from '~/locale'; @@ -11,8 +11,21 @@ import { spriteIcon } from './lib/utils/common_utils'; import { parsePikadayDate } from './lib/utils/datetime_utility'; import glRegexp from './lib/utils/regexp'; -function sanitize(str) { - return str.replace(/<(?:.|\n)*?>/gm, ''); +/** + * Escapes user input before we pass it to at.js, which + * renders it as HTML in the autocomplete dropdown. + * + * at.js allows you to reference data using `${}` syntax + * (e.g. ${search}) which it replaces with the actual data + * before rendering it in the autocomplete dropdown. + * To prevent user input from executing this `${}` syntax, + * we also need to escape the $ character. + * + * @param string user input + * @return {string} escaped user input + */ +function escape(string) { + return lodashEscape(string).replace(/\$/g, '$'); } function createMemberSearchString(member) { @@ -44,8 +57,8 @@ export function membersBeforeSave(members) { return { username: member.username, avatarTag: autoCompleteAvatar.length === 1 ? txtAvatar : imgAvatar, - title: sanitize(title), - search: sanitize(createMemberSearchString(member)), + title, + search: createMemberSearchString(member), icon: avatarIcon, availability: member?.availability, }; @@ -366,7 +379,7 @@ class GfmAutoComplete { } return { id: i.iid, - title: sanitize(i.title), + title: i.title, reference: i.reference, search: `${i.iid} ${i.title}`, }; @@ -404,7 +417,7 @@ class GfmAutoComplete { return { id: m.iid, - title: sanitize(m.title), + title: m.title, search: m.title, expired, dueDate, @@ -456,7 +469,7 @@ class GfmAutoComplete { } return { id: m.iid, - title: sanitize(m.title), + title: m.title, reference: m.reference, search: `${m.iid} ${m.title}`, }; @@ -492,7 +505,7 @@ class GfmAutoComplete { beforeSave(merges) { if (GfmAutoComplete.isLoading(merges)) return merges; return $.map(merges, (m) => ({ - title: sanitize(m.title), + title: m.title, color: m.color, search: m.title, set: m.set, @@ -586,7 +599,7 @@ class GfmAutoComplete { } return { id: m.id, - title: sanitize(m.title), + title: m.title, search: `${m.id} ${m.title}`, }; }); diff --git a/app/models/user.rb b/app/models/user.rb index 80b8c9173d1..903149a5252 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1892,7 +1892,8 @@ class User < ApplicationRecord def password_expired_if_applicable? return false if bot? - return false unless password_expired? && password_automatically_set? + return false unless password_expired? + return false if password_automatically_set? return false unless allow_password_authentication? true diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index e27ea7c07e5..afa8de04fca 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -117,6 +117,7 @@ module Projects trash_relation_repositories! trash_project_repositories! destroy_web_hooks! + destroy_project_bots! # Rails attempts to load all related records into memory before # destroying: https://github.com/rails/rails/issues/22510 @@ -149,6 +150,16 @@ module Projects end end + # The project can have multiple project bots with personal access tokens generated. + # We need to remove them when a project is deleted + # rubocop: disable CodeReuse/ActiveRecord + def destroy_project_bots! + project.members.includes(:user).references(:user).merge(User.project_bot).each do |member| + Users::DestroyService.new(current_user).execute(member.user, skip_authorization: true) + end + end + # rubocop: enable CodeReuse/ActiveRecord + def remove_registry_tags return true unless Gitlab.config.registry.enabled return false unless remove_legacy_registry_tags diff --git a/app/views/projects/_export.html.haml b/app/views/projects/_export.html.haml index eb4630b84d5..97f5cdb54e5 100644 --- a/app/views/projects/_export.html.haml +++ b/app/views/projects/_export.html.haml @@ -16,6 +16,7 @@ %li= _('Job logs and artifacts') %li= _('Container registry images') %li= _('CI variables') + %li= _('Pipeline triggers') %li= _('Webhooks') %li= _('Any encrypted tokens') - if project.export_status == :finished diff --git a/db/post_migrate/20210914095310_cleanup_orphan_project_access_tokens.rb b/db/post_migrate/20210914095310_cleanup_orphan_project_access_tokens.rb new file mode 100644 index 00000000000..9d981413437 --- /dev/null +++ b/db/post_migrate/20210914095310_cleanup_orphan_project_access_tokens.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +class CleanupOrphanProjectAccessTokens < ActiveRecord::Migration[6.1] + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + TMP_INDEX_NAME = 'idx_users_on_user_type_project_bots_batched' + + def up + users_table = define_batchable_model('users') + + add_concurrent_index(:users, :id, name: TMP_INDEX_NAME, where: 'user_type = 6') + + accumulated_orphans = [] + users_table.where(user_type: 6).each_batch(of: 500) do |relation| + orphan_ids = relation.where("not exists(select 1 from members where members.user_id = users.id)").pluck(:id) + + orphan_ids.each_slice(10) do |ids| + users_table.where(id: ids).update_all(state: 'deactivated') + end + + accumulated_orphans += orphan_ids + end + + schedule_deletion(accumulated_orphans) + ensure + remove_concurrent_index_by_name(:users, TMP_INDEX_NAME) + end + + def down + remove_concurrent_index_by_name(:users, TMP_INDEX_NAME) if index_exists_by_name?(:users, TMP_INDEX_NAME) + end + + private + + def schedule_deletion(orphan_ids) + return unless deletion_worker + + orphan_ids.each_slice(100) do |ids| + job_arguments = ids.map do |orphan_id| + [orphan_id, orphan_id, { skip_authorization: true }] + end + + deletion_worker.bulk_perform_async(job_arguments) + end + rescue StandardError + # Ignore any errors or interface changes since this part of migration is optional + end + + def deletion_worker + @deletion_worker = "DeleteUserWorker".safe_constantize unless defined?(@deletion_worker) + + @deletion_worker + end +end diff --git a/db/schema_migrations/20210914095310 b/db/schema_migrations/20210914095310 new file mode 100644 index 00000000000..fee7e0b9719 --- /dev/null +++ b/db/schema_migrations/20210914095310 @@ -0,0 +1 @@ +6fcf3ff9867df68f5e9603ae0311b29bec33aa5c5b826786b094ab0960ebcd90 \ No newline at end of file diff --git a/doc/user/project/settings/import_export.md b/doc/user/project/settings/import_export.md index 7cbc22bef63..22fb2da8038 100644 --- a/doc/user/project/settings/import_export.md +++ b/doc/user/project/settings/import_export.md @@ -133,6 +133,7 @@ The following items are **not** exported: - Build traces and artifacts - Container registry images - CI/CD variables +- Pipeline triggers - Webhooks - Any encrypted tokens - Merge Request Approvers diff --git a/lib/api/import_bitbucket_server.rb b/lib/api/import_bitbucket_server.rb index ecd78c6e6db..0f0d62dcbfb 100644 --- a/lib/api/import_bitbucket_server.rb +++ b/lib/api/import_bitbucket_server.rb @@ -4,6 +4,10 @@ module API class ImportBitbucketServer < ::API::Base feature_category :importers + before do + forbidden! unless Gitlab::CurrentSettings.import_sources&.include?('bitbucket_server') + end + helpers do def client @client ||= BitbucketServer::Client.new(credentials) diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 13e78e72175..2848f7c3ef8 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -172,7 +172,11 @@ module Gitlab user = find_with_user_password(login, password) return unless user - raise Gitlab::Auth::MissingPersonalAccessTokenError if user.two_factor_enabled? + verifier = TwoFactorAuthVerifier.new(user) + + if user.two_factor_enabled? || verifier.two_factor_authentication_enforced? + raise Gitlab::Auth::MissingPersonalAccessTokenError + end Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities) end diff --git a/lib/gitlab/auth/two_factor_auth_verifier.rb b/lib/gitlab/auth/two_factor_auth_verifier.rb index 86552ef1267..5a203a1fe9c 100644 --- a/lib/gitlab/auth/two_factor_auth_verifier.rb +++ b/lib/gitlab/auth/two_factor_auth_verifier.rb @@ -9,6 +9,10 @@ module Gitlab @current_user = current_user end + def two_factor_authentication_enforced? + two_factor_authentication_required? && two_factor_grace_period_expired? + end + def two_factor_authentication_required? Gitlab::CurrentSettings.require_two_factor_authentication? || current_user&.require_two_factor_authentication_from_group? diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index e5a39efb798..2772e7ef02b 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -88,7 +88,6 @@ tree: - :external_pull_request - :merge_request - :auto_devops - - :triggers - :pipeline_schedules - :container_expiration_policy - protected_branches: diff --git a/spec/fixtures/lib/gitlab/import_export/complex/project.json b/spec/fixtures/lib/gitlab/import_export/complex/project.json index e3aeace6383..1072e63b20b 100644 --- a/spec/fixtures/lib/gitlab/import_export/complex/project.json +++ b/spec/fixtures/lib/gitlab/import_export/complex/project.json @@ -7579,23 +7579,6 @@ } } ], - "triggers": [ - { - "id": 123, - "token": "cdbfasdf44a5958c83654733449e585", - "project_id": 5, - "owner_id": 1, - "created_at": "2017-01-16T15:25:28.637Z", - "updated_at": "2017-01-16T15:25:28.637Z" - }, - { - "id": 456, - "token": "33a66349b5ad01fc00174af87804e40", - "project_id": 5, - "created_at": "2017-01-16T15:25:29.637Z", - "updated_at": "2017-01-16T15:25:29.637Z" - } - ], "pipeline_schedules": [ { "id": 1, diff --git a/spec/fixtures/lib/gitlab/import_export/complex/tree/project/triggers.ndjson b/spec/fixtures/lib/gitlab/import_export/complex/tree/project/triggers.ndjson index 93619f4fb44..2b5bda687b8 100644 --- a/spec/fixtures/lib/gitlab/import_export/complex/tree/project/triggers.ndjson +++ b/spec/fixtures/lib/gitlab/import_export/complex/tree/project/triggers.ndjson @@ -1,2 +1,2 @@ {"id":123,"token":"cdbfasdf44a5958c83654733449e585","project_id":5,"owner_id":1,"created_at":"2017-01-16T15:25:28.637Z","updated_at":"2017-01-16T15:25:28.637Z"} -{"id":456,"token":"33a66349b5ad01fc00174af87804e40","project_id":5,"created_at":"2017-01-16T15:25:29.637Z","updated_at":"2017-01-16T15:25:29.637Z"} +{"id":456,"token":"33a66349b5ad01fc00174af87804e40","project_id":5,"created_at":"2017-01-16T15:25:29.637Z","updated_at":"2017-01-16T15:25:29.637Z"} \ No newline at end of file diff --git a/spec/frontend/gfm_auto_complete_spec.js b/spec/frontend/gfm_auto_complete_spec.js index 211ed064762..94ad7759110 100644 --- a/spec/frontend/gfm_auto_complete_spec.js +++ b/spec/frontend/gfm_auto_complete_spec.js @@ -574,6 +574,15 @@ describe('GfmAutoComplete', () => { }), ).toBe('
  • grp/proj#5 Some Issue
  • '); }); + + it('escapes title in the template as it is user input', () => { + expect( + GfmAutoComplete.Issues.templateFunction({ + id: 5, + title: '${search}