From 1cb5200450aae75c0bfca5de2dfe52c3274651a2 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 5 Jan 2023 09:34:51 +0800 Subject: [PATCH] Revert "SECURITY: BCC active user emails from group SMTP (#19724)" This reverts commit 7bd83ef6b5bd36a57d64abd3547d4bf4caf8579f. --- 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, 6 insertions(+), 65 deletions(-) delete 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 eaabf2c3a11..61bc09fa0b3 100644 --- a/app/jobs/regular/group_smtp_email.rb +++ b/app/jobs/regular/group_smtp_email.rb @@ -46,12 +46,6 @@ 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, @@ -74,13 +68,7 @@ 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: cc_addresses, - bcc_addresses: bcc_addresses - ) + message = GroupSmtpMailer.send_mail(group, email, post, cc_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 0b889adfe99..ce12e4b0cc2 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, bcc_addresses: nil) + def send_mail(from_group, to_address, post, cc_addresses = nil) raise 'SMTP is disabled' if !SiteSetting.enable_smtp op_incoming_email = post.topic.first_post.incoming_email @@ -51,8 +51,7 @@ 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, - bcc: bcc_addresses + cc: cc_addresses ) end diff --git a/app/models/email_log.rb b/app/models/email_log.rb index a9725970d6e..e8dd29a5dfa 100644 --- a/app/models/email_log.rb +++ b/app/models/email_log.rb @@ -132,7 +132,6 @@ 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 deleted file mode 100644 index a54c55312df..00000000000 --- a/db/migrate/20221125001635_add_bcc_addresses_to_email_log.rb +++ /dev/null @@ -1,7 +0,0 @@ -# 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 0b3fe595eda..187bfc364d4 100644 --- a/lib/email/message_builder.rb +++ b/lib/email/message_builder.rb @@ -141,8 +141,7 @@ module Email body: body, charset: 'UTF-8', from: from_value, - cc: @opts[:cc], - bcc: @opts[:bcc] + cc: @opts[:cc] } 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 aab854ed288..e91adecd793 100644 --- a/lib/email/sender.rb +++ b/lib/email/sender.rb @@ -98,10 +98,6 @@ 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') @@ -327,12 +323,6 @@ 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 6e68d4ed92d..88872a93943 100644 --- a/spec/jobs/regular/group_smtp_email_spec.rb +++ b/spec/jobs/regular/group_smtp_email_spec.rb @@ -40,16 +40,7 @@ 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, - cc_addresses: %w[otherguy@test.com cormac@lit.com], - bcc_addresses: [], - ) - .returns(message) + GroupSmtpMailer.expects(:send_mail).with(group, "test@test.com", post, ["otherguy@test.com", "cormac@lit.com"]).returns(message) subject.execute(args) end @@ -185,27 +176,11 @@ 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) - 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"]) + expect(ActionMailer::Base.deliveries.last.subject).to eq("Re: Help I need support") 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 5be377e8f5d..afca00f11e5 100644 --- a/spec/mailers/group_smtp_mailer_spec.rb +++ b/spec/mailers/group_smtp_mailer_spec.rb @@ -36,7 +36,6 @@ 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, @@ -77,7 +76,6 @@ 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)