From 7f50380221f4a77bec7dfedc141ef99213de9438 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Tue, 3 Oct 2017 11:23:18 +0200 Subject: [PATCH] FIX: respect email domain whitelist/blacklist when creating staged users --- config/locales/server.en.yml | 9 ++++++ lib/email/processor.rb | 1 + lib/email/receiver.rb | 29 +++++++++++-------- lib/validators/email_validator.rb | 25 +++++++++------- spec/components/email/receiver_spec.rb | 27 +++++++++++++++++ .../validators/email_validator_spec.rb | 8 +++++ .../emails/blacklist_whitelist_email.eml | 9 ++++++ 7 files changed, 86 insertions(+), 22 deletions(-) create mode 100644 spec/fixtures/emails/blacklist_whitelist_email.eml diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 245b329690d..374123e632a 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -80,6 +80,7 @@ en: bounced_email_error: "Email is a bounced email report." screened_email_error: "Happens when the sender's email address was already screened." unsubscribe_not_allowed: "Happens when unsubscribing via email is not allowed for this user." + email_not_allowed: "Happens when the email address is not on the whitelist or is on the blacklist." unrecognized_error: "Unrecognized Error" errors: &errors @@ -2151,6 +2152,14 @@ en: Your reply was sent from a blocked email address. Try sending from another email address, or [contact a staff member](%{base_url}/about). + email_reject_not_allowed_email: + title: "Email Reject Not Allowed Email" + subject_template: "[%{email_prefix}] Email issue -- Blocked Email" + text_body_template: | + We're sorry, but your email message to %{destination} (titled %{former_title}) didn't work. + + Your reply was sent from a blocked email address. Try sending from another email address, or [contact a staff member](%{base_url}/about). + email_reject_inactive_user: title: "Email Reject Inactive User" subject_template: "[%{email_prefix}] Email issue -- Inactive User" diff --git a/lib/email/processor.rb b/lib/email/processor.rb index f1ee881531f..b57fb9a0038 100644 --- a/lib/email/processor.rb +++ b/lib/email/processor.rb @@ -40,6 +40,7 @@ module Email when Email::Receiver::NoBodyDetectedError then :email_reject_empty when Email::Receiver::UserNotFoundError then :email_reject_user_not_found when Email::Receiver::ScreenedEmailError then :email_reject_screened_email + when Email::Receiver::EmailNotAllowed then :email_reject_not_allowed_email when Email::Receiver::AutoGeneratedEmailError then :email_reject_auto_generated when Email::Receiver::InactiveUserError then :email_reject_inactive_user when Email::Receiver::BlockedUserError then :email_reject_blocked_user diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 8405bde4a4a..69569f82b54 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -31,6 +31,7 @@ module Email class InvalidPost < ProcessingError; end class InvalidPostAction < ProcessingError; end class UnsubscribeNotAllowed < ProcessingError; end + class EmailNotAllowed < ProcessingError; end attr_reader :incoming_email attr_reader :raw_email @@ -86,7 +87,7 @@ module Email user = find_user(@from_email) if user.present? - process_user(user) + log_and_validate_user(user) else raise UserNotFoundError unless SiteSetting.enable_staged_users end @@ -109,8 +110,10 @@ module Email # Lets create a staged user if there isn't one yet. We will try to # delete staged users in process!() if something bad happens. - user = find_or_create_user(@from_email, @from_display_name) if user.nil? - process_user(user) + if user.nil? + user = find_or_create_user(@from_email, @from_display_name) + log_and_validate_user(user) + end if post = find_related_post create_reply(user: user, @@ -136,11 +139,11 @@ module Email end end - def process_user(user) + def log_and_validate_user(user) @incoming_email.update_columns(user_id: user.id) raise InactiveUserError if !user.active && !user.staged - raise BlockedUserError if user.blocked + raise BlockedUserError if user.blocked end def is_bounce? @@ -333,10 +336,12 @@ module Email user = nil User.transaction do - begin - user = User.find_by_email(email) + user = User.find_by_email(email) - if user.nil? && SiteSetting.enable_staged_users + if user.nil? && SiteSetting.enable_staged_users + raise EmailNotAllowed unless EmailValidator.allowed?(email) + + begin username = UserNameSuggester.sanitize_username(display_name) if display_name.present? user = User.create!( email: email, @@ -345,9 +350,9 @@ module Email staged: true ) @staged_users << user + rescue + user = nil end - rescue - user = nil end end @@ -717,8 +722,8 @@ module Email return end end - rescue ActiveRecord::RecordInvalid - # don't care if user already allowed + rescue ActiveRecord::RecordInvalid, EmailNotAllowed + # don't care if user already allowed or the user's email address is not allowed end end end diff --git a/lib/validators/email_validator.rb b/lib/validators/email_validator.rb index 468c0050710..64463d53cb3 100644 --- a/lib/validators/email_validator.rb +++ b/lib/validators/email_validator.rb @@ -1,27 +1,32 @@ class EmailValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - if (setting = SiteSetting.email_domains_whitelist).present? - unless email_in_restriction_setting?(setting, value) || is_developer?(value) - record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) - end - elsif (setting = SiteSetting.email_domains_blacklist).present? - if email_in_restriction_setting?(setting, value) && !is_developer?(value) - record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) - end + unless EmailValidator.allowed?(value) + record.errors.add(attribute, I18n.t(:'user.email.not_allowed')) end + if record.errors[attribute].blank? && value && ScreenedEmail.should_block?(value) record.errors.add(attribute, I18n.t(:'user.email.blocked')) end end - def email_in_restriction_setting?(setting, value) + def self.allowed?(email) + if (setting = SiteSetting.email_domains_whitelist).present? + return email_in_restriction_setting?(setting, email) || is_developer?(email) + elsif (setting = SiteSetting.email_domains_blacklist).present? + return !(email_in_restriction_setting?(setting, email) && !is_developer?(email)) + end + + true + end + + def self.email_in_restriction_setting?(setting, value) domains = setting.gsub('.', '\.') regexp = Regexp.new("@(.+\\.)?(#{domains})", true) value =~ regexp end - def is_developer?(value) + def self.is_developer?(value) Rails.configuration.respond_to?(:developer_emails) && Rails.configuration.developer_emails.include?(value) end diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 40dc01a0f3f..839f36192f3 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -23,6 +23,16 @@ describe Email::Receiver do expect { process(:screened_email) }.to raise_error(Email::Receiver::ScreenedEmailError) end + it "raises EmailNotAllowed when email address is not on whitelist" do + SiteSetting.email_domains_whitelist = "example.com|bar.com" + expect { process(:blacklist_whitelist_email) }.to raise_error(Email::Receiver::EmailNotAllowed) + end + + it "raises EmailNotAllowed when email address is on blacklist" do + SiteSetting.email_domains_blacklist = "email.com|mail.com" + expect { process(:blacklist_whitelist_email) }.to raise_error(Email::Receiver::EmailNotAllowed) + end + it "raises an UserNotFoundError when staged users are disabled" do SiteSetting.enable_staged_users = false expect { process(:user_not_found) }.to raise_error(Email::Receiver::UserNotFoundError) @@ -665,6 +675,23 @@ describe Email::Receiver do context "when unsubscribe via email is not allowed" do include_examples "no staged users", :unsubscribe_new_user end + + context "when email address is not on whitelist" do + before do + SiteSetting.email_domains_whitelist = "example.com|bar.com" + end + + include_examples "no staged users", :blacklist_whitelist_email + end + + context "when email address is on blacklist" do + before do + SiteSetting.email_domains_blacklist = "email.com|mail.com" + end + + include_examples "no staged users", :blacklist_whitelist_email + end + end end diff --git a/spec/components/validators/email_validator_spec.rb b/spec/components/validators/email_validator_spec.rb index 5b06cafe57f..71f6bb746ae 100644 --- a/spec/components/validators/email_validator_spec.rb +++ b/spec/components/validators/email_validator_spec.rb @@ -30,6 +30,14 @@ describe EmailValidator do expect(blocks?('sam@e-mail.com')).to eq(true) expect(blocks?('sam@googlemail.com')).to eq(false) end + + it "blocks based on email_domains_whitelist" do + SiteSetting.email_domains_whitelist = "googlemail.com|email.com" + expect(blocks?('sam@email.com')).to eq(false) + expect(blocks?('sam@bob.email.com')).to eq(false) + expect(blocks?('sam@e-mail.com')).to eq(true) + expect(blocks?('sam@googlemail.com')).to eq(false) + end end context '.email_regex' do diff --git a/spec/fixtures/emails/blacklist_whitelist_email.eml b/spec/fixtures/emails/blacklist_whitelist_email.eml new file mode 100644 index 00000000000..f6b38bfcce3 --- /dev/null +++ b/spec/fixtures/emails/blacklist_whitelist_email.eml @@ -0,0 +1,9 @@ +Return-Path: +From: Foo +Date: Fri, 15 Jan 2016 00:12:43 +0100 +Message-ID: <51@foo.bar.mail> +Mime-Version: 1.0 +Content-Type: text/plain +Content-Transfer-Encoding: 7bit + +Email from a domain on blacklist or whitelist.