FIX: Properly attach secure images to email for non-secure uploads (#23865)

There are cases where a user can copy image markdown from a public
post (such as via the discourse-templates plugin) into a PM which
is then sent via an email. Since a PM is a secure context (via the
.with_secure_uploads? check on Post), the image will get a secure
URL in the PM post even though the backing upload is not secure.

This fixes the bug in that case where the image would be stripped
from the email (since it had a /secure-uploads/ URL) but not re-attached
further down the line using the secure_uploads_allow_embed_images_in_emails
setting because the upload itself was not secure.

The flow in Email::Sender for doing this is still not ideal, but
there are chicken and egg problems around when to strip the images,
how to fit in with other attachments and email size limits, and
when to apply the images inline via Email::Styles. It's convoluted,
but at least this fixes the Template use case for now.

app/models/upload.rb

@@ -433,6 +433,10 @@ class Upload < ActiveRecord::Base
     self.sha1_from_base62_encoded($2) if url =~ %r{(upload://)?([a-zA-Z0-9]+)(\..*)?}
   end
 
+  def self.sha1_from_long_url(url)
+    $2 if url =~ URL_REGEX || url =~ OptimizedImage::URL_REGEX
+  end
+
   def self.sha1_from_base62_encoded(encoded_sha1)
     sha1 = Base62.decode(encoded_sha1).to_s(16)
 

lib/email/sender.rb

@@ -129,6 +129,8 @@ module Email
       # always set a default Message ID from the host
       @message.header["Message-ID"] = Email::MessageIdService.generate_default
 
+      post = nil
+      topic = nil
       if topic_id.present? && post_id.present?
         post = Post.find_by(id: post_id, topic_id: topic_id)
 
@@ -138,7 +140,6 @@ module Email
         topic = post.topic
         return skip(SkippedEmailLog.reason_types[:sender_topic_deleted]) if topic.blank?
 
-        add_attachments(post)
         add_identification_field_headers(topic, post)
 
         # See https://www.ietf.org/rfc/rfc2919.txt for the List-ID
@@ -249,6 +250,10 @@ module Email
       # Parse the HTML again so we can make any final changes before
       # sending
       style = Email::Styles.new(@message.html_part.body.to_s)
+      if post.present?
+        @stripped_secure_upload_shas = style.stripped_upload_sha_map.values
+        add_attachments(post)
+      end
 
       # Suppress images from short emails
       if SiteSetting.strip_images_from_short_emails &&
@@ -386,7 +391,17 @@ module Email
     end
 
     def should_attach_image?(upload, optimized_1X = nil)
-      return if !SiteSetting.secure_uploads_allow_embed_images_in_emails || !upload.secure?
+      if !SiteSetting.secure_uploads_allow_embed_images_in_emails ||
+           # Sometimes images in a post have a secure URL but are not secure uploads,
+           # for example if a user uploads an image to a public post then copies the markdown
+           # into a PM which sends an email, so we have to make sure we attached those
+           # stripped images here as well.
+           (
+             !upload.secure? && !@stripped_secure_upload_shas.include?(upload.sha1) &&
+               !@stripped_secure_upload_shas.include?(optimized_1X&.sha1)
+           )
+        return
+      end
       if (optimized_1X&.filesize || upload.filesize) >
            SiteSetting.secure_uploads_max_email_embed_image_size_kb.kilobytes
         return

lib/email/styles.rb

@@ -314,26 +314,36 @@ module Email
       @@plugin_callbacks.each { |block| block.call(@fragment, @opts) }
     end
 
-    def inline_secure_images(attachments, attachments_index)
-      stripped_media = @fragment.css("[data-stripped-secure-media], [data-stripped-secure-upload]")
-      upload_shas = {}
-      stripped_media.each do |div|
-        url = div["data-stripped-secure-media"] || div["data-stripped-secure-upload"]
-        filename = File.basename(url)
-        filename_bare = filename.gsub(File.extname(filename), "")
-        sha1 = filename_bare.partition("_").first
-        upload_shas[url] = sha1
-      end
-      uploads = Upload.select(:original_filename, :sha1).where(sha1: upload_shas.values)
+    def stripped_media
+      @stripped_media ||=
+        @fragment.css("[data-stripped-secure-media], [data-stripped-secure-upload]")
+    end
 
+    def stripped_upload_sha_map
+      @stripped_upload_sha_map ||=
+        begin
+          upload_shas = {}
+          stripped_media.each do |div|
+            url = div["data-stripped-secure-media"] || div["data-stripped-secure-upload"]
+            upload_shas[url] = Upload.sha1_from_long_url(url)
+          end
+          upload_shas
+        end
+    end
+
+    def stripped_secure_image_uploads
+      upload_shas = stripped_upload_sha_map
+      Upload.select(:original_filename, :sha1).where(sha1: upload_shas.values)
+    end
+
+    def inline_secure_images(attachments, attachments_index)
+      uploads = stripped_secure_image_uploads
+      upload_shas = stripped_upload_sha_map
       stripped_media.each do |div|
         upload =
           uploads.find do |upl|
             upl.sha1 ==
-              (
-                upload_shas[div["data-stripped-secure-media"]] ||
-                  upload_shas[div["data-stripped-secure-upload"]]
-              )
+              upload_shas[div["data-stripped-secure-media"] || div["data-stripped-secure-upload"]]
           end
         next if !upload
 

spec/lib/email/sender_spec.rb

@@ -651,7 +651,15 @@ RSpec.describe Email::Sender do
           )
           expect(message.html_part.body).to include("cid:")
           expect(message.html_part.body).to include("embedded-secure-image")
+        end
+
+        it "embeds an image with a secure URL that has an upload that is not secure" do
+          @secure_image.update_secure_status(override: false)
+          Email::Sender.new(message, :valid_type).send
           expect(message.attachments.length).to eq(4)
+          expect(message.attachments["logo.png"].body.raw_source.force_encoding("UTF-8")).to eq(
+            File.read(@secure_image_file),
+          )
         end
 
         it "uses correct UTF-8 encoding for the body of the email" do

spec/lib/email/styles_spec.rb

@@ -294,7 +294,7 @@ RSpec.describe Email::Styles do
     end
   end
 
-  describe "inline_secure_images" do
+  describe "#inline_secure_images" do
     before do
       setup_s3
       SiteSetting.secure_uploads = true

spec/models/upload_spec.rb

@@ -324,6 +324,32 @@ RSpec.describe Upload do
     end
   end
 
+  describe ".sha1_from_long_url" do
+    it "should be able to get the sha1 from a regular upload URL" do
+      expect(
+        Upload.sha1_from_long_url(
+          "https://cdn.test.com/test/original/4X/7/6/5/1b6453892473a467d07372d45eb05abc2031647a.png",
+        ),
+      ).to eq("1b6453892473a467d07372d45eb05abc2031647a")
+    end
+
+    it "should be able to get the sha1 from a secure upload URL" do
+      expect(
+        Upload.sha1_from_long_url(
+          "#{Discourse.base_url}\/secure-uploads/original/1X/1b6453892473a467d07372d45eb05abc2031647a.png",
+        ),
+      ).to eq("1b6453892473a467d07372d45eb05abc2031647a")
+    end
+
+    it "doesn't get a sha1 for a URL that does not match our scheme" do
+      expect(
+        Upload.sha1_from_long_url(
+          "#{Discourse.base_url}\/blah/1b6453892473a467d07372d45eb05abc2031647a.png",
+        ),
+      ).to eq(nil)
+    end
+  end
+
   describe "#base62_sha1" do
     it "should return the right value" do
       upload.update!(sha1: "0000c513e1da04f7b4e99230851ea2aafeb8cc4e")