Merge branch 'bvl-10-2-email-disclosure' into 'security-10-2'

(10.2) Avoid partial partial email adresses for matching

See merge request gitlab/gitlabhq!2232

(cherry picked from commit 081aa1e91a777c9acb31be4a1e76b3dd7032fa9a)

fa85a3fd Don't allow searching for partial user emails

4 files changed, 40 insertions, 33 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index 3209138a772..316df26942d 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 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 3b5ee70ae7c..32b1ad19903 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -889,11 +889,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
@@ -949,8 +949,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
@@ -973,29 +973,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