diff --git a/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb b/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb index 5810a8c4109a497fed3fba598392bffe2d544b5a..7c78f4423fdb10b35a27babc0ab8a1ee259c54fc 100644 --- a/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb +++ b/ee/spec/lib/ee/gitlab/scim/group/provisioning_service_spec.rb @@ -161,6 +161,17 @@ def user expect(user.username).to eq('ricky.the.raccoon') end + context 'and there is an existing user with the sanitized username' do + before do + create(:user, :with_namespace, username: 'ricky.the.raccoon') + end + + it 'creates new user with non-conflicting username' do + expect { result }.to change { User.count }.by(1) + expect(user.username).to eq('ricky.the.raccoon1') + end + end + context 'and extra_slug_sanitization FF is disabled' do before do stub_feature_flags(extra_slug_path_sanitization: false) diff --git a/ee/spec/lib/ee/gitlab/scim/provisioning_service_spec.rb b/ee/spec/lib/ee/gitlab/scim/provisioning_service_spec.rb index a5ce4092fab51a2c419264a139b0d17c4d06e0fb..4b7b7ff41002d1b07844e8af7fcfa8b66b68fb66 100644 --- a/ee/spec/lib/ee/gitlab/scim/provisioning_service_spec.rb +++ b/ee/spec/lib/ee/gitlab/scim/provisioning_service_spec.rb @@ -110,6 +110,17 @@ def user expect(user.username).to eq('ricky.the.raccoon') end + context 'and there is an existing user with the sanitized username' do + before do + create(:user, :with_namespace, username: 'ricky.the.raccoon') + end + + it 'creates new user with non-conflicting username' do + expect { result }.to change { User.count }.by(1) + expect(user.username).to eq('ricky.the.raccoon1') + end + end + context 'and extra_slug_sanitization FF is disabled' do before do stub_feature_flags(extra_slug_path_sanitization: false) diff --git a/lib/gitlab/auth/external_username_sanitizer.rb b/lib/gitlab/auth/external_username_sanitizer.rb index d8b64783fc63e8b213aefce18a11192927e49bb6..5c0f1ee20a691162d53962a7f892f011e65d1346 100644 --- a/lib/gitlab/auth/external_username_sanitizer.rb +++ b/lib/gitlab/auth/external_username_sanitizer.rb @@ -11,14 +11,33 @@ def initialize(external_username) def sanitize # remove most characters illegal in usernames / slugs - valid_username = ::Namespace.clean_path(external_username) + slug = Gitlab::Slug::Path.new(external_username).generate + # remove leading - , _ , or . characters not removed by Namespace.clean_path - valid_username = valid_username.sub(/\A[_.-]+/, '') + slug = slug.sub(/\A[_.-]+/, '') + # remove trailing - , _ or . characters not removed by Namespace.clean_path - valid_username = valid_username.sub(/[_.-]+\z/, '') + # hard to write a regex to match end-of-string without ReDoS, so just use plain Ruby + slug = slug[0...-1] while slug.end_with?('.', '-', '_') + # remove consecutive - , _ , or . characters - valid_username = valid_username.gsub(/([_.-])[_.-]+/, '\1') - Gitlab::Utils::Uniquify.new.string(valid_username) do |s| + slug = slug.gsub(/([_.-])[_.-]+/, '\1') + + slug = unique_by_namespace(slug) + + validated_path(slug) + end + + # decomposed from Namespace.clean_path + def unique_by_namespace(slug) + path = Namespaces::RandomizedSuffixPath.new(slug).to_s + Gitlab::Utils::Uniquify.new.string(path) do |s| + Namespace.all.find_by_path_or_name(s) + end + end + + def validated_path(path) + Gitlab::Utils::Uniquify.new.string(path) do |s| !NamespacePathValidator.valid_path?(s) end end diff --git a/spec/lib/gitlab/auth/external_username_sanitizer_spec.rb b/spec/lib/gitlab/auth/external_username_sanitizer_spec.rb index 21837f84e902273cc2fb74440368d468bad9bfaa..3962742ed408ae77f91dc3e7ee66bc5c5a708854 100644 --- a/spec/lib/gitlab/auth/external_username_sanitizer_spec.rb +++ b/spec/lib/gitlab/auth/external_username_sanitizer_spec.rb @@ -26,4 +26,17 @@ it { is_expected.to eq(output) } end end + + context 'when external username is a ReDoS pattern' do + let(:external_username) { "#{'-' * 54773}\x00-z" } + + it { is_expected.to eq('z') } + end + + context 'with existing users' do + let!(:user_capybara) { create(:user, :with_namespace, username: 'carly_the_capybara') } + let(:external_username) { '___carly_the_capybara' } + + it { is_expected.to eq('carly_the_capybara1') } + end end diff --git a/spec/lib/gitlab/auth/o_auth/user_spec.rb b/spec/lib/gitlab/auth/o_auth/user_spec.rb index 423fbfaf5c9cfd98ff5a18980f8fd87840956550..d02c9d16373f2f51503be2d09894f57090c69740 100644 --- a/spec/lib/gitlab/auth/o_auth/user_spec.rb +++ b/spec/lib/gitlab/auth/o_auth/user_spec.rb @@ -908,6 +908,13 @@ def result_identities(dn, uid) expect(oauth_user2.gl_user.username).to eq('johngitlab-ETC1') end + + it 'generates the username with a counter for special characters' do + oauth_user.save # rubocop:disable Rails/SaveBang -- not an ActiveRecord model, no save! method + oauth_user2 = described_class.new(OmniAuth::AuthHash.new(uid: 'my-uid2', provider: provider, info: { nickname: 'johngitlab---ETC@othermail.com', email: 'john@othermail.com' })) + + expect(oauth_user2.gl_user.username).to eq('johngitlab-ETC1') + end end context 'when username is a reserved word' do