Change validation rules for email addresses

Currently there is a mismatch between the email addresses
that are accepted by the Devise email regular expression and
what is an actual valid email address according to RFC 3696 and
what will therefore be sent reliably by most SMTP servers.

This commit replaces the use of Devise's default regex with
the valid_email gem which is stricter.

Related Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/215919

Gemfile

@@ -495,3 +495,6 @@ gem 'mail', '= 2.7.1'
 
 # File encryption
 gem 'lockbox', '~> 0.3.3'
+
+# Email validation
+gem 'valid_email', '~> 0.1'

Gemfile.lock

@@ -842,7 +842,7 @@ GEM
       rake (>= 0.8.7)
       thor (>= 0.20.3, < 2.0)
     rainbow (3.0.0)
-    raindrops (0.19.0)
+    raindrops (0.19.1)
     rake (12.3.3)
     rb-fsevent (0.10.2)
     rb-inotify (0.9.10)
@@ -1109,6 +1109,9 @@ GEM
       equalizer (~> 0.0.9)
       parser (>= 2.6.5)
       procto (~> 0.0.2)
+    valid_email (0.1.3)
+      activemodel
+      mail (>= 2.6.1)
     validate_email (0.1.6)
       activemodel (>= 3.0)
       mail (>= 2.2.5)
@@ -1402,6 +1405,7 @@ DEPENDENCIES
   unicorn (~> 5.5)
   unicorn-worker-killer (~> 0.4.4)
   unleash (~> 0.1.5)
+  valid_email (~> 0.1)
   validates_hostname (~> 1.0.6)
   version_sorter (~> 2.2.4)
   vmstat (~> 2.3.0)

app/models/email.rb

@@ -6,7 +6,8 @@ class Email < ApplicationRecord
 
   belongs_to :user, optional: false
 
-  validates :email, presence: true, uniqueness: true, devise_email: true
+  validates :email, presence: true, uniqueness: true
+  validate :validate_email_format
   validate :unique_email, if: ->(email) { email.email_changed? }
 
   scope :confirmed, -> { where.not(confirmed_at: nil) }
@@ -30,6 +31,10 @@ class Email < ApplicationRecord
     user.accept_pending_invitations!
   end
 
+  def validate_email_format
+    self.errors.add(:email, I18n.t(:invalid, scope: 'valid_email.validations.email')) unless ValidateEmail.valid?(self.email)
+  end
+
   # once email is confirmed, update the gpg signatures
   def update_invalid_gpg_signatures
     user.update_invalid_gpg_signatures if confirmed?

changelogs/unreleased/215919-email-validation.yml

+---
+title: Change validation rules for profile email addresses
+merge_request: 30633
+author:
+type: fixed

spec/controllers/profiles/emails_controller_spec.rb

@@ -10,12 +10,20 @@ describe Profiles::EmailsController do
   end
 
   describe '#create' do
+    context 'when email address is valid' do
       let(:email_params) { { email: "add_email@example.com" } }
 
       it 'sends an email confirmation' do
         expect { post(:create, params: { email: email_params }) }.to change { ActionMailer::Base.deliveries.size }
-      expect(ActionMailer::Base.deliveries.last.to).to eq [email_params[:email]]
-      expect(ActionMailer::Base.deliveries.last.subject).to match "Confirmation instructions"
+      end
+    end
+
+    context 'when email address is invalid' do
+      let(:email_params) { { email: "test.@example.com" } }
+
+      it 'does not send an email confirmation' do
+        expect { post(:create, params: { email: email_params }) }.not_to change { ActionMailer::Base.deliveries.size }
+      end
     end
   end
 

spec/features/profiles/emails_spec.rb

@@ -31,6 +31,15 @@ describe 'Profile > Emails' do
       expect(email).to be_nil
       expect(page).to have_content('Email has already been taken')
     end
+
+    it 'does not add an invalid email' do
+      fill_in('Email', with: 'test.@example.com')
+      click_button('Add email address')
+
+      email = user.emails.find_by(email: email)
+      expect(email).to be_nil
+      expect(page).to have_content('Email is invalid')
+    end
   end
 
   it 'User removes email' do

spec/models/email_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 describe Email do
   describe 'validations' do
-    it_behaves_like 'an object with email-formated attributes', :email do
+    it_behaves_like 'an object with RFC3696 compliant email-formated attributes', :email do
       subject { build(:email) }
     end
   end

spec/models/user_spec.rb

@@ -295,7 +295,7 @@ describe User, :do_not_mock_admin_mode do
       subject { build(:user) }
     end
 
-    it_behaves_like 'an object with email-formated attributes', :public_email, :notification_email do
+    it_behaves_like 'an object with RFC3696 compliant email-formated attributes', :public_email, :notification_email do
       subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } }
     end
 
@@ -927,7 +927,6 @@ describe User, :do_not_mock_admin_mode do
             user.tap { |u| u.update!(email: new_email) }.reload
           end.to change(user, :unconfirmed_email).to(new_email)
         end
-
         it 'does not change :notification_email' do
           expect do
             user.tap { |u| u.update!(email: new_email) }.reload

spec/support/shared_examples/models/email_format_shared_examples.rb

@@ -44,3 +44,44 @@ RSpec.shared_examples 'an object with email-formated attributes' do |*attributes
     end
   end
 end
+
+RSpec.shared_examples 'an object with RFC3696 compliant email-formated attributes' do |*attributes|
+  attributes.each do |attribute|
+    describe "specifically its :#{attribute} attribute" do
+      %w[
+        info@example.com
+        info+test@example.com
+        o'reilly@example.com
+      ].each do |valid_email|
+        context "with a value of '#{valid_email}'" do
+          let(:email_value) { valid_email }
+
+          it 'is valid' do
+            subject.send("#{attribute}=", valid_email)
+
+            expect(subject).to be_valid
+          end
+        end
+      end
+
+      %w[
+        foobar
+        test@test@example.com
+        test.test.@example.com
+        .test.test@example.com
+        mailto:test@example.com
+        lol!'+=?><#$%^&*()@gmail.com
+      ].each do |invalid_email|
+        context "with a value of '#{invalid_email}'" do
+          let(:email_value) { invalid_email }
+
+          it 'is invalid' do
+            subject.send("#{attribute}=", invalid_email)
+
+            expect(subject).to be_invalid
+          end
+        end
+      end
+    end
+  end
+end