FIX: Remove List-Post email header (#14554)

* FIX: Remove List-Post email header

This header is used for mailing lists and can confuse some email clients
such as Thunderbird to display wrong replying options.

* FIX: Replace reply_key in email custom headers

Admins can add custom email headers from site settings. Email sender
will try to replace the reply key if %{reply_key} exists or remove the
header if a reply key does not exist.

lib/email/sender.rb

@@ -102,7 +102,7 @@ module Email
 
       post_id   = header_value('X-Discourse-Post-Id')
       topic_id  = header_value('X-Discourse-Topic-Id')
-      reply_key = set_reply_key(post_id, user_id)
+      reply_key = get_reply_key(post_id, user_id)
       from_address = @message.from&.first
       smtp_group_id = from_address.blank? ? nil : Group.where(
         email_username: from_address, smtp_enabled: true
@@ -192,11 +192,6 @@ module Email
         end
       end
 
-      if reply_key.present? && @message.header['Reply-To'].to_s =~ /\<([^\>]+)\>/ && !smtp_group_id
-        email = Regexp.last_match[1]
-        @message.header['List-Post'] = "<mailto:#{email}>"
-      end
-
       if Email::Sender.bounceable_reply_address?
         email_log.bounce_key = SecureRandom.hex
 
@@ -214,9 +209,22 @@ module Email
       @message.header['X-Discourse-Post-Id']  = nil if post_id.present?
 
       if reply_key.present?
+        @message.header['Reply-To'] = header_value('Reply-To').gsub!("%{reply_key}", reply_key)
         @message.header[Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER] = nil
       end
 
+      # Replace reply_key in custom headers or remove
+      MessageBuilder.custom_headers(SiteSetting.email_custom_headers).each do |key, _|
+        value = header_value(key)
+        if value&.include?('%{reply_key}')
+          # Delete old header first or else the same header will be added twice
+          @message.header[key] = nil
+          if reply_key.present?
+            @message.header[key] = value.gsub!('%{reply_key}', reply_key)
+          end
+        end
+      end
+
       # pass the original message_id when using mailjet/mandrill/sparkpost
       case ActionMailer::Base.smtp_settings[:address]
       when /\.mailjet\.com/
@@ -430,7 +438,7 @@ module Email
       @message.header[name] = data.to_json
     end
 
-    def set_reply_key(post_id, user_id)
+    def get_reply_key(post_id, user_id)
       # ALLOW_REPLY_BY_EMAIL_HEADER is only added if we are _not_ sending
       # via group SMTP and if reply by email site settings are configured
       return if !user_id || !post_id || !header_value(Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER).present?
@@ -440,9 +448,6 @@ module Email
         post_id: post_id,
         user_id: user_id
       ).reply_key
-
-      @message.header['Reply-To'] =
-        header_value('Reply-To').gsub!("%{reply_key}", reply_key)
     end
 
     def self.bounceable_reply_address?

spec/components/email/sender_spec.rb

@@ -181,6 +181,43 @@ describe Email::Sender do
       end
     end
 
+    context "replaces reply_key in custom headers" do
+      fab!(:user) { Fabricate(:user) }
+      let(:email_sender) { Email::Sender.new(message, :valid_type, user) }
+      let(:reply_key) { PostReplyKey.find_by!(post_id: post.id, user_id: user.id).reply_key }
+
+      before do
+        SiteSetting.reply_by_email_address = 'replies+%{reply_key}@test.com'
+        SiteSetting.email_custom_headers = 'Auto-Submitted: auto-generated|Mail-Reply-To: sender-name+%{reply_key}@domain.net'
+
+        message.header['X-Discourse-Post-Id'] = post.id
+      end
+
+      it 'replaces headers with reply_key if present' do
+        message.header[Email::MessageBuilder::ALLOW_REPLY_BY_EMAIL_HEADER] = 'test-%{reply_key}@test.com'
+        message.header['Reply-To'] = 'Test <test-%{reply_key}@test.com>'
+        message.header['Auto-Submitted'] = 'auto-generated'
+        message.header['Mail-Reply-To'] = 'sender-name+%{reply_key}@domain.net'
+
+        email_sender.send
+
+        expect(message.header['Reply-To'].to_s).to eq("Test <test-#{reply_key}@test.com>")
+        expect(message.header['Auto-Submitted'].to_s).to eq('auto-generated')
+        expect(message.header['Mail-Reply-To'].to_s).to eq("sender-name+#{reply_key}@domain.net")
+      end
+
+      it 'removes headers with reply_key if absent' do
+        message.header['Auto-Submitted'] = 'auto-generated'
+        message.header['Mail-Reply-To'] = 'sender-name+%{reply_key}@domain.net'
+
+        email_sender.send
+
+        expect(message.header['Reply-To'].to_s).to eq('')
+        expect(message.header['Auto-Submitted'].to_s). to eq('auto-generated')
+        expect(message.header['Mail-Reply-To'].to_s).to eq('')
+      end
+    end
+
     context "adds Precedence header" do
       fab!(:topic) { Fabricate(:topic) }
       fab!(:post) { Fabricate(:post, topic: topic) }
@@ -373,7 +410,6 @@ describe Email::Sender do
           email_sender.send
 
           expect(message.header['List-ID']).to eq(nil)
-          expect(message.header['List-Post']).to eq(nil)
           expect(message.header['List-Archive']).to eq(nil)
           expect(message.header['Precedence']).to eq(nil)
           expect(message.header['List-Unsubscribe']).to eq(nil)