diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb
index 15c3d0cf530..ac203661bbc 100644
--- a/app/mailers/group_smtp_mailer.rb
+++ b/app/mailers/group_smtp_mailer.rb
@@ -79,26 +79,13 @@ class GroupSmtpMailer < ActionMailer::Base
end
def email_post_markdown(post, add_posted_by = false)
- result = +"#{post.with_secure_media? ? strip_secure_urls(post.raw) : post.raw}\n\n"
+ result = +"#{post.raw}\n\n"
if add_posted_by
result << "#{I18n.t('user_notifications.posted_by', username: post.username, post_date: post.created_at.strftime("%m/%d/%Y"))}\n\n"
end
result
end
- def strip_secure_urls(raw)
- urls = Set.new
- raw.scan(Discourse::Utils::URI_REGEXP) { urls << $& }
-
- urls.each do |url|
- if (url.start_with?(Discourse.store.s3_upload_host) && FileHelper.is_supported_media?(url))
- raw = raw.sub(url, "
#{I18n.t("emails.secure_media_placeholder")}
")
- end
- end
-
- raw
- end
-
def html_override(post, context_posts: nil)
UserNotificationRenderer.render(
template: 'email/notification',
diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb
index aa700f5c028..6e8b69aa3de 100644
--- a/app/mailers/user_notifications.rb
+++ b/app/mailers/user_notifications.rb
@@ -356,26 +356,13 @@ class UserNotifications < ActionMailer::Base
end
def email_post_markdown(post, add_posted_by = false)
- result = +"#{post.with_secure_media? ? strip_secure_urls(post.raw) : post.raw}\n\n"
+ result = +"#{post.raw}\n\n"
if add_posted_by
result << "#{I18n.t('user_notifications.posted_by', username: post.username, post_date: post.created_at.strftime("%m/%d/%Y"))}\n\n"
end
result
end
- def strip_secure_urls(raw)
- urls = Set.new
- raw.scan(Discourse::Utils::URI_REGEXP) { urls << $& }
-
- urls.each do |url|
- if (url.start_with?(Discourse.store.s3_upload_host) && FileHelper.is_supported_media?(url))
- raw = raw.sub(url, "#{I18n.t("emails.secure_media_placeholder")}
")
- end
- end
-
- raw
- end
-
def self.get_context_posts(post, topic_user, user)
if (user.user_option.email_previous_replies == UserOption.previous_replies_type[:never]) ||
SiteSetting.private_email?
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index cfb7acdd794..9402b508a11 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -125,7 +125,8 @@ en:
unsubscribe_not_allowed: "Happens when unsubscribing via email is not allowed for this user."
email_not_allowed: "Happens when the email address is not on the allowlist or is on the blocklist."
unrecognized_error: "Unrecognized Error"
- secure_media_placeholder: "Redacted: this site has secure media enabled, visit the topic to see the attached image/audio/video."
+ secure_media_placeholder: "Redacted: This site has secure media enabled, visit the topic or click View Media to see the attached media."
+ view_redacted_media: "View Media"
errors: &errors
format: ! "%{attribute} %{message}"
@@ -2116,6 +2117,8 @@ en:
prevent_anons_from_downloading_files: "Prevent anonymous users from downloading attachments. WARNING: this will prevent any non-image site assets posted as attachments from working."
secure_media: 'Limits access to ALL uploads (images, video, audio, text, pdfs, zips, and others). If “login required” is enabled, only logged-in users can access uploads. Otherwise, access will be limited only for media uploads in private messages and private categories. WARNING: This setting is complex and requires deep administrative understanding. See the secure media topic on Meta for details.'
+ secure_media_allow_embed_images_in_emails: "Allows embedding secure images that would normally be redacted in emails, if their size is smaller than the 'secure media max email embed image size kb' setting."
+ secure_media_max_email_embed_image_size_kb: "The size cutoff for secure images that will be embedded in emails if the 'secure media allow embed in emails' setting is enabled. Without that setting enabled, this setting has no effect."
slug_generation_method: "Choose a slug generation method. 'encoded' will generate percent encoding string. 'none' will disable slug at all."
enable_emoji: "Enable emoji"
diff --git a/config/site_settings.yml b/config/site_settings.yml
index a69e84e10e6..931d32eb00e 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -1234,6 +1234,12 @@ files:
secure_media:
default: false
client: true
+ secure_media_allow_embed_images_in_emails:
+ default: false
+ secure_media_max_email_embed_image_size_kb:
+ default: 1024
+ min: 1
+ max: 10240
enable_s3_uploads:
default: false
client: true
diff --git a/lib/email/sender.rb b/lib/email/sender.rb
index df3d519c139..f5b3c4bb706 100644
--- a/lib/email/sender.rb
+++ b/lib/email/sender.rb
@@ -199,14 +199,25 @@ module Email
merge_json_x_header('X-MSYS-API', metadata: { message_id: @message.message_id })
end
+ # Parse the HTML again so we can make any final changes before
+ # sending
+ style = Email::Styles.new(@message.html_part.body.to_s)
+
# Suppress images from short emails
if SiteSetting.strip_images_from_short_emails &&
@message.html_part.body.to_s.bytesize <= SiteSetting.short_email_length &&
@message.html_part.body =~ /
]+>/
- style = Email::Styles.new(@message.html_part.body.to_s)
- @message.html_part.body = style.strip_avatars_and_emojis
+ style.strip_avatars_and_emojis
end
+ # Embeds any of the secure images that have been attached inline,
+ # removing the redaction notice.
+ if SiteSetting.secure_media_allow_embed_images_in_emails
+ style.inline_secure_images(@message.attachments)
+ end
+
+ @message.html_part.body = style.to_s
+
email_log.message_id = @message.message_id
DiscourseEvent.trigger(:before_email_send, @message, @email_type)
@@ -249,7 +260,11 @@ module Email
email_size = 0
post.uploads.each do |upload|
- next if FileHelper.is_supported_image?(upload.original_filename)
+ if FileHelper.is_supported_image?(upload.original_filename) &&
+ !should_attach_image?(upload)
+ next
+ end
+
next if email_size + upload.filesize > max_email_size
begin
@@ -277,6 +292,12 @@ module Email
fix_parts_after_attachments!
end
+ def should_attach_image?(upload)
+ return if !SiteSetting.secure_media_allow_embed_images_in_emails || !upload.secure?
+ return if upload.filesize > SiteSetting.secure_media_max_email_embed_image_size_kb.kilobytes
+ true
+ end
+
#
# Two behaviors in the mail gem collide:
#
diff --git a/lib/email/styles.rb b/lib/email/styles.rb
index 2126d456f98..4b5a01b3b33 100644
--- a/lib/email/styles.rb
+++ b/lib/email/styles.rb
@@ -198,7 +198,6 @@ module Email
style('code', 'background-color: #f1f1ff; padding: 2px 5px;')
style('pre code', 'display: block; background-color: #f1f1ff; padding: 5px;')
style('.featured-topic a', "text-decoration: none; font-weight: bold; color: #{SiteSetting.email_link_color}; line-height:1.5em;")
- style('.secure-image-notice', 'font-style: italic; background-color: #f1f1ff; padding: 5px;')
style('.summary-email', "-moz-box-sizing:border-box;-ms-text-size-adjust:100%;-webkit-box-sizing:border-box;-webkit-text-size-adjust:100%;box-sizing:border-box;color:#0a0a0a;font-family:Helvetica,Arial,sans-serif;font-size:14px;font-weight:400;line-height:1.3;margin:0;min-width:100%;padding:0;width:100%")
style('.previous-discussion', 'font-size: 17px; color: #444; margin-bottom:10px;')
@@ -237,10 +236,40 @@ module Email
@@plugin_callbacks.each { |block| block.call(@fragment, @opts) }
end
+ def inline_secure_images(attachments)
+ stripped_media = @fragment.css('[data-stripped-secure-media]')
+ upload_shas = {}
+ stripped_media.each do |div|
+ url = div['data-stripped-secure-media']
+ filename = File.basename(url)
+ sha1 = filename.gsub(File.extname(filename), "")
+ upload_shas[url] = sha1
+ end
+ uploads = Upload.select(:original_filename, :sha1).where(sha1: upload_shas.values)
+
+ stripped_media.each do |div|
+ upload = uploads.find { |upl| upl.sha1 == upload_shas[div['data-stripped-secure-media']] }
+ next if !upload
+
+ original_filename = upload.original_filename
+
+ if attachments[original_filename]
+ url = attachments[original_filename].url
+
+ div.add_next_sibling(
+ "
"
+ )
+ div.remove
+ end
+ end
+ end
+
def to_html
+ # needs to be before class + id strip because we need to style redacted
+ # media and also not double-redact already redacted from lower levels
+ replace_secure_media_urls
strip_classes_and_ids
replace_relative_urls
- replace_secure_media_urls
if SiteSetting.preserve_email_structure_when_styling
@fragment.to_html
@@ -249,6 +278,10 @@ module Email
end
end
+ def to_s
+ @fragment.to_s
+ end
+
def include_body?
@html =~ //i
end
@@ -267,8 +300,6 @@ module Email
img.remove
end
end
-
- @fragment.to_s
end
def make_all_links_absolute
@@ -298,19 +329,12 @@ module Email
end
def replace_secure_media_urls
- @fragment.css('[href]').each do |a|
- if Upload.secure_media_url?(a['href'])
- a.add_next_sibling "#{I18n.t("emails.secure_media_placeholder")}
"
- a.remove
- end
- end
+ # strip again, this can be done at a lower level like in the user
+ # notification template but that may not catch everything
+ PrettyText.strip_secure_media(@fragment)
- @fragment.search('img[src]').each do |img|
- if Upload.secure_media_url?(img['src'])
- img.add_next_sibling "#{I18n.t("emails.secure_media_placeholder")}
"
- img.remove
- end
- end
+ style('div.secure-media-notice', 'border: 5px solid #e9e9e9; padding: 5px; display: inline-block;')
+ style('div.secure-media-notice a', "color: #{SiteSetting.email_link_color}")
end
def correct_first_body_margin
diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb
index 46e3bc58c7e..743b28d351c 100644
--- a/lib/pretty_text.rb
+++ b/lib/pretty_text.rb
@@ -408,9 +408,25 @@ module PrettyText
doc.css("a[href]").each do |a|
if Upload.secure_media_url?(a["href"])
target = %w(video audio).include?(a&.parent&.name) ? a.parent : a
- target.replace "#{I18n.t("emails.secure_media_placeholder")}
"
+ next if target.to_s.include?("stripped-secure-view-media")
+ target.add_next_sibling secure_media_placeholder(doc, a['href'])
+ target.remove
end
end
+ doc.css('img[src]').each do |img|
+ if Upload.secure_media_url?(img['src'])
+ img.add_next_sibling secure_media_placeholder(doc, img['src'])
+ img.remove
+ end
+ end
+ end
+
+ def self.secure_media_placeholder(doc, url)
+ <<~HTML
+
+ HTML
end
def self.format_for_email(html, post = nil)
diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb
index 26d4dcffbe9..7525171eb1b 100644
--- a/spec/components/email/sender_spec.rb
+++ b/spec/components/email/sender_spec.rb
@@ -404,6 +404,87 @@ describe Email::Sender do
.to contain_exactly(*[small_pdf, large_pdf, csv_file].map(&:original_filename))
end
+ context "when secure media enabled" do
+ def enable_s3_uploads
+ SiteSetting.enable_s3_uploads = true
+ SiteSetting.s3_upload_bucket = "s3-upload-bucket"
+ SiteSetting.s3_access_key_id = "some key"
+ SiteSetting.s3_secret_access_key = "some secrets3_region key"
+ stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
+ stub_request(
+ :put,
+ "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/original/1X/#{image.sha1}.#{image.extension}?acl"
+ )
+ store = FileStore::S3Store.new
+ s3_helper = store.instance_variable_get(:@s3_helper)
+ client = Aws::S3::Client.new(stub_responses: true)
+ s3_helper.stubs(:s3_client).returns(client)
+ Discourse.stubs(:store).returns(store)
+ end
+
+ before do
+ enable_s3_uploads
+ SiteSetting.secure_media = true
+ SiteSetting.login_required = true
+ SiteSetting.email_total_attachment_size_limit_kb = 14_000
+ SiteSetting.secure_media_max_email_embed_image_size_kb = 5_000
+
+ Jobs.run_immediately!
+ Jobs::PullHotlinkedImages.any_instance.expects(:execute)
+ FileStore::S3Store.any_instance.expects(:has_been_uploaded?).returns(true).at_least_once
+ CookedPostProcessor.any_instance.stubs(:get_size).returns([244, 66])
+
+ @secure_image = UploadCreator.new(file_from_fixtures("logo.png", "images"), "logo.png")
+ .create_for(Discourse.system_user.id)
+ @secure_image.update_secure_status(secure_override_value: true)
+ @secure_image.update(access_control_post_id: reply.id)
+ reply.update(raw: reply.raw + "\n" + "#{UploadMarkdown.new(@secure_image).image_markdown}")
+ reply.rebake!
+ end
+
+ it "does not attach images when embedding them is not allowed" do
+ Email::Sender.new(message, :valid_type).send
+ expect(message.attachments.length).to eq(3)
+ end
+
+ context "when embedding secure images in email is allowed" do
+ before do
+ SiteSetting.secure_media_allow_embed_images_in_emails = true
+ end
+
+ it "does not attach images that are not marked as secure" do
+ Email::Sender.new(message, :valid_type).send
+ expect(message.attachments.length).to eq(4)
+ end
+
+ it "does not embed images that are too big" do
+ SiteSetting.secure_media_max_email_embed_image_size_kb = 1
+ Email::Sender.new(message, :valid_type).send
+ expect(message.attachments.length).to eq(3)
+ end
+
+ it "uses the email styles to inline secure images and attaches the secure image upload to the email" do
+ Email::Sender.new(message, :valid_type).send
+ expect(message.attachments.length).to eq(4)
+ expect(message.attachments.map(&:filename))
+ .to contain_exactly(*[small_pdf, large_pdf, csv_file, @secure_image].map(&:original_filename))
+ expect(message.html_part.body).to include("cid:")
+ expect(message.html_part.body).to include("embedded-secure-image")
+ expect(message.attachments.length).to eq(4)
+ end
+ end
+ end
+
+ it "adds only non-image uploads as attachments to the email and leaves the image intact with original source" do
+ SiteSetting.email_total_attachment_size_limit_kb = 10_000
+ Email::Sender.new(message, :valid_type).send
+
+ expect(message.attachments.length).to eq(3)
+ expect(message.attachments.map(&:filename))
+ .to contain_exactly(*[small_pdf, large_pdf, csv_file].map(&:original_filename))
+ expect(message.html_part.body).to include("
stub(url: 'email/test.png') } }
it "replaces secure media within a link with a placeholder" do
frag = html_fragment("
")
- expect(frag.at('p.secure-media-notice')).to be_present
expect(frag.at('img')).not_to be_present
- expect(frag.at('a')).not_to be_present
+ expect(frag.to_s).to include("Redacted")
end
it "replaces secure images with a placeholder" do
frag = html_fragment("
")
- expect(frag.at('p.secure-media-notice')).to be_present
expect(frag.at('img')).not_to be_present
+ expect(frag.to_s).to include("Redacted")
end
it "does not replace topic links with secure-media-uploads in the name" do
frag = html_fragment("Visit Topic")
- expect(frag.at('p.secure-media-notice')).not_to be_present
+ expect(frag.to_s).not_to include("Redacted")
+ end
+ end
+
+ context "inline_secure_images" do
+ let(:attachments) { { 'testimage.png' => stub(url: 'cid:email/test.png') } }
+ fab!(:upload) { Fabricate(:upload, original_filename: 'testimage.png', secure: true, sha1: '123456') }
+
+ def strip_and_inline
+ html = "
"
+
+ # strip out the secure media
+ styler = Email::Styles.new(html)
+ styler.format_basic
+ styler.format_html
+ html = styler.to_html
+
+ # pass in the attachments to match uploads based on sha + original filename
+ styler = Email::Styles.new(html)
+ styler.inline_secure_images(attachments)
+ @frag = Nokogiri::HTML5.fragment(styler.to_s)
+ end
+
+ it "inlines attachments where stripped-secure-media data attr is present" do
+ strip_and_inline
+ expect(@frag.to_s).to include("cid:email/test.png")
+ expect(@frag.css('[data-stripped-secure-media]')).not_to be_present
+ end
+
+ it "does not inline anything if the upload cannot be found" do
+ upload.update(sha1: 'blah12')
+ strip_and_inline
+
+ expect(@frag.to_s).not_to include("cid:email/test.png")
+ expect(@frag.css('[data-stripped-secure-media]')).to be_present
end
end
end
diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb
index 073957026b7..72e22124730 100644
--- a/spec/components/pretty_text_spec.rb
+++ b/spec/components/pretty_text_spec.rb
@@ -928,6 +928,40 @@ describe PrettyText do
expect(md.to_s).to match(I18n.t("emails.secure_media_placeholder"))
expect(md.to_s).not_to match(SiteSetting.Upload.s3_cdn_url)
end
+
+ it "replaces secure media within a link with a placeholder, keeping the url in an attribute" do
+ url = "#{Discourse.base_url}\/secure-media-uploads/original/1X/testimage.png"
+ html = <<~HTML
+
+ HTML
+ md = PrettyText.format_for_email(html, post)
+ expect(md).not_to include('![]()
+ HTML
+ md = PrettyText.format_for_email(html, post)
+ md = PrettyText.format_for_email(md, post)
+
+ expect(md.scan(/stripped-secure-view-media/).length).to eq(1)
+ expect(md.scan(/Redacted/).length).to eq(1)
+ end
+
+ it "replaces secure images with a placeholder, keeping the url in an attribute" do
+ url = "/secure-media-uploads/original/1X/testimage.png"
+ html = <<~HTML
+
+ HTML
+ md = PrettyText.format_for_email(html, post)
+ expect(md).not_to include('![]()