From a09dc2d5c2d9b4a3e199daf375f196d5f3ae3f7d Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 5 Jan 2023 08:50:54 +0800 Subject: [PATCH] SECURITY: BCC active user emails from group SMTP (#19724) 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 61bc09fa0b..eaabf2c3a1 100644 --- a/app/jobs/regular/group_smtp_email.rb +++ b/app/jobs/regular/group_smtp_email.rb @@ -46,6 +46,12 @@ module Jobs cc.match(EmailValidator.email_regex) ? cc : nil end.compact + # 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, @@ -68,7 +74,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 ce12e4b0cc..0b889adfe9 100644 --- a/app/mailers/group_smtp_mailer.rb +++ b/app/mailers/group_smtp_mailer.rb @@ -5,7 +5,7 @@ require_dependency 'email/message_builder' 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 @@ -51,7 +51,8 @@ class GroupSmtpMailer < ActionMailer::Base from: from_group.email_username, from_alias: I18n.t('email_from_without_site', user_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 e8dd29a5df..a9725970d6 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -132,6 +132,7 @@ end # cc_user_ids :integer is an Array # raw :text # topic_id :integer +# 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 0000000000..21ea1d7735 --- /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[6.1] + 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 187bfc364d..0b3fe595ed 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 e91adecd79..aab854ed28 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') @@ -323,6 +327,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 88872a9394..6e68d4ed92 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/spec/jobs/regular/group_smtp_email_spec.rb @@ -40,7 +40,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 @@ -176,11 +185,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 afca00f11e..5be377e8f5 100644 --- a/spec/mailers/group_smtp_mailer_spec.rb +++ b/spec/mailers/group_smtp_mailer_spec.rb @@ -36,6 +36,7 @@ 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, @@ -76,6 +77,7 @@ 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)