From 8d217d7231be46af552c63aff9e53d0ed5dca0f6 Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 12 Aug 2020 12:40:25 +0200 Subject: [PATCH] Improve email address validation (#14565) * Increase DNS timeout from 1 second to 5 seconds for MX check 1 seconds is rather short when using a recursive DNS resolver which hasn't got a cached result already available. Use 5 seconds instead, which is the timeout value we use for outgoing HTTP queries. * Add more precise error messages for invalid e-mail addresses --- .../admin/email_domain_blocks_controller.rb | 2 +- app/validators/blacklisted_email_validator.rb | 2 +- app/validators/email_mx_validator.rb | 30 ++++++++++++++----- config/locales/en.yml | 2 ++ lib/mastodon/email_domain_blocks_cli.rb | 2 +- .../blacklisted_email_validator_spec.rb | 4 +-- 6 files changed, 29 insertions(+), 13 deletions(-) diff --git a/app/controllers/admin/email_domain_blocks_controller.rb b/app/controllers/admin/email_domain_blocks_controller.rb index c259197262..f7bdfb0c5f 100644 --- a/app/controllers/admin/email_domain_blocks_controller.rb +++ b/app/controllers/admin/email_domain_blocks_controller.rb @@ -27,7 +27,7 @@ module Admin ips = [] Resolv::DNS.open do |dns| - dns.timeouts = 1 + dns.timeouts = 5 hostnames = dns.getresources(@email_domain_block.domain, Resolv::DNS::Resource::IN::MX).to_a.map { |e| e.exchange.to_s } diff --git a/app/validators/blacklisted_email_validator.rb b/app/validators/blacklisted_email_validator.rb index 0d01a1c47f..16e3abf129 100644 --- a/app/validators/blacklisted_email_validator.rb +++ b/app/validators/blacklisted_email_validator.rb @@ -6,7 +6,7 @@ class BlacklistedEmailValidator < ActiveModel::Validator @email = user.email - user.errors.add(:email, I18n.t('users.invalid_email')) if blocked_email? + user.errors.add(:email, I18n.t('users.blocked_email_provider')) if blocked_email? end private diff --git a/app/validators/email_mx_validator.rb b/app/validators/email_mx_validator.rb index 9b50099668..ef1554494c 100644 --- a/app/validators/email_mx_validator.rb +++ b/app/validators/email_mx_validator.rb @@ -4,22 +4,38 @@ require 'resolv' class EmailMxValidator < ActiveModel::Validator def validate(user) - user.errors.add(:email, I18n.t('users.invalid_email')) if invalid_mx?(user.email) + domain = get_domain(user.email) + + if domain.nil? + user.errors.add(:email, I18n.t('users.invalid_email')) + else + ips, hostnames = resolve_mx(domain) + if ips.empty? + user.errors.add(:email, I18n.t('users.invalid_email_mx')) + elsif on_blacklist?(hostnames + ips) + user.errors.add(:email, I18n.t('users.blocked_email_provider')) + end + end end private - def invalid_mx?(value) + def get_domain(value) _, domain = value.split('@', 2) - return true if domain.nil? + return nil if domain.nil? + + TagManager.instance.normalize_domain(domain) + rescue Addressable::URI::InvalidURIError + nil + end - domain = TagManager.instance.normalize_domain(domain) + def resolve_mx(domain) hostnames = [] ips = [] Resolv::DNS.open do |dns| - dns.timeouts = 1 + dns.timeouts = 5 hostnames = dns.getresources(domain, Resolv::DNS::Resource::IN::MX).to_a.map { |e| e.exchange.to_s } @@ -29,9 +45,7 @@ class EmailMxValidator < ActiveModel::Validator end end - ips.empty? || on_blacklist?(hostnames + ips) - rescue Addressable::URI::InvalidURIError - true + [ips, hostnames] end def on_blacklist?(values) diff --git a/config/locales/en.yml b/config/locales/en.yml index 2cae0a3e3b..40adfc21e4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1325,9 +1325,11 @@ en: tips: Tips title: Welcome aboard, %{name}! users: + blocked_email_provider: This e-mail provider isn't allowed follow_limit_reached: You cannot follow more than %{limit} people generic_access_help_html: Trouble accessing your account? You may get in touch with %{email} for assistance invalid_email: The e-mail address is invalid + invalid_email_mx: The e-mail address does not seem to exist invalid_otp_token: Invalid two-factor code invalid_sign_in_token: Invalid security code otp_lost_help_html: If you lost access to both, you may get in touch with %{email} diff --git a/lib/mastodon/email_domain_blocks_cli.rb b/lib/mastodon/email_domain_blocks_cli.rb index 7fe1efaaa5..55a637d682 100644 --- a/lib/mastodon/email_domain_blocks_cli.rb +++ b/lib/mastodon/email_domain_blocks_cli.rb @@ -63,7 +63,7 @@ module Mastodon ips = [] Resolv::DNS.open do |dns| - dns.timeouts = 1 + dns.timeouts = 5 hostnames = dns.getresources(email_domain_block.domain, Resolv::DNS::Resource::IN::MX).to_a.map { |e| e.exchange.to_s } ([email_domain_block.domain] + hostnames).uniq.each do |hostname| diff --git a/spec/validators/blacklisted_email_validator_spec.rb b/spec/validators/blacklisted_email_validator_spec.rb index ccc5dc0f48..f0708dc462 100644 --- a/spec/validators/blacklisted_email_validator_spec.rb +++ b/spec/validators/blacklisted_email_validator_spec.rb @@ -17,7 +17,7 @@ RSpec.describe BlacklistedEmailValidator, type: :validator do let(:blocked_email) { true } it 'calls errors.add' do - expect(errors).to have_received(:add).with(:email, I18n.t('users.invalid_email')) + expect(errors).to have_received(:add).with(:email, I18n.t('users.blocked_email_provider')) end end @@ -25,7 +25,7 @@ RSpec.describe BlacklistedEmailValidator, type: :validator do let(:blocked_email) { false } it 'not calls errors.add' do - expect(errors).not_to have_received(:add).with(:email, I18n.t('users.invalid_email')) + expect(errors).not_to have_received(:add).with(:email, I18n.t('users.blocked_email_provider')) end end end