From 5734c7f3f36a4f1afe3289042b20e955cd745994 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 7 Apr 2016 22:21:17 +0800 Subject: [PATCH] FIX: Don't send rejection mailer to bounced emails. --- app/jobs/scheduled/poll_mailbox.rb | 38 ++++++++++++++++-- config/locales/server.en.yml | 2 + lib/email/message_builder.rb | 9 ++++- lib/email/receiver.rb | 18 +++++++++ spec/components/email/message_builder_spec.rb | 19 +++++++-- spec/components/email/receiver_spec.rb | 15 +++++-- spec/fixtures/emails/bounced_email.eml | 39 +++++++++++++++++++ spec/fixtures/emails/bounced_email_2.eml | 29 ++++++++++++++ spec/jobs/poll_mailbox_spec.rb | 28 +++++++++++++ spec/support/helpers.rb | 4 ++ 10 files changed, 188 insertions(+), 13 deletions(-) create mode 100644 spec/fixtures/emails/bounced_email.eml create mode 100644 spec/fixtures/emails/bounced_email_2.eml diff --git a/app/jobs/scheduled/poll_mailbox.rb b/app/jobs/scheduled/poll_mailbox.rb index 320b98588..331a7a644 100644 --- a/app/jobs/scheduled/poll_mailbox.rb +++ b/app/jobs/scheduled/poll_mailbox.rb @@ -25,17 +25,31 @@ module Jobs mail_string = popmail.pop receiver = Email::Receiver.new(mail_string) receiver.process! + rescue Email::Receiver::BouncedEmailError => e + log_email_process_failure(mail_string, e) + + set_incoming_email_rejection_message( + receiver.incoming_email, I18n.t("email.incoming.errors.bounced_email_report") + ) + rescue Email::Receiver::AutoGeneratedEmailReplyError => e + log_email_process_failure(mail_string, e) + + set_incoming_email_rejection_message( + receiver.incoming_email, + I18n.t("email.incoming.errors.auto_generated_email_reply") + ) rescue => e rejection_message = handle_failure(mail_string, e) - if rejection_message.present? && receiver && receiver.incoming_email - receiver.incoming_email.rejection_message = rejection_message.body.to_s - receiver.incoming_email.save + if rejection_message.present? && receiver && (incoming_email = receiver.incoming_email) + set_incoming_email_rejection_message( + incoming_email, rejection_message.body.to_s + ) end end end def handle_failure(mail_string, e) - Rails.logger.warn("Email can not be processed: #{e}\n\n#{mail_string}") if SiteSetting.log_mail_processing_failures + log_email_process_failure(mail_string, e) message_template = case e when Email::Receiver::EmptyEmailError then :email_reject_empty @@ -70,6 +84,10 @@ module Jobs template_args[:rate_limit_description] = e.description end + if message_template == :email_reject_auto_generated + template_args[:mark_as_reply_to_auto_generated] = true + end + if message_template # inform the user about the rejection message = Mail::Message.new(mail_string) @@ -116,5 +134,17 @@ module Jobs $redis.zadd(POLL_MAILBOX_ERRORS_KEY, now, now.to_s) end + private + + def set_incoming_email_rejection_message(incoming_email, message) + incoming_email.update_attributes!(rejection_message: message) + end + + def log_email_process_failure(mail_string, exception) + if SiteSetting.log_mail_processing_failures + Rails.logger.warn("Email can not be processed: #{exception}\n\n#{mail_string}") + end + end + end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 8cde59cb7..6fcf56838 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -64,6 +64,8 @@ en: reply_user_not_matching_error: "Happens when a reply came in from a different email address the notification was sent to." topic_not_found_error: "Happens when a reply came in but the related topic has been deleted." topic_closed_error: "Happens when a reply came in but the related topic has been closed." + bounced_email_report: "Email is a bounced email report." + auto_generated_email_reply: "Email contains a reply to an auto generated email." errors: &errors format: ! '%{attribute} %{message}' diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index 991620e3a..911321afd 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -17,6 +17,9 @@ module Email class MessageBuilder attr_reader :template_args + REPLY_TO_AUTO_GENERATED_HEADER_KEY = "X-Discourse-Reply-to-Auto-Generated".freeze + REPLY_TO_AUTO_GENERATED_HEADER_VALUE = "marked".freeze + def initialize(to, opts=nil) @to = to @opts = opts || {} @@ -132,7 +135,11 @@ module Email def header_args result = {} if @opts[:add_unsubscribe_link] - result['List-Unsubscribe'] = "<#{template_args[:user_preferences_url]}>" if @opts[:add_unsubscribe_link] + result['List-Unsubscribe'] = "<#{template_args[:user_preferences_url]}>" + end + + if @opts[:mark_as_reply_to_auto_generated] + result[REPLY_TO_AUTO_GENERATED_HEADER_KEY] = REPLY_TO_AUTO_GENERATED_HEADER_VALUE end result['X-Discourse-Post-Id'] = @opts[:post_id].to_s if @opts[:post_id] diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index f742733c6..152237dd2 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -12,6 +12,8 @@ module Email class EmptyEmailError < ProcessingError; end class UserNotFoundError < ProcessingError; end class AutoGeneratedEmailError < ProcessingError; end + class AutoGeneratedEmailReplyError < ProcessingError; end + class BouncedEmailError < ProcessingError; end class NoBodyDetectedError < ProcessingError; end class InactiveUserError < ProcessingError; end class BlockedUserError < ProcessingError; end @@ -62,6 +64,8 @@ module Email body, @elided = select_body body ||= "" + raise BouncedEmailError if (@mail.bounced? && !@mail.retryable?) + raise AutoGeneratedEmailReplyError if check_reply_to_auto_generated_header raise AutoGeneratedEmailError if is_auto_generated? raise NoBodyDetectedError if body.blank? && !@mail.has_attachments? raise InactiveUserError if !user.active && !user.staged @@ -422,6 +426,20 @@ module Email !topic.topic_allowed_groups.where("group_id IN (SELECT group_id FROM group_users WHERE user_id = ?)", user.id).exists? end + private + + def check_reply_to_auto_generated_header + headers = Mail::Header.new(@mail.body.to_s.gsub("\r\n\r\n", "\r\n")).to_a + + index = headers.find_index do |f| + f.name == Email::MessageBuilder::REPLY_TO_AUTO_GENERATED_HEADER_KEY + end + + if index + headers[index].value == Email::MessageBuilder::REPLY_TO_AUTO_GENERATED_HEADER_VALUE + end + end + end end diff --git a/spec/components/email/message_builder_spec.rb b/spec/components/email/message_builder_spec.rb index 05a4abd92..15a0a8e79 100644 --- a/spec/components/email/message_builder_spec.rb +++ b/spec/components/email/message_builder_spec.rb @@ -136,10 +136,15 @@ describe Email::MessageBuilder do context "header args" do - let(:message_with_header_args) { Email::MessageBuilder.new(to_address, - body: 'hello world', - topic_id: 1234, - post_id: 4567) } + let(:message_with_header_args) do + Email::MessageBuilder.new( + to_address, + body: 'hello world', + topic_id: 1234, + post_id: 4567, + mark_as_reply_to_auto_generated: true + ) + end it "passes through a post_id" do expect(message_with_header_args.header_args['X-Discourse-Post-Id']).to eq('4567') @@ -149,6 +154,12 @@ describe Email::MessageBuilder do expect(message_with_header_args.header_args['X-Discourse-Topic-Id']).to eq('1234') end + it "marks the email as replying to an auto generated email" do + expect(message_with_header_args.header_args[ + Email::MessageBuilder::REPLY_TO_AUTO_GENERATED_HEADER_KEY + ]).to eq(Email::MessageBuilder::REPLY_TO_AUTO_GENERATED_HEADER_VALUE) + end + end context "unsubscribe link" do diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 01afbb6f8..84e7d1001 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -8,10 +8,6 @@ describe Email::Receiver do SiteSetting.reply_by_email_address = "reply+%{reply_key}@bar.com" end - def email(email_name) - fixture_file("emails/#{email_name}.eml") - end - def process(email_name) Email::Receiver.new(email(email_name)).process! end @@ -54,6 +50,17 @@ describe Email::Receiver do expect { process(:bad_destinations) }.to raise_error(Email::Receiver::BadDestinationAddress) end + it "raises an BouncerEmailError when email is a bounced email" do + expect { process(:bounced_email) }.to raise_error(Email::Receiver::BouncedEmailError) + end + + it "raises an AutoGeneratedEmailReplyError when email contains a reply marked + as reply to an auto generated email".squish do + + expect { process(:bounced_email_2) } + .to raise_error(Email::Receiver::AutoGeneratedEmailReplyError) + end + context "reply" do let(:reply_key) { "4f97315cc828096c9cb34c6f1a0d6fe8" } diff --git a/spec/fixtures/emails/bounced_email.eml b/spec/fixtures/emails/bounced_email.eml new file mode 100644 index 000000000..536588153 --- /dev/null +++ b/spec/fixtures/emails/bounced_email.eml @@ -0,0 +1,39 @@ +Delivered-To: someguy@discourse.org +Date: Thu, 7 Apr 2016 19:04:30 +0900 (JST) +From: MAILER-DAEMON@b-s-c.co.jp (Mail Delivery System) +Subject: Undelivered Mail Returned to Sender +To: someguy@discourse.org +MIME-Version: 1.0 +Content-Type: multipart/report; report-type=delivery-status; + boundary="18F5D18A0075.1460023470/some@daemon.com" + +This is a MIME-encapsulated message. + +--18F5D18A0075.1460023470/some@daemon.com +Content-Description: Notification +Content-Type: text/plain; charset=us-ascii + +Your email bounced + +--18F5D18A0075.1460023470/some@daemon.com +Content-Description: Delivery report +Content-Type: message/delivery-status + +Final-Recipient: rfc822; linux-admin@b-s-c.co.jp +Original-Recipient: rfc822;linux-admin@b-s-c.co.jp +Action: failed +Status: 5.1.1 +Diagnostic-Code: X-Postfix; unknown user: "linux-admin" + +--18F5D18A0075.1460023470/some@daemon.com +Content-Description: Undelivered Message +Content-Type: message/rfc822 + +Return-Path: +Date: Thu, 07 Apr 2016 03:04:28 -0700 (PDT) +From: someguy@discourse.org +X-Discourse-Auto-Generated: marked + +This is the body + +--18F5D18A0075.1460023470/some@daemon.com-- diff --git a/spec/fixtures/emails/bounced_email_2.eml b/spec/fixtures/emails/bounced_email_2.eml new file mode 100644 index 000000000..d17ba08ff --- /dev/null +++ b/spec/fixtures/emails/bounced_email_2.eml @@ -0,0 +1,29 @@ +Delivered-To: someguy@discourse.org +Date: Thu, 7 Apr 2016 19:04:30 +0900 (JST) +From: MAILER-DAEMON@b-s-c.co.jp (Mail Delivery System) +Subject: Undelivered Mail Returned to Sender +To: someguy@discourse.org +MIME-Version: 1.0 +Content-Type: multipart/report; report-type=delivery-status; + boundary="18F5D18A0075.1460023470/some@daemon.com" + +This is a MIME-encapsulated message. + +--18F5D18A0075.1460023470/some@daemon.com +Content-Description: Notification +Content-Type: text/plain; charset=us-ascii + +Your email bounced + +--18F5D18A0075.1460023470/some@daemon.com +Content-Description: Undelivered Message +Content-Type: message/rfc822 + +Return-Path: +Date: Thu, 07 Apr 2016 03:04:28 -0700 (PDT) +From: someguy@discourse.org +X-Discourse-Reply-to-Auto-Generated: marked + +This is the body + +--18F5D18A0075.1460023470/some@daemon.com-- diff --git a/spec/jobs/poll_mailbox_spec.rb b/spec/jobs/poll_mailbox_spec.rb index 451cf07e3..a695be614 100644 --- a/spec/jobs/poll_mailbox_spec.rb +++ b/spec/jobs/poll_mailbox_spec.rb @@ -44,4 +44,32 @@ describe Jobs::PollMailbox do end end + describe "#process_popmail" do + def process_popmail(email_name) + pop_mail = stub("pop mail") + pop_mail.expects(:pop).returns(email(email_name)) + Jobs::PollMailbox.new.process_popmail(pop_mail) + end + + it "does not reply to a bounced email" do + expect { process_popmail(:bounced_email) }.to_not change { ActionMailer::Base.deliveries.count } + + incoming_email = IncomingEmail.last + + expect(incoming_email.rejection_message).to eq( + I18n.t("email.incoming.errors.bounced_email_report") + ) + end + + it "does not reply to an email containing a reply to an auto generated email" do + expect { process_popmail(:bounced_email_2) }.to_not change { ActionMailer::Base.deliveries.count } + + incoming_email = IncomingEmail.last + + expect(incoming_email.rejection_message).to eq( + I18n.t("email.incoming.errors.auto_generated_email_reply") + ) + end + end + end diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb index 020a240c5..39457073a 100644 --- a/spec/support/helpers.rb +++ b/spec/support/helpers.rb @@ -79,4 +79,8 @@ module Helpers result end + def email(email_name) + fixture_file("emails/#{email_name}.eml") + end + end