FIX: Inline secure images with duplicated names (#13926)

Inlining secure images with the same name was not possible because they
were indexed by filename. If an email contained two files with the same
name, only the first image was used for both of them. The other file
was still attached to the email.

lib/email/sender.rb

@@ -26,7 +26,8 @@ module Email
   class Sender
 
     def initialize(message, email_type, user = nil)
-      @message =  message
+      @message = message
+      @message_attachments_index = {}
       @email_type = email_type
       @user = user
     end
@@ -240,7 +241,7 @@ module Email
       # 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)
+        style.inline_secure_images(@message.attachments, @message_attachments_index)
       end
 
       @message.html_part.body = style.to_s
@@ -328,6 +329,7 @@ module Email
             Discourse.store.download(attached_upload).path
           end
 
+          @message_attachments_index[original_upload.sha1] = @message.attachments.size
           @message.attachments[original_upload.original_filename] = File.read(path)
           email_size += File.size(path)
         rescue => e

lib/email/styles.rb

@@ -253,7 +253,7 @@ module Email
       @@plugin_callbacks.each { |block| block.call(@fragment, @opts) }
     end
 
-    def inline_secure_images(attachments)
+    def inline_secure_images(attachments, attachments_index)
       stripped_media = @fragment.css('[data-stripped-secure-media]')
       upload_shas = {}
       stripped_media.each do |div|
@@ -269,10 +269,8 @@ module Email
         upload = uploads.find { |upl| upl.sha1 == upload_shas[div['data-stripped-secure-media']] }
         next if !upload
 
-        original_filename = upload.original_filename
+        if attachments[attachments_index[upload.sha1]]
-
+          url = attachments[attachments_index[upload.sha1]].url
-        if attachments[original_filename]
-          url = attachments[original_filename].url
 
           onebox_type = div['data-onebox-type']
           style = if onebox_type

spec/components/email/sender_spec.rb

@@ -498,6 +498,21 @@ describe Email::Sender do
           SiteSetting.secure_media_allow_embed_images_in_emails = true
         end
 
+        it "can inline images with duplicate names" do
+          @secure_image_2 = UploadCreator.new(file_from_fixtures("logo-dev.png", "images"), "logo.png").create_for(Discourse.system_user.id)
+          @secure_image_2.update_secure_status(override: true)
+          @secure_image_2.update(access_control_post_id: reply.id)
+
+          Jobs::PullHotlinkedImages.any_instance.expects(:execute)
+          reply.update(raw: "#{UploadMarkdown.new(@secure_image).image_markdown}\n#{UploadMarkdown.new(@secure_image_2).image_markdown}")
+          reply.rebake!
+
+          Email::Sender.new(message, :valid_type).send
+          expect(message.attachments.size).to eq(2)
+          expect(message.to_s.scan(/cid:[\w\-@.]+/).length).to eq(2)
+          expect(message.to_s.scan(/cid:[\w\-@.]+/).uniq.length).to eq(2)
+        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)

spec/components/email/styles_spec.rb

@@ -252,8 +252,9 @@ describe Email::Styles do
       SiteSetting.secure_media = true
     end
 
-    let(:attachments) { { 'testimage.png' => stub(url: 'cid:email/test.png') } }
     fab!(:upload) { Fabricate(:upload, original_filename: 'testimage.png', secure: true, sha1: '123456') }
+    let(:attachments) { [stub(url: 'cid:email/test.png')] }
+    let(:attachments_index) { { upload.sha1 => 0 } }
     let(:html) { "<a href=\"#{Discourse.base_url}\/secure-media-uploads/original/1X/123456.png\"><img src=\"/secure-media-uploads/original/1X/123456.png\" width=\"20\" height=\"30\"></a>" }
 
     def strip_and_inline
@@ -265,7 +266,7 @@ describe Email::Styles do
 
       # pass in the attachments to match uploads based on sha + original filename
       styler = Email::Styles.new(html)
-      styler.inline_secure_images(attachments)
+      styler.inline_secure_images(attachments, attachments_index)
       @frag = Nokogiri::HTML5.fragment(styler.to_s)
     end
 
@@ -302,12 +303,8 @@ describe Email::Styles do
       end
 
       let(:siteicon) { Fabricate(:upload, original_filename: "siteicon.ico") }
-      let(:attachments) do
+      let(:attachments) { [stub(url: 'cid:email/test.png'), stub(url: 'cid:email/test2.ico')] }
-        {
+      let(:attachments_index) { { upload.sha1 => 0, siteicon.sha1 => 1 } }
-          'testimage.png' => stub(url: 'cid:email/test.png'),
-          'siteicon.ico' => stub(url: 'cid:email/test2.ico')
-        }
-      end
       let(:html) do
         <<~HTML
 <aside class="onebox allowlistedgeneric">