diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-11-24 13:15:50 +0000 |
---|---|---|
committer | Winnie Hellmann <winnie@gitlab.com> | 2017-12-06 13:27:52 +0000 |
commit | 9f0d47cf2845fa57e85cbe724ef49cfb0782b997 (patch) | |
tree | 3bddc713dd1e22142c40a1e21b1f5a15937d033a | |
parent | c887c03aaa7079c56c6451a4c6037ca5f99c99fc (diff) | |
download | gitlab-ce-9f0d47cf2845fa57e85cbe724ef49cfb0782b997.tar.gz |
Merge branch 'bvl-email-disclosure' into 'security-10-1'
(10.1) Avoid partial partial email adresses for matching
See merge request gitlab/gitlabhq!2227
(cherry picked from commit 860fdd1a695ff657a447acec1b5e2119cff36cc2)
5df36f36 Don't allow searching for partial user emails
-rw-r--r-- | app/models/user.rb | 12 | ||||
-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, 40 insertions, 33 deletions
diff --git a/app/models/user.rb b/app/models/user.rb index 91dbe4c7023..f3fdb282dd7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -314,7 +314,8 @@ class User < ActiveRecord::Base # # Returns an ActiveRecord::Relation. def search(query) - table = arel_table + table = arel_table + query = query.downcase pattern = User.to_pattern(query) order = <<~SQL @@ -328,7 +329,7 @@ class User < ActiveRecord::Base where( table[:name].matches(pattern) - .or(table[:email].matches(pattern)) + .or(table[:email].eq(query)) .or(table[:username].matches(pattern)) ).reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name) end @@ -340,12 +341,13 @@ class User < ActiveRecord::Base def search_with_secondary_emails(query) table = arel_table email_table = Email.arel_table - pattern = "%#{query}%" - matched_by_emails_user_ids = email_table.project(email_table[:user_id]).where(email_table[:email].matches(pattern)) + 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].matches(pattern)) + .or(table[:email].eq(query)) .or(table[:username].matches(pattern)) .or(table[:id].in(matched_by_emails_user_ids)) ) 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 9039b283393..2d8a736884d 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 fa09d0a3ed4..f525679a164 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -879,11 +879,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 @@ -939,8 +939,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 @@ -963,29 +963,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 |