FIX: Get email address from email_log if bounced with verp

FIX: Get email address from email_log if bounced with verp

We can not access mail.final_recipient attr if it bounced with verp

From 25253dec56ccc84a64a39453e2230b57eb03f7c8 Mon Sep 17 00:00:00 2001
From: Vinoth Kannan <vinothkannan@vinkas.com>
Date: Wed, 28 Nov 2018 19:04:09 +0530
Subject: [PATCH] FIX: Get email address from email_log if bounced with verp

We can not access mail.final_recipient attr if it bounced with verp

diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb
index 0a9e4ef..810afa2 100644
--- a/lib/email/receiver.rb
+++ b/lib/email/receiver.rb
@@ -129,7 +129,7 @@ module Email
       body, elided = select_body
       body ||= ""
 
-      raise NoBodyDetectedError if body.blank? && attachments.empty?
+      raise NoBodyDetectedError if body.blank? && attachments.empty? && !is_bounce?
 
       if is_auto_generated? && !sent_to_mailinglist_mirror?
         @incoming_email.update_columns(is_auto_generated: true)
@@ -202,24 +202,19 @@ module Email
     def handle_bounce
       @incoming_email.update_columns(is_bounce: true)
 
-      if bounce_key && (email_log = EmailLog.find_by(bounce_key: bounce_key))
+      if email_log.present?
         email_log.update_columns(bounced: true)
-        user = email_log.user
-        email = user&.email.presence
         topic = email_log.topic
       end
 
-      email ||= @from_email
-      user ||= @from_user
-
       if @mail.error_status.present? && Array.wrap(@mail.error_status).any? { |s| s.start_with?("4.") }
-        Email::Receiver.update_bounce_score(email, SiteSetting.soft_bounce_score)
+        Email::Receiver.update_bounce_score(@from_email, SiteSetting.soft_bounce_score)
       else
-        Email::Receiver.update_bounce_score(email, SiteSetting.hard_bounce_score)
+        Email::Receiver.update_bounce_score(@from_email, SiteSetting.hard_bounce_score)
       end
 
       return if SiteSetting.enable_whispers? &&
-                user&.staged? &&
+                @from_user&.staged? &&
                 (topic.blank? || topic.archetype == Archetype.private_message)
 
       raise BouncedEmailError
@@ -236,6 +231,10 @@ module Email
       end
     end
 
+    def email_log
+      @email_log ||= EmailLog.find_by(bounce_key: bounce_key)
+    end
+
     def self.update_bounce_score(email, score)
       if user = User.find_by_email(email)
         old_bounce_score = user.user_stat.bounce_score
@@ -457,10 +456,13 @@ module Email
     end
 
     def parse_from_field(mail)
-      if is_bounce?
+      if mail.bounced?
         Array.wrap(mail.final_recipient).each do |from|
           return extract_from_address_and_name(from)
         end
+      elsif email_log.present?
+        email = email_log.user&.email || email_log.to_address
+        return [email, email_log.user&.username]
       end
 
       return unless mail[:from]
diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb
index c178dd6..a6e0225 100644
--- a/spec/components/email/receiver_spec.rb
+++ b/spec/components/email/receiver_spec.rb
@@ -114,6 +114,8 @@ describe Email::Receiver do
 
     describe "creating whisper post in PMs for staged users" do
       let(:email_address) { "linux-admin@b-s-c.co.jp" }
+      let(:user1) { user1 = Fabricate(:user) }
+      let(:user2) { user2 = Fabricate(:staged, email: email_address) }
 
       before do
         SiteSetting.enable_staged_users = true
@@ -121,12 +123,11 @@ describe Email::Receiver do
       end
 
       def create_post_reply_key(value)
-        user = Fabricate(:staged, email: email_address)
-        pm = Fabricate(:topic, archetype: 'private_message', category_id: nil, user: user, allowed_users: [user])
+        pm = Fabricate(:topic, archetype: 'private_message', category_id: nil, user: user1, allowed_users: [user1, user2])
         Fabricate(:post_reply_key,
           reply_key: value,
-          user: user,
-          post: create_post(topic: pm, user: user)
+          user: user2,
+          post: create_post(topic: pm, user: user1)
         )
       end
 
@@ -140,9 +141,11 @@ describe Email::Receiver do
         expect(IncomingEmail.last.is_bounce).to eq(true)
       end
 
-      it "when bounce without verp" do
+      it "when bounce with verp" do
         SiteSetting.reply_by_email_address = "foo+%{reply_key}@discourse.org"
-        create_post_reply_key("14b08c855160d67f2e0c2f8ef36e251e")
+        bounce_key = "14b08c855160d67f2e0c2f8ef36e251e"
+        create_post_reply_key(bounce_key)
+        Fabricate(:email_log, to_address: email_address, user: user2, bounce_key: bounce_key)
 
         expect { process(:hard_bounce_via_verp) }.to raise_error(Email::Receiver::BouncedEmailError)
         post = Post.last
@@ -169,9 +172,9 @@ describe Email::Receiver do
 
     let(:bounce_key) { "14b08c855160d67f2e0c2f8ef36e251e" }
     let(:bounce_key_2) { "b542fb5a9bacda6d28cc061d18e4eb83" }
-    let!(:user) { Fabricate(:user, email: "foo@bar.com") }
-    let!(:email_log) { Fabricate(:email_log, user: user, bounce_key: bounce_key) }
-    let!(:email_log_2) { Fabricate(:email_log, user: user, bounce_key: bounce_key_2) }
+    let!(:user) { Fabricate(:user, email: "linux-admin@b-s-c.co.jp") }
+    let!(:email_log) { Fabricate(:email_log, to_address: user.email, user: user, bounce_key: bounce_key) }
+    let!(:email_log_2) { Fabricate(:email_log, to_address: user.email, user: user, bounce_key: bounce_key_2) }
 
     it "deals with soft bounces" do
       expect { process(:soft_bounce_via_verp) }.to raise_error(Email::Receiver::BouncedEmailError)

GitHub

I’m working on fixing a different bug with VERP handling, but I want to make sure I don’t regress this fix.

@vinothkannans do you have an example of where this was causing a problem? VERP messages should include the final_recipient attribute - it’s a required part of RFC3464 (2.3.2).

-      if is_bounce?
+      if mail.bounced?
         Array.wrap(mail.final_recipient).each do |from|
           return extract_from_address_and_name(from)
         end
+      elsif email_log.present?
+        email = email_log.user&.email || email_log.to_address
+        return [email, email_log.user&.username]
       end

I tried removing the elseif block, and all of our tests still pass. Ideally I need an example of where a VERP bounce would give mail.bounced? = false :thinking:.

2 Likes

Yes, it worked in rspec test cases. But in realtime, sometimes it didn’t include the expected email address in final_recipient array. Maybe I tested it incorrectly :man_shrugging:. It looks like later I added another commit too DEV: create bounce alert earlier if email_log detected from bounce_key · discourse/discourse@bfb3c4d · GitHub.

The above code explains that a bounced email can have the false value for mail.bounced?.

2 Likes