From ffc59a3b28242bfa7de4b5fe2876814a84831903 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 + app/models/user.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 ++ 9 files changed, 66 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 61bc09fa0b3..eaabf2c3a11 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 ce12e4b0cc2..0b889adfe99 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 e8dd29a5dfa..a9725970d6e 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/app/models/user.rb b/app/models/user.rb index d610cc524f7..5a8ceee5c92 100644 --- a/app/models/user.rb +++ b/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) 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..21ea1d77354 --- /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 187bfc364d4..0b3fe595eda 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 e91adecd793..aab854ed288 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 88872a93943..6e68d4ed92d 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 afca00f11e5..5be377e8f5d 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)