From ab3a032b4b129486d39c53b96a29379e65f3416b Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 5 Jan 2023 06:07:50 +0800 Subject: [PATCH] SECURITY: BCC active user emails from group SMTP (#19725) When sending emails out via group SMTP, if we are sending them to non-staged users we want to mask those emails with BCC, just so we don't expose them to anyone we shouldn't. Staged users are ones that have likely only interacted with support via email, and will likely include other people who were CC'd on the original email to the group. Co-authored-by: Martin Brennan --- app/jobs/regular/group_smtp_email.rb | 14 ++++++++- app/mailers/group_smtp_mailer.rb | 5 ++-- app/models/email_log.rb | 1 + ...25001635_add_bcc_addresses_to_email_log.rb | 7 +++++ lib/email/message_builder.rb | 3 +- lib/email/sender.rb | 10 +++++++ spec/jobs/regular/group_smtp_email_spec.rb | 29 +++++++++++++++++-- spec/mailers/group_smtp_mailer_spec.rb | 2 ++ 8 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20221125001635_add_bcc_addresses_to_email_log.rb diff --git a/app/jobs/regular/group_smtp_email.rb b/app/jobs/regular/group_smtp_email.rb index b06801be188..b0fc4a890e9 100644 --- a/app/jobs/regular/group_smtp_email.rb +++ b/app/jobs/regular/group_smtp_email.rb @@ -44,6 +44,12 @@ module Jobs EmailAddressValidator.valid_value?(address) end + # Mask the email addresses of non-staged users so + # they are not revealed unnecessarily when we are sending + # the email notification out. + bcc_addresses = User.not_staged.with_email(cc_addresses).pluck(:email) + cc_addresses = cc_addresses - bcc_addresses + # There is a rare race condition causing the Imap::Sync class to create # an incoming email and associated post/topic, which then kicks off # the PostAlerter to notify others in the PM about a reply in the topic, @@ -66,7 +72,13 @@ module Jobs # The EmailLog record created by the sender will have the raw email # stored, the group smtp ID, and any cc addresses recorded for later # cross referencing. - message = GroupSmtpMailer.send_mail(group, email, post, cc_addresses) + message = GroupSmtpMailer.send_mail( + group, + email, + post, + cc_addresses: cc_addresses, + bcc_addresses: bcc_addresses + ) Email::Sender.new(message, :group_smtp, recipient_user).send # Create an incoming email record to avoid importing again from IMAP diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb index 5a72c1c92c7..c394439026a 100644 --- a/app/mailers/group_smtp_mailer.rb +++ b/app/mailers/group_smtp_mailer.rb @@ -3,7 +3,7 @@ class GroupSmtpMailer < ActionMailer::Base include Email::BuildEmailHelper - def send_mail(from_group, to_address, post, cc_addresses = nil) + def send_mail(from_group, to_address, post, cc_addresses: nil, bcc_addresses: nil) raise 'SMTP is disabled' if !SiteSetting.enable_smtp op_incoming_email = post.topic.first_post.incoming_email @@ -46,7 +46,8 @@ class GroupSmtpMailer < ActionMailer::Base from: from_group.smtp_from_address, from_alias: I18n.t('email_from_without_site', group_name: group_name), html_override: html_override(post), - cc: cc_addresses + cc: cc_addresses, + bcc: bcc_addresses ) end diff --git a/app/models/email_log.rb b/app/models/email_log.rb index 8e3c7b163d3..b5ab6d6402d 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -144,6 +144,7 @@ end # topic_id :integer # bounce_error_code :string # smtp_transaction_response :string(500) +# bcc_addresses :text # # Indexes # diff --git a/db/migrate/20221125001635_add_bcc_addresses_to_email_log.rb b/db/migrate/20221125001635_add_bcc_addresses_to_email_log.rb new file mode 100644 index 00000000000..a54c55312df --- /dev/null +++ b/db/migrate/20221125001635_add_bcc_addresses_to_email_log.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddBccAddressesToEmailLog < ActiveRecord::Migration[7.0] + def change + add_column :email_logs, :bcc_addresses, :text, null: true + end +end diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb index 62236252e0d..9308705cd64 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -141,7 +141,8 @@ module Email body: body, charset: 'UTF-8', from: from_value, - cc: @opts[:cc] + cc: @opts[:cc], + bcc: @opts[:bcc] } args[:delivery_method_options] = @opts[:delivery_method_options] if @opts[:delivery_method_options] diff --git a/lib/email/sender.rb b/lib/email/sender.rb index fd6148cdc65..c31df8b76ac 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -98,6 +98,10 @@ module Email email_log.cc_user_ids = User.with_email(cc_addresses).pluck(:id) end + if bcc_addresses.any? + email_log.bcc_addresses = bcc_addresses.join(";") + end + host = Email::Sender.host_for(Discourse.base_url) post_id = header_value('X-Discourse-Post-Id') @@ -300,6 +304,12 @@ module Email end end + def bcc_addresses + @bcc_addresses ||= begin + @message.try(:bcc) || [] + end + end + def self.host_for(base_url) host = "localhost" if base_url.present? diff --git a/spec/jobs/regular/group_smtp_email_spec.rb b/spec/jobs/regular/group_smtp_email_spec.rb index 898506f4f4c..d01ce2c02d8 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/spec/jobs/regular/group_smtp_email_spec.rb @@ -36,7 +36,16 @@ RSpec.describe Jobs::GroupSmtpEmail do it "sends an email using the GroupSmtpMailer and Email::Sender" do message = Mail::Message.new(body: "hello", to: "myemail@example.invalid") - GroupSmtpMailer.expects(:send_mail).with(group, "test@test.com", post, ["otherguy@test.com", "cormac@lit.com"]).returns(message) + GroupSmtpMailer + .expects(:send_mail) + .with( + group, + "test@test.com", + post, + cc_addresses: %w[otherguy@test.com cormac@lit.com], + bcc_addresses: [], + ) + .returns(message) subject.execute(args) end @@ -172,11 +181,27 @@ RSpec.describe Jobs::GroupSmtpEmail do it "has the cc_addresses and cc_user_ids filled in correctly" do subject.execute(args) expect(ActionMailer::Base.deliveries.count).to eq(1) - expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") + sent_mail = ActionMailer::Base.deliveries.last + expect(sent_mail.subject).to eq("Re: Help I need support") + expect(sent_mail.cc).to eq(["otherguy@test.com", "cormac@lit.com"]) email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) expect(email_log.cc_addresses).to eq("otherguy@test.com;cormac@lit.com") expect(email_log.cc_user_ids).to match_array([staged1.id, staged2.id]) end + + it "where cc_addresses match non-staged users, convert to bcc_addresses" do + staged2.update!(staged: false, active: true) + subject.execute(args) + expect(ActionMailer::Base.deliveries.count).to eq(1) + sent_mail = ActionMailer::Base.deliveries.last + expect(sent_mail.subject).to eq("Re: Help I need support") + expect(sent_mail.cc).to eq(["otherguy@test.com"]) + expect(sent_mail.bcc).to eq(["cormac@lit.com"]) + email_log = EmailLog.find_by(post_id: post.id, topic_id: post.topic_id, user_id: recipient_user.id) + expect(email_log.cc_addresses).to eq("otherguy@test.com") + expect(email_log.bcc_addresses).to eq("cormac@lit.com") + expect(email_log.cc_user_ids).to match_array([staged1.id]) + end end context "when the post in the argument is the OP" do diff --git a/spec/mailers/group_smtp_mailer_spec.rb b/spec/mailers/group_smtp_mailer_spec.rb index 9bf091f697e..7677cf455c4 100644 --- a/spec/mailers/group_smtp_mailer_spec.rb +++ b/spec/mailers/group_smtp_mailer_spec.rb @@ -35,6 +35,7 @@ RSpec.describe GroupSmtpMailer do Message-ID: Subject: Hello from John To: "bugs@gmail.com" + Cc: someotherperson@test.com Content-Type: text/plain; charset="UTF-8" Hello, @@ -75,6 +76,7 @@ RSpec.describe GroupSmtpMailer do sent_mail = ActionMailer::Base.deliveries[0] expect(sent_mail.to).to contain_exactly('john@doe.com') + expect(sent_mail.cc).to contain_exactly('someotherperson@test.com') expect(sent_mail.reply_to).to eq(nil) expect(sent_mail.subject).to eq('Re: Hello from John') expect(sent_mail.to_s).to include(raw)