From f26b8b448dbaea6c0886d13b25f879114f52c954 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 8 Dec 2021 08:14:48 +1000 Subject: [PATCH] FIX: References header leading to broken email threading (#15206) Since 3b13f1146b2a406238c50d6b45bc9aa721094f46 the email threading in mail clients has been broken, because the random suffix meant that the References header would always be different for non-group SMTP email notifications sent out. This commit fixes the issue by always using the "canonical" topic reference ID inside the References header in the format: topic/TOPIC_ID@HOST Which was the old format. We also add the References header to notifications sent for the first post arriving, so the threading works for subsequent emails. The Message-ID header is still random as per the previous change. --- lib/email/message_id_service.rb | 8 +++++-- lib/email/sender.rb | 36 +++++++++++++++++++++------- spec/components/email/sender_spec.rb | 27 ++++++++++++++------- spec/lib/message_id_service_spec.rb | 10 ++++++-- 4 files changed, 60 insertions(+), 21 deletions(-) diff --git a/lib/email/message_id_service.rb b/lib/email/message_id_service.rb index 744d51990ff..a3ba96424ae 100644 --- a/lib/email/message_id_service.rb +++ b/lib/email/message_id_service.rb @@ -37,14 +37,18 @@ module Email "" end - def generate_for_topic(topic, use_incoming_email_if_present: false) + def generate_for_topic(topic, use_incoming_email_if_present: false, canonical: false) first_post = topic.ordered_posts.first if use_incoming_email_if_present && first_post.incoming_email&.message_id.present? return "<#{first_post.incoming_email.message_id}>" end - "" + if canonical + "" + else + "" + end end def find_post_from_message_ids(message_ids) diff --git a/lib/email/sender.rb b/lib/email/sender.rb index 14a538b4c6d..e91adecd793 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -122,8 +122,20 @@ module Email add_attachments(post) - topic_message_id = Email::MessageIdService.generate_for_topic(topic, use_incoming_email_if_present: true) - post_message_id = Email::MessageIdService.generate_for_post(post, use_incoming_email_if_present: true) + # If the topic was created from an incoming email, then the Message-ID from + # that email will be the canonical reference, otherwise the canonical reference + # will be . The canonical reference is used in the + # References header. + # + # This is so the sender of the original email still gets their nice threading + # maintained (because their mail client will initiate threading based on + # the Message-ID it generated) in the case where there is an incoming email. + # + # In the latter case, everyone will start their thread with the canonical reference, + # because we send it in the References header for all emails. + topic_canonical_reference_id = Email::MessageIdService.generate_for_topic( + topic, canonical: true, use_incoming_email_if_present: true + ) referenced_posts = Post.includes(:incoming_email) .joins("INNER JOIN post_replies ON post_replies.post_id = posts.id ") @@ -135,23 +147,29 @@ module Email "<#{referenced_post.incoming_email.message_id}>" else if referenced_post.post_number == 1 - Email::MessageIdService.generate_for_topic(topic) + topic_canonical_reference_id else Email::MessageIdService.generate_for_post(referenced_post) end end end - # https://www.ietf.org/rfc/rfc2822.txt + # See https://www.ietf.org/rfc/rfc2822.txt for the message format + # specification, more useful information can be found in Email::MessageIdService + # + # The References header is how mail clients handle threading. The Message-ID + # must always be unique. if post.post_number == 1 - @message.header['Message-ID'] = topic_message_id + @message.header['Message-ID'] = Email::MessageIdService.generate_for_topic(topic) + @message.header['References'] = [topic_canonical_reference_id] else - @message.header['Message-ID'] = post_message_id - @message.header['In-Reply-To'] = referenced_post_message_ids[0] || topic_message_id - @message.header['References'] = [topic_message_id, referenced_post_message_ids].flatten.compact.uniq + @message.header['Message-ID'] = Email::MessageIdService.generate_for_post(post) + @message.header['In-Reply-To'] = referenced_post_message_ids[0] || topic_canonical_reference_id + @message.header['References'] = [topic_canonical_reference_id, referenced_post_message_ids].flatten.compact.uniq end - # https://www.ietf.org/rfc/rfc2919.txt + # See https://www.ietf.org/rfc/rfc2919.txt for the List-ID + # specification. if topic&.category && !topic.category.uncategorized? list_id = "#{SiteSetting.title} | #{topic.category.name} <#{topic.category.name.downcase.tr(' ', '-')}.#{host}>" diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index a2ab5f6c519..940326a1a4b 100644 --- a/spec/components/email/sender_spec.rb +++ b/spec/components/email/sender_spec.rb @@ -277,23 +277,34 @@ describe Email::Sender do Email::MessageIdService.stubs(:random_suffix).returns(random_message_id_suffix) end - it "doesn't set the 'In-Reply-To' and 'References' headers on the first post" do + it "doesn't set the 'In-Reply-To' header but does set the 'References' header on the first post" do message.header['X-Discourse-Post-Id'] = post_1.id email_sender.send expect(message.header['Message-Id'].to_s).to eq("") expect(message.header['In-Reply-To'].to_s).to be_blank - expect(message.header['References'].to_s).to be_blank + expect(message.header['References'].to_s).to eq("") end - it "sets the 'In-Reply-To' header to the topic by default" do + it "sets the 'References' header with the incoming email Message-ID if it exists on the first post" do + incoming = Fabricate(:incoming_email, topic: topic, post: post_1, message_id: "blah1234@someemailprovider.com") + message.header['X-Discourse-Post-Id'] = post_1.id + + email_sender.send + + expect(message.header['Message-Id'].to_s).to eq("") + expect(message.header['In-Reply-To'].to_s).to be_blank + expect(message.header['References'].to_s).to eq("") + end + + it "sets the 'In-Reply-To' header to the topic canonical reference by default" do message.header['X-Discourse-Post-Id'] = post_2.id email_sender.send expect(message.header['Message-Id'].to_s).to eq("") - expect(message.header['In-Reply-To'].to_s).to eq("") + expect(message.header['In-Reply-To'].to_s).to eq("") end it "sets the 'In-Reply-To' header to the newest replied post" do @@ -305,13 +316,13 @@ describe Email::Sender do expect(message.header['In-Reply-To'].to_s).to eq("") end - it "sets the 'References' header to the topic and all replied posts" do + it "sets the 'References' header to the topic canonical reference and all replied posts" do message.header['X-Discourse-Post-Id'] = post_4.id email_sender.send references = [ - "", + "", "", "", ] @@ -319,7 +330,7 @@ describe Email::Sender do expect(message.header['References'].to_s).to eq(references.join(" ")) end - it "uses the incoming_email message_id when available" do + it "uses the incoming_email message_id when available, but always uses a random message-id" do topic_incoming_email = IncomingEmail.create(topic: topic, post: post_1, message_id: "foo@bar") post_2_incoming_email = IncomingEmail.create(topic: topic, post: post_2, message_id: "bar@foo") post_4_incoming_email = IncomingEmail.create(topic: topic, post: post_4, message_id: "wat@wat") @@ -328,7 +339,7 @@ describe Email::Sender do email_sender.send - expect(message.header['Message-Id'].to_s).to eq("<#{post_4_incoming_email.message_id}>") + expect(message.header['Message-Id'].to_s).to eq("") references = [ "<#{topic_incoming_email.message_id}>", diff --git a/spec/lib/message_id_service_spec.rb b/spec/lib/message_id_service_spec.rb index 151b55bff63..65077ca5818 100644 --- a/spec/lib/message_id_service_spec.rb +++ b/spec/lib/message_id_service_spec.rb @@ -24,15 +24,21 @@ describe Email::MessageIdService do describe "#generate_for_topic" do it "generates for the topic using the message_id on the first post's incoming_email" do - Fabricate(:incoming_email, message_id: "test@test.localhost", post: post) + Fabricate(:incoming_email, message_id: "test213428@somemailservice.com", post: post) post.reload - expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to eq("") + expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to eq("") end it "generates for the topic without an incoming_email record" do expect(subject.generate_for_topic(topic)).to match(subject.message_id_topic_id_regexp) expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to match(subject.message_id_topic_id_regexp) end + + it "generates canonical for the topic" do + canonical_topic_id = subject.generate_for_topic(topic, canonical: true) + expect(canonical_topic_id).to match(subject.message_id_topic_id_regexp) + expect(canonical_topic_id).to eq("") + end end describe "find_post_from_message_ids" do