From b8a5f95eb640ca03f8b05b68abc8a97f6ef48e13 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 26 Nov 2024 11:12:40 +1000 Subject: [PATCH] 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 | 16 ++++++---- .../email_to_group_email_username_3.eml | 14 ++++++++ spec/lib/email/receiver_spec.rb | 32 +++++++++++++------ 3 files changed, 47 insertions(+), 15 deletions(-) create mode 100644 spec/fixtures/emails/email_to_group_email_username_3.eml diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index 645722c8324..fce8c76b82e 100644 --- a/lib/email/receiver.rb +++ b/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) diff --git a/spec/fixtures/emails/email_to_group_email_username_3.eml b/spec/fixtures/emails/email_to_group_email_username_3.eml new file mode 100644 index 00000000000..d445aa5b17c --- /dev/null +++ b/spec/fixtures/emails/email_to_group_email_username_3.eml @@ -0,0 +1,14 @@ +Return-Path: +From: Two +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: + +Mime-Version: 1.0 +Content-Type: text/plain +Content-Transfer-Encoding: 7bit + +Hey thanks for the suggestion, it didn't work. diff --git a/spec/lib/email/receiver_spec.rb b/spec/lib/email/receiver_spec.rb index db28035f47a..bb5d553d02d 100644 --- a/spec/lib/email/receiver_spec.rb +++ b/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}> ", + ) + 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)