From 38dadcee569adfbbb1c9dc99634bba4e9a9128bc Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 29 Mar 2023 23:49:08 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-9-stable-ee --- .../merge_requests/push_options_handler_service.rb | 10 ++- ..._nullify_last_error_from_project_mirror_data.rb | 26 +++++++ db/schema_migrations/20230208131808 | 1 + .../nullify_last_error_from_project_mirror_data.rb | 17 ++++ lib/gitlab/url_sanitizer.rb | 90 ++++++++++++++-------- ...ify_last_error_from_project_mirror_data_spec.rb | 84 ++++++++++++++++++++ spec/lib/gitlab/url_sanitizer_spec.rb | 31 +++++--- ...ify_last_error_from_project_mirror_data_spec.rb | 37 +++++++++ .../push_options_handler_service_spec.rb | 15 ++++ 9 files changed, 267 insertions(+), 44 deletions(-) create mode 100644 db/post_migrate/20230208131808_nullify_last_error_from_project_mirror_data.rb create mode 100644 db/schema_migrations/20230208131808 create mode 100644 lib/gitlab/background_migration/nullify_last_error_from_project_mirror_data.rb create mode 100644 spec/lib/gitlab/background_migration/nullify_last_error_from_project_mirror_data_spec.rb create mode 100644 spec/migrations/nullify_last_error_from_project_mirror_data_spec.rb diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb index 235dc6678df..e9abafceb13 100644 --- a/app/services/merge_requests/push_options_handler_service.rb +++ b/app/services/merge_requests/push_options_handler_service.rb @@ -54,7 +54,15 @@ module MergeRequests end def validate_service - errors << 'User is required' if current_user.nil? + if current_user.nil? + errors << 'User is required' + return + end + + unless current_user&.can?(:read_code, target_project) + errors << 'User access was denied' + return + end unless target_project.merge_requests_enabled? errors << "Merge requests are not enabled for project #{target_project.full_path}" diff --git a/db/post_migrate/20230208131808_nullify_last_error_from_project_mirror_data.rb b/db/post_migrate/20230208131808_nullify_last_error_from_project_mirror_data.rb new file mode 100644 index 00000000000..73e6f257498 --- /dev/null +++ b/db/post_migrate/20230208131808_nullify_last_error_from_project_mirror_data.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class NullifyLastErrorFromProjectMirrorData < Gitlab::Database::Migration[2.1] + MIGRATION = 'NullifyLastErrorFromProjectMirrorData' + INTERVAL = 2.minutes + BATCH_SIZE = 10_000 + SUB_BATCH_SIZE = 1_000 + + disable_ddl_transaction! + restrict_gitlab_migration gitlab_schema: :gitlab_main + + def up + queue_batched_background_migration( + MIGRATION, + :project_mirror_data, + :id, + job_interval: INTERVAL, + batch_size: BATCH_SIZE, + sub_batch_size: SUB_BATCH_SIZE + ) + end + + def down + delete_batched_background_migration(MIGRATION, :project_mirror_data, :id, []) + end +end diff --git a/db/schema_migrations/20230208131808 b/db/schema_migrations/20230208131808 new file mode 100644 index 00000000000..24c5b21f6ad --- /dev/null +++ b/db/schema_migrations/20230208131808 @@ -0,0 +1 @@ +784f8f189eee7b5cf3136f0a859874a1d170d2b148f4c260f968b144816f1322 \ No newline at end of file diff --git a/lib/gitlab/background_migration/nullify_last_error_from_project_mirror_data.rb b/lib/gitlab/background_migration/nullify_last_error_from_project_mirror_data.rb new file mode 100644 index 00000000000..6ea5c17353b --- /dev/null +++ b/lib/gitlab/background_migration/nullify_last_error_from_project_mirror_data.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # Nullifies last_error value from project_mirror_data table as they + # potentially included sensitive data. + # https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/3041 + class NullifyLastErrorFromProjectMirrorData < BatchedMigrationJob + feature_category :source_code_management + operation_name :update_all + + def perform + each_sub_batch { |rel| rel.update_all(last_error: nil) } + end + end + end +end diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb index e3bf11b00b4..79e124a58f5 100644 --- a/lib/gitlab/url_sanitizer.rb +++ b/lib/gitlab/url_sanitizer.rb @@ -2,15 +2,37 @@ module Gitlab class UrlSanitizer + include Gitlab::Utils::StrongMemoize + ALLOWED_SCHEMES = %w[http https ssh git].freeze ALLOWED_WEB_SCHEMES = %w[http https].freeze + SCHEMIFIED_SCHEME = 'glschemelessuri' + SCHEMIFY_PLACEHOLDER = "#{SCHEMIFIED_SCHEME}://".freeze + # URI::DEFAULT_PARSER.make_regexp will only match URLs with schemes or + # relative URLs. This section will match schemeless URIs with userinfo + # e.g. user:pass@gitlab.com but will not match scp-style URIs e.g. + # user@server:path/to/file) + # + # The userinfo part is very loose compared to URI's implementation so we + # also match non-escaped userinfo e.g foo:b?r@gitlab.com which should be + # encoded as foo:b%3Fr@gitlab.com + URI_REGEXP = %r{ + (?: + #{URI::DEFAULT_PARSER.make_regexp(ALLOWED_SCHEMES)} + | + (?:(?:(?!@)[%#{URI::REGEXP::PATTERN::UNRESERVED}#{URI::REGEXP::PATTERN::RESERVED}])+(?:@)) + (?# negative lookahead ensures this isn't an SCP-style URL: [host]:[rel_path|abs_path] server:path/to/file) + (?!#{URI::REGEXP::PATTERN::HOST}:(?:#{URI::REGEXP::PATTERN::REL_PATH}|#{URI::REGEXP::PATTERN::ABS_PATH})) + #{URI::REGEXP::PATTERN::HOSTPORT} + ) + }x def self.sanitize(content) - regexp = URI::DEFAULT_PARSER.make_regexp(ALLOWED_SCHEMES) - - content.gsub(regexp) { |url| new(url).masked_url } - rescue Addressable::URI::InvalidURIError - content.gsub(regexp, '') + content.gsub(URI_REGEXP) do |url| + new(url).masked_url + rescue Addressable::URI::InvalidURIError + '' + end end def self.valid?(url, allowed_schemes: ALLOWED_SCHEMES) @@ -37,34 +59,45 @@ module Gitlab @url = parse_url(url) end + def credentials + @credentials ||= { user: @url.user.presence, password: @url.password.presence } + end + + def user + credentials[:user] + end + def sanitized_url - @sanitized_url ||= safe_url.to_s + safe_url = @url.dup + safe_url.password = nil + safe_url.user = nil + reverse_schemify(safe_url.to_s) end + strong_memoize_attr :sanitized_url def masked_url url = @url.dup url.password = "*****" if url.password.present? url.user = "*****" if url.user.present? - url.to_s - end - - def credentials - @credentials ||= { user: @url.user.presence, password: @url.password.presence } - end - - def user - credentials[:user] + reverse_schemify(url.to_s) end + strong_memoize_attr :masked_url def full_url - @full_url ||= generate_full_url.to_s + return reverse_schemify(@url.to_s) unless valid_credentials? + + url = @url.dup + url.password = encode_percent(credentials[:password]) if credentials[:password].present? + url.user = encode_percent(credentials[:user]) if credentials[:user].present? + reverse_schemify(url.to_s) end + strong_memoize_attr :full_url private def parse_url(url) - url = url.to_s.strip - match = url.match(%r{\A(?:git|ssh|http(?:s?))\://(?:(.+)(?:@))?(.+)}) + url = schemify(url.to_s.strip) + match = url.match(%r{\A(?:(?:#{SCHEMIFIED_SCHEME}|git|ssh|http(?:s?)):)?//(?:(.+)(?:@))?(.+)}o) raw_credentials = match[1] if match if raw_credentials.present? @@ -83,24 +116,19 @@ module Gitlab url end - def generate_full_url - return @url unless valid_credentials? - - @url.dup.tap do |generated| - generated.password = encode_percent(credentials[:password]) if credentials[:password].present? - generated.user = encode_percent(credentials[:user]) if credentials[:user].present? - end + def schemify(url) + # Prepend the placeholder scheme unless the URL has a scheme or is relative + url.prepend(SCHEMIFY_PLACEHOLDER) unless url.starts_with?(%r{(?:#{URI::REGEXP::PATTERN::SCHEME}:)?//}o) + url end - def safe_url - safe_url = @url.dup - safe_url.password = nil - safe_url.user = nil - safe_url + def reverse_schemify(url) + url.slice!(SCHEMIFY_PLACEHOLDER) if url.starts_with?(SCHEMIFY_PLACEHOLDER) + url end def valid_credentials? - credentials && credentials.is_a?(Hash) && credentials.any? + credentials.is_a?(Hash) && credentials.values.any? end def encode_percent(string) diff --git a/spec/lib/gitlab/background_migration/nullify_last_error_from_project_mirror_data_spec.rb b/spec/lib/gitlab/background_migration/nullify_last_error_from_project_mirror_data_spec.rb new file mode 100644 index 00000000000..62f908ed79b --- /dev/null +++ b/spec/lib/gitlab/background_migration/nullify_last_error_from_project_mirror_data_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BackgroundMigration::NullifyLastErrorFromProjectMirrorData, feature_category: :source_code_management do # rubocop:disable Layout/LineLength + it 'nullifies last_error column on all rows' do + namespaces = table(:namespaces) + projects = table(:projects) + project_import_states = table(:project_mirror_data) + + group = namespaces.create!(name: 'gitlab', path: 'gitlab-org') + + project_namespace_1 = namespaces.create!(name: 'gitlab', path: 'gitlab-org') + project_namespace_2 = namespaces.create!(name: 'gitlab', path: 'gitlab-org') + project_namespace_3 = namespaces.create!(name: 'gitlab', path: 'gitlab-org') + + project_1 = projects.create!( + namespace_id: group.id, + project_namespace_id: project_namespace_1.id, + name: 'test1' + ) + project_2 = projects.create!( + namespace_id: group.id, + project_namespace_id: project_namespace_2.id, + name: 'test2' + ) + project_3 = projects.create!( + namespace_id: group.id, + project_namespace_id: project_namespace_3.id, + name: 'test3' + ) + + project_import_state_1 = project_import_states.create!( + project_id: project_1.id, + status: 0, + last_update_started_at: 1.hour.ago, + last_update_scheduled_at: 1.hour.ago, + last_update_at: 1.hour.ago, + last_successful_update_at: 2.days.ago, + last_error: '13:fetch remote: "fatal: unable to look up user:pass@gitlab.com (port 9418) (nodename nor servname provided, or not known)\n": exit status 128.', # rubocop:disable Layout/LineLength + correlation_id_value: SecureRandom.uuid, + jid: SecureRandom.uuid + ) + + project_import_states.create!( + project_id: project_2.id, + status: 1, + last_update_started_at: 1.hour.ago, + last_update_scheduled_at: 1.hour.ago, + last_update_at: 1.hour.ago, + last_successful_update_at: nil, + next_execution_timestamp: 1.day.from_now, + last_error: '', + correlation_id_value: SecureRandom.uuid, + jid: SecureRandom.uuid + ) + + project_import_state_3 = project_import_states.create!( + project_id: project_3.id, + status: 2, + last_update_started_at: 1.hour.ago, + last_update_scheduled_at: 1.hour.ago, + last_update_at: 1.hour.ago, + last_successful_update_at: 1.hour.ago, + next_execution_timestamp: 1.day.from_now, + last_error: nil, + correlation_id_value: SecureRandom.uuid, + jid: SecureRandom.uuid + ) + + migration = described_class.new( + start_id: project_import_state_1.id, + end_id: project_import_state_3.id, + batch_table: :project_mirror_data, + batch_column: :id, + sub_batch_size: 1, + pause_ms: 0, + connection: ApplicationRecord.connection + ) + + w_last_error_count = -> { project_import_states.where.not(last_error: nil).count } # rubocop:disable CodeReuse/ActiveRecord + expect { migration.perform }.to change(&w_last_error_count).from(2).to(0) + end +end diff --git a/spec/lib/gitlab/url_sanitizer_spec.rb b/spec/lib/gitlab/url_sanitizer_spec.rb index 0ffbf5f81e7..c02cbef8328 100644 --- a/spec/lib/gitlab/url_sanitizer_spec.rb +++ b/spec/lib/gitlab/url_sanitizer_spec.rb @@ -10,29 +10,36 @@ RSpec.describe Gitlab::UrlSanitizer do # We want to try with multi-line content because is how error messages are formatted described_class.sanitize(%Q{ remote: Not Found - fatal: repository '#{url}' not found + fatal: repository `#{url}` not found }) end where(:input, :output) do - 'http://user:pass@test.com/root/repoC.git/' | 'http://*****:*****@test.com/root/repoC.git/' - 'https://user:pass@test.com/root/repoA.git/' | 'https://*****:*****@test.com/root/repoA.git/' - 'ssh://user@host.test/path/to/repo.git' | 'ssh://*****@host.test/path/to/repo.git' - - # git protocol does not support authentication but clean any details anyway - 'git://user:pass@host.test/path/to/repo.git' | 'git://*****:*****@host.test/path/to/repo.git' - 'git://host.test/path/to/repo.git' | 'git://host.test/path/to/repo.git' + # http(s), ssh, git, relative, and schemeless URLs should all be masked correctly + urls = ['http://', 'https://', 'ssh://', 'git://', '//', ''].flat_map do |protocol| + [ + ["#{protocol}test.com", "#{protocol}test.com"], + ["#{protocol}test.com/", "#{protocol}test.com/"], + ["#{protocol}test.com/path/to/repo.git", "#{protocol}test.com/path/to/repo.git"], + ["#{protocol}user@test.com", "#{protocol}*****@test.com"], + ["#{protocol}user:pass@test.com", "#{protocol}*****:*****@test.com"], + ["#{protocol}user:@test.com", "#{protocol}*****@test.com"], + ["#{protocol}:pass@test.com", "#{protocol}:*****@test.com"] + ] + end # SCP-style URLs are left unmodified - 'user@server:project.git' | 'user@server:project.git' - 'user:pass@server:project.git' | 'user:pass@server:project.git' + urls << ['user@server:project.git', 'user@server:project.git'] + urls << ['user:@server:project.git', 'user:@server:project.git'] + urls << [':pass@server:project.git', ':pass@server:project.git'] + urls << ['user:pass@server:project.git', 'user:pass@server:project.git'] # return an empty string for invalid URLs - 'ssh://' | '' + urls << ['ssh://', ''] end with_them do - it { expect(sanitize_url(input)).to include("repository '#{output}' not found") } + it { expect(sanitize_url(input)).to include("repository `#{output}` not found") } end end diff --git a/spec/migrations/nullify_last_error_from_project_mirror_data_spec.rb b/spec/migrations/nullify_last_error_from_project_mirror_data_spec.rb new file mode 100644 index 00000000000..6c5679b674e --- /dev/null +++ b/spec/migrations/nullify_last_error_from_project_mirror_data_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require_migration! + +RSpec.describe NullifyLastErrorFromProjectMirrorData, feature_category: :source_code_management do + let(:migration) { described_class::MIGRATION } + + before do + migrate! + end + + describe '#up' do + it 'schedules background jobs for each batch of projects' do + expect(migration).to( + have_scheduled_batched_migration( + table_name: :project_mirror_data, + column_name: :id, + interval: described_class::INTERVAL, + batch_size: described_class::BATCH_SIZE, + sub_batch_size: described_class::SUB_BATCH_SIZE + ) + ) + end + end + + describe '#down' do + before do + schema_migrate_down! + end + + it 'deletes all batched migration records' do + expect(migration).not_to have_scheduled_batched_migration + end + end +end diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index 251bf6f0d9d..03f3d56cdd2 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -861,6 +861,21 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end end + describe 'when user does not have access to target project' do + let(:push_options) { { create: true, target: 'my-branch' } } + let(:changes) { default_branch_changes } + + before do + allow(user1).to receive(:can?).with(:read_code, project).and_return(false) + end + + it 'records an error', :sidekiq_inline do + service.execute + + expect(service.errors).to eq(["User access was denied"]) + end + end + describe 'when MRs are not enabled' do let(:project) { create(:project, :public, :repository).tap { |pr| pr.add_developer(user1) } } let(:push_options) { { create: true } } -- cgit v1.2.1