FIX: Add random suffix to outbound Message-ID for email (#15179)

Currently the Message-IDs we send out for outbound email
are not unique; for a post they look like:

topic/TOPIC_ID/POST_ID@HOST

And for a topic they look like:

topic/TOPIC_ID@HOST

This commit changes the outbound Message-IDs to also have
a random suffix before the host, so the new format is
like this:

topic/TOPIC_ID/POST_ID.RANDOM_SUFFIX@HOST

Or:

topic/TOPIC_ID.RANDOM_SUFFIX@HOST

This should help with email deliverability. This change
is backwards-compatible, the old Message-ID format will
still be recognized in the mail receiver flow, so people
will still be able to reply using Message-IDs, In-Reply-To,
and References headers that have already been sent.

This commit also refactors Message-ID related logic
to a central location, and adds judicious amounts of
tests and documentation.

lib/email/message_id_service.rb

@@ -0,0 +1,103 @@
+# frozen_string_literal: true
+
+module Email
+  ##
+  # Email Message-IDs are used in both our outbound and inbound email
+  # flow. For the outbound flow via Email::Sender, we assign a unique
+  # Message-ID for any emails sent out from the application.
+  # If we are sending an email related to a topic, such as through the
+  # PostAlerter class, then the Message-ID will contain references to
+  # the topic ID, and if it is for a specific post, the post ID,
+  # along with a random suffix to make the Message-ID truly unique.
+  # The host must also be included on the Message-IDs.
+  #
+  # For the inbound email flow via Email::Receiver, we use Message-IDs
+  # to discern which topic or post the inbound email reply should be
+  # in response to. In this case, the Message-ID is extracted from the
+  # References and/or In-Reply-To headers, and compared with either
+  # the IncomingEmail table, the Post table, or the IncomingEmail to
+  # determine where to send the reply.
+  #
+  # See https://datatracker.ietf.org/doc/html/rfc2822#section-3.6.4 for
+  # more specific information around Message-IDs in email.
+  #
+  # See https://tools.ietf.org/html/rfc850#section-2.1.7 for the
+  # Message-ID format specification.
+  class MessageIdService
+    class << self
+      def generate_default
+        "<#{SecureRandom.uuid}@#{host}>"
+      end
+
+      def generate_for_post(post, use_incoming_email_if_present: false)
+        if use_incoming_email_if_present && post.incoming_email&.message_id.present?
+          return "<#{post.incoming_email.message_id}>"
+        end
+
+        "<topic/#{post.topic_id}/#{post.id}.#{random_suffix}@#{host}>"
+      end
+
+      def generate_for_topic(topic, use_incoming_email_if_present: 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}>"
+      end
+
+      def find_post_from_message_ids(message_ids)
+        message_ids = message_ids.map { |message_id| message_id_clean(message_id) }
+        post_ids =  message_ids.map { |message_id| message_id[message_id_post_id_regexp, 1] }.compact.map(&:to_i)
+        post_ids << Post.where(
+          topic_id: message_ids.map { |message_id| message_id[message_id_topic_id_regexp, 1] }.compact,
+          post_number: 1
+        ).pluck(:id)
+        post_ids << EmailLog.where(message_id: message_ids).pluck(:post_id)
+        post_ids << IncomingEmail.where(message_id: message_ids).pluck(:post_id)
+
+        post_ids.flatten!
+        post_ids.compact!
+        post_ids.uniq!
+
+        return if post_ids.empty?
+
+        Post.where(id: post_ids).order(:created_at).last
+      end
+
+      def random_suffix
+        SecureRandom.hex(12)
+      end
+
+      def discourse_generated_message_id?(message_id)
+        !!(message_id =~ message_id_post_id_regexp) ||
+          !!(message_id =~ message_id_topic_id_regexp)
+      end
+
+      def message_id_post_id_regexp
+        @message_id_post_id_regexp ||= Regexp.new "topic/\\d+/(\\d+|\\d+\.\\w+)@#{Regexp.escape(host)}"
+      end
+
+      def message_id_topic_id_regexp
+        @message_id_topic_id_regexp ||= Regexp.new "topic/(\\d+|\\d+\.\\w+)@#{Regexp.escape(host)}"
+      end
+
+      def message_id_rfc_format(message_id)
+        message_id.present? && !is_message_id_rfc?(message_id) ? "<#{message_id}>" : message_id
+      end
+
+      def message_id_clean(message_id)
+        message_id.present? && is_message_id_rfc?(message_id) ? message_id.gsub(/^<|>$/, "") : message_id
+      end
+
+      def is_message_id_rfc?(message_id)
+        message_id.start_with?('<') && message_id.include?('@') && message_id.end_with?('>')
+      end
+
+      def host
+        @host ||= Email::Sender.host_for(Discourse.base_url)
+      end
+    end
+  end
+end

lib/email/receiver.rb

@@ -107,7 +107,7 @@ module Email
       # server (e.g. a message_id generated by Gmail) and does not need to
       # be updated, because message_ids from the IMAP server are not guaranteed
       # to be unique.
-      return unless discourse_generated_message_id?(@message_id)
+      return unless Email::MessageIdService.discourse_generated_message_id?(@message_id)
 
       incoming_email.update(
         imap_uid_validity: @opts[:imap_uid_validity],
@@ -801,7 +801,7 @@ module Email
       # 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 discourse_generated_message_id?(mail.in_reply_to)
+      if Email::MessageIdService.discourse_generated_message_id?(mail.in_reply_to)
         post_id_from_email_log = EmailLog.where(message_id: mail.in_reply_to)
           .addressed_to_user(user)
           .order(created_at: :desc)
@@ -1056,35 +1056,7 @@ module Email
       message_ids = Email::Receiver.extract_reply_message_ids(@mail, max_message_id_count: 5)
       return if message_ids.empty?
 
-      post_ids =  message_ids.map { |message_id| message_id[message_id_post_id_regexp, 1] }.compact.map(&:to_i)
-      post_ids << Post.where(topic_id: message_ids.map { |message_id| message_id[message_id_topic_id_regexp, 1] }.compact, post_number: 1).pluck(:id)
-      post_ids << EmailLog.where(message_id: message_ids).pluck(:post_id)
-      post_ids << IncomingEmail.where(message_id: message_ids).pluck(:post_id)
-
-      post_ids.flatten!
-      post_ids.compact!
-      post_ids.uniq!
-
-      return if post_ids.empty?
-
-      Post.where(id: post_ids).order(:created_at).last
-    end
-
-    def host
-      @host ||= Email::Sender.host_for(Discourse.base_url)
-    end
-
-    def discourse_generated_message_id?(message_id)
-      !!(message_id =~ message_id_post_id_regexp) ||
-        !!(message_id =~ message_id_topic_id_regexp)
-    end
-
-    def message_id_post_id_regexp
-      @message_id_post_id_regexp ||= Regexp.new "topic/\\d+/(\\d+)@#{Regexp.escape(host)}"
-    end
-
-    def message_id_topic_id_regexp
-      @message_id_topic_id_regexp ||= Regexp.new "topic/(\\d+)@#{Regexp.escape(host)}"
+      Email::MessageIdService.find_post_from_message_ids(message_ids)
     end
 
     def self.extract_reply_message_ids(mail, max_message_id_count:)
@@ -1100,7 +1072,7 @@ module Email
         references
       elsif references.present?
         references.split(/[\s,]/).map do |r|
-          Email.message_id_clean(r)
+          Email::MessageIdService.message_id_clean(r)
         end
       end
     end

lib/email/sender.rb

@@ -109,7 +109,7 @@ module Email
       ).pluck_first(:id)
 
       # always set a default Message ID from the host
-      @message.header['Message-ID'] = "<#{SecureRandom.uuid}@#{host}>"
+      @message.header['Message-ID'] = Email::MessageIdService.generate_default
 
       if topic_id.present? && post_id.present?
         post = Post.find_by(id: post_id, topic_id: topic_id)
@@ -121,15 +121,9 @@ module Email
         return skip(SkippedEmailLog.reason_types[:sender_topic_deleted]) if topic.blank?
 
         add_attachments(post)
-        first_post = topic.ordered_posts.first
 
-        topic_message_id = first_post.incoming_email&.message_id.present? ?
-          "<#{first_post.incoming_email.message_id}>" :
-          "<topic/#{topic_id}@#{host}>"
-
-        post_message_id = post.incoming_email&.message_id.present? ?
-          "<#{post.incoming_email.message_id}>" :
-          "<topic/#{topic_id}/#{post_id}@#{host}>"
+        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)
 
         referenced_posts = Post.includes(:incoming_email)
           .joins("INNER JOIN post_replies ON post_replies.post_id = posts.id ")
@@ -141,9 +135,9 @@ module Email
             "<#{referenced_post.incoming_email.message_id}>"
           else
             if referenced_post.post_number == 1
-              "<topic/#{topic_id}@#{host}>"
+              Email::MessageIdService.generate_for_topic(topic)
             else
-              "<topic/#{topic_id}/#{referenced_post.id}@#{host}>"
+              Email::MessageIdService.generate_for_post(referenced_post)
             end
           end
         end