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](https://user-images.githubusercontent.com/920448/92341012-b9a2c380-f0ff-11ea-860e-b376b4528357.png)

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',

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?

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"

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

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:
     #

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(
+            "<img src=\"#{url}\" data-embedded-secure-image=\"true\" style=\"max-width: 50%; max-height: 400px;\" />"
+          )
+          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 =~ /<body>/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 "<p class='secure-media-notice'>#{I18n.t("emails.secure_media_placeholder")}</p>"
-          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 "<p class='secure-media-notice'>#{I18n.t("emails.secure_media_placeholder")}</p>"
-          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

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 "<p class='secure-media-notice'>#{I18n.t("emails.secure_media_placeholder")}</p>"
+        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
+    <div class="secure-media-notice" data-stripped-secure-media="#{url}">
+      #{I18n.t('emails.secure_media_placeholder')} <a class='stripped-secure-view-media' href="#{url}">#{I18n.t("emails.view_redacted_media")}</a>.
+    </div>
+    HTML
   end
 
   def self.format_for_email(html, post = nil)

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("<img src=\"#{Discourse.base_url}#{image.url}\"")
+    end
+
     it "respects the size limit and attaches only files that fit into the max email size" do
       SiteSetting.email_total_attachment_size_limit_kb = 40
       Email::Sender.new(message, :valid_type).send

spec/components/email/styles_spec.rb

@@ -4,6 +4,7 @@ require 'rails_helper'
 require 'email'
 
 describe Email::Styles do
+  let(:attachments) { {} }
 
   def basic_fragment(html)
     styler = Email::Styles.new(html)
@@ -186,23 +187,57 @@ describe Email::Styles do
     end
   end
 
-  context "replace_relative_urls" do
+  context "replace_secure_media_urls" do
+    let(:attachments) { { 'testimage.png' => stub(url: 'email/test.png') } }
     it "replaces secure media within a link with a placeholder" do
       frag = html_fragment("<a href=\"#{Discourse.base_url}\/secure-media-uploads/original/1X/testimage.png\"><img src=\"/secure-media-uploads/original/1X/testimage.png\"></a>")
-      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("<img src=\"/secure-media-uploads/original/1X/testimage.png\">")
-      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("<a href=\"#{Discourse.base_url}\/t/secure-media-uploads/235723\">Visit Topic</a>")
-      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 = "<a href=\"#{Discourse.base_url}\/secure-media-uploads/original/1X/123456.png\"><img src=\"/secure-media-uploads/original/1X/123456.png\"></a>"
+
+      # 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

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
+        <a href=\"#{url}\"><img src=\"/secure-media-uploads/original/1X/testimage.png\"></a>
+        HTML
+        md = PrettyText.format_for_email(html, post)
+        expect(md).not_to include('<img')
+        expect(md).to include("Redacted")
+        expect(md).to include("data-stripped-secure-media=\"#{url}\"")
+      end
+
+      it "does not create nested redactions from double processing because of the view media link" do
+        url = "#{Discourse.base_url}\/secure-media-uploads/original/1X/testimage.png"
+        html = <<~HTML
+        <a href=\"#{url}\"><img src=\"/secure-media-uploads/original/1X/testimage.png\"></a>
+        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
+        <img src=\"#{url}\">
+        HTML
+        md = PrettyText.format_for_email(html, post)
+        expect(md).not_to include('<img')
+        expect(md).to include("Redacted")
+        expect(md).to include("data-stripped-secure-media=\"#{url}\"")
+      end
     end
   end
 

spec/mailers/user_notifications_spec.rb

@@ -699,50 +699,6 @@ describe UserNotifications do
     expect(mail.body.to_s).to match(I18n.t("user_notifications.reached_limit", count: 2))
   end
 
-  describe "secure media" do
-    let(:video_upload) { Fabricate(:upload, extension: "mov") }
-    let(:user) { Fabricate(:user) }
-    let(:post) { Fabricate(:post) }
-
-    before do
-      SiteSetting.s3_upload_bucket = "some-bucket-on-s3"
-      SiteSetting.s3_access_key_id = "s3-access-key-id"
-      SiteSetting.s3_secret_access_key = "s3-secret-access-key"
-      SiteSetting.s3_cdn_url = "https://s3.cdn.com"
-      SiteSetting.enable_s3_uploads = true
-      SiteSetting.secure_media = true
-      SiteSetting.login_required = true
-
-      video_upload.update!(url: "#{SiteSetting.s3_cdn_url}/#{Discourse.store.get_path_for_upload(video_upload)}")
-      user.email_logs.create!(
-        email_type: 'blah',
-        to_address: user.email,
-        user_id: user.id
-      )
-    end
-
-    it "replaces secure audio/video with placeholder" do
-      reply = Fabricate(:post, topic_id: post.topic_id, raw: "Video: #{video_upload.url}")
-
-      notification = Fabricate(
-        :notification,
-        topic_id: post.topic_id,
-        post_number: reply.post_number,
-        user: post.user,
-        data: { original_username: 'bob' }.to_json
-      )
-
-      mail = UserNotifications.user_replied(
-        user,
-        post: reply,
-        notification_type: notification.notification_type,
-        notification_data_hash: notification.data_hash
-      )
-
-      expect(mail.body.to_s).to match(I18n.t("emails.secure_media_placeholder"))
-    end
-  end
-
   def expects_build_with(condition)
     UserNotifications.any_instance.expects(:build_email).with(user.email, condition)
     mailer = UserNotifications.public_send(