diff options
| -rw-r--r-- | app/models/user.rb | 27 | ||||
| -rw-r--r-- | changelogs/unreleased/bvl-email-disclosure.yml | 5 | ||||
| -rw-r--r-- | spec/features/groups/members/manage_members.rb | 21 | ||||
| -rw-r--r-- | spec/models/user_spec.rb | 35 |
4 files changed, 60 insertions, 28 deletions
diff --git a/app/models/user.rb b/app/models/user.rb index af1c36d9c93..7dc18c351e6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -315,6 +315,13 @@ class User < ActiveRecord::Base # # Returns an ActiveRecord::Relation. def search(query) +<<<<<<< HEAD +======= + table = arel_table + query = query.downcase + pattern = User.to_pattern(query) + +>>>>>>> f45fc58d84... Merge branch 'bvl-10-2-email-disclosure' into 'security-10-2' order = <<~SQL CASE WHEN users.name = %{query} THEN 0 @@ -324,8 +331,16 @@ class User < ActiveRecord::Base END SQL +<<<<<<< HEAD fuzzy_search(query, [:name, :email, :username]) .reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name) +======= + where( + table[:name].matches(pattern) + .or(table[:email].eq(query)) + .or(table[:username].matches(pattern)) + ).reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name) +>>>>>>> f45fc58d84... Merge branch 'bvl-10-2-email-disclosure' into 'security-10-2' end # searches user by given pattern @@ -334,6 +349,7 @@ class User < ActiveRecord::Base def search_with_secondary_emails(query) email_table = Email.arel_table +<<<<<<< HEAD matched_by_emails_user_ids = email_table .project(email_table[:user_id]) .where(Email.fuzzy_arel_match(:email, query)) @@ -343,6 +359,17 @@ class User < ActiveRecord::Base .or(fuzzy_arel_match(:email, query)) .or(fuzzy_arel_match(:username, query)) .or(arel_table[:id].in(matched_by_emails_user_ids)) +======= + query = query.downcase + pattern = User.to_pattern(query) + matched_by_emails_user_ids = email_table.project(email_table[:user_id]).where(email_table[:email].eq(query)) + + where( + table[:name].matches(pattern) + .or(table[:email].eq(query)) + .or(table[:username].matches(pattern)) + .or(table[:id].in(matched_by_emails_user_ids)) +>>>>>>> f45fc58d84... Merge branch 'bvl-10-2-email-disclosure' into 'security-10-2' ) end diff --git a/changelogs/unreleased/bvl-email-disclosure.yml b/changelogs/unreleased/bvl-email-disclosure.yml new file mode 100644 index 00000000000..d6cd8709d9f --- /dev/null +++ b/changelogs/unreleased/bvl-email-disclosure.yml @@ -0,0 +1,5 @@ +--- +title: Don't match partial email adresses +merge_request: 2227 +author: +type: security diff --git a/spec/features/groups/members/manage_members.rb b/spec/features/groups/members/manage_members.rb index da1e17225db..21f7b4999ad 100644 --- a/spec/features/groups/members/manage_members.rb +++ b/spec/features/groups/members/manage_members.rb @@ -38,6 +38,27 @@ feature 'Groups > Members > Manage members' do end end + scenario 'do not disclose email addresses', :js do + group.add_owner(user1) + create(:user, email: 'undisclosed_email@gitlab.com', name: "Jane 'invisible' Doe") + + visit group_group_members_path(group) + + find('.select2-container').click + select_input = find('.select2-input') + + select_input.send_keys('@gitlab.com') + wait_for_requests + + expect(page).to have_content('No matches found') + + select_input.native.clear + select_input.send_keys('undisclosed_email@gitlab.com') + wait_for_requests + + expect(page).to have_content("Jane 'invisible' Doe") + end + scenario 'remove user from group', :js do group.add_owner(user1) group.add_developer(user2) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index cdabd35b6ba..4687d9dfa00 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -913,11 +913,11 @@ describe User do describe 'email matching' do it 'returns users with a matching Email' do - expect(described_class.search(user.email)).to eq([user, user2]) + expect(described_class.search(user.email)).to eq([user]) end - it 'returns users with a partially matching Email' do - expect(described_class.search(user.email[0..2])).to eq([user, user2]) + it 'does not return users with a partially matching Email' do + expect(described_class.search(user.email[0..2])).not_to include(user, user2) end it 'returns users with a matching Email regardless of the casing' do @@ -973,8 +973,8 @@ describe User do expect(search_with_secondary_emails(user.email)).to eq([user]) end - it 'returns users with a partially matching email' do - expect(search_with_secondary_emails(user.email[0..2])).to eq([user]) + it 'does not return users with a partially matching email' do + expect(search_with_secondary_emails(user.email[0..2])).not_to include([user]) end it 'returns users with a matching email regardless of the casing' do @@ -997,29 +997,8 @@ describe User do expect(search_with_secondary_emails(email.email)).to eq([email.user]) end - it 'returns users with a matching part of secondary email' do - expect(search_with_secondary_emails(email.email[1..4])).to eq([email.user]) - end - - it 'return users with a matching part of secondary email regardless of case' do - expect(search_with_secondary_emails(email.email[1..4].upcase)).to eq([email.user]) - expect(search_with_secondary_emails(email.email[1..4].downcase)).to eq([email.user]) - expect(search_with_secondary_emails(email.email[1..4].capitalize)).to eq([email.user]) - end - - it 'returns multiple users with matching secondary emails' do - email1 = create(:email, email: '1_testemail@example.com') - email2 = create(:email, email: '2_testemail@example.com') - email3 = create(:email, email: 'other@email.com') - email3.user.update_attributes!(email: 'another@mail.com') - - expect( - search_with_secondary_emails('testemail@example.com').map(&:id) - ).to include(email1.user.id, email2.user.id) - - expect( - search_with_secondary_emails('testemail@example.com').map(&:id) - ).not_to include(email3.user.id) + it 'does not return users with a matching part of secondary email' do + expect(search_with_secondary_emails(email.email[1..4])).not_to include([email.user]) end end |