FIX: References header leading to broken email threading (#15206)

Since 3b13f1146b 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

@@ -37,14 +37,18 @@ module Email
         "<topic/#{post.topic_id}/#{post.id}.#{random_suffix}@#{host}>"
       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
 
-        "<topic/#{topic.id}.#{random_suffix}@#{host}>"
+        if canonical
+          "<topic/#{topic.id}@#{host}>"
+        else
+          "<topic/#{topic.id}.#{random_suffix}@#{host}>"
+        end
       end
 
       def find_post_from_message_ids(message_ids)

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)
+        # If the topic was created from an incoming email, then the Message-ID from
-        post_message_id = Email::MessageIdService.generate_for_post(post, use_incoming_email_if_present: true)
+        # that email will be the canonical reference, otherwise the canonical reference
+        # will be <topic/TOPIC_ID@host>. 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['Message-ID']  = Email::MessageIdService.generate_for_post(post)
-          @message.header['In-Reply-To'] = referenced_post_message_ids[0] || topic_message_id
+          @message.header['In-Reply-To'] = referenced_post_message_ids[0] || topic_canonical_reference_id
-          @message.header['References']  = [topic_message_id, referenced_post_message_ids].flatten.compact.uniq
+          @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}>"
 

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("<topic/#{topic.id}.#{random_message_id_suffix}@test.localhost>")
         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("<topic/#{topic.id}@test.localhost>")
       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("<topic/#{topic.id}.#{random_message_id_suffix}@test.localhost>")
+        expect(message.header['In-Reply-To'].to_s).to be_blank
+        expect(message.header['References'].to_s).to eq("<blah1234@someemailprovider.com>")
+      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("<topic/#{topic.id}/#{post_2.id}.#{random_message_id_suffix}@test.localhost>")
-        expect(message.header['In-Reply-To'].to_s).to eq("<topic/#{topic.id}.#{random_message_id_suffix}@test.localhost>")
+        expect(message.header['In-Reply-To'].to_s).to eq("<topic/#{topic.id}@test.localhost>")
       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("<topic/#{topic.id}/#{post_3.id}.#{random_message_id_suffix}@test.localhost>")
       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 = [
-          "<topic/#{topic.id}.#{random_message_id_suffix}@test.localhost>",
+          "<topic/#{topic.id}@test.localhost>",
           "<topic/#{topic.id}/#{post_3.id}.#{random_message_id_suffix}@test.localhost>",
           "<topic/#{topic.id}/#{post_2.id}.#{random_message_id_suffix}@test.localhost>",
         ]
@@ -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("<topic/#{topic.id}/#{post_4.id}.5f1330cfd941f323d7f99b9e@test.localhost>")
 
         references = [
           "<#{topic_incoming_email.message_id}>",

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("<test@test.localhost>")
+      expect(subject.generate_for_topic(topic, use_incoming_email_if_present: true)).to eq("<test213428@somemailservice.com>")
     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("<topic/#{topic.id}@test.localhost>")
+    end
   end
 
   describe "find_post_from_message_ids" do