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 <martin@discourse.org>

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

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
 

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
 #

app/models/user.rb

@@ -218,6 +218,7 @@ class User < ActiveRecord::Base
   scope :suspended, -> { where('suspended_till IS NOT NULL AND suspended_till > ?', Time.zone.now) }
   scope :not_suspended, -> { where('suspended_till IS NULL OR suspended_till <= ?', Time.zone.now) }
   scope :activated, -> { where(active: true) }
+  scope :not_staged, -> { where(staged: false) }
 
   scope :filter_by_username, ->(filter) do
     if filter.is_a?(Array)

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

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]

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?

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

spec/mailers/group_smtp_mailer_spec.rb

@@ -36,6 +36,7 @@ describe GroupSmtpMailer do
     Message-ID: <a52f67a3d3560f2a35276cda8519b10b595623bcb66912bb92df6651ad5f75be@mail.gmail.com>
     Subject: Hello from John
     To: "bugs@gmail.com" <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)