From 214f5bff5cd6ee8260bf294e244fff899049beb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Tue, 28 Jun 2016 16:42:05 +0200 Subject: [PATCH] don't send more than 1 reply per day to auto-generated emails --- config/locales/server.en.yml | 1 - lib/email/message_builder.rb | 7 ---- lib/email/processor.rb | 33 +++++++++++------- lib/email/receiver.rb | 34 +++++-------------- spec/components/email/message_builder_spec.rb | 7 ---- spec/components/email/receiver_spec.rb | 4 --- spec/fixtures/emails/bounced_email_2.eml | 29 ---------------- spec/jobs/poll_mailbox_spec.rb | 9 ----- 8 files changed, 30 insertions(+), 94 deletions(-) delete mode 100644 spec/fixtures/emails/bounced_email_2.eml diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index b7c74a51d66..c7ab81feb01 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -73,7 +73,6 @@ en: 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_error: "Email is a bounced email report." - auto_generated_email_reply: "Email contains a reply to an auto generated email." screened_email_error: "Happens when the sender's email address was already screened." errors: &errors diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index f099b11f1ee..79fe8a46b45 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -17,9 +17,6 @@ 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 || {} @@ -138,10 +135,6 @@ module Email 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] result['X-Discourse-Topic-Id'] = @opts[:topic_id].to_s if @opts[:topic_id] diff --git a/lib/email/processor.rb b/lib/email/processor.rb index 30dcef9275f..4287eab81dd 100644 --- a/lib/email/processor.rb +++ b/lib/email/processor.rb @@ -15,16 +15,14 @@ module Email receiver = Email::Receiver.new(@mail) receiver.process! rescue Email::Receiver::BouncedEmailError => e + # never reply to bounced emails log_email_process_failure(@mail, e) set_incoming_email_rejection_message(receiver.incoming_email, I18n.t("emails.incoming.errors.bounced_email_error")) - rescue Email::Receiver::AutoGeneratedEmailReplyError => e - log_email_process_failure(@mail, e) - set_incoming_email_rejection_message(receiver.incoming_email, I18n.t("emails.incoming.errors.auto_generated_email_reply")) rescue => e log_email_process_failure(@mail, e) - - rejection_message = handle_failure(@mail, e) - if rejection_message.present? && receiver && (incoming_email = receiver.incoming_email) + incoming_email = receiver.try(:incoming_email) + rejection_message = handle_failure(@mail, e, incoming_email) + if rejection_message.present? set_incoming_email_rejection_message(incoming_email, rejection_message.body.to_s) end end @@ -32,7 +30,7 @@ module Email private - def handle_failure(mail_string, e) + def handle_failure(mail_string, e, incoming_email) message_template = case e when Email::Receiver::EmptyEmailError then :email_reject_empty when Email::Receiver::NoBodyDetectedError then :email_reject_empty @@ -67,10 +65,6 @@ module Email 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) @@ -79,7 +73,11 @@ module Email template_args[:site_name] = SiteSetting.title client_message = RejectionMailer.send_rejection(message_template, message.from, template_args) - Email::Sender.new(client_message, message_template).send + + # don't send more than 1 reply per day to auto-generated emails + if !incoming_email.try(:is_auto_generated) || can_reply_to_auto_generated?(message.from) + Email::Sender.new(client_message, message_template).send + end else Rails.logger.error("Unrecognized error type (#{e}) when processing incoming email\n\nMail:\n#{mail_string}") end @@ -87,6 +85,17 @@ module Email client_message end + def can_reply_to_auto_generated?(email) + key = "auto_generated_reply:#{email}:#{Date.today}" + + if $redis.setnx(key, "1") + $redis.expire(key, 25.hours) + true + else + false + end + end + def set_incoming_email_rejection_message(incoming_email, message) incoming_email.update_attributes!(rejection_message: message) end diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 604d0220fe0..583c762cd09 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -13,7 +13,6 @@ module Email class ScreenedEmailError < 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 @@ -82,8 +81,7 @@ module Email if is_auto_generated? @incoming_email.update_columns(is_auto_generated: true) - raise AutoGeneratedEmailReplyError if check_reply_to_auto_generated_header - raise AutoGeneratedEmailError if SiteSetting.block_auto_generated_emails? + raise AutoGeneratedEmailError if SiteSetting.block_auto_generated_emails? end if action = subscription_action_for(body, subject) @@ -151,20 +149,20 @@ module Email if bounce_key && (email_log = EmailLog.find_by(bounce_key: bounce_key)) email_log.update_columns(bounced: true) email = email_log.user.try(:email) || @from_email - if email.present? - if @mail.error_status.present? - if @mail.error_status.start_with?("4.") - Email::Receiver.update_bounce_score(email, SOFT_BOUNCE_SCORE) - elsif @mail.error_status.start_with?("5.") - Email::Receiver.update_bounce_score(email, HARD_BOUNCE_SCORE) - end - elsif is_auto_generated? + if email.present? && @mail.error_status.present? + if @mail.error_status.start_with?("4.") + Email::Receiver.update_bounce_score(email, SOFT_BOUNCE_SCORE) + else @mail.error_status.start_with?("5.") Email::Receiver.update_bounce_score(email, HARD_BOUNCE_SCORE) end end end end + if is_auto_generated? + Email::Receiver.update_bounce_score(@from_email, SOFT_BOUNCE_SCORE) + end + true end @@ -534,20 +532,6 @@ 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 e63e704e20d..e5baacde569 100644 --- a/spec/components/email/message_builder_spec.rb +++ b/spec/components/email/message_builder_spec.rb @@ -142,7 +142,6 @@ describe Email::MessageBuilder do body: 'hello world', topic_id: 1234, post_id: 4567, - mark_as_reply_to_auto_generated: true ) end @@ -154,12 +153,6 @@ 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 bd60b249b3b..573719437f0 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -61,10 +61,6 @@ describe Email::Receiver do expect(IncomingEmail.last.is_bounce).to eq(true) end - it "raises an AutoGeneratedEmailReplyError when email contains a marked reply" do - expect { process(:bounced_email_2) }.to raise_error(Email::Receiver::AutoGeneratedEmailReplyError) - end - context "bounces to VERP" do let(:bounce_key) { "14b08c855160d67f2e0c2f8ef36e251e" } diff --git a/spec/fixtures/emails/bounced_email_2.eml b/spec/fixtures/emails/bounced_email_2.eml deleted file mode 100644 index d17ba08ffdf..00000000000 --- a/spec/fixtures/emails/bounced_email_2.eml +++ /dev/null @@ -1,29 +0,0 @@ -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 10566133ea1..b6856bcdb33 100644 --- a/spec/jobs/poll_mailbox_spec.rb +++ b/spec/jobs/poll_mailbox_spec.rb @@ -90,15 +90,6 @@ describe Jobs::PollMailbox do ) 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("emails.incoming.errors.auto_generated_email_reply") - ) - end end end