From 3bf3b9a4a5e066598482bc7add7a44dcb334178c Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Thu, 17 Feb 2022 20:12:51 -0500 Subject: [PATCH] DEV: pull email address validation out to a new EmailAddressValidator We validate the *format* of email addresses in many places with a match against a regex, often with very slightly different syntax. Adding a separate EmailAddressValidator simplifies the code in a few spots and feels cleaner. Deprecated the old location in case someone is using it in a plugin. No functionality change is in this commit. Note: the regex used at the moment does not support using address literals, e.g.: * localpart@[192.168.0.1] * localpart@[2001:db8::1] --- app/controllers/session_controller.rb | 2 +- app/controllers/users_controller.rb | 2 +- app/jobs/regular/bulk_invite.rb | 2 +- app/jobs/regular/group_smtp_email.rb | 6 +++--- app/models/email_change_request.rb | 2 +- app/models/user.rb | 2 +- lib/email.rb | 2 +- lib/validators/email_address_validator.rb | 11 +++++++++++ lib/validators/email_setting_validator.rb | 3 ++- lib/validators/email_validator.rb | 12 ++++++++---- script/bulk_import/base.rb | 2 +- script/import_scripts/base.rb | 2 +- script/import_scripts/drupal.rb | 2 +- script/import_scripts/vbulletin.rb | 2 +- .../validators/email_address_validator_spec.rb | 17 +++++++++++++++++ .../validators/email_validator_spec.rb | 13 ------------- 16 files changed, 51 insertions(+), 31 deletions(-) create mode 100644 lib/validators/email_address_validator.rb create mode 100644 spec/components/validators/email_address_validator_spec.rb diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 1ce588b4f92..277be0ef215 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -546,7 +546,7 @@ class SessionController < ApplicationController RateLimiter.new(nil, "forgot-password-min-#{request.remote_ip}", 3, 1.minute).performed! user = if SiteSetting.hide_email_address_taken && !current_user&.staff? - raise Discourse::InvalidParameters.new(:login) if EmailValidator.email_regex !~ normalized_login_param + raise Discourse::InvalidParameters.new(:login) if !EmailAddressValidator.valid_value?(normalized_login_param) User.real.where(staged: false).find_by_email(Email.downcase(normalized_login_param)) else User.real.where(staged: false).find_by_username_or_email(normalized_login_param) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 68cf75bd87f..493cbd112b4 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -600,7 +600,7 @@ class UsersController < ApplicationController return render json: success_json end - if !(email =~ EmailValidator.email_regex) + if !EmailAddressValidator.valid_value?(email) error = User.new.errors.full_message(:email, I18n.t(:'user.email.invalid')) return render json: failed_json.merge(errors: [error]) end diff --git a/app/jobs/regular/bulk_invite.rb b/app/jobs/regular/bulk_invite.rb index abef2ee196b..08fc977f459 100644 --- a/app/jobs/regular/bulk_invite.rb +++ b/app/jobs/regular/bulk_invite.rb @@ -37,7 +37,7 @@ module Jobs def process_invites(invites) invites.each do |invite| - if (EmailValidator.email_regex =~ invite[:email]) + if EmailAddressValidator.valid_value?(invite[:email]) # email is valid send_invite(invite) @sent += 1 diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb index 61bc09fa0b3..dbe9f822245 100644 --- a/app/jobs/regular/group_smtp_email.rb +++ b/app/jobs/regular/group_smtp_email.rb @@ -42,9 +42,9 @@ module Jobs return skip(email, post, recipient_user, :group_smtp_topic_deleted) end - cc_addresses = args[:cc_emails].map do |cc| - cc.match(EmailValidator.email_regex) ? cc : nil - end.compact + cc_addresses = args[:cc_emails].filter do |address| + EmailAddressValidator.valid_value?(address) + end # There is a rare race condition causing the Imap::Sync class to create # an incoming email and associated post/topic, which then kicks off diff --git a/app/models/email_change_request.rb b/app/models/email_change_request.rb index 5282d5d2fd5..44ac085c2da 100644 --- a/app/models/email_change_request.rb +++ b/app/models/email_change_request.rb @@ -6,7 +6,7 @@ class EmailChangeRequest < ActiveRecord::Base belongs_to :new_email_token, class_name: 'EmailToken', dependent: :destroy belongs_to :requested_by, class_name: "User", foreign_key: :requested_by_user_id - validates :new_email, presence: true, format: { with: EmailValidator.email_regex } + validates :new_email, presence: true, format: { with: EmailAddressValidator.email_regex } def self.states @states ||= Enum.new(authorizing_old: 1, authorizing_new: 2, complete: 3) diff --git a/app/models/user.rb b/app/models/user.rb index d610cc524f7..f4f80e70e7b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1151,7 +1151,7 @@ class User < ActiveRecord::Base end def find_email - last_sent_email_address.present? && EmailValidator.email_regex =~ last_sent_email_address ? last_sent_email_address : email + last_sent_email_address.present? && EmailAddressValidator.valid_value?(last_sent_email_address) ? last_sent_email_address : email end def tl3_requirements diff --git a/lib/email.rb b/lib/email.rb index 8b7058b28f3..8993c2a4163 100644 --- a/lib/email.rb +++ b/lib/email.rb @@ -10,7 +10,7 @@ module Email def self.is_valid?(email) return false unless String === email - !!(EmailValidator.email_regex =~ email) + EmailAddressValidator.valid_value?(email) end def self.downcase(email) diff --git a/lib/validators/email_address_validator.rb b/lib/validators/email_address_validator.rb new file mode 100644 index 00000000000..054dbba9250 --- /dev/null +++ b/lib/validators/email_address_validator.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class EmailAddressValidator + def self.valid_value?(email) + email.match? email_regex + end + + def self.email_regex + /\A[a-zA-Z0-9!#\$%&'*+\/=?\^_`{|}~\-]+(?:\.[a-zA-Z0-9!#\$%&'\*+\/=?\^_`{|}~\-]+)*@(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?\.)+[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?$\z/ + end +end diff --git a/lib/validators/email_setting_validator.rb b/lib/validators/email_setting_validator.rb index 0d4c98552a7..f35328bfce9 100644 --- a/lib/validators/email_setting_validator.rb +++ b/lib/validators/email_setting_validator.rb @@ -6,7 +6,8 @@ class EmailSettingValidator end def valid_value?(val) - !val.present? || !!(EmailValidator.email_regex =~ val) + return true if val.blank? + EmailAddressValidator.valid_value?(val) end def error_message diff --git a/lib/validators/email_validator.rb b/lib/validators/email_validator.rb index 6ce7bd0ded7..3f96c2d7164 100644 --- a/lib/validators/email_validator.rb +++ b/lib/validators/email_validator.rb @@ -3,7 +3,7 @@ class EmailValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - unless value =~ EmailValidator.email_regex + if !EmailAddressValidator.valid_value?(value) if Invite === record && attribute == :email record.errors.add(:base, I18n.t(:'invite.invalid_email', email: CGI.escapeHTML(value))) else @@ -12,7 +12,7 @@ class EmailValidator < ActiveModel::EachValidator invalid = true end - unless EmailValidator.allowed?(value) + if !EmailValidator.allowed?(value) record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) invalid = true end @@ -51,7 +51,11 @@ class EmailValidator < ActiveModel::EachValidator end def self.email_regex - /\A[a-zA-Z0-9!#\$%&'*+\/=?\^_`{|}~\-]+(?:\.[a-zA-Z0-9!#\$%&'\*+\/=?\^_`{|}~\-]+)*@(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?\.)+[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?$\z/ + Discourse.deprecate( + "EmailValidator.email_regex is deprecated. Please use EmailAddressValidator instead.", + output_in_test: true, + drop_from: '2.9.0', + ) + EmailAddressValidator.email_regex end - end diff --git a/script/bulk_import/base.rb b/script/bulk_import/base.rb index ab01e0c9bcd..aeab3877542 100644 --- a/script/bulk_import/base.rb +++ b/script/bulk_import/base.rb @@ -452,7 +452,7 @@ class BulkImport::Base user_email[:email] ||= random_email user_email[:email].downcase! # unique email - user_email[:email] = random_email until user_email[:email] =~ EmailValidator.email_regex && !@emails.has_key?(user_email[:email]) + user_email[:email] = random_email until EmailAddressValidator.valid_value?(user_email[:email]) && !@emails.has_key?(user_email[:email]) user_email end diff --git a/script/import_scripts/base.rb b/script/import_scripts/base.rb index 98fc685b1fc..3fcfce549a6 100644 --- a/script/import_scripts/base.rb +++ b/script/import_scripts/base.rb @@ -320,7 +320,7 @@ class ImportScripts::Base opts[:username] = UserNameSuggester.suggest(opts[:username].presence || opts[:name].presence || opts[:email]) end - unless opts[:email][EmailValidator.email_regex] + if !EmailAddressValidator.valid_value?(opts[:email]) opts[:email] = fake_email puts "Invalid email '#{original_email}' for '#{opts[:username]}'. Using '#{opts[:email]}'" end diff --git a/script/import_scripts/drupal.rb b/script/import_scripts/drupal.rb index 23348c055bf..3759bad8e04 100644 --- a/script/import_scripts/drupal.rb +++ b/script/import_scripts/drupal.rb @@ -76,7 +76,7 @@ class ImportScripts::Drupal < ImportScripts::Base create_users(users, total: user_count, offset: offset) do |user| email = user["email"].presence || fake_email - email = fake_email unless email[EmailValidator.email_regex] + email = fake_email if !EmailAddressValidator.valid_value?(email) username = @htmlentities.decode(user["username"]).strip diff --git a/script/import_scripts/vbulletin.rb b/script/import_scripts/vbulletin.rb index 9107823d3f0..5882fabd63d 100644 --- a/script/import_scripts/vbulletin.rb +++ b/script/import_scripts/vbulletin.rb @@ -151,7 +151,7 @@ EOM create_users(users, total: user_count, offset: offset) do |user| email = user["email"].presence || fake_email - email = fake_email unless email[EmailValidator.email_regex] + email = fake_email if !EmailAddressValidator.valid_value?(email) password = [user["password"].presence, user["salt"].presence].compact.join(":") diff --git a/spec/components/validators/email_address_validator_spec.rb b/spec/components/validators/email_address_validator_spec.rb new file mode 100644 index 00000000000..3450bcc216e --- /dev/null +++ b/spec/components/validators/email_address_validator_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe EmailAddressValidator do + it 'should match valid emails' do + ['test@discourse.org', 'good_user@discourse.org', 'incoming+%{reply_key}@discourse.org'].each do |email| + expect(EmailAddressValidator.valid_value?(email)).to eq(true) + end + end + + it 'should not match invalid emails' do + ['testdiscourse.org', 'frank@invalid_host.contoso.com', 'frank@invalid_host.com', 'test@discourse.org; a@discourse.org', 'random', ''].each do |email| + expect(EmailAddressValidator.valid_value?(email)).to eq(false) + end + end +end diff --git a/spec/components/validators/email_validator_spec.rb b/spec/components/validators/email_validator_spec.rb index 0290c7c99c7..a2a746849d2 100644 --- a/spec/components/validators/email_validator_spec.rb +++ b/spec/components/validators/email_validator_spec.rb @@ -57,17 +57,4 @@ describe EmailValidator do expect(EmailValidator.can_auto_approve_user?("foobar@googlemail.com")).to eq(true) end end - - context '.email_regex' do - it 'should match valid emails' do - expect(!!('test@discourse.org' =~ EmailValidator.email_regex)).to eq(true) - end - - it 'should not match invalid emails' do - ['testdiscourse.org', 'test@discourse.org; a@discourse.org', 'random'].each do |email| - expect(!!(email =~ EmailValidator.email_regex)).to eq(false) - end - end - end - end