From a6be4972a8ee3bb174272189b3d4d7ffcd4ad833 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Wed, 11 May 2022 13:47:12 +1000
Subject: [PATCH] FIX: Use our header value instead of custom header on
 duplicates (#16711)

When we build and send emails using MessageBuilder and Email::Sender
we add custom headers defined in SiteSetting.email_custom_headers.
However this was causing errors in cases where the custom headers
defined a header that we already specify in outbound emails (e.g.
the Precedence: list header for topic/post emails).

This commit makes it so we always use the header value defined in Discourse
core if there is a duplicate, discarding the custom header value
from the site setting.

cf. https://meta.discourse.org/t/email-notifications-fail-if-duplicate-headers-exist/222960/14
---
 app/mailers/test_mailer.rb              |  4 ++--
 lib/email/sender.rb                     | 29 +++++++++++++++++++++++++
 spec/integration/email_outbound_spec.rb | 29 +++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 2 deletions(-)
 create mode 100644 spec/integration/email_outbound_spec.rb

diff --git a/app/mailers/test_mailer.rb b/app/mailers/test_mailer.rb
index ab65d300e8..4abd75263a 100644
--- a/app/mailers/test_mailer.rb
+++ b/app/mailers/test_mailer.rb
@@ -3,7 +3,7 @@
 class TestMailer < ActionMailer::Base
   include Email::BuildEmailHelper
 
-  def send_test(to_address)
-    build_email(to_address, template: 'test_mailer')
+  def send_test(to_address, opts = {})
+    build_email(to_address, template: 'test_mailer', **opts)
   end
 end
diff --git a/lib/email/sender.rb b/lib/email/sender.rb
index b20c69a94b..7583c200d7 100644
--- a/lib/email/sender.rb
+++ b/lib/email/sender.rb
@@ -226,6 +226,26 @@ module Email
       end
 
       MessageBuilder.custom_headers(SiteSetting.email_custom_headers).each do |key, _|
+        # Any custom headers added via MessageBuilder that are doubled up here
+        # with values that we determine should be set to the last value, which is
+        # the one we determined. Our header values should always override the email_custom_headers.
+        #
+        # While it is valid via RFC5322 to have more than one value for certain headers,
+        # we just want to keep it to one, especially in cases where the custom value
+        # would conflict with our own.
+        #
+        # See https://datatracker.ietf.org/doc/html/rfc5322#section-3.6 and
+        # https://github.com/mikel/mail/blob/8ef377d6a2ca78aa5bd7f739813f5a0648482087/lib/mail/header.rb#L109-L132
+        custom_header = @message.header[key]
+        if custom_header.is_a?(Array)
+          our_value = custom_header.last.value
+
+          # Must be set to nil first otherwise another value is just added
+          # to the array of values for the header.
+          @message.header[key] = nil
+          @message.header[key] = our_value
+        end
+
         value = header_value(key)
 
         # Remove Auto-Submitted header for group private message emails, it does
@@ -433,6 +453,15 @@ module Email
     def header_value(name)
       header = @message.header[name]
       return nil unless header
+
+      # NOTE: In most cases this is not a problem, but if a header has
+      # doubled up the header[] method will return an array. So we always
+      # get the last value of the array and assume that is the correct
+      # value.
+      #
+      # See https://github.com/mikel/mail/blob/8ef377d6a2ca78aa5bd7f739813f5a0648482087/lib/mail/header.rb#L109-L132
+      return header.last.value if header.is_a?(Array)
+
       header.value
     end
 
diff --git a/spec/integration/email_outbound_spec.rb b/spec/integration/email_outbound_spec.rb
new file mode 100644
index 0000000000..25d441d517
--- /dev/null
+++ b/spec/integration/email_outbound_spec.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+# We already have Email::Sender and Email::MessageBuilder specs along
+# with mailer specific mailer specs like UserEmail, but sometimes we need
+# to test things along the whole outbound flow including the MessageBuilder
+# and the Sender.
+describe "Outbound Email" do
+  def send_email(opts = {})
+    message = TestMailer.send_test("test@test.com", opts)
+    result = Email::Sender.new(message, :test_message).send
+    [message, result]
+  end
+
+  context "email custom headers" do
+    it "discards the custom header if it is one that has already been set based on arguments" do
+      SiteSetting.email_custom_headers = "Precedence: bulk"
+      post = Fabricate(:post)
+      message, result = send_email(post_id: post.id, topic_id: post.topic_id)
+      expect(message.header["Precedence"].value).to eq("list")
+    end
+
+    it "does send unique custom headers" do
+      SiteSetting.email_custom_headers = "SuperUrgent: wow-cool"
+      post = Fabricate(:post)
+      message, result = send_email(post_id: post.id, topic_id: post.topic_id)
+      expect(message.header["SuperUrgent"].value).to eq("wow-cool")
+    end
+  end
+end