Improve e-mail MX validator and add tests (#9489)

app/models/user.rb

@@ -73,7 +73,7 @@ class User < ApplicationRecord
 
   validates :locale, inclusion: I18n.available_locales.map(&:to_s), if: :locale?
   validates_with BlacklistedEmailValidator, if: :email_changed?
-  validates_with EmailMxValidator, if: :email_changed?
+  validates_with EmailMxValidator, if: :validate_email_dns?
 
   scope :recent, -> { order(id: :desc) }
   scope :admins, -> { where(admin: true) }
@@ -360,4 +360,8 @@ class User < ApplicationRecord
   def needs_feed_update?
     last_sign_in_at < ACTIVE_DURATION.ago
   end
+
+  def validate_email_dns?
+    email_changed? && !(Rails.env.test? || Rails.env.development?)
+  end
 end

app/validators/email_mx_validator.rb

@@ -4,7 +4,6 @@ require 'resolv'
 
 class EmailMxValidator < ActiveModel::Validator
   def validate(user)
-    return if Rails.env.test? || Rails.env.development?
     user.errors.add(:email, I18n.t('users.invalid_email')) if invalid_mx?(user.email)
   end
 
@@ -15,13 +14,23 @@ class EmailMxValidator < ActiveModel::Validator
 
     return true if domain.nil?
 
-    records = Resolv::DNS.new.getresources(domain, Resolv::DNS::Resource::IN::MX).to_a.map { |e| e.exchange.to_s }
-    records = Resolv::DNS.new.getresources(domain, Resolv::DNS::Resource::IN::A).to_a.map { |e| e.address.to_s } if records.empty?
+    hostnames = []
+    ips       = []
 
-    records.empty? || on_blacklist?(records)
+    Resolv::DNS.open do |dns|
+      dns.timeouts = 1
+
+      hostnames = dns.getresources(domain, Resolv::DNS::Resource::IN::MX).to_a.map { |e| e.exchange.to_s }
+
+      ([domain] + hostnames).uniq.each do |hostname|
+        ips.concat(dns.getresources(hostname, Resolv::DNS::Resource::IN::A).to_a.map { |e| e.address.to_s })
+      end
+    end
+
+    ips.empty? || on_blacklist?(hostnames + ips)
   end
 
   def on_blacklist?(values)
-    EmailDomainBlock.where(domain: values).any?
+    EmailDomainBlock.where(domain: values.uniq).any?
   end
 end

spec/validators/email_mx_validator_spec.rb

+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe EmailMxValidator do
+  describe '#validate' do
+    let(:user) { double(email: 'foo@example.com', errors: double(add: nil)) }
+
+    it 'adds an error if there are no DNS records for the e-mail domain' do
+      resolver = double
+
+      allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::MX).and_return([])
+      allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::A).and_return([])
+      allow(resolver).to receive(:timeouts=).and_return(nil)
+      allow(Resolv::DNS).to receive(:open).and_yield(resolver)
+
+      subject.validate(user)
+      expect(user.errors).to have_received(:add)
+    end
+
+    it 'adds an error if a MX record exists but does not lead to an IP' do
+      resolver = double
+
+      allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::MX).and_return([double(exchange: 'mail.example.com')])
+      allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::A).and_return([])
+      allow(resolver).to receive(:getresources).with('mail.example.com', Resolv::DNS::Resource::IN::A).and_return([])
+      allow(resolver).to receive(:timeouts=).and_return(nil)
+      allow(Resolv::DNS).to receive(:open).and_yield(resolver)
+
+      subject.validate(user)
+      expect(user.errors).to have_received(:add)
+    end
+
+    it 'adds an error if the A record is blacklisted' do
+      EmailDomainBlock.create!(domain: '1.2.3.4')
+      resolver = double
+
+      allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::MX).and_return([])
+      allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::A).and_return([double(address: '1.2.3.4')])
+      allow(resolver).to receive(:timeouts=).and_return(nil)
+      allow(Resolv::DNS).to receive(:open).and_yield(resolver)
+
+      subject.validate(user)
+      expect(user.errors).to have_received(:add)
+    end
+
+    it 'adds an error if the MX record is blacklisted' do
+      EmailDomainBlock.create!(domain: '2.3.4.5')
+      resolver = double
+
+      allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::MX).and_return([double(exchange: 'mail.example.com')])
+      allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::A).and_return([])
+      allow(resolver).to receive(:getresources).with('mail.example.com', Resolv::DNS::Resource::IN::A).and_return([double(address: '2.3.4.5')])
+      allow(resolver).to receive(:timeouts=).and_return(nil)
+      allow(Resolv::DNS).to receive(:open).and_yield(resolver)
+
+      subject.validate(user)
+      expect(user.errors).to have_received(:add)
+    end
+
+    it 'adds an error if the MX hostname is blacklisted' do
+      EmailDomainBlock.create!(domain: 'mail.example.com')
+      resolver = double
+
+      allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::MX).and_return([double(exchange: 'mail.example.com')])
+      allow(resolver).to receive(:getresources).with('example.com', Resolv::DNS::Resource::IN::A).and_return([])
+      allow(resolver).to receive(:getresources).with('mail.example.com', Resolv::DNS::Resource::IN::A).and_return([double(address: '2.3.4.5')])
+      allow(resolver).to receive(:timeouts=).and_return(nil)
+      allow(Resolv::DNS).to receive(:open).and_yield(resolver)
+
+      subject.validate(user)
+      expect(user.errors).to have_received(:add)
+    end
+  end
+end