From 79e55ec3f0a78834b227b99b6573802856ded983 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Mon, 11 Oct 2021 20:57:42 +0300 Subject: [PATCH] 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 | 25 ++++++++++-------- spec/components/email/sender_spec.rb | 38 +++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 11 deletions(-) diff --git a/lib/email/sender.rb b/lib/email/sender.rb index 456f2054c81..f6547a42a35 100644 --- a/lib/email/sender.rb +++ b/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'] = "" - 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? diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb index 7d6643d9f2c..783b70c0507 100644 --- a/spec/components/email/sender_spec.rb +++ b/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 ' + 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 ") + 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)