From e79910431f6da4a66b8834a126dc6659215d6fc0 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 23 Mar 2020 19:06:44 -0500 Subject: [PATCH] emails: validate that email addresses are deliverable. Reject email addresses that known to be undeliverable during signup. Some users signup with invalid email addresses, which causes the welcome email (which contains the email confirmation link) to bounce. Too many bounces hurt our ability to send mail. We check that an email address is undeliverable by checking if the domain has a mail server and if the server returns an invalid address error when attempting to send mail. This isn't foolproof since some servers don't return an error if the address doesn't exist. If the checks fail we know the address is bad, but if the checks pass that doesn't guarantee the address is good. However, this is still good enough to filter out bad addresses for popular providers like Gmail and Microsoft that do return nonexistent address errors. The address existence check requires being able to connect to mail servers over port 25. This may fail if your network blocks port 25, which many home ISPs and hosting providers do by default. --- Gemfile | 1 + Gemfile.lock | 1 + app/controllers/users_controller.rb | 37 ++++++++++------- ...email_normalizer.rb => email_validator.rb} | 41 ++++++++++++++++++- app/models/email_address.rb | 9 +++- app/views/users/new.html.erb | 4 +- test/functional/users_controller_test.rb | 22 ++++++---- test/unit/email_validator_test.rb | 23 +++++++++++ 8 files changed, 111 insertions(+), 27 deletions(-) rename app/logical/{email_normalizer.rb => email_validator.rb} (67%) create mode 100644 test/unit/email_validator_test.rb diff --git a/Gemfile b/Gemfile index 3f9c05419..99addc66b 100644 --- a/Gemfile +++ b/Gemfile @@ -46,6 +46,7 @@ gem 'ipaddress' gem 'http' gem 'activerecord-hierarchical_query' gem 'pundit' +gem 'mail' # needed for looser jpeg header compat gem 'ruby-imagespec', :require => "image_spec", :git => "https://github.com/r888888888/ruby-imagespec.git", :branch => "exif-fixes" diff --git a/Gemfile.lock b/Gemfile.lock index 6218be49d..00191d539 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -436,6 +436,7 @@ DEPENDENCIES ipaddress jquery-rails listen + mail mechanize memoist memory_profiler diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 6f8a54572..06dbc9e9e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -59,22 +59,31 @@ class UsersController < ApplicationController end def create - @user = authorize User.new(last_ip_addr: CurrentUser.ip_addr, **permitted_attributes(User)) + @user = authorize User.new( + last_ip_addr: CurrentUser.ip_addr, + name: params[:user][:name], + password: params[:user][:password], + password_confirmation: params[:user][:password_confirmation] + ) - if !Danbooru.config.enable_recaptcha? || verify_recaptcha(model: @user) - @user.save - if @user.errors.empty? - session[:user_id] = @user.id - UserMailer.welcome_user(@user).deliver_later - else - flash[:notice] = "Sign up failed: #{@user.errors.full_messages.join("; ")}" - end - set_current_user - respond_with(@user) - else - flash[:notice] = "Sign up failed" - redirect_to new_user_path + if params[:user][:email].present? + @user.email_address = EmailAddress.new(address: params[:user][:email]) end + + if Danbooru.config.enable_recaptcha? && !verify_recaptcha(model: @user) + flash[:notice] = "Sign up failed" + elsif @user.email_address&.invalid?(:deliverable) + flash[:notice] = "Sign up failed: email address is invalid or doesn't exist" + @user.errors[:base] << @user.email_address.errors.full_messages.join("; ") + elsif !@user.save + flash[:notice] = "Sign up failed: #{@user.errors.full_messages.join("; ")}" + else + session[:user_id] = @user.id + UserMailer.welcome_user(@user).deliver_later + set_current_user + end + + respond_with(@user) end def update diff --git a/app/logical/email_normalizer.rb b/app/logical/email_validator.rb similarity index 67% rename from app/logical/email_normalizer.rb rename to app/logical/email_validator.rb index 19bb6c439..ada3d638e 100644 --- a/app/logical/email_normalizer.rb +++ b/app/logical/email_validator.rb @@ -1,4 +1,6 @@ -module EmailNormalizer +require 'resolv' + +module EmailValidator module_function IGNORE_DOTS = %w[gmail.com] @@ -73,4 +75,41 @@ module EmailNormalizer "#{name}@#{domain}" end + + def undeliverable?(to_address, from_address: Danbooru.config.contact_email, timeout: 3) + mail_server = mx_domain(to_address, timeout: timeout) + mail_server.nil? || rcpt_to_failed?(to_address, from_address, mail_server, timeout: timeout) + rescue + false + end + + def rcpt_to_failed?(to_address, from_address, mail_server, timeout: nil) + return false unless smtp_enabled? + + from_domain = Mail::Address.new(from_address).domain + + smtp = Net::SMTP.new(mail_server) + smtp.read_timeout = timeout + smtp.open_timeout = timeout + + smtp.start(from_domain) do |conn| + conn.mailfrom(from_address) + + # Net::SMTPFatalError is raised if RCPT TO returns a 5xx error. + response = conn.rcptto(to_address) rescue $! + return response.is_a?(Net::SMTPFatalError) + end + end + + def mx_domain(to_address, timeout: nil) + domain = Mail::Address.new(to_address).domain + + dns = Resolv::DNS.new + dns.timeouts = timeout + response = dns.getresource(domain, Resolv::DNS::Resource::IN::MX) + + response.exchange.to_s + rescue Resolv::ResolvError + nil + end end diff --git a/app/models/email_address.rb b/app/models/email_address.rb index 7812d5131..d96470eff 100644 --- a/app/models/email_address.rb +++ b/app/models/email_address.rb @@ -7,12 +7,19 @@ class EmailAddress < ApplicationRecord validates :address, presence: true, confirmation: true, format: { with: EMAIL_REGEX } validates :normalized_address, uniqueness: true validates :user_id, uniqueness: true + validate :validate_deliverable, on: :deliverable def address=(value) - self.normalized_address = EmailNormalizer.normalize(value) || address + self.normalized_address = EmailValidator.normalize(value) || address super end + def validate_deliverable + if EmailValidator.undeliverable?(address) + errors[:address] << "is invalid or does not exist" + end + end + concerning :VerificationMethods do def verifier @verifier ||= Danbooru::MessageVerifier.new(:email_verification_key) diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index 3081fdf0d..80ca23b11 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -15,9 +15,7 @@
<%= edit_form_for(@user, html: { id: "signup-form" }) do |f| %> <%= f.input :name, as: :string %> - <%= f.simple_fields_for :email_address do |fe| %> - <%= fe.input :address, label: "Email", required: false, as: :email, hint: "Optional" %> - <% end %> + <%= f.input :email, label: "Email", required: false, as: :email, hint: "Optional" %> <%= f.input :password %> <%= f.input :password_confirmation %> diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index 9ff3370f2..fa4c01a91 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -114,29 +114,35 @@ class UsersControllerTest < ActionDispatch::IntegrationTest context "create action" do should "create a user" do - user_params = { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" } - post users_path, params: { user: user_params } + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" }} assert_redirected_to User.last assert_equal("xxx", User.last.name) + assert_equal(nil, User.last.email_address) assert_no_emails end should "create a user with a valid email" do - user_params = { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1", email_address_attributes: { address: "test@gmail.com" } } - post users_path, params: { user: user_params } + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1", email: "webmaster@danbooru.donmai.us" }} assert_redirected_to User.last assert_equal("xxx", User.last.name) - assert_equal("test@gmail.com", User.last.email_address.address) + assert_equal("webmaster@danbooru.donmai.us", User.last.email_address.address) assert_enqueued_email_with UserMailer, :welcome_user, args: [User.last] end should "not create a user with an invalid email" do - user_params = { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1", email_address_attributes: { address: "test" } } + assert_no_difference(["User.count", "EmailAddress.count"]) do + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1", email: "test" }} - assert_no_difference("User.count") do - post users_path, params: { user: user_params } + assert_response :success + assert_no_emails + end + end + + should "not create a user with an undeliverable email address" do + assert_no_difference(["User.count", "EmailAddress.count"]) do + post users_path, params: { user: { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1", email: "nobody@nothing.donmai.us" } } assert_response :success assert_no_emails diff --git a/test/unit/email_validator_test.rb b/test/unit/email_validator_test.rb new file mode 100644 index 000000000..cf1bf0557 --- /dev/null +++ b/test/unit/email_validator_test.rb @@ -0,0 +1,23 @@ +require 'test_helper' + +class EmailValidatorTest < ActiveSupport::TestCase + context "EmailValidator" do + context "#undeliverable?" do + should "return good addresses as deliverable" do + assert_equal(false, EmailValidator.undeliverable?("webmaster@danbooru.donmai.us")) + assert_equal(false, EmailValidator.undeliverable?("noizave+spam@gmail.com")) + end + + should "return nonexistent domains as undeliverable" do + assert_equal(true, EmailValidator.undeliverable?("nobody@does.not.exist.donmai.us")) + end + + # XXX these tests are known to fail if your network blocks port 25. + should_eventually "return nonexistent addresses as undeliverable" do + assert_equal(true, EmailValidator.undeliverable?("does.not.exist.13yoigo34iy@gmail.com")) + assert_equal(true, EmailValidator.undeliverable?("does.not.exist.13yoigo34iy@outlook.com")) + assert_equal(true, EmailValidator.undeliverable?("does.not.exist.13yoigo34iy@hotmail.com")) + end + end + end +end