From 63a19a71aedcafe0148912c536a36768ed126533 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 31 May 2021 11:39:05 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee --- app/assets/javascripts/notebook/cells/markdown.vue | 1 + lib/gitlab/x509/signature.rb | 10 ++- spec/frontend/notebook/cells/markdown_spec.js | 13 +++- spec/lib/gitlab/x509/signature_spec.rb | 77 +++++++++++----------- spec/tasks/gitlab/x509/update_rake_spec.rb | 20 ++---- 5 files changed, 66 insertions(+), 55 deletions(-) diff --git a/app/assets/javascripts/notebook/cells/markdown.vue b/app/assets/javascripts/notebook/cells/markdown.vue index 9bf26e5a182..a7fcce02ab3 100644 --- a/app/assets/javascripts/notebook/cells/markdown.vue +++ b/app/assets/javascripts/notebook/cells/markdown.vue @@ -195,6 +195,7 @@ export default { 'var', ], ALLOWED_ATTR: ['class', 'style', 'href', 'src'], + ALLOW_DATA_ATTR: false, }); }, }, diff --git a/lib/gitlab/x509/signature.rb b/lib/gitlab/x509/signature.rb index c83213e973b..a6761e211fa 100644 --- a/lib/gitlab/x509/signature.rb +++ b/lib/gitlab/x509/signature.rb @@ -23,7 +23,7 @@ module Gitlab end def user - User.find_by_any_email(@email) + strong_memoize(:user) { User.find_by_any_email(@email) } end def verified_signature @@ -31,9 +31,13 @@ module Gitlab end def verification_status - return :unverified if x509_certificate.nil? || x509_certificate.revoked? + return :unverified if + x509_certificate.nil? || + x509_certificate.revoked? || + !verified_signature || + user.nil? - if verified_signature && certificate_email == @email + if user.verified_emails.include?(@email) && certificate_email == @email :verified else :unverified diff --git a/spec/frontend/notebook/cells/markdown_spec.js b/spec/frontend/notebook/cells/markdown_spec.js index d250ffed1a9..deeee5d6589 100644 --- a/spec/frontend/notebook/cells/markdown_spec.js +++ b/spec/frontend/notebook/cells/markdown_spec.js @@ -39,7 +39,7 @@ describe('Markdown component', () => { expect(vm.$el.querySelector('.markdown h1')).not.toBeNull(); }); - it('sanitizes output', async () => { + it('sanitizes Markdown output', async () => { Object.assign(cell, { source: [ '[XSS](data:text/html;base64,PHNjcmlwdD5hbGVydChkb2N1bWVudC5kb21haW4pPC9zY3JpcHQ+Cg==)\n', @@ -50,6 +50,17 @@ describe('Markdown component', () => { expect(vm.$el.querySelector('a').getAttribute('href')).toBeNull(); }); + it('sanitizes HTML', async () => { + const findLink = () => vm.$el.querySelector('.xss-link'); + Object.assign(cell, { + source: ['XSS\n'], + }); + + await vm.$nextTick(); + expect(findLink().getAttribute('data-remote')).toBe(null); + expect(findLink().getAttribute('data-type')).toBe(null); + }); + describe('tables', () => { beforeEach(() => { json = getJSONFixture('blob/notebook/markdown-table.json'); diff --git a/spec/lib/gitlab/x509/signature_spec.rb b/spec/lib/gitlab/x509/signature_spec.rb index 2ac9c1f3a3b..7ba15faf910 100644 --- a/spec/lib/gitlab/x509/signature_spec.rb +++ b/spec/lib/gitlab/x509/signature_spec.rb @@ -12,20 +12,30 @@ RSpec.describe Gitlab::X509::Signature do end shared_examples "a verified signature" do - it 'returns a verified signature if email does match' do - signature = described_class.new( + let_it_be(:user) { create(:user, email: X509Helpers::User1.certificate_email) } + + subject(:signature) do + described_class.new( X509Helpers::User1.signed_commit_signature, X509Helpers::User1.signed_commit_base_data, X509Helpers::User1.certificate_email, X509Helpers::User1.signed_commit_time ) + end + it 'returns a verified signature if email does match' do expect(signature.x509_certificate).to have_attributes(certificate_attributes) expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes) expect(signature.verified_signature).to be_truthy expect(signature.verification_status).to eq(:verified) end + it "returns an unverified signature if the email matches but isn't confirmed" do + user.update!(confirmed_at: nil) + + expect(signature.verification_status).to eq(:unverified) + end + it 'returns an unverified signature if email does not match' do signature = described_class.new( X509Helpers::User1.signed_commit_signature, @@ -55,13 +65,6 @@ RSpec.describe Gitlab::X509::Signature do end it 'returns an unverified signature if certificate is revoked' do - signature = described_class.new( - X509Helpers::User1.signed_commit_signature, - X509Helpers::User1.signed_commit_base_data, - X509Helpers::User1.certificate_email, - X509Helpers::User1.signed_commit_time - ) - expect(signature.verification_status).to eq(:verified) signature.x509_certificate.revoked! @@ -253,23 +256,25 @@ RSpec.describe Gitlab::X509::Signature do end describe '#user' do - signature = described_class.new( - X509Helpers::User1.signed_tag_signature, - X509Helpers::User1.signed_tag_base_data, - X509Helpers::User1.certificate_email, - X509Helpers::User1.signed_commit_time - ) + subject do + described_class.new( + X509Helpers::User1.signed_tag_signature, + X509Helpers::User1.signed_tag_base_data, + X509Helpers::User1.certificate_email, + X509Helpers::User1.signed_commit_time + ).user + end context 'if email is assigned to a user' do let!(:user) { create(:user, email: X509Helpers::User1.certificate_email) } it 'returns user' do - expect(signature.user).to eq(user) + is_expected.to eq(user) end end it 'if email is not assigned to a user, return nil' do - expect(signature.user).to be_nil + is_expected.to be_nil end end @@ -292,6 +297,17 @@ RSpec.describe Gitlab::X509::Signature do end context 'verified signature' do + let_it_be(:user) { create(:user, email: X509Helpers::User1.certificate_email) } + + subject(:signature) do + described_class.new( + X509Helpers::User1.signed_tag_signature, + X509Helpers::User1.signed_tag_base_data, + X509Helpers::User1.certificate_email, + X509Helpers::User1.signed_commit_time + ) + end + context 'with trusted certificate store' do before do store = OpenSSL::X509::Store.new @@ -301,19 +317,18 @@ RSpec.describe Gitlab::X509::Signature do end it 'returns a verified signature if email does match' do - signature = described_class.new( - X509Helpers::User1.signed_tag_signature, - X509Helpers::User1.signed_tag_base_data, - X509Helpers::User1.certificate_email, - X509Helpers::User1.signed_commit_time - ) - expect(signature.x509_certificate).to have_attributes(certificate_attributes) expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes) expect(signature.verified_signature).to be_truthy expect(signature.verification_status).to eq(:verified) end + it "returns an unverified signature if the email matches but isn't confirmed" do + user.update!(confirmed_at: nil) + + expect(signature.verification_status).to eq(:unverified) + end + it 'returns an unverified signature if email does not match' do signature = described_class.new( X509Helpers::User1.signed_tag_signature, @@ -343,13 +358,6 @@ RSpec.describe Gitlab::X509::Signature do end it 'returns an unverified signature if certificate is revoked' do - signature = described_class.new( - X509Helpers::User1.signed_tag_signature, - X509Helpers::User1.signed_tag_base_data, - X509Helpers::User1.certificate_email, - X509Helpers::User1.signed_commit_time - ) - expect(signature.verification_status).to eq(:verified) signature.x509_certificate.revoked! @@ -368,13 +376,6 @@ RSpec.describe Gitlab::X509::Signature do end it 'returns an unverified signature' do - signature = described_class.new( - X509Helpers::User1.signed_tag_signature, - X509Helpers::User1.signed_tag_base_data, - X509Helpers::User1.certificate_email, - X509Helpers::User1.signed_commit_time - ) - expect(signature.x509_certificate).to have_attributes(certificate_attributes) expect(signature.x509_certificate.x509_issuer).to have_attributes(issuer_attributes) expect(signature.verified_signature).to be_falsey diff --git a/spec/tasks/gitlab/x509/update_rake_spec.rb b/spec/tasks/gitlab/x509/update_rake_spec.rb index 93e97ab38ad..b166e73935a 100644 --- a/spec/tasks/gitlab/x509/update_rake_spec.rb +++ b/spec/tasks/gitlab/x509/update_rake_spec.rb @@ -8,12 +8,13 @@ RSpec.describe 'gitlab:x509 namespace rake task' do end describe 'update_signatures' do - subject { run_rake_task('gitlab:x509:update_signatures') } - - let(:project) { create :project, :repository, path: X509Helpers::User1.path } + let(:user) { create(:user, email: X509Helpers::User1.certificate_email) } + let(:project) { create(:project, :repository, path: X509Helpers::User1.path, creator: user) } let(:x509_signed_commit) { project.commit_by(oid: '189a6c924013fc3fe40d6f1ec1dc20214183bc97') } let(:x509_commit) { Gitlab::X509::Commit.new(x509_signed_commit).signature } + subject { run_rake_task('gitlab:x509:update_signatures') } + it 'changes from unverified to verified if the certificate store contains the root certificate' do x509_commit @@ -22,21 +23,14 @@ RSpec.describe 'gitlab:x509 namespace rake task' do store.add_cert(certificate) allow(OpenSSL::X509::Store).to receive(:new).and_return(store) - expect(x509_commit.verification_status).to eq('unverified') expect_any_instance_of(Gitlab::X509::Commit).to receive(:update_signature!).and_call_original - - subject - - x509_commit.reload - expect(x509_commit.verification_status).to eq('verified') + expect { subject }.to change { x509_commit.reload.verification_status }.from('unverified').to('verified') end it 'returns if no signature is available' do - expect_any_instance_of(Gitlab::X509::Commit) do |x509_commit| - expect(x509_commit).not_to receive(:update_signature!) + expect_any_instance_of(Gitlab::X509::Commit).not_to receive(:update_signature!) - subject - end + subject end end end -- cgit v1.2.3 From 6e4e4023b46c786a99e1cfe8832fa5eff2728e0d Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 31 May 2021 11:41:25 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee --- .../jira_connect/app_descriptor_controller.rb | 1 + app/helpers/markup_helper.rb | 1 + ...rs_where_two_factor_auth_required_from_group.rb | 34 ++++++ db/schema_migrations/20210519154058 | 1 + db/structure.sql | 2 + lib/banzai/filter/truncate_source_filter.rb | 21 +++- ...rs_where_two_factor_auth_required_from_group.rb | 129 +++++++++++++++++++++ spec/helpers/markup_helper_spec.rb | 7 ++ .../banzai/filter/truncate_source_filter_spec.rb | 70 ++++++++--- ...ere_two_factor_auth_required_from_group_spec.rb | 84 ++++++++++++++ ...ere_two_factor_auth_required_from_group_spec.rb | 29 +++++ 11 files changed, 364 insertions(+), 15 deletions(-) create mode 100644 db/migrate/20210519154058_schedule_update_users_where_two_factor_auth_required_from_group.rb create mode 100644 db/schema_migrations/20210519154058 create mode 100644 lib/gitlab/background_migration/update_users_where_two_factor_auth_required_from_group.rb create mode 100644 spec/lib/gitlab/background_migration/update_users_where_two_factor_auth_required_from_group_spec.rb create mode 100644 spec/migrations/schedule_update_users_where_two_factor_auth_required_from_group_spec.rb diff --git a/app/controllers/jira_connect/app_descriptor_controller.rb b/app/controllers/jira_connect/app_descriptor_controller.rb index 137f830e40b..fee8b43aa6b 100644 --- a/app/controllers/jira_connect/app_descriptor_controller.rb +++ b/app/controllers/jira_connect/app_descriptor_controller.rb @@ -31,6 +31,7 @@ class JiraConnect::AppDescriptorController < JiraConnect::ApplicationController scopes: %w(READ WRITE DELETE), apiVersion: 1, apiMigrations: { + 'context-qsh': true, gdpr: true } } diff --git a/app/helpers/markup_helper.rb b/app/helpers/markup_helper.rb index 05a55a09271..7525481047e 100644 --- a/app/helpers/markup_helper.rb +++ b/app/helpers/markup_helper.rb @@ -118,6 +118,7 @@ module MarkupHelper def markup(file_name, text, context = {}) context[:project] ||= @project + context[:text_source] ||= :blob html = context.delete(:rendered) || markup_unsafe(file_name, text, context) prepare_for_rendering(html, context) end diff --git a/db/migrate/20210519154058_schedule_update_users_where_two_factor_auth_required_from_group.rb b/db/migrate/20210519154058_schedule_update_users_where_two_factor_auth_required_from_group.rb new file mode 100644 index 00000000000..2da84301a72 --- /dev/null +++ b/db/migrate/20210519154058_schedule_update_users_where_two_factor_auth_required_from_group.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class ScheduleUpdateUsersWhereTwoFactorAuthRequiredFromGroup < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + MIGRATION = 'UpdateUsersWhereTwoFactorAuthRequiredFromGroup' + DELAY_INTERVAL = 2.minutes + BATCH_SIZE = 10_000 + INDEX_NAME = 'index_users_require_two_factor_authentication_from_group_false' + + disable_ddl_transaction! + + class User < ActiveRecord::Base + include EachBatch + + self.table_name = 'users' + end + + def up + add_concurrent_index :users, + :require_two_factor_authentication_from_group, + where: 'require_two_factor_authentication_from_group = FALSE', + name: INDEX_NAME + + relation = User.where(require_two_factor_authentication_from_group: false) + + queue_background_migration_jobs_by_range_at_intervals( + relation, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + end + + def down + remove_concurrent_index_by_name :users, INDEX_NAME + end +end diff --git a/db/schema_migrations/20210519154058 b/db/schema_migrations/20210519154058 new file mode 100644 index 00000000000..9bd277e92db --- /dev/null +++ b/db/schema_migrations/20210519154058 @@ -0,0 +1 @@ +bdd82fc5cb2bbb322125c153c741002725853e23cd0ae0edbfd80563a4a87f2f \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 5d33a61e942..77b7557b14f 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -24646,6 +24646,8 @@ CREATE INDEX index_users_ops_dashboard_projects_on_project_id ON users_ops_dashb CREATE UNIQUE INDEX index_users_ops_dashboard_projects_on_user_id_and_project_id ON users_ops_dashboard_projects USING btree (user_id, project_id); +CREATE INDEX index_users_require_two_factor_authentication_from_group_false ON users USING btree (require_two_factor_authentication_from_group) WHERE (require_two_factor_authentication_from_group = false); + CREATE INDEX index_users_security_dashboard_projects_on_user_id ON users_security_dashboard_projects USING btree (user_id); CREATE INDEX index_users_star_projects_on_project_id ON users_star_projects USING btree (project_id); diff --git a/lib/banzai/filter/truncate_source_filter.rb b/lib/banzai/filter/truncate_source_filter.rb index 44f88b253d9..a21d4a44295 100644 --- a/lib/banzai/filter/truncate_source_filter.rb +++ b/lib/banzai/filter/truncate_source_filter.rb @@ -3,12 +3,29 @@ module Banzai module Filter class TruncateSourceFilter < HTML::Pipeline::TextFilter + CHARACTER_COUNT_LIMIT = 1.megabyte + USER_MSG_LIMIT = 10_000 + def call - return text unless context.key?(:limit) + # don't truncate if it's a :blob and no limit is set + return text if context[:text_source] == :blob && !context.key?(:limit) + + limit = context[:limit] || CHARACTER_COUNT_LIMIT + + # no sense in allowing `truncate_bytes` to duplicate a large + # string unless it's too big + return text if text.bytesize <= limit # Use three dots instead of the ellipsis Unicode character because # some clients show the raw Unicode value in the merge commit. - text.truncate_bytes(context[:limit], omission: '...') + trunc = text.truncate_bytes(limit, omission: '...') + + # allows us to indicate to the user that what they see is a truncated copy + if limit > USER_MSG_LIMIT + trunc.prepend("_The text is longer than #{limit} characters and has been visually truncated._\n\n") + end + + trunc end end end diff --git a/lib/gitlab/background_migration/update_users_where_two_factor_auth_required_from_group.rb b/lib/gitlab/background_migration/update_users_where_two_factor_auth_required_from_group.rb new file mode 100644 index 00000000000..f5ba9e63333 --- /dev/null +++ b/lib/gitlab/background_migration/update_users_where_two_factor_auth_required_from_group.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true +# rubocop:disable Style/Documentation + +module Gitlab + module BackgroundMigration + class UpdateUsersWhereTwoFactorAuthRequiredFromGroup # rubocop:disable Metrics/ClassLength + def perform(start_id, stop_id) + ActiveRecord::Base.connection.execute <<~SQL + UPDATE + users + SET + require_two_factor_authentication_from_group = TRUE + WHERE + users.id BETWEEN #{start_id} + AND #{stop_id} + AND users.require_two_factor_authentication_from_group = FALSE + AND users.id IN ( + SELECT + DISTINCT users_groups_query.user_id + FROM + ( + SELECT + users.id AS user_id, + members.source_id AS group_ids + FROM + users + LEFT JOIN members ON members.source_type = 'Namespace' + AND members.requested_at IS NULL + AND members.user_id = users.id + AND members.type = 'GroupMember' + WHERE + users.require_two_factor_authentication_from_group = FALSE + AND users.id BETWEEN #{start_id} + AND #{stop_id}) AS users_groups_query + INNER JOIN LATERAL ( + WITH RECURSIVE "base_and_ancestors" AS ( + ( + SELECT + "namespaces"."type", + "namespaces"."id", + "namespaces"."parent_id", + "namespaces"."require_two_factor_authentication" + FROM + "namespaces" + WHERE + "namespaces"."type" = 'Group' + AND "namespaces"."id" = users_groups_query.group_ids + ) + UNION + ( + SELECT + "namespaces"."type", + "namespaces"."id", + "namespaces"."parent_id", + "namespaces"."require_two_factor_authentication" + FROM + "namespaces", + "base_and_ancestors" + WHERE + "namespaces"."type" = 'Group' + AND "namespaces"."id" = "base_and_ancestors"."parent_id" + ) + ), + "base_and_descendants" AS ( + ( + SELECT + "namespaces"."type", + "namespaces"."id", + "namespaces"."parent_id", + "namespaces"."require_two_factor_authentication" + FROM + "namespaces" + WHERE + "namespaces"."type" = 'Group' + AND "namespaces"."id" = users_groups_query.group_ids + ) + UNION + ( + SELECT + "namespaces"."type", + "namespaces"."id", + "namespaces"."parent_id", + "namespaces"."require_two_factor_authentication" + FROM + "namespaces", + "base_and_descendants" + WHERE + "namespaces"."type" = 'Group' + AND "namespaces"."parent_id" = "base_and_descendants"."id" + ) + ) + SELECT + "namespaces".* + FROM + ( + ( + SELECT + "namespaces"."type", + "namespaces"."id", + "namespaces"."parent_id", + "namespaces"."require_two_factor_authentication" + FROM + "base_and_ancestors" AS "namespaces" + WHERE + "namespaces"."type" = 'Group' + ) + UNION + ( + SELECT + "namespaces"."type", + "namespaces"."id", + "namespaces"."parent_id", + "namespaces"."require_two_factor_authentication" + FROM + "base_and_descendants" AS "namespaces" + WHERE + "namespaces"."type" = 'Group' + ) + ) namespaces + WHERE + "namespaces"."type" = 'Group' + AND "namespaces"."require_two_factor_authentication" = TRUE + ) AS hierarchy_tree ON TRUE + ); + SQL + end + end + end +end diff --git a/spec/helpers/markup_helper_spec.rb b/spec/helpers/markup_helper_spec.rb index 00a59f037e0..e946857ac77 100644 --- a/spec/helpers/markup_helper_spec.rb +++ b/spec/helpers/markup_helper_spec.rb @@ -418,6 +418,13 @@ FooBar describe '#markup' do let(:content) { 'Noël' } + it 'sets the :text_source to :blob in the context' do + context = {} + helper.markup('foo.md', content, context) + + expect(context).to include(text_source: :blob) + end + it 'preserves encoding' do expect(content.encoding.name).to eq('UTF-8') expect(helper.markup('foo.rst', content).encoding.name).to eq('UTF-8') diff --git a/spec/lib/banzai/filter/truncate_source_filter_spec.rb b/spec/lib/banzai/filter/truncate_source_filter_spec.rb index d5eb8b738b1..8970aa1d382 100644 --- a/spec/lib/banzai/filter/truncate_source_filter_spec.rb +++ b/spec/lib/banzai/filter/truncate_source_filter_spec.rb @@ -8,24 +8,68 @@ RSpec.describe Banzai::Filter::TruncateSourceFilter do let(:short_text) { 'foo' * 10 } let(:long_text) { ([short_text] * 10).join(' ') } - it 'does nothing when limit is unspecified' do - output = filter(long_text) - - expect(output).to eq(long_text) + before do + stub_const("#{described_class}::CHARACTER_COUNT_LIMIT", 50) + stub_const("#{described_class}::USER_MSG_LIMIT", 20) end - it 'does nothing to a short-enough text' do - output = filter(short_text, limit: short_text.bytesize) + context 'when markdown belongs to a blob' do + it 'does nothing when limit is unspecified' do + output = filter(long_text, text_source: :blob) + + expect(output).to eq(long_text) + end + + it 'truncates normally when limit specified' do + truncated = 'foofoof...' + + output = filter(long_text, text_source: :blob, limit: 10) - expect(output).to eq(short_text) + expect(output).to eq(truncated) + end end - it 'truncates UTF-8 text by bytes, on a character boundary' do - utf8_text = '日本語の文字が大きい' - truncated = '日...' + context 'when markdown belongs to a field (non-blob)' do + it 'does nothing when limit is greater' do + output = filter(long_text, limit: 1.megabyte) + + expect(output).to eq(long_text) + end + + it 'truncates to the default when limit is unspecified' do + stub_const("#{described_class}::USER_MSG_LIMIT", 200) + truncated = 'foofoofoofoofoofoofoofoofoofoo foofoofoofoofoof...' + + output = filter(long_text) + + expect(output).to eq(truncated) + end + + it 'prepends the user message' do + truncated = <<~TEXT + _The text is longer than 50 characters and has been visually truncated._ + + foofoofoofoofoofoofoofoofoofoo foofoofoofoofoof... + TEXT + + output = filter(long_text) + + expect(output).to eq(truncated.strip) + end + + it 'does nothing to a short-enough text' do + output = filter(short_text, limit: short_text.bytesize) + + expect(output).to eq(short_text) + end + + it 'truncates UTF-8 text by bytes, on a character boundary' do + utf8_text = '日本語の文字が大きい' + truncated = '日...' - expect(filter(utf8_text, limit: truncated.bytesize)).to eq(truncated) - expect(filter(utf8_text, limit: utf8_text.bytesize)).to eq(utf8_text) - expect(filter(utf8_text, limit: utf8_text.mb_chars.size)).not_to eq(utf8_text) + expect(filter(utf8_text, limit: truncated.bytesize)).to eq(truncated) + expect(filter(utf8_text, limit: utf8_text.bytesize)).to eq(utf8_text) + expect(filter(utf8_text, limit: utf8_text.mb_chars.size)).not_to eq(utf8_text) + end end end diff --git a/spec/lib/gitlab/background_migration/update_users_where_two_factor_auth_required_from_group_spec.rb b/spec/lib/gitlab/background_migration/update_users_where_two_factor_auth_required_from_group_spec.rb new file mode 100644 index 00000000000..e14328b6150 --- /dev/null +++ b/spec/lib/gitlab/background_migration/update_users_where_two_factor_auth_required_from_group_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BackgroundMigration::UpdateUsersWhereTwoFactorAuthRequiredFromGroup, :migration, schema: 20210519154058 do + include MigrationHelpers::NamespacesHelpers + + let(:group_with_2fa_parent) { create_namespace('parent', Gitlab::VisibilityLevel::PRIVATE, require_two_factor_authentication: true) } + let(:group_with_2fa_child) { create_namespace('child', Gitlab::VisibilityLevel::PRIVATE, parent_id: group_with_2fa_parent.id) } + let(:members_table) { table(:members) } + let(:users_table) { table(:users) } + + subject { described_class.new } + + describe '#perform' do + context 'with group members' do + let(:user_1) { create_user('user@example.com') } + let!(:member) { create_group_member(user_1, group_with_2fa_parent) } + let!(:user_without_group) { create_user('user_without@example.com') } + let(:user_other) { create_user('user_other@example.com') } + let!(:member_other) { create_group_member(user_other, group_with_2fa_parent) } + + it 'updates user when user should be required to establish two factor authentication' do + subject.perform(user_1.id, user_without_group.id) + + expect(user_1.reload.require_two_factor_authentication_from_group).to eq(true) + end + + it 'does not update user who is not in current batch' do + subject.perform(user_1.id, user_without_group.id) + + expect(user_other.reload.require_two_factor_authentication_from_group).to eq(false) + end + + it 'updates all users in current batch' do + subject.perform(user_1.id, user_other.id) + + expect(user_other.reload.require_two_factor_authentication_from_group).to eq(true) + end + + it 'updates user when user is member of group in which parent group requires two factor authentication' do + member.destroy! + + subgroup = create_namespace('subgroup', Gitlab::VisibilityLevel::PRIVATE, require_two_factor_authentication: false, parent_id: group_with_2fa_child.id) + create_group_member(user_1, subgroup) + + subject.perform(user_1.id, user_other.id) + + expect(user_1.reload.require_two_factor_authentication_from_group).to eq(true) + end + + it 'updates user when user is member of a group and the subgroup requires two factor authentication' do + member.destroy! + + parent = create_namespace('other_parent', Gitlab::VisibilityLevel::PRIVATE, require_two_factor_authentication: false) + create_namespace('other_subgroup', Gitlab::VisibilityLevel::PRIVATE, require_two_factor_authentication: true, parent_id: parent.id) + create_group_member(user_1, parent) + + subject.perform(user_1.id, user_other.id) + + expect(user_1.reload.require_two_factor_authentication_from_group).to eq(true) + end + + it 'does not update user when not a member of a group that requires two factor authentication' do + member_other.destroy! + + other_group = create_namespace('other_group', Gitlab::VisibilityLevel::PRIVATE, require_two_factor_authentication: false) + create_group_member(user_other, other_group) + + subject.perform(user_1.id, user_other.id) + + expect(user_other.reload.require_two_factor_authentication_from_group).to eq(false) + end + end + end + + def create_user(email, require_2fa: false) + users_table.create!(email: email, projects_limit: 10, require_two_factor_authentication_from_group: require_2fa) + end + + def create_group_member(user, group) + members_table.create!(user_id: user.id, source_id: group.id, access_level: GroupMember::MAINTAINER, source_type: "Namespace", type: "GroupMember", notification_level: 3) + end +end diff --git a/spec/migrations/schedule_update_users_where_two_factor_auth_required_from_group_spec.rb b/spec/migrations/schedule_update_users_where_two_factor_auth_required_from_group_spec.rb new file mode 100644 index 00000000000..cec141cacc9 --- /dev/null +++ b/spec/migrations/schedule_update_users_where_two_factor_auth_required_from_group_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20210519154058_schedule_update_users_where_two_factor_auth_required_from_group.rb') + +RSpec.describe ScheduleUpdateUsersWhereTwoFactorAuthRequiredFromGroup do + let(:users) { table(:users) } + let!(:user_1) { users.create!(require_two_factor_authentication_from_group: false, name: "user1", email: "user1@example.com", projects_limit: 1) } + let!(:user_2) { users.create!(require_two_factor_authentication_from_group: true, name: "user2", email: "user2@example.com", projects_limit: 1) } + let!(:user_3) { users.create!(require_two_factor_authentication_from_group: false, name: "user3", email: "user3@example.com", projects_limit: 1) } + + before do + stub_const("#{described_class.name}::BATCH_SIZE", 1) + end + + it 'schedules jobs for users that do not require two factor authentication' do + Sidekiq::Testing.fake! do + freeze_time do + migrate! + + expect(described_class::MIGRATION).to be_scheduled_delayed_migration( + 2.minutes, user_1.id, user_1.id) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration( + 4.minutes, user_3.id, user_3.id) + expect(BackgroundMigrationWorker.jobs.size).to eq(2) + end + end + end +end -- cgit v1.2.3 From 15c040a6bd71894260b66a90685070c0babfee76 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 31 May 2021 11:42:18 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee --- app/policies/concerns/policy_actor.rb | 4 ++ app/policies/global_policy.rb | 10 ++++ app/serializers/member_entity.rb | 4 +- config/initializers/lograge.rb | 1 + lib/banzai/filter/absolute_link_filter.rb | 5 +- .../filter/ascii_doc_post_processing_filter.rb | 10 +++- lib/banzai/filter/base_relative_link_filter.rb | 5 +- lib/banzai/filter/color_filter.rb | 5 +- lib/banzai/filter/custom_emoji_filter.rb | 2 +- lib/banzai/filter/emoji_filter.rb | 2 +- lib/banzai/filter/footnote_filter.rb | 12 +++-- lib/banzai/filter/gollum_tags_filter.rb | 2 +- lib/banzai/filter/image_lazy_load_filter.rb | 5 +- lib/banzai/filter/inline_diff_filter.rb | 2 +- .../filter/inline_metrics_redactor_filter.rb | 3 +- lib/banzai/filter/kroki_filter.rb | 5 +- lib/banzai/filter/markdown_post_escape_filter.rb | 9 +++- lib/banzai/filter/math_filter.rb | 9 +++- lib/banzai/filter/mermaid_filter.rb | 5 +- lib/banzai/filter/plantuml_filter.rb | 7 ++- lib/banzai/filter/suggestion_filter.rb | 5 +- lib/banzai/filter/syntax_highlight_filter.rb | 5 +- lib/banzai/filter/table_of_contents_filter.rb | 5 +- lib/banzai/filter/wiki_link_filter.rb | 13 +++-- lib/gitlab/auth.rb | 12 +++-- lib/gitlab/auth/user_access_denied_reason.rb | 5 ++ lib/gitlab/diff/suggestions_parser.rb | 5 +- lib/gitlab/utils.rb | 18 +++++++ lib/gitlab/utils/nokogiri.rb | 24 +++++++++ spec/features/users/login_spec.rb | 19 +++++++ spec/initializers/lograge_spec.rb | 21 ++++++++ .../banzai/pipeline/post_process_pipeline_spec.rb | 7 ++- .../gitlab/auth/user_access_denied_reason_spec.rb | 8 +++ spec/lib/gitlab/git_access_spec.rb | 14 +++++ spec/lib/gitlab/utils/nokogiri_spec.rb | 34 ++++++++++++ spec/lib/gitlab/utils_spec.rb | 23 ++++++++ spec/policies/global_policy_spec.rb | 24 +++++++++ spec/requests/git_http_spec.rb | 61 ++++++++++++++++++++++ spec/requests/lfs_http_spec.rb | 4 +- 39 files changed, 374 insertions(+), 40 deletions(-) create mode 100644 lib/gitlab/utils/nokogiri.rb create mode 100644 spec/lib/gitlab/utils/nokogiri_spec.rb diff --git a/app/policies/concerns/policy_actor.rb b/app/policies/concerns/policy_actor.rb index cd19b46ad6c..08a26da6673 100644 --- a/app/policies/concerns/policy_actor.rb +++ b/app/policies/concerns/policy_actor.rb @@ -80,6 +80,10 @@ module PolicyActor def can_read_all_resources? false end + + def password_expired? + false + end end PolicyActor.prepend_mod_with('PolicyActor') diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb index 85263ec7c87..73757891cd6 100644 --- a/app/policies/global_policy.rb +++ b/app/policies/global_policy.rb @@ -15,6 +15,10 @@ class GlobalPolicy < BasePolicy @user&.required_terms_not_accepted? end + condition(:password_expired, scope: :user) do + @user&.password_expired? + end + condition(:project_bot, scope: :user) { @user&.project_bot? } condition(:migration_bot, scope: :user) { @user&.migration_bot? } @@ -73,6 +77,12 @@ class GlobalPolicy < BasePolicy prevent :access_git end + rule { password_expired }.policy do + prevent :access_api + prevent :access_git + prevent :use_slash_commands + end + rule { can_create_group }.policy do enable :create_group end diff --git a/app/serializers/member_entity.rb b/app/serializers/member_entity.rb index 7559a03bd3b..5100a41638e 100644 --- a/app/serializers/member_entity.rb +++ b/app/serializers/member_entity.rb @@ -40,7 +40,9 @@ class MemberEntity < Grape::Entity expose :valid_level_roles, as: :valid_roles - expose :user, if: -> (member) { member.user.present? }, using: MemberUserEntity + expose :user, if: -> (member) { member.user.present? } do |member, options| + MemberUserEntity.represent(member.user, source: options[:source]) + end expose :invite, if: -> (member) { member.invite? } do expose :email do |member| diff --git a/config/initializers/lograge.rb b/config/initializers/lograge.rb index e8479bc6aa4..61e357808d9 100644 --- a/config/initializers/lograge.rb +++ b/config/initializers/lograge.rb @@ -18,6 +18,7 @@ unless Gitlab::Runtime.sidekiq? data[:db_duration_s] = Gitlab::Utils.ms_to_round_sec(data.delete(:db)) if data[:db] data[:view_duration_s] = Gitlab::Utils.ms_to_round_sec(data.delete(:view)) if data[:view] data[:duration_s] = Gitlab::Utils.ms_to_round_sec(data.delete(:duration)) if data[:duration] + data[:location] = Gitlab::Utils.removes_sensitive_data_from_url(data[:location]) if data[:location] # Remove empty hashes to prevent type mismatches # These are set to empty hashes in Lograge's ActionCable subscriber diff --git a/lib/banzai/filter/absolute_link_filter.rb b/lib/banzai/filter/absolute_link_filter.rb index a9bdb004c4b..cc7bf3ed556 100644 --- a/lib/banzai/filter/absolute_link_filter.rb +++ b/lib/banzai/filter/absolute_link_filter.rb @@ -6,10 +6,13 @@ module Banzai module Filter # HTML filter that converts relative urls into absolute ones. class AbsoluteLinkFilter < HTML::Pipeline::Filter + CSS = 'a.gfm' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze + def call return doc unless context[:only_path] == false - doc.search('a.gfm').each do |el| + doc.xpath(XPATH).each do |el| process_link_attr el.attribute('href') end diff --git a/lib/banzai/filter/ascii_doc_post_processing_filter.rb b/lib/banzai/filter/ascii_doc_post_processing_filter.rb index 09f0fd7df45..83c729e13b5 100644 --- a/lib/banzai/filter/ascii_doc_post_processing_filter.rb +++ b/lib/banzai/filter/ascii_doc_post_processing_filter.rb @@ -3,14 +3,20 @@ module Banzai module Filter class AsciiDocPostProcessingFilter < HTML::Pipeline::Filter + CSS_MATH = '[data-math-style]' + XPATH_MATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_MATH).freeze + CSS_MERM = '[data-mermaid-style]' + XPATH_MERM = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_MERM).freeze + def call - doc.search('[data-math-style]').each do |node| + doc.xpath(XPATH_MATH).each do |node| node.set_attribute('class', 'code math js-render-math') end - doc.search('[data-mermaid-style]').each do |node| + doc.xpath(XPATH_MERM).each do |node| node.set_attribute('class', 'js-render-mermaid') end + doc end end diff --git a/lib/banzai/filter/base_relative_link_filter.rb b/lib/banzai/filter/base_relative_link_filter.rb index 84a6e18e77b..3f775abb185 100644 --- a/lib/banzai/filter/base_relative_link_filter.rb +++ b/lib/banzai/filter/base_relative_link_filter.rb @@ -7,6 +7,9 @@ module Banzai class BaseRelativeLinkFilter < HTML::Pipeline::Filter include Gitlab::Utils::StrongMemoize + CSS = 'a:not(.gfm), img:not(.gfm), video:not(.gfm), audio:not(.gfm)' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze + protected def linkable_attributes @@ -41,7 +44,7 @@ module Banzai def fetch_linkable_attributes attrs = [] - attrs += doc.search('a:not(.gfm), img:not(.gfm), video:not(.gfm), audio:not(.gfm)').flat_map do |el| + attrs += doc.xpath(XPATH).flat_map do |el| [el.attribute('href'), el.attribute('src'), el.attribute('data-src')] end diff --git a/lib/banzai/filter/color_filter.rb b/lib/banzai/filter/color_filter.rb index 0aca7441638..58e9b8cdba1 100644 --- a/lib/banzai/filter/color_filter.rb +++ b/lib/banzai/filter/color_filter.rb @@ -7,8 +7,11 @@ module Banzai class ColorFilter < HTML::Pipeline::Filter COLOR_CHIP_CLASS = 'gfm-color_chip' + CSS = 'code' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze + def call - doc.css('code').each do |node| + doc.xpath(XPATH).each do |node| color = ColorParser.parse(node.content) node << color_chip(color) if color end diff --git a/lib/banzai/filter/custom_emoji_filter.rb b/lib/banzai/filter/custom_emoji_filter.rb index 3171231dc9b..a5f1a22c483 100644 --- a/lib/banzai/filter/custom_emoji_filter.rb +++ b/lib/banzai/filter/custom_emoji_filter.rb @@ -11,7 +11,7 @@ module Banzai return doc unless context[:project] return doc unless Feature.enabled?(:custom_emoji, context[:project]) - doc.search(".//text()").each do |node| + doc.xpath('descendant-or-self::text()').each do |node| content = node.to_html next if has_ancestor?(node, IGNORED_ANCESTOR_TAGS) diff --git a/lib/banzai/filter/emoji_filter.rb b/lib/banzai/filter/emoji_filter.rb index 8952a3ff6b4..9d24bf028b6 100644 --- a/lib/banzai/filter/emoji_filter.rb +++ b/lib/banzai/filter/emoji_filter.rb @@ -11,7 +11,7 @@ module Banzai IGNORE_UNICODE_EMOJIS = %w(™ © ®).freeze def call - doc.search(".//text()").each do |node| + doc.xpath('descendant-or-self::text()').each do |node| content = node.to_html next if has_ancestor?(node, IGNORED_ANCESTOR_TAGS) diff --git a/lib/banzai/filter/footnote_filter.rb b/lib/banzai/filter/footnote_filter.rb index 5474242e03c..0f856dc0eb9 100644 --- a/lib/banzai/filter/footnote_filter.rb +++ b/lib/banzai/filter/footnote_filter.rb @@ -23,17 +23,23 @@ module Banzai FOOTNOTE_LINK_REFERENCE_PATTERN = /\A#{FOOTNOTE_LINK_ID_PREFIX}\d+\z/.freeze FOOTNOTE_START_NUMBER = 1 + CSS_SECTION = "ol > li[id=#{FOOTNOTE_ID_PREFIX}#{FOOTNOTE_START_NUMBER}]" + XPATH_SECTION = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_SECTION).freeze + CSS_FOOTNOTE = 'sup > a[id]' + XPATH_FOOTNOTE = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_FOOTNOTE).freeze + def call - return doc unless first_footnote = doc.at_css("ol > li[id=#{fn_id(FOOTNOTE_START_NUMBER)}]") + return doc unless first_footnote = doc.at_xpath(XPATH_SECTION) # Sanitization stripped off the section wrapper - add it back in first_footnote.parent.wrap('
') rand_suffix = "-#{random_number}" modified_footnotes = {} - doc.css('sup > a[id]').each do |link_node| + doc.xpath(XPATH_FOOTNOTE).each do |link_node| ref_num = link_node[:id].delete_prefix(FOOTNOTE_LINK_ID_PREFIX) - footnote_node = doc.at_css("li[id=#{fn_id(ref_num)}]") + node_xpath = Gitlab::Utils::Nokogiri.css_to_xpath("li[id=#{fn_id(ref_num)}]") + footnote_node = doc.at_xpath(node_xpath) if INTEGER_PATTERN.match?(ref_num) && (footnote_node || modified_footnotes[ref_num]) link_node[:href] += rand_suffix diff --git a/lib/banzai/filter/gollum_tags_filter.rb b/lib/banzai/filter/gollum_tags_filter.rb index 6de9f2b86f6..0548d5a9997 100644 --- a/lib/banzai/filter/gollum_tags_filter.rb +++ b/lib/banzai/filter/gollum_tags_filter.rb @@ -60,7 +60,7 @@ module Banzai IGNORED_ANCESTOR_TAGS = %w(pre code tt).to_set def call - doc.search(".//text()").each do |node| + doc.xpath('descendant-or-self::text()').each do |node| next if has_ancestor?(node, IGNORED_ANCESTOR_TAGS) next unless node.content =~ TAGS_PATTERN diff --git a/lib/banzai/filter/image_lazy_load_filter.rb b/lib/banzai/filter/image_lazy_load_filter.rb index d8b9eb29cf5..916c135b777 100644 --- a/lib/banzai/filter/image_lazy_load_filter.rb +++ b/lib/banzai/filter/image_lazy_load_filter.rb @@ -6,8 +6,11 @@ module Banzai # HTML filter that moves the value of image `src` attributes to `data-src` # so they can be lazy loaded. class ImageLazyLoadFilter < HTML::Pipeline::Filter + CSS = 'img' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze + def call - doc.xpath('descendant-or-self::img').each do |img| + doc.xpath(XPATH).each do |img| img.add_class('lazy') img['data-src'] = img['src'] img['src'] = LazyImageTagHelper.placeholder_image diff --git a/lib/banzai/filter/inline_diff_filter.rb b/lib/banzai/filter/inline_diff_filter.rb index 5a1c0bee32d..e47ff15e7b7 100644 --- a/lib/banzai/filter/inline_diff_filter.rb +++ b/lib/banzai/filter/inline_diff_filter.rb @@ -7,7 +7,7 @@ module Banzai IGNORED_ANCESTOR_TAGS = %w(pre code tt).to_set def call - doc.search(".//text()").each do |node| + doc.xpath('descendant-or-self::text()').each do |node| next if has_ancestor?(node, IGNORED_ANCESTOR_TAGS) content = node.to_html diff --git a/lib/banzai/filter/inline_metrics_redactor_filter.rb b/lib/banzai/filter/inline_metrics_redactor_filter.rb index 2259115acfc..b256815ae84 100644 --- a/lib/banzai/filter/inline_metrics_redactor_filter.rb +++ b/lib/banzai/filter/inline_metrics_redactor_filter.rb @@ -8,6 +8,7 @@ module Banzai include Gitlab::Utils::StrongMemoize METRICS_CSS_CLASS = '.js-render-metrics' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(METRICS_CSS_CLASS).freeze EMBED_LIMIT = 100 Route = Struct.new(:regex, :permission) @@ -41,7 +42,7 @@ module Banzai # @return [Nokogiri::XML::NodeSet] def nodes strong_memoize(:nodes) do - nodes = doc.css(METRICS_CSS_CLASS) + nodes = doc.xpath(XPATH) nodes.drop(EMBED_LIMIT).each(&:remove) nodes diff --git a/lib/banzai/filter/kroki_filter.rb b/lib/banzai/filter/kroki_filter.rb index dbd4de32a47..3803302c324 100644 --- a/lib/banzai/filter/kroki_filter.rb +++ b/lib/banzai/filter/kroki_filter.rb @@ -15,10 +15,11 @@ module Banzai .map { |diagram_type| %(pre[lang="#{diagram_type}"] > code) } .join(', ') - return doc unless doc.at(diagram_selectors) + xpath = Gitlab::Utils::Nokogiri.css_to_xpath(diagram_selectors) + return doc unless doc.at_xpath(xpath) diagram_format = "svg" - doc.css(diagram_selectors).each do |node| + doc.xpath(xpath).each do |node| diagram_type = node.parent['lang'] img_tag = Nokogiri::HTML::DocumentFragment.parse(%()) node.parent.replace(img_tag) diff --git a/lib/banzai/filter/markdown_post_escape_filter.rb b/lib/banzai/filter/markdown_post_escape_filter.rb index ad32e9afbf5..b69afdcfebe 100644 --- a/lib/banzai/filter/markdown_post_escape_filter.rb +++ b/lib/banzai/filter/markdown_post_escape_filter.rb @@ -8,6 +8,11 @@ module Banzai NOT_LITERAL_REGEX = %r{#{LITERAL_KEYWORD}-((%5C|\\).+?)-#{LITERAL_KEYWORD}}.freeze SPAN_REGEX = %r{(.*?)}.freeze + CSS_A = 'a' + XPATH_A = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_A).freeze + CSS_CODE = 'code' + XPATH_CODE = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_CODE).freeze + def call return doc unless result[:escaped_literals] @@ -24,12 +29,12 @@ module Banzai # Banzai::Renderer::CommonMark::HTML. However, we eventually want to use # the built-in compiled renderer, rather than the ruby version, for speed. # So let's do this work here. - doc.css('a').each do |node| + doc.xpath(XPATH_A).each do |node| node.attributes['href'].value = node.attributes['href'].value.gsub(SPAN_REGEX, '\1') if node.attributes['href'] node.attributes['title'].value = node.attributes['title'].value.gsub(SPAN_REGEX, '\1') if node.attributes['title'] end - doc.css('code').each do |node| + doc.xpath(XPATH_CODE).each do |node| node.attributes['lang'].value = node.attributes['lang'].value.gsub(SPAN_REGEX, '\1') if node.attributes['lang'] end diff --git a/lib/banzai/filter/math_filter.rb b/lib/banzai/filter/math_filter.rb index 2247984b86d..53dafe45fb3 100644 --- a/lib/banzai/filter/math_filter.rb +++ b/lib/banzai/filter/math_filter.rb @@ -10,6 +10,11 @@ module Banzai # HTML filter that adds class="code math" and removes the dollar sign in $`2+2`$. # class MathFilter < HTML::Pipeline::Filter + CSS_MATH = 'pre.code.language-math' + XPATH_MATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_MATH).freeze + CSS_CODE = 'code' + XPATH_CODE = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_CODE).freeze + # Attribute indicating inline or display math. STYLE_ATTRIBUTE = 'data-math-style' @@ -21,7 +26,7 @@ module Banzai DOLLAR_SIGN = '$' def call - doc.css('code').each do |code| + doc.xpath(XPATH_CODE).each do |code| closing = code.next opening = code.previous @@ -39,7 +44,7 @@ module Banzai end end - doc.css('pre.code.language-math').each do |el| + doc.xpath(XPATH_MATH).each do |el| el[STYLE_ATTRIBUTE] = 'display' el[:class] += " #{TAG_CLASS}" end diff --git a/lib/banzai/filter/mermaid_filter.rb b/lib/banzai/filter/mermaid_filter.rb index f0adb83af8a..aaaf851ccf0 100644 --- a/lib/banzai/filter/mermaid_filter.rb +++ b/lib/banzai/filter/mermaid_filter.rb @@ -4,8 +4,11 @@ module Banzai module Filter class MermaidFilter < HTML::Pipeline::Filter + CSS = 'pre[lang="mermaid"] > code' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze + def call - doc.css('pre[lang="mermaid"] > code').add_class('js-render-mermaid') + doc.xpath(XPATH).add_class('js-render-mermaid') doc end diff --git a/lib/banzai/filter/plantuml_filter.rb b/lib/banzai/filter/plantuml_filter.rb index 37d4126c1ba..93370178a61 100644 --- a/lib/banzai/filter/plantuml_filter.rb +++ b/lib/banzai/filter/plantuml_filter.rb @@ -8,12 +8,15 @@ module Banzai # HTML that replaces all `code plantuml` tags with PlantUML img tags. # class PlantumlFilter < HTML::Pipeline::Filter + CSS = 'pre > code[lang="plantuml"]' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze + def call - return doc unless settings.plantuml_enabled? && doc.at('pre > code[lang="plantuml"]') + return doc unless settings.plantuml_enabled? && doc.at_xpath(XPATH) plantuml_setup - doc.css('pre > code[lang="plantuml"]').each do |node| + doc.xpath(XPATH).each do |node| img_tag = Nokogiri::HTML::DocumentFragment.parse( Asciidoctor::PlantUml::Processor.plantuml_content(node.content, {})) node.parent.replace(img_tag) diff --git a/lib/banzai/filter/suggestion_filter.rb b/lib/banzai/filter/suggestion_filter.rb index 56a14ec0737..aa1fcb1021c 100644 --- a/lib/banzai/filter/suggestion_filter.rb +++ b/lib/banzai/filter/suggestion_filter.rb @@ -7,10 +7,13 @@ module Banzai # Class used for tagging elements that should be rendered TAG_CLASS = 'js-render-suggestion' + CSS = 'pre.language-suggestion > code' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze + def call return doc unless suggestions_filter_enabled? - doc.search('pre.language-suggestion > code').each do |node| + doc.xpath(XPATH).each do |node| node.add_class(TAG_CLASS) end diff --git a/lib/banzai/filter/syntax_highlight_filter.rb b/lib/banzai/filter/syntax_highlight_filter.rb index b16ea689d2e..f1440c13d47 100644 --- a/lib/banzai/filter/syntax_highlight_filter.rb +++ b/lib/banzai/filter/syntax_highlight_filter.rb @@ -14,8 +14,11 @@ module Banzai PARAMS_DELIMITER = ':' LANG_PARAMS_ATTR = 'data-lang-params' + CSS = 'pre:not([data-math-style]):not([data-mermaid-style]):not([data-kroki-style]) > code' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze + def call - doc.search('pre:not([data-math-style]):not([data-mermaid-style]):not([data-kroki-style]) > code').each do |node| + doc.xpath(XPATH).each do |node| highlight_node(node) end diff --git a/lib/banzai/filter/table_of_contents_filter.rb b/lib/banzai/filter/table_of_contents_filter.rb index b362607aed2..13ca9cde567 100644 --- a/lib/banzai/filter/table_of_contents_filter.rb +++ b/lib/banzai/filter/table_of_contents_filter.rb @@ -19,6 +19,9 @@ module Banzai class TableOfContentsFilter < HTML::Pipeline::Filter include Gitlab::Utils::Markdown + CSS = 'h1, h2, h3, h4, h5, h6' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze + def call return doc if context[:no_header_anchors] @@ -27,7 +30,7 @@ module Banzai headers = Hash.new(0) header_root = current_header = HeaderNode.new - doc.css('h1, h2, h3, h4, h5, h6').each do |node| + doc.xpath(XPATH).each do |node| if header_content = node.children.first id = string_to_anchor(node.text) diff --git a/lib/banzai/filter/wiki_link_filter.rb b/lib/banzai/filter/wiki_link_filter.rb index 44f13612fde..2b95d87ff8e 100644 --- a/lib/banzai/filter/wiki_link_filter.rb +++ b/lib/banzai/filter/wiki_link_filter.rb @@ -10,14 +10,21 @@ module Banzai class WikiLinkFilter < HTML::Pipeline::Filter include Gitlab::Utils::SanitizeNodeLink + CSS_A = 'a:not(.gfm)' + XPATH_A = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_A).freeze + CSS_VA = 'video, audio' + XPATH_VA = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_VA).freeze + CSS_IMG = 'img' + XPATH_IMG = Gitlab::Utils::Nokogiri.css_to_xpath(CSS_IMG).freeze + def call return doc unless wiki? - doc.search('a:not(.gfm)').each { |el| process_link(el.attribute('href'), el) } + doc.xpath(XPATH_A).each { |el| process_link(el.attribute('href'), el) } - doc.search('video, audio').each { |el| process_link(el.attribute('src'), el) } + doc.xpath(XPATH_VA).each { |el| process_link(el.attribute('src'), el) } - doc.search('img').each do |el| + doc.xpath(XPATH_IMG).each do |el| attr = el.attribute('data-src') || el.attribute('src') process_link(attr, el) diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index c6997288b65..4489fc9f3b2 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -84,7 +84,7 @@ module Gitlab Gitlab::Auth::UniqueIpsLimiter.limit_user! do user = User.by_login(login) - break if user && !user.can?(:log_in) + break if user && !can_user_login_with_non_expired_password?(user) authenticators = [] @@ -182,7 +182,7 @@ module Gitlab if valid_oauth_token?(token) user = User.id_in(token.resource_owner_id).first - return unless user&.can?(:log_in) + return unless user && can_user_login_with_non_expired_password?(user) Gitlab::Auth::Result.new(user, nil, :oauth, full_authentication_abilities) end @@ -200,7 +200,7 @@ module Gitlab return if project && token.user.project_bot? && !project.bots.include?(token.user) - if token.user.can?(:log_in) || token.user.project_bot? + if can_user_login_with_non_expired_password?(token.user) || token.user.project_bot? Gitlab::Auth::Result.new(token.user, nil, :personal_access_token, abilities_for_scopes(token.scopes)) end end @@ -285,7 +285,7 @@ module Gitlab return unless build.project.builds_enabled? if build.user - return unless build.user.can?(:log_in) || (build.user.project_bot? && build.project.bots&.include?(build.user)) + return unless can_user_login_with_non_expired_password?(build.user) || (build.user.project_bot? && build.project.bots&.include?(build.user)) # If user is assigned to build, use restricted credentials of user Gitlab::Auth::Result.new(build.user, build.project, :build, build_authentication_abilities) @@ -380,6 +380,10 @@ module Gitlab user.increment_failed_attempts! end + + def can_user_login_with_non_expired_password?(user) + user.can?(:log_in) && !user.password_expired? + end end end end diff --git a/lib/gitlab/auth/user_access_denied_reason.rb b/lib/gitlab/auth/user_access_denied_reason.rb index 36b54ba2e46..6639000dba8 100644 --- a/lib/gitlab/auth/user_access_denied_reason.rb +++ b/lib/gitlab/auth/user_access_denied_reason.rb @@ -23,6 +23,9 @@ module Gitlab "Your primary email address is not confirmed. "\ "Please check your inbox for the confirmation instructions. "\ "In case the link is expired, you can request a new confirmation email at #{Rails.application.routes.url_helpers.new_user_confirmation_url}" + when :password_expired + "Your password expired. "\ + "Please access GitLab from a web browser to update your password." else "Your account has been blocked." end @@ -41,6 +44,8 @@ module Gitlab :deactivated elsif !@user.confirmed? :unconfirmed + elsif @user.password_expired? + :password_expired else :blocked end diff --git a/lib/gitlab/diff/suggestions_parser.rb b/lib/gitlab/diff/suggestions_parser.rb index f3e6fc455ac..6f126147113 100644 --- a/lib/gitlab/diff/suggestions_parser.rb +++ b/lib/gitlab/diff/suggestions_parser.rb @@ -6,6 +6,9 @@ module Gitlab # Matches for instance "-1", "+1" or "-1+2". SUGGESTION_CONTEXT = /^(\-(?\d+))?(\+(?\d+))?$/.freeze + CSS = 'pre.language-suggestion' + XPATH = Gitlab::Utils::Nokogiri.css_to_xpath(CSS).freeze + class << self # Returns an array of Gitlab::Diff::Suggestion which represents each # suggestion in the given text. @@ -17,7 +20,7 @@ module Gitlab no_original_data: true, suggestions_filter_enabled: supports_suggestion) doc = Nokogiri::HTML(html) - suggestion_nodes = doc.search('pre.language-suggestion') + suggestion_nodes = doc.xpath(XPATH) return [] if suggestion_nodes.empty? diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index d70e5c3594c..77e0e2ca96c 100644 --- a/lib/gitlab/utils.rb +++ b/lib/gitlab/utils.rb @@ -197,6 +197,24 @@ module Gitlab rescue Addressable::URI::InvalidURIError, TypeError end + def removes_sensitive_data_from_url(uri_string) + uri = parse_url(uri_string) + + return unless uri + return uri_string unless uri.fragment + + stripped_params = CGI.parse(uri.fragment) + if stripped_params['access_token'] + stripped_params['access_token'] = 'filtered' + filtered_query = Addressable::URI.new + filtered_query.query_values = stripped_params + + uri.fragment = filtered_query.query + end + + uri.to_s + end + # Invert a hash, collecting all keys that map to a given value in an array. # # Unlike `Hash#invert`, where the last encountered pair wins, and which has the diff --git a/lib/gitlab/utils/nokogiri.rb b/lib/gitlab/utils/nokogiri.rb new file mode 100644 index 00000000000..4b37bb7e5ea --- /dev/null +++ b/lib/gitlab/utils/nokogiri.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Gitlab + module Utils + class Nokogiri + class << self + # Use Nokogiri to convert a css selector into an xpath selector. + # Nokogiri can use css selectors with `doc.search()`. However + # for large node trees, it is _much_ slower than using xpath, + # by several orders of magnitude. + # https://gitlab.com/gitlab-org/gitlab/-/issues/329186 + def css_to_xpath(css) + xpath = ::Nokogiri::CSS.xpath_for(css) + + # Due to https://github.com/sparklemotion/nokogiri/issues/572, + # we remove the leading `//` and add `descendant-or-self::` + # in order to ensure we're searching from this node and all + # descendants. + xpath.map { |t| "descendant-or-self::#{t[2..-1]}" }.join('|') + end + end + end + end +end diff --git a/spec/features/users/login_spec.rb b/spec/features/users/login_spec.rb index e60d9d6ab69..1d7099ba443 100644 --- a/spec/features/users/login_spec.rb +++ b/spec/features/users/login_spec.rb @@ -394,6 +394,25 @@ RSpec.describe 'Login' do gitlab_sign_in(user) end + + context 'when the users password is expired' do + before do + user.update!(password_expires_at: Time.parse('2018-05-08 11:29:46 UTC')) + end + + it 'asks for a new password' do + expect(authentication_metrics) + .to increment(:user_authenticated_counter) + + visit new_user_session_path + + fill_in 'user_login', with: user.email + fill_in 'user_password', with: '12345678' + click_button 'Sign in' + + expect(current_path).to eq(new_profile_password_path) + end + end end context 'with invalid username and password' do diff --git a/spec/initializers/lograge_spec.rb b/spec/initializers/lograge_spec.rb index abb1673bb88..421f6373eff 100644 --- a/spec/initializers/lograge_spec.rb +++ b/spec/initializers/lograge_spec.rb @@ -173,6 +173,27 @@ RSpec.describe 'lograge', type: :request do end end + describe 'with access token in url' do + before do + event.payload[:location] = 'http://example.com/auth.html#access_token=secret_token&token_type=Bearer' + end + + it 'strips location from sensitive information' do + subscriber.redirect_to(event) + subscriber.process_action(event) + + expect(log_data['location']).not_to include('secret_token') + expect(log_data['location']).to include('filtered') + end + + it 'leaves non-sensitive information from location' do + subscriber.redirect_to(event) + subscriber.process_action(event) + + expect(log_data['location']).to include('&token_type=Bearer') + end + end + context 'with db payload' do context 'when RequestStore is enabled', :request_store do it 'includes db counters' do diff --git a/spec/lib/banzai/pipeline/post_process_pipeline_spec.rb b/spec/lib/banzai/pipeline/post_process_pipeline_spec.rb index ebe1ca4d403..55038d58f22 100644 --- a/spec/lib/banzai/pipeline/post_process_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/post_process_pipeline_spec.rb @@ -36,9 +36,11 @@ RSpec.describe Banzai::Pipeline::PostProcessPipeline do end let(:doc) { HTML::Pipeline.parse(html) } + let(:non_related_xpath_calls) { 2 } it 'searches for attributes only once' do - expect(doc).to receive(:search).once.and_call_original + expect(doc).to receive(:xpath).exactly(non_related_xpath_calls + 1).times + .and_call_original subject end @@ -49,7 +51,8 @@ RSpec.describe Banzai::Pipeline::PostProcessPipeline do end it 'searches for attributes twice' do - expect(doc).to receive(:search).twice.and_call_original + expect(doc).to receive(:xpath).exactly(non_related_xpath_calls + 2).times + .and_call_original subject end diff --git a/spec/lib/gitlab/auth/user_access_denied_reason_spec.rb b/spec/lib/gitlab/auth/user_access_denied_reason_spec.rb index d3c6cde5590..102d6fba97f 100644 --- a/spec/lib/gitlab/auth/user_access_denied_reason_spec.rb +++ b/spec/lib/gitlab/auth/user_access_denied_reason_spec.rb @@ -57,5 +57,13 @@ RSpec.describe Gitlab::Auth::UserAccessDeniedReason do it { is_expected.to eq('Your account is pending approval from your administrator and hence blocked.') } end + + context 'when the user has expired password' do + before do + user.update!(password_expires_at: 2.days.ago) + end + + it { is_expected.to eq('Your password expired. Please access GitLab from a web browser to update your password.') } + end end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index ae9c697e0b9..3d6c04fd484 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -433,6 +433,13 @@ RSpec.describe Gitlab::GitAccess do expect { pull_access_check }.to raise_forbidden("Your account has been deactivated by your administrator. Please log back in from a web browser to reactivate your account at #{Gitlab.config.gitlab.url}") end + it 'disallows users with expired password to pull' do + project.add_maintainer(user) + user.update!(password_expires_at: 2.minutes.ago) + + expect { pull_access_check }.to raise_forbidden("Your password expired. Please access GitLab from a web browser to update your password.") + end + context 'when the project repository does not exist' do before do project.add_guest(user) @@ -969,6 +976,13 @@ RSpec.describe Gitlab::GitAccess do expect { push_access_check }.to raise_forbidden("Your account has been deactivated by your administrator. Please log back in from a web browser to reactivate your account at #{Gitlab.config.gitlab.url}") end + it 'disallows users with expired password to push' do + project.add_maintainer(user) + user.update!(password_expires_at: 2.minutes.ago) + + expect { push_access_check }.to raise_forbidden("Your password expired. Please access GitLab from a web browser to update your password.") + end + it 'cleans up the files' do expect(project.repository).to receive(:clean_stale_repository_files).and_call_original expect { push_access_check }.not_to raise_error diff --git a/spec/lib/gitlab/utils/nokogiri_spec.rb b/spec/lib/gitlab/utils/nokogiri_spec.rb new file mode 100644 index 00000000000..90f137f53c8 --- /dev/null +++ b/spec/lib/gitlab/utils/nokogiri_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Utils::Nokogiri do + describe '#css_to_xpath' do + using RSpec::Parameterized::TableSyntax + + where(:css, :xpath) do + 'img' | "descendant-or-self::img" + 'a.gfm' | "descendant-or-self::a[contains(concat(' ',normalize-space(@class),' '),' gfm ')]" + 'a:not(.gfm)' | "descendant-or-self::a[not(contains(concat(' ',normalize-space(@class),' '),' gfm '))]" + 'video, audio' | "descendant-or-self::video|descendant-or-self::audio" + '[data-math-style]' | "descendant-or-self::*[@data-math-style]" + '[data-mermaid-style]' | "descendant-or-self::*[@data-mermaid-style]" + '.js-render-metrics' | "descendant-or-self::*[contains(concat(' ',normalize-space(@class),' '),' js-render-metrics ')]" + 'h1, h2, h3, h4, h5, h6' | "descendant-or-self::h1|descendant-or-self::h2|descendant-or-self::h3|descendant-or-self::h4|descendant-or-self::h5|descendant-or-self::h6" + 'pre.code.language-math' | "descendant-or-self::pre[contains(concat(' ',normalize-space(@class),' '),' code ') and contains(concat(' ',normalize-space(@class),' '),' language-math ')]" + 'pre > code[lang="plantuml"]' | "descendant-or-self::pre/code[@lang=\"plantuml\"]" + 'pre[lang="mermaid"] > code' | "descendant-or-self::pre[@lang=\"mermaid\"]/code" + 'pre.language-suggestion' | "descendant-or-self::pre[contains(concat(' ',normalize-space(@class),' '),' language-suggestion ')]" + 'pre.language-suggestion > code' | "descendant-or-self::pre[contains(concat(' ',normalize-space(@class),' '),' language-suggestion ')]/code" + 'a.gfm[data-reference-type="user"]' | "descendant-or-self::a[contains(concat(' ',normalize-space(@class),' '),' gfm ') and @data-reference-type=\"user\"]" + 'a:not(.gfm), img:not(.gfm), video:not(.gfm), audio:not(.gfm)' | "descendant-or-self::a[not(contains(concat(' ',normalize-space(@class),' '),' gfm '))]|descendant-or-self::img[not(contains(concat(' ',normalize-space(@class),' '),' gfm '))]|descendant-or-self::video[not(contains(concat(' ',normalize-space(@class),' '),' gfm '))]|descendant-or-self::audio[not(contains(concat(' ',normalize-space(@class),' '),' gfm '))]" + 'pre:not([data-math-style]):not([data-mermaid-style]):not([data-kroki-style]) > code' | "descendant-or-self::pre[not(@data-math-style) and not(@data-mermaid-style) and not(@data-kroki-style)]/code" + end + + with_them do + it 'generates the xpath' do + expect(described_class.css_to_xpath(css)).to eq xpath + end + end + end +end diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index 11dba610faf..a7ccce0aaab 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/spec/lib/gitlab/utils_spec.rb @@ -417,6 +417,29 @@ RSpec.describe Gitlab::Utils do end end + describe '.removes_sensitive_data_from_url' do + it 'returns string object' do + expect(described_class.removes_sensitive_data_from_url('http://gitlab.com')).to be_instance_of(String) + end + + it 'returns nil when URI cannot be parsed' do + expect(described_class.removes_sensitive_data_from_url('://gitlab.com')).to be nil + end + + it 'returns nil with invalid parameter' do + expect(described_class.removes_sensitive_data_from_url(1)).to be nil + end + + it 'returns string with filtered access_token param' do + expect(described_class.removes_sensitive_data_from_url('http://gitlab.com/auth.html#access_token=secret_token')).to eq('http://gitlab.com/auth.html#access_token=filtered') + end + + it 'returns string with filtered access_token param but other params preserved' do + expect(described_class.removes_sensitive_data_from_url('http://gitlab.com/auth.html#access_token=secret_token&token_type=Bearer&state=test')) + .to include('&token_type=Bearer', '&state=test') + end + end + describe 'multiple_key_invert' do it 'invert keys with array values' do hash = { diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb index e677f5558fd..bbbc5d08c07 100644 --- a/spec/policies/global_policy_spec.rb +++ b/spec/policies/global_policy_spec.rb @@ -239,6 +239,14 @@ RSpec.describe GlobalPolicy do it { is_expected.not_to be_allowed(:access_api) } end + context 'user with expired password' do + before do + current_user.update!(password_expires_at: 2.minutes.ago) + end + + it { is_expected.not_to be_allowed(:access_api) } + end + context 'when terms are enforced' do before do enforce_terms @@ -418,6 +426,14 @@ RSpec.describe GlobalPolicy do it { is_expected.not_to be_allowed(:access_git) } end + + context 'user with expired password' do + before do + current_user.update!(password_expires_at: 2.minutes.ago) + end + + it { is_expected.not_to be_allowed(:access_git) } + end end describe 'read instance metadata' do @@ -494,6 +510,14 @@ RSpec.describe GlobalPolicy do it { is_expected.not_to be_allowed(:use_slash_commands) } end + + context 'user with expired password' do + before do + current_user.update!(password_expires_at: 2.minutes.ago) + end + + it { is_expected.not_to be_allowed(:use_slash_commands) } + end end describe 'create_snippet' do diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index a1e28c18769..279c65fc2f4 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -35,6 +35,26 @@ RSpec.describe 'Git HTTP requests' do expect(response.header['WWW-Authenticate']).to start_with('Basic ') end end + + context "when password is expired" do + it "responds to downloads with status 401 Unauthorized" do + user.update!(password_expires_at: 2.days.ago) + + download(path, user: user.username, password: user.password) do |response| + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end + + context "when user is blocked" do + let(:user) { create(:user, :blocked) } + + it "responds to downloads with status 401 Unauthorized" do + download(path, user: user.username, password: user.password) do |response| + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end end context "when authentication succeeds" do @@ -75,6 +95,15 @@ RSpec.describe 'Git HTTP requests' do expect(response.header['WWW-Authenticate']).to start_with('Basic ') end end + + context "when password is expired" do + it "responds to uploads with status 401 Unauthorized" do + user.update!(password_expires_at: 2.days.ago) + upload(path, user: user.username, password: user.password) do |response| + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end end context "when authentication succeeds" do @@ -576,6 +605,16 @@ RSpec.describe 'Git HTTP requests' do it_behaves_like 'pulls are allowed' it_behaves_like 'pushes are allowed' + + context "when password is expired" do + it "responds to downloads with status 401 unauthorized" do + user.update!(password_expires_at: 2.days.ago) + + download(path, **env) do |response| + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end end context 'when user has 2FA enabled' do @@ -649,6 +688,18 @@ RSpec.describe 'Git HTTP requests' do expect(response).to have_gitlab_http_status(:ok) end end + + context "when password is expired" do + it "responds to uploads with status 401 unauthorized" do + user.update!(password_expires_at: 2.days.ago) + + write_access_token = create(:personal_access_token, user: user, scopes: [:write_repository]) + + upload(path, user: user.username, password: write_access_token.token) do |response| + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end end end @@ -860,6 +911,16 @@ RSpec.describe 'Git HTTP requests' do expect(response).to have_gitlab_http_status(:not_found) end + + context 'when users password is expired' do + it 'rejects pulls with 401 unauthorized' do + user.update!(password_expires_at: 2.days.ago) + + download(path, user: 'gitlab-ci-token', password: build.token) do |response| + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end end end end diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 4e18c9cb4ca..0e3a0252638 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -346,9 +346,7 @@ RSpec.describe 'Git LFS API and storage' do let_it_be(:user) { create(:user, password_expires_at: 1.minute.ago)} let(:role) { :reporter} - # TODO: This should return a 404 response - # https://gitlab.com/gitlab-org/gitlab/-/issues/292006 - it_behaves_like 'LFS http 200 response' + it_behaves_like 'LFS http 401 response' end context 'when user is blocked' do -- cgit v1.2.3 From 4530f5d0bdc9b2f60eed2146eaf1b6f35fc53b0e Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 31 May 2021 11:43:43 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee --- app/controllers/oauth/authorizations_controller.rb | 3 +- .../doorkeeper/authorizations/redirect.html.haml | 7 ++ locale/gitlab.pot | 3 + .../oauth/authorizations_controller_spec.rb | 77 +++++++++------------- 4 files changed, 42 insertions(+), 48 deletions(-) create mode 100644 app/views/doorkeeper/authorizations/redirect.html.haml diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 857f36e3833..ddf70c1892a 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -14,8 +14,9 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController if pre_auth.authorizable? if skip_authorization? || matching_token? auth = authorization.authorize + parsed_redirect_uri = URI.parse(auth.redirect_uri) session.delete(:user_return_to) - redirect_to auth.redirect_uri + render "doorkeeper/authorizations/redirect", locals: { redirect_uri: parsed_redirect_uri }, layout: false else render "doorkeeper/authorizations/new" end diff --git a/app/views/doorkeeper/authorizations/redirect.html.haml b/app/views/doorkeeper/authorizations/redirect.html.haml new file mode 100644 index 00000000000..2fefbac3802 --- /dev/null +++ b/app/views/doorkeeper/authorizations/redirect.html.haml @@ -0,0 +1,7 @@ +%h3.page-title= _("Redirecting") + +%div + %a{ :href => redirect_uri } Click here to redirect to #{redirect_uri} + +:javascript + window.location= "#{redirect_uri}"; diff --git a/locale/gitlab.pot b/locale/gitlab.pot index af1d2ea1221..49a36e7d5e7 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -26967,6 +26967,9 @@ msgstr "" msgid "Redirect to SAML provider to test configuration" msgstr "" +msgid "Redirecting" +msgstr "" + msgid "Redis" msgstr "" diff --git a/spec/controllers/oauth/authorizations_controller_spec.rb b/spec/controllers/oauth/authorizations_controller_spec.rb index 5fc5cdfc9b9..0e25f6a96d7 100644 --- a/spec/controllers/oauth/authorizations_controller_spec.rb +++ b/spec/controllers/oauth/authorizations_controller_spec.rb @@ -70,76 +70,59 @@ RSpec.describe Oauth::AuthorizationsController do describe 'GET #new' do subject { get :new, params: params } - include_examples 'OAuth Authorizations require confirmed user' include_examples "Implicit grant can't be used in confidential application" - context 'rendering of views based on the ownership of the application' do - shared_examples 'render views' do - render_views - - it 'returns 200 and renders view with correct info', :aggregate_failures do - subject + context 'when the user is confirmed' do + let(:confirmed_at) { 1.hour.ago } - expect(response).to have_gitlab_http_status(:ok) - expect(response.body).to include(application.owner.name) - expect(response).to render_template('doorkeeper/authorizations/new') - end - end + context 'when there is already an access token for the application with a matching scope' do + before do + scopes = Doorkeeper::OAuth::Scopes.from_string('api') - subject { get :new, params: params } + allow(Doorkeeper.configuration).to receive(:scopes).and_return(scopes) - context 'when auth app owner is a user' do - context 'with valid params' do - it_behaves_like 'render views' + create(:oauth_access_token, application: application, resource_owner_id: user.id, scopes: scopes) end - end - - context 'when auth app owner is a group' do - let(:group) { create(:group) } - context 'when auth app owner is a root group' do - let(:application) { create(:oauth_application, owner_id: group.id, owner_type: 'Namespace') } + it 'authorizes the request and shows the user a page that redirects' do + subject - it_behaves_like 'render views' + expect(request.session['user_return_to']).to be_nil + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template('doorkeeper/authorizations/redirect') end + end - context 'when auth app owner is a subgroup' do - let(:subgroup) { create(:group, parent: group) } - let(:application) { create(:oauth_application, owner_id: subgroup.id, owner_type: 'Namespace') } + context 'without valid params' do + it 'returns 200 code and renders error view' do + get :new - it_behaves_like 'render views' + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template('doorkeeper/authorizations/error') end end - context 'when there is no owner associated' do - let(:application) { create(:oauth_application, owner_id: nil, owner_type: nil) } + context 'with valid params' do + render_views - it 'renders view' do + it 'returns 200 code and renders view' do subject expect(response).to have_gitlab_http_status(:ok) expect(response).to render_template('doorkeeper/authorizations/new') end - end - end - context 'without valid params' do - it 'returns 200 code and renders error view' do - get :new + it 'deletes session.user_return_to and redirects when skip authorization' do + application.update!(trusted: true) + request.session['user_return_to'] = 'http://example.com' - expect(response).to have_gitlab_http_status(:ok) - expect(response).to render_template('doorkeeper/authorizations/error') - end - end - - it 'deletes session.user_return_to and redirects when skip authorization' do - application.update!(trusted: true) - request.session['user_return_to'] = 'http://example.com' - - subject + subject - expect(request.session['user_return_to']).to be_nil - expect(response).to have_gitlab_http_status(:found) + expect(request.session['user_return_to']).to be_nil + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template('doorkeeper/authorizations/redirect') + end + end end end -- cgit v1.2.3 From 185d6a2578f64ffafd80bea5314915811a54486a Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 31 May 2021 11:45:01 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee --- lib/api/lint.rb | 2 +- lib/gitlab/current_settings.rb | 4 ++++ spec/lib/gitlab/current_settings_spec.rb | 36 ++++++++++++++++++++++++++++++++ spec/requests/api/lint_spec.rb | 28 ++++++++++++++++++++++++- 4 files changed, 68 insertions(+), 2 deletions(-) diff --git a/lib/api/lint.rb b/lib/api/lint.rb index e0806674c6a..945cdf3edb2 100644 --- a/lib/api/lint.rb +++ b/lib/api/lint.rb @@ -11,7 +11,7 @@ module API optional :include_merged_yaml, type: Boolean, desc: 'Whether or not to include merged CI config yaml in the response' end post '/lint' do - unauthorized! if Gitlab::CurrentSettings.signup_disabled? && current_user.nil? + unauthorized! if (Gitlab::CurrentSettings.signup_disabled? || Gitlab::CurrentSettings.signup_limited?) && current_user.nil? result = Gitlab::Ci::YamlProcessor.new(params[:content], user: current_user).execute diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 7f55734f796..e7ffeeb9849 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -7,6 +7,10 @@ module Gitlab !signup_enabled? end + def signup_limited? + domain_allowlist.present? || email_restrictions_enabled? || require_admin_approval_after_user_signup? + end + def current_application_settings Gitlab::SafeRequestStore.fetch(:current_application_settings) { ensure_application_settings! } end diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index f5cb1987c5c..a5ab1047a40 100644 --- a/spec/lib/gitlab/current_settings_spec.rb +++ b/spec/lib/gitlab/current_settings_spec.rb @@ -24,6 +24,42 @@ RSpec.describe Gitlab::CurrentSettings do end end + describe '.signup_limited?' do + subject { described_class.signup_limited? } + + context 'when there are allowed domains' do + before do + create(:application_setting, domain_allowlist: ['www.gitlab.com']) + end + + it { is_expected.to be_truthy } + end + + context 'when there are email restrictions' do + before do + create(:application_setting, email_restrictions_enabled: true) + end + + it { is_expected.to be_truthy } + end + + context 'when the admin has to approve signups' do + before do + create(:application_setting, require_admin_approval_after_user_signup: true) + end + + it { is_expected.to be_truthy } + end + + context 'when there are no restrictions' do + before do + create(:application_setting, domain_allowlist: [], email_restrictions_enabled: false, require_admin_approval_after_user_signup: false) + end + + it { is_expected.to be_falsey } + end + end + describe '.signup_disabled?' do subject { described_class.signup_disabled? } diff --git a/spec/requests/api/lint_spec.rb b/spec/requests/api/lint_spec.rb index f26236e0253..57aa0f36192 100644 --- a/spec/requests/api/lint_spec.rb +++ b/spec/requests/api/lint_spec.rb @@ -27,9 +27,10 @@ RSpec.describe API::Lint do end end - context 'when signup settings are enabled' do + context 'when signup is enabled and not limited' do before do Gitlab::CurrentSettings.signup_enabled = true + stub_application_setting(domain_allowlist: [], email_restrictions_enabled: false, require_admin_approval_after_user_signup: false) end context 'when unauthenticated' do @@ -50,6 +51,31 @@ RSpec.describe API::Lint do end end + context 'when limited signup is enabled' do + before do + stub_application_setting(domain_allowlist: ['www.gitlab.com']) + Gitlab::CurrentSettings.signup_enabled = true + end + + context 'when unauthenticated' do + it 'returns unauthorized' do + post api('/ci/lint'), params: { content: 'content' } + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context 'when authenticated' do + let_it_be(:api_user) { create(:user) } + + it 'returns authentication success' do + post api('/ci/lint', api_user), params: { content: 'content' } + + expect(response).to have_gitlab_http_status(:ok) + end + end + end + context 'when authenticated' do let_it_be(:api_user) { create(:user) } -- cgit v1.2.3 From d63e7805c1a51d25d6461395a9e1471856bfa45b Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 31 May 2021 13:44:00 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index f6d6e07d3a3..3bf70170a73 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -146,7 +146,7 @@ GEM coderay (>= 1.0.0) erubi (>= 1.0.0) rack (>= 0.9.0) - bindata (2.4.8) + bindata (2.4.10) binding_ninja (0.2.3) bootsnap (1.4.6) msgpack (~> 1.0) -- cgit v1.2.3 From b0aabe1553cae0df7282d3f1e309c09b4210f930 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 31 May 2021 23:55:21 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee --- app/views/doorkeeper/authorizations/redirect.html.haml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/views/doorkeeper/authorizations/redirect.html.haml b/app/views/doorkeeper/authorizations/redirect.html.haml index 2fefbac3802..9580f33c88a 100644 --- a/app/views/doorkeeper/authorizations/redirect.html.haml +++ b/app/views/doorkeeper/authorizations/redirect.html.haml @@ -3,5 +3,6 @@ %div %a{ :href => redirect_uri } Click here to redirect to #{redirect_uri} -:javascript - window.location= "#{redirect_uri}"; += javascript_tag do + :plain + window.location= "#{redirect_uri}"; -- cgit v1.2.3 From 1b8cdf54da52b8243e2d143ed49c63a823614a7c Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Tue, 1 Jun 2021 07:46:06 +0000 Subject: Update VERSION files [ci skip] --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 1e66d67b1b7..d786287fbde 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -13.12.1 \ No newline at end of file +13.12.2 \ No newline at end of file -- cgit v1.2.3 From d98457affdf684f129fd52388ac9e290e4d2924f Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 1 Jun 2021 07:50:11 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee --- GITALY_SERVER_VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 1e66d67b1b7..d786287fbde 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -13.12.1 \ No newline at end of file +13.12.2 \ No newline at end of file -- cgit v1.2.3 From a8bae339ea3b2080f95de2d0c0805f5507763479 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 1 Jun 2021 12:24:01 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee --- CHANGELOG.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 669dec87484..69e1cb880ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,22 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 13.12.2 (2021-06-01) + +### Security (11 changes) + +- [Bump BinData version](gitlab-org/security/gitlab@609303ac4904cad6bbcc776bb809a46a66891d53) ([merge request](gitlab-org/security/gitlab!1414)) +- [Block access to GitLab for users with expired password](gitlab-org/security/gitlab@f0a07ce8a596a4dd6f1311dff893c896b0cdf82f) ([merge request](gitlab-org/security/gitlab!1446)) +- [Adds redirect page to OAuth](gitlab-org/security/gitlab@6ed6dfc8f9ba785fd5337ee0f4701c983b6f07b0) ([merge request](gitlab-org/security/gitlab!1441)) +- [Update users two factor required from group](gitlab-org/security/gitlab@8c3fe378289d2dced2139c9db396b6270d3bc0ab) ([merge request](gitlab-org/security/gitlab!1432)) +- [Updates authorization for lint](gitlab-org/security/gitlab@be33caed9684af07ac715038d7a2865d9d0c7247) ([merge request](gitlab-org/security/gitlab!1429)) +- [Opt in to Atlassians new context qsh](gitlab-org/security/gitlab@f1d06250fbef6fa2af8a8c88d3b3f9391c332089) ([merge request](gitlab-org/security/gitlab!1408)) +- [Limit oncall projects shown to scope of source](gitlab-org/security/gitlab@a70859aaac44c9b3bd3cc673737e01e2a3aba99c) ([merge request](gitlab-org/security/gitlab!1410)) **GitLab Enterprise Edition** +- [Only verify commit signatures if the user email is verified](gitlab-org/security/gitlab@9039fdffdf109cdf667be8db3d792a502aad8bb9) ([merge request](gitlab-org/security/gitlab!1385)) +- [Prevent XSS on notebooks](gitlab-org/security/gitlab@9a2dc30920c2a271257ccec92aebcfabec276096) ([merge request](gitlab-org/security/gitlab!1421)) +- [Truncate all non-blob markdown to 1MB by default](gitlab-org/security/gitlab@e9e6bc0450639ee25fd0ced983da231700a4d4f9) ([merge request](gitlab-org/security/gitlab!1420)) +- [Use xpath search of Nokogiri instead of css search](gitlab-org/security/gitlab@7e5c79021ab54ffc70d22bba3c663ce38ae83a88) ([merge request](gitlab-org/security/gitlab!1416)) + ## 13.12.1 (2021-05-25) ### Fixed (3 changes) -- cgit v1.2.3