From 4c0367f774786a7f49d645cf4c56df934ab82006 Mon Sep 17 00:00:00 2001 From: Isaac Janzen <50783505+janzenisaac@users.noreply.github.com> Date: Thu, 5 Mar 2026 13:58:51 -0600 Subject: [PATCH] DEV: Strip unsubscribe links when sending digest previews to arbitrary addresses (#38298) # Problem When an admin sends a digest preview to an arbitrary email address, the email contained functional unsubscribe links belonging to the target user, allowing the recipient to unsubscribe that user without their consent. # Solution Pass `skip_unsubscribe_links: true` when generating preview digests, which skips `UnsubscribeKey` creation and suppresses unsubscribe content from both email headers and body templates at the source. --- app/controllers/admin/email_controller.rb | 9 ++++++- app/mailers/user_notifications.rb | 6 +++-- .../digest/_footer.html.erb | 10 ++++--- .../digest/_footer.text.erb | 10 ++++--- .../controllers/admin-email/preview-digest.js | 1 + .../acceptance/admin-email-preview-test.js | 27 ++++++++++++++++++- spec/requests/admin/email_controller_spec.rb | 26 +++++++++++++++++- 7 files changed, 76 insertions(+), 13 deletions(-) diff --git a/app/controllers/admin/email_controller.rb b/app/controllers/admin/email_controller.rb index 59d95aa1863..793b8c2167f 100644 --- a/app/controllers/admin/email_controller.rb +++ b/app/controllers/admin/email_controller.rb @@ -45,12 +45,19 @@ class Admin::EmailController < Admin::AdminController params.require(:username) params.require(:email) user = User.find_by_username(params[:username]) + raise Discourse::InvalidParameters unless user message, skip_reason = - UserNotifications.public_send(:digest, user, since: params[:last_seen_at]) + UserNotifications.public_send( + :digest, + user, + since: params[:last_seen_at], + skip_unsubscribe_links: true, + ) if message message.to = params[:email] + begin Email::Sender.new(message, :digest).send render json: success_json diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 0eae8a12717..633e10affed 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -251,7 +251,9 @@ class UserNotifications < ActionMailer::Base def digest(user, opts = {}) build_summary_for(user) - @unsubscribe_key = UnsubscribeKey.create_key_for(@user, UnsubscribeKey::DIGEST_TYPE) + if !opts[:skip_unsubscribe_links] + @unsubscribe_key = UnsubscribeKey.create_key_for(@user, UnsubscribeKey::DIGEST_TYPE) + end @since = opts[:since].presence @since ||= [user.last_seen_at, user.user_stat&.digest_attempted_at, 1.month.ago].compact.max @@ -371,7 +373,7 @@ class UserNotifications < ActionMailer::Base email_prefix: @email_prefix, date: short_date(Time.now), ), - add_unsubscribe_link: true, + add_unsubscribe_link: !opts[:skip_unsubscribe_links], unsubscribe_url: "#{Discourse.base_url}/email/unsubscribe/#{@unsubscribe_key}", topic_ids: topics_for_digest.pluck(:id), post_ids: diff --git a/app/views/user_notifications/digest/_footer.html.erb b/app/views/user_notifications/digest/_footer.html.erb index 7ffc059023e..bdfbb7f4e00 100644 --- a/app/views/user_notifications/digest/_footer.html.erb +++ b/app/views/user_notifications/digest/_footer.html.erb @@ -3,10 +3,12 @@ - <%=raw(t 'user_notifications.digest.unsubscribe', - site_link: html_site_link, - email_preferences_link: link_to(t('user_notifications.digest.your_email_settings'), Discourse.base_url + '/my/preferences/emails'), - unsubscribe_link: link_to(t('user_notifications.digest.click_here'), "#{Discourse.base_url}/email/unsubscribe/#{@unsubscribe_key}")) %> + <% if @unsubscribe_key %> + <%=raw(t 'user_notifications.digest.unsubscribe', + site_link: html_site_link, + email_preferences_link: link_to(t('user_notifications.digest.your_email_settings'), Discourse.base_url + '/my/preferences/emails'), + unsubscribe_link: link_to(t('user_notifications.digest.click_here'), "#{Discourse.base_url}/email/unsubscribe/#{@unsubscribe_key}")) %> + <% end %> diff --git a/app/views/user_notifications/digest/_footer.text.erb b/app/views/user_notifications/digest/_footer.text.erb index 92db8e32127..c9a85a90584 100644 --- a/app/views/user_notifications/digest/_footer.text.erb +++ b/app/views/user_notifications/digest/_footer.text.erb @@ -3,10 +3,12 @@ <%= raw(@markdown_linker.references) %> <%= digest_custom_text("above_footer") %> -<%=raw(t :'user_notifications.digest.unsubscribe', - site_link: site_link, - email_preferences_link: raw(@markdown_linker.create(t('user_notifications.digest.your_email_settings'), '/my/preferences/emails')), - unsubscribe_link: raw(@markdown_linker.create(t('user_notifications.digest.click_here'), "/email/unsubscribe/#{@unsubscribe_key}"))) %> +<%- if @unsubscribe_key %> + <%=raw(t :'user_notifications.digest.unsubscribe', + site_link: site_link, + email_preferences_link: raw(@markdown_linker.create(t('user_notifications.digest.your_email_settings'), '/my/preferences/emails')), + unsubscribe_link: raw(@markdown_linker.create(t('user_notifications.digest.click_here'), "/email/unsubscribe/#{@unsubscribe_key}"))) %> +<%- end %> <%= raw(@markdown_linker.references) %> diff --git a/frontend/discourse/admin/controllers/admin-email/preview-digest.js b/frontend/discourse/admin/controllers/admin-email/preview-digest.js index e9e4db0479b..598b5b51bf9 100644 --- a/frontend/discourse/admin/controllers/admin-email/preview-digest.js +++ b/frontend/discourse/admin/controllers/admin-email/preview-digest.js @@ -10,6 +10,7 @@ export default class AdminEmailPreviewDigestController extends Controller { username = null; lastSeen = null; + email = null; @empty("email") emailEmpty; @or("emailEmpty", "sendingEmail") sendEmailDisabled; diff --git a/frontend/discourse/tests/acceptance/admin-email-preview-test.js b/frontend/discourse/tests/acceptance/admin-email-preview-test.js index 0ee9e291015..79c1580e868 100644 --- a/frontend/discourse/tests/acceptance/admin-email-preview-test.js +++ b/frontend/discourse/tests/acceptance/admin-email-preview-test.js @@ -1,4 +1,4 @@ -import { click, visit, waitUntil } from "@ember/test-helpers"; +import { click, fillIn, visit, waitUntil } from "@ember/test-helpers"; import { test } from "qunit"; import { acceptance } from "discourse/tests/helpers/qunit-helpers"; @@ -36,4 +36,29 @@ acceptance("Admin - email-preview", function (needs) { "text content is escaped correctly" ); }); + + test("send digest form requires a destination email", async function (assert) { + await visit("/admin/email/preview-digest"); + + assert + .dom(".email-preview-digest .controls") + .exists("send email form is shown"); + + assert + .dom(".email-preview-digest .controls input[type='text']") + .exists("an email text field is shown for sending this digest preview"); + + assert + .dom(".email-preview-digest .controls .btn-default") + .isDisabled("send button is disabled until an email address is entered"); + + await fillIn( + ".email-preview-digest .controls input[type='text']", + "preview@example.com" + ); + + assert + .dom(".email-preview-digest .controls .btn-default") + .isNotDisabled("send button is enabled once an email address is entered"); + }); }); diff --git a/spec/requests/admin/email_controller_spec.rb b/spec/requests/admin/email_controller_spec.rb index 8baec4f7073..4199bce7c67 100644 --- a/spec/requests/admin/email_controller_spec.rb +++ b/spec/requests/admin/email_controller_spec.rb @@ -238,10 +238,34 @@ RSpec.describe Admin::EmailController do params: { last_seen_at: 1.week.ago, username: admin.username, - email: email("previous_replies"), + email: admin.email, } expect(response.status).to eq(200) end + + it "sends the digest to the requested email without unsubscribe links" do + Fabricate(:topic, created_at: 1.day.ago) + ActionMailer::Base.deliveries.clear + + expect { + post "/admin/email/send-digest.json", + params: { + last_seen_at: 1.month.ago, + username: user.username, + email: "attacker@evil.com", + } + }.not_to change { UnsubscribeKey.count } + + expect(response.status).to eq(200) + expect(response.parsed_body["success"]).to eq("OK") + + message = ActionMailer::Base.deliveries.last + expect(message.to).to eq(["attacker@evil.com"]) + expect(message.html_part.body.to_s).not_to include("/email/unsubscribe/") + expect(message.text_part.body.to_s).not_to include("/email/unsubscribe/") + expect(message.header["List-Unsubscribe"].to_s).to be_blank + expect(message.header["List-Unsubscribe-Post"].to_s).to be_blank + end end end