From 7e4b46b65c7d801e5f962105c183c76174df6fe7 Mon Sep 17 00:00:00 2001 From: Jens Pfeifer Date: Thu, 21 Nov 2019 16:49:14 +0100 Subject: [PATCH] Refactoring: Added consistent email address validation to backend. --- app/controllers/form_controller.rb | 16 +-- app/controllers/tickets_controller.rb | 9 +- app/models/channel/driver/sendmail.rb | 3 +- app/models/channel/driver/smtp.rb | 3 +- app/models/channel/filter/identify_sender.rb | 17 ++- app/models/email_address.rb | 6 +- app/models/ticket.rb | 10 +- app/models/user.rb | 12 +- lib/email_address_validation.rb | 38 ++++++ spec/lib/email_address_validation_spec.rb | 125 +++++++++++++++++++ test/unit/email_process_test.rb | 16 +-- 11 files changed, 215 insertions(+), 40 deletions(-) create mode 100644 lib/email_address_validation.rb create mode 100644 spec/lib/email_address_validation_spec.rb diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb index 527c5f79e..3b0f5e511 100644 --- a/app/controllers/form_controller.rb +++ b/app/controllers/form_controller.rb @@ -41,13 +41,6 @@ class FormController < ApplicationController if params[:name].blank? errors['name'] = 'required' end - if params[:email].blank? - errors['email'] = 'required' - elsif !/@/.match?(params[:email]) - errors['email'] = 'invalid' - elsif params[:email].match?(/(>|<|\||\!|"|§|'|\$|%|&|\(|\)|\?|\s|\.\.)/) - errors['email'] = 'invalid' - end if params[:title].blank? errors['title'] = 'required' end @@ -55,11 +48,12 @@ class FormController < ApplicationController errors['body'] = 'required' end - # realtime verify - if errors['email'].blank? + if params[:email].blank? + errors['email'] = 'required' + else begin - address = ValidEmail2::Address.new(params[:email]) - if !address || !address.valid? || !address.valid_mx? + email_address_validation = EmailAddressValidation.new(params[:email]) + if !email_address_validation.valid_format? || !email_address_validation.valid_mx? errors['email'] = 'invalid' end rescue => e diff --git a/app/controllers/tickets_controller.rb b/app/controllers/tickets_controller.rb index d90295973..daa361264 100644 --- a/app/controllers/tickets_controller.rb +++ b/app/controllers/tickets_controller.rb @@ -97,18 +97,19 @@ class TicketsController < ApplicationController # try to create customer if needed if clean_params[:customer_id].present? && clean_params[:customer_id] =~ /^guess:(.+?)$/ - email = $1 - if email !~ /@/ || email =~ /(>|<|\||\!|"|§|'|\$|%|&|\(|\)|\?|\s)/ + email_address = $1 + email_address_validation = EmailAddressValidation.new(email_address) + if !email_address_validation.valid_format? render json: { error: 'Invalid email of customer' }, status: :unprocessable_entity return end - local_customer = User.find_by(email: email.downcase) + local_customer = User.find_by(email: email_address.downcase) if !local_customer role_ids = Role.signup_role_ids local_customer = User.create( firstname: '', lastname: '', - email: email, + email: email_address, password: '', active: true, role_ids: role_ids, diff --git a/app/models/channel/driver/sendmail.rb b/app/models/channel/driver/sendmail.rb index 17f507247..1fe74d072 100644 --- a/app/models/channel/driver/sendmail.rb +++ b/app/models/channel/driver/sendmail.rb @@ -8,7 +8,8 @@ class Channel::Driver::Sendmail # set system_bcc of config if defined system_bcc = Setting.get('system_bcc') - if system_bcc.present? && system_bcc =~ /@/ + email_address_validation = EmailAddressValidation.new(system_bcc) + if system_bcc.present? && email_address_validation.valid_format? attr[:bcc] ||= '' attr[:bcc] += ', ' if attr[:bcc].present? attr[:bcc] += system_bcc diff --git a/app/models/channel/driver/smtp.rb b/app/models/channel/driver/smtp.rb index 0e343c26a..53ffe9a01 100644 --- a/app/models/channel/driver/smtp.rb +++ b/app/models/channel/driver/smtp.rb @@ -55,7 +55,8 @@ class Channel::Driver::Smtp # set system_bcc of config if defined system_bcc = Setting.get('system_bcc') - if system_bcc.present? && system_bcc =~ /@/ + email_address_validation = EmailAddressValidation.new(system_bcc) + if system_bcc.present? && email_address_validation.valid_format? attr[:bcc] ||= '' attr[:bcc] += ', ' if attr[:bcc].present? attr[:bcc] += system_bcc diff --git a/app/models/channel/filter/identify_sender.rb b/app/models/channel/filter/identify_sender.rb index 9ae0c5a3b..33432bf09 100644 --- a/app/models/channel/filter/identify_sender.rb +++ b/app/models/channel/filter/identify_sender.rb @@ -102,10 +102,11 @@ module Channel::Filter::IdentifySender next if items.blank? items.each do |address_data| - email_address = address_data.address + email_address = sanitize_email(address_data.address) next if email_address.blank? - next if !email_address.match?(/@/) - next if email_address.match?(/\s/) + + email_address_validation = EmailAddressValidation.new(email_address) + next if !email_address_validation.valid_format? user_create( firstname: address_data.display_name, @@ -131,9 +132,13 @@ module Channel::Filter::IdentifySender if recipient =~ /^(.+?)<(.+?)>/ display_name = $1 end + next if address.blank? - next if !address.match?(/@/) - next if address.match?(/\s/) + + address = sanitize_email(address) + + email_address_validation = EmailAddressValidation.new(address) + next if !email_address_validation.valid_format? user_create( firstname: display_name, @@ -194,10 +199,12 @@ module Channel::Filter::IdentifySender string.downcase .strip .delete('"') + .delete("'") .delete(' ') # see https://github.com/zammad/zammad/issues/2254 .sub(/^<|>$/, '') # see https://github.com/zammad/zammad/issues/2254 .sub(/\A'(.*)'\z/, '\1') # see https://github.com/zammad/zammad/issues/2154 .gsub(/\s/, '') # see https://github.com/zammad/zammad/issues/2198 + .gsub(/\.\z/, '') end end diff --git a/app/models/email_address.rb b/app/models/email_address.rb index eaa1868a4..28bd94ea4 100644 --- a/app/models/email_address.rb +++ b/app/models/email_address.rb @@ -48,8 +48,10 @@ check and if channel not exists reset configured channels for email addresses return true if email.blank? self.email = email.downcase.strip - raise Exceptions::UnprocessableEntity, 'Invalid email' if !email.match?(/@/) - raise Exceptions::UnprocessableEntity, 'Invalid email' if email.match?(/\s/) + email_address_validation = EmailAddressValidation.new(email) + if !email_address_validation.valid_format? + raise Exceptions::UnprocessableEntity, 'Invalid email' + end true end diff --git a/app/models/ticket.rb b/app/models/ticket.rb index fb97ae3f5..873bf2757 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -1352,13 +1352,13 @@ result # send notifications only to email addresses next if recipient_email.blank? - next if !recipient_email.match?(/@/) # check if address is valid begin Mail::AddressList.new(recipient_email).addresses.each do |address| recipient_email = address.address - break if recipient_email.present? && recipient_email =~ /@/ && !recipient_email.match?(/\s/) + email_address_validation = EmailAddressValidation.new(recipient_email) + break if recipient_email.present? && email_address_validation.valid_format? end rescue if recipient_email.present? @@ -1368,11 +1368,11 @@ result recipient_email = "#{$2}@#{$3}" end - next if recipient_email.blank? - next if !recipient_email.match?(/@/) - next if recipient_email.match?(/\s/) end + email_address_validation = EmailAddressValidation.new(recipient_email) + next if !email_address_validation.valid_format? + # do not sent notifications to this recipients send_no_auto_response_reg_exp = Setting.get('send_no_auto_response_reg_exp') begin diff --git a/app/models/user.rb b/app/models/user.rb index 3f7fb5f7a..409cd4af1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -959,8 +959,11 @@ try to find correct name self.email = email.downcase.strip return true if id == 1 - raise Exceptions::UnprocessableEntity, 'Invalid email' if !email.match?(/@/) - raise Exceptions::UnprocessableEntity, 'Invalid email' if email.match?(/\s/) + + email_address_validation = EmailAddressValidation.new(email) + if !email_address_validation.valid_format? + raise Exceptions::UnprocessableEntity, 'Invalid email' + end true end @@ -1182,7 +1185,10 @@ raise 'Minimum one user need to have admin permissions' def avatar_for_email_check return true if Setting.get('import_mode') return true if email.blank? - return true if !email.match?(/@/) + + email_address_validation = EmailAddressValidation.new(email) + return true if !email_address_validation.valid_format? + return true if !saved_change_to_attribute?('email') && updated_at > Time.zone.now - 10.days # save/update avatar diff --git a/lib/email_address_validation.rb b/lib/email_address_validation.rb new file mode 100644 index 000000000..a9d6f8960 --- /dev/null +++ b/lib/email_address_validation.rb @@ -0,0 +1,38 @@ +# Validation for email addresses + +class EmailAddressValidation + + attr_reader :email_address + + # @param [String] email_address Email address to be validated + def initialize(email_address) + @email_address = email_address + end + + def to_s + email_address + end + + # Checks if the email address has a valid format. + # Reports email addresses without dot in domain as valid (zammad@localhost). + # + # @return [true] if email address has valid format + # @return [false] if email address has no valid format + def valid_format? + # Note: Don't use ValidEmail2::Address.valid? here because it requires the + # email address to have a dot in its domain. + @valid_format ||= email_address.match?(URI::MailTo::EMAIL_REGEXP) + end + + # Checks if the domain of the email address has a valid MX record. + # + # @return [true] if email address domain has an MX record + # @return [false] if email address domain has no MX record + def valid_mx? + return @valid_mx if @valid_mx + + validated_email_address = ValidEmail2::Address.new(email_address) + @valid_mx = validated_email_address&.valid_mx? + end + +end diff --git a/spec/lib/email_address_validation_spec.rb b/spec/lib/email_address_validation_spec.rb new file mode 100644 index 000000000..4cfbfcc97 --- /dev/null +++ b/spec/lib/email_address_validation_spec.rb @@ -0,0 +1,125 @@ +require 'rails_helper' + +RSpec.describe EmailAddressValidation do + + describe 'Valid email address' do + + describe 'with dot in domain' do + + describe 'with MX record' do + let(:email_address) { 'support@zammad.com' } + let(:email_address_validation) { described_class.new(email_address) } + + it 'reports given email address' do + expect(email_address_validation.email_address).to eq(email_address) + expect(email_address_validation.to_s).to eq(email_address) + end + + it 'reports email address as valid' do + expect(email_address_validation.valid_format?).to be(true) + end + + it 'reports email address to have domain with MX record' do + expect(email_address_validation.valid_mx?).to be(true) + end + end + + describe 'without MX record' do + let(:email_address) { 'someone@this-is-probably-a-non-existent-domain.com.example' } + let(:email_address_validation) { described_class.new(email_address) } + + it 'reports given email address' do + expect(email_address_validation.email_address).to eq(email_address) + expect(email_address_validation.to_s).to eq(email_address) + end + + it 'reports email address as valid' do + expect(email_address_validation.valid_format?).to be(true) + end + + it 'reports email address to have domain without MX record' do + expect(email_address_validation.valid_mx?).to be(false) + end + end + + end + + describe 'without dot in domain' do + let(:email_address) { 'zammad@localhost' } + let(:email_address_validation) { described_class.new(email_address) } + + it 'reports given email address' do + expect(email_address_validation.email_address).to eq(email_address) + expect(email_address_validation.to_s).to eq(email_address) + end + + it 'reports email address as valid' do + expect(email_address_validation.valid_format?).to be(true) + end + + it 'reports email address to have domain without MX record' do + expect(email_address_validation.valid_mx?).to be(false) + end + end + + end + + describe 'Email address' do + + describe 'without domain' do + let(:email_address) { 'zammad' } + let(:email_address_validation) { described_class.new(email_address) } + + it 'reports given email address' do + expect(email_address_validation.email_address).to eq(email_address) + expect(email_address_validation.to_s).to eq(email_address) + end + + it 'reports email address as invalid' do + expect(email_address_validation.valid_format?).to be(false) + end + + it 'reports email address to have domain without MX record' do + expect(email_address_validation.valid_mx?).to be(false) + end + end + + describe 'with invalid domain format' do + let(:email_address) { 'support@zammad..com' } + let(:email_address_validation) { described_class.new(email_address) } + + it 'reports given email address' do + expect(email_address_validation.email_address).to eq(email_address) + expect(email_address_validation.to_s).to eq(email_address) + end + + it 'reports email address as invalid' do + expect(email_address_validation.valid_format?).to be(false) + end + + it 'reports email address to have domain without MX record' do + expect(email_address_validation.valid_mx?).to be(false) + end + end + + describe 'which is empty' do + let(:email_address) { '' } + let(:email_address_validation) { described_class.new(email_address) } + + it 'reports given email address' do + expect(email_address_validation.email_address).to eq(email_address) + expect(email_address_validation.to_s).to eq(email_address) + end + + it 'reports email address as invalid' do + expect(email_address_validation.valid_format?).to be(false) + end + + it 'reports email address to have domain without MX record' do + expect(email_address_validation.valid_mx?).to be(false) + end + end + + end + +end diff --git a/test/unit/email_process_test.rb b/test/unit/email_process_test.rb index 3d860d04b..b6e1cb140 100644 --- a/test/unit/email_process_test.rb +++ b/test/unit/email_process_test.rb @@ -95,14 +95,14 @@ Some Textäöü", { firstname: '', lastname: '', - fullname: 'me@exampl\'e.com', - email: 'me@exampl\'e.com', + fullname: 'me@example.com', + email: 'me@example.com', }, { firstname: '', lastname: '', - fullname: 'customer@exampl\'e.com', - email: 'customer@exampl\'e.com', + fullname: 'customer@example.com', + email: 'customer@example.com', }, ], }, @@ -2777,7 +2777,7 @@ Some Text', firstname: 'Clement.Si', lastname: '', fullname: 'Clement.Si', - email: 'claudia.shu@yahoo.com.', + email: 'claudia.shu@yahoo.com', }, { firstname: '', @@ -3266,7 +3266,7 @@ To: customer@example.com Subject: some subject X-Zammad-Ticket-Followup-State: closed X-Zammad-Ticket-priority: 3 high -X-Zammad-Ticket-owner: agent1@example.com +X-Zammad-Ticket-owner: agent1@example.com X-Zammad-Article-sender: System x-Zammad-Article-type: phone x-Zammad-Article-Internal: true @@ -3296,7 +3296,7 @@ To: customer@example.com Subject: some subject X-Zammad-Ticket-Followup-State: closed X-Zammad-Ticket-priority_id: 777777 -X-Zammad-Ticket-owner: not_existing@example.com +X-Zammad-Ticket-owner: not_existing@example.com X-Zammad-Article-sender_id: 999999 x-Zammad-Article-type: phone x-Zammad-Article-Internal: true @@ -3324,7 +3324,7 @@ Some Text', data: 'From: me@example.com To: customer@example.com Subject: some subject / with customer as agent - customer can not be owner -X-Zammad-Ticket-owner: customer1@example.com +X-Zammad-Ticket-owner: customer1@example.com Some Text', channel: {