FIX: Handle multiple In-Reply-To Message-ID in group inbox (#29912)

This fix handles the case where an In-Reply-To mail header
can contain multiple Message-IDs. We use this header to
try look up an EmailLog record to find the post to reply
to in the group email inbox flow.

Since the case where multiple In-Reply-To Message-IDs is
rare (we've only seen a couple of instances of this causing
errors in the wild), we are just going to use the first one
in the array.

Also, Discourse does not support replying to multiple posts
at once, so it doesn't really make sense to use multiple
In-Reply-To Message-IDs anyway.

lib/email/receiver.rb

@@ -927,23 +927,27 @@ module Email
     def create_group_post(group, user, body, elided)
       message_ids = Email::Receiver.extract_reply_message_ids(@mail, max_message_id_count: 5)
 
-      # incoming emails with matching message ids, and then cross references
+      # Incoming emails with matching message ids, and then cross references
       # these with any email addresses for the user vs to/from/cc of the
       # incoming emails. in effect, any incoming email record for these
-      # message ids where the user is involved in any way will be returned
+      # message ids where the user is involved in any way will be returned.
       incoming_emails = IncomingEmail.where(message_id: message_ids)
       if !group.allow_unknown_sender_topic_replies
         incoming_emails = incoming_emails.addressed_to_user(user)
       end
       post_ids = incoming_emails.pluck(:post_id) || []
 
-      # if the user is directly replying to an email send to them from discourse,
+      # If the user is directly replying to an email send to them from discourse,
       # there will be a corresponding EmailLog record, so we can use that as the
-      # reply post if it exists
-      if Email::MessageIdService.discourse_generated_message_id?(mail.in_reply_to)
+      # reply post if it exists.
+      #
+      # Since In-Reply-To can technically have multiple message ids, we only
+      # consider the first one here to simplify things.
+      first_in_reply_to = Array.wrap(mail.in_reply_to).first
+      if Email::MessageIdService.discourse_generated_message_id?(first_in_reply_to)
         post_id_from_email_log =
           EmailLog
-            .where(message_id: mail.in_reply_to)
+            .where(message_id: first_in_reply_to)
             .addressed_to_user(user)
             .order(created_at: :desc)
             .limit(1)

spec/fixtures/emails/email_to_group_email_username_3.eml

@@ -0,0 +1,14 @@
+Return-Path: <two@foo.com>
+From: Two <two@foo.com>
+To: team@somesmtpaddress.com
+Subject: Full email group username flow
+Date: Saturday, 16 Jan 2016 00:12:43 +0100
+Message-ID: <348ct38794nyt9338dsfsd@foo.bar.mail>
+In-Reply-To: MESSAGE_ID_REPLY_TO
+References: <u4w8c9r4y984yh98r3h69873@foo.bar.mail>
+  <MESSAGE_ID_REPLY_TO>
+Mime-Version: 1.0
+Content-Type: text/plain
+Content-Transfer-Encoding: 7bit
+
+Hey thanks for the suggestion, it didn't work.

spec/lib/email/receiver_spec.rb

@@ -218,9 +218,7 @@ RSpec.describe Email::Receiver do
   end
 
   it "strips null bytes from the subject" do
-    expect do process(:null_byte_in_subject) end.to raise_error(
-      Email::Receiver::BadDestinationAddress,
-    )
+    expect { process(:null_byte_in_subject) }.to raise_error(Email::Receiver::BadDestinationAddress)
   end
 
   describe "bounces to VERP" do
@@ -1366,13 +1364,29 @@ RSpec.describe Email::Receiver do
         expect(IncomingEmail.exists?(post_id: group_post.id)).to eq(false)
       end
 
-      it "processes a reply from the OP user to the group SMTP username, linking the reply_to_post_number correctly by
-      matching in_reply_to to the email log" do
+      it "processes a reply from the OP user to the group SMTP username, linking the reply_to_post_number correctly by matching in_reply_to to the email log" do
         email_log, group_post = reply_as_group_user
 
         reply_email = email(:email_to_group_email_username_2)
         reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
-        expect do Email::Receiver.new(reply_email).process! end.to not_change {
+        expect { Email::Receiver.new(reply_email).process! }.to not_change {
+          Topic.count
+        }.and change { Post.count }.by(1)
+
+        reply_post = Post.last
+        expect(reply_post.reply_to_user).to eq(user_in_group)
+        expect(reply_post.reply_to_post_number).to eq(group_post.post_number)
+      end
+
+      it "handles multiple message IDs in the in_reply_to header by only using the first one" do
+        email_log, group_post = reply_as_group_user
+
+        reply_email = email(:email_to_group_email_username_3)
+        reply_email.gsub!(
+          "MESSAGE_ID_REPLY_TO",
+          "<#{email_log.message_id}> <test/message/id@discourse.com>",
+        )
+        expect { Email::Receiver.new(reply_email).process! }.to not_change {
           Topic.count
         }.and change { Post.count }.by(1)
 
@@ -1386,7 +1400,7 @@ RSpec.describe Email::Receiver do
 
         reply_email = email(:email_to_group_email_username_2_as_unknown_sender)
         reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
-        expect do Email::Receiver.new(reply_email).process! end.to change { Topic.count }.by(
+        expect { Email::Receiver.new(reply_email).process! }.to change { Topic.count }.by(
           1,
         ).and change { Post.count }.by(1)
 
@@ -1400,7 +1414,7 @@ RSpec.describe Email::Receiver do
 
         reply_email = email(:email_to_group_email_username_2_as_unknown_sender)
         reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
-        expect do Email::Receiver.new(reply_email).process! end.to not_change {
+        expect { Email::Receiver.new(reply_email).process! }.to not_change {
           Topic.count
         }.and change { Post.count }.by(1)
 
@@ -1417,7 +1431,7 @@ RSpec.describe Email::Receiver do
 
         reply_email = email(:email_to_group_email_username_2)
         reply_email.gsub!("MESSAGE_ID_REPLY_TO", email_log.message_id)
-        expect do Email::Receiver.new(reply_email).process! end.to change { Topic.count }.by(
+        expect { Email::Receiver.new(reply_email).process! }.to change { Topic.count }.by(
           1,
         ).and change { Post.count }.by(1)