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
This commit is contained in:
Martin Brennan 2022-05-11 13:47:12 +10:00 committed by GitHub
parent a76256756f
commit a6be4972a8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 60 additions and 2 deletions

View File

@ -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

View File

@ -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

View File

@ -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