FEATURE: Allow email image embed with secure media (#10563)

FEATURE: Allow email image embed with secure media (#10563)

This PR introduces a few important changes to secure media redaction in emails. First of all, two new site settings have been introduced:

  • secure_media_allow_embed_images_in_emails: If enabled we will embed secure images in emails instead of redacting them.
  • secure_media_max_email_embed_image_size_kb: The cap to the size of the secure image we will embed, defaulting to 1mb, so the email does not become too big. Max is 10mb. Works in tandem with email_total_attachment_size_limit_kb.

Email::Sender will now attach images to the email based on these settings. The sender will also call inline_secure_images in Email::Styles after secure media is redacted and attachments are added to replace redaction messages with attached images. I went with attachment and cid URLs because base64 image support is still flaky in email clients.

All redaction of secure media is now handled in Email::Styles and calls out to PrettyText.strip_secure_media to do the actual stripping and replacing with placeholders. app/mailers/group_smtp_mailer.rb and app/mailers/user_notifications.rb no longer do any stripping because they are earlier in the pipeline than Email::Styles.

Finally the redaction notice has been restyled and includes a link to the media that the user can click, which will show it to them if they have the necessary permissions.

image

diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb
index 15c3d0c..ac20366 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, "<p class='secure-media-notice'>#{I18n.t("emails.secure_media_placeholder")}</p>")
-      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 aa700f5..6e8b69a 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, "<p class='secure-media-notice'>#{I18n.t("emails.secure_media_placeholder")}</p>")
-      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 cfb7acd..9402b50 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 <a target="_blank" href="https://meta.discourse.org/t/secure-media-uploads/140017">the secure media topic on Meta</a> 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 a69e84e..931d32e 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 df3d519..f5b3c4b 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 =~ /<img[^>]+>/
-        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 2126d45..4b5a01b 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%")
 

[... diff too long, it was truncated ...]

GitHub sha: dede9420

This commit appears in #10563 which was approved by pmusaraj. It was merged by martin.