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

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.

diff --git a/lib/email/sender.rb b/lib/email/sender.rb
index aafd834..456f205 100644
--- a/lib/email/sender.rb
+++ b/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
diff --git a/lib/email/styles.rb b/lib/email/styles.rb
index 5938dff..c848ecf 100644
--- a/lib/email/styles.rb
+++ b/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[original_filename]
-          url = attachments[original_filename].url
+        if attachments[attachments_index[upload.sha1]]
+          url = attachments[attachments_index[upload.sha1]].url
 
           onebox_type = div['data-onebox-type']
           style = if onebox_type
diff --git a/spec/components/email/sender_spec.rb b/spec/components/email/sender_spec.rb
index 34c1a2e..7d6643d 100644
--- a/spec/components/email/sender_spec.rb
+++ b/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)
diff --git a/spec/components/email/styles_spec.rb b/spec/components/email/styles_spec.rb
index 55fe518..be5b634 100644
--- a/spec/components/email/styles_spec.rb
+++ b/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
-        {
-          'testimage.png' => stub(url: 'cid:email/test.png'),
-          'siteicon.ico' => stub(url: 'cid:email/test2.ico')
-        }
-      end
+      let(:attachments) { [stub(url: 'cid:email/test.png'), stub(url: 'cid:email/test2.ico')] }
+      let(:attachments_index) { { upload.sha1 => 0, siteicon.sha1 => 1 } }
       let(:html) do
         <<~HTML
 <aside class="onebox allowlistedgeneric">

GitHub sha: 52520638caee89a6fc9692ab10e679f731587b13

This commit appears in #13926 which was approved by eviltrout and ZogStriP. It was merged by nbianca.