From 86807e4799d21863084d52d4070f3da7c120e260 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 5 Apr 2024 09:48:45 +0200 Subject: [PATCH] Improve email address validation (#29838) --- Gemfile | 2 ++ Gemfile.lock | 1 + app/models/user.rb | 2 ++ app/validators/email_address_validator.rb | 18 ++++++++++++++++++ spec/models/user_spec.rb | 6 ++++++ 5 files changed, 29 insertions(+) create mode 100644 app/validators/email_address_validator.rb diff --git a/Gemfile b/Gemfile index 829e7d8574..1d038ad9f8 100644 --- a/Gemfile +++ b/Gemfile @@ -204,3 +204,5 @@ gem 'net-http', '~> 0.3.2' gem 'rubyzip', '~> 2.3' gem 'hcaptcha', '~> 7.1' + +gem 'mail', '~> 2.8' diff --git a/Gemfile.lock b/Gemfile.lock index 9290a84aea..94d2d0ba49 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -871,6 +871,7 @@ DEPENDENCIES letter_opener_web (~> 2.0) link_header (~> 0.0) lograge (~> 0.12) + mail (~> 2.8) mario-redis-lock (~> 1.2) md-paperclip-azure (~> 2.2) memory_profiler diff --git a/app/models/user.rb b/app/models/user.rb index ee90979279..0c8d481c4c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -96,6 +96,8 @@ class User < ApplicationRecord accepts_nested_attributes_for :invite_request, reject_if: ->(attributes) { attributes['text'].blank? && !Setting.require_invite_text } validates :invite_request, presence: true, on: :create, if: :invite_text_required? + validates :email, presence: true, email_address: true + validates_with BlacklistedEmailValidator, if: -> { ENV['EMAIL_DOMAIN_LISTS_APPLY_AFTER_CONFIRMATION'] == 'true' || !confirmed? } validates_with EmailMxValidator, if: :validate_email_dns? validates :agreement, acceptance: { allow_nil: false, accept: [true, 'true', '1'] }, on: :create diff --git a/app/validators/email_address_validator.rb b/app/validators/email_address_validator.rb new file mode 100644 index 0000000000..ed0bb11652 --- /dev/null +++ b/app/validators/email_address_validator.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +# NOTE: I initially wrote this as `EmailValidator` but it ended up clashing +# with an indirect dependency of ours, `validate_email`, which, turns out, +# has the same approach as we do, but with an extra check disallowing +# single-label domains. Decided to not switch to `validate_email` because +# we do want to allow at least `localhost`. + +class EmailAddressValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + value = value.strip + + address = Mail::Address.new(value) + record.errors.add(attribute, :invalid) if address.address != value + rescue Mail::Field::FieldError + record.errors.add(attribute, :invalid) + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ffd1889cbd..f06150f02c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -39,6 +39,12 @@ RSpec.describe User do expect(user.valid?).to be true end + it 'is valid with a localhost e-mail address' do + user = Fabricate.build(:user, email: 'admin@localhost') + user.valid? + expect(user.valid?).to be true + end + it 'cleans out invalid locale' do user = Fabricate.build(:user, locale: 'toto') expect(user.valid?).to be true