From 61c87fb59f03999d9e7fe6f9c056e44ed984f6a3 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 17 Oct 2023 14:08:21 +1000 Subject: [PATCH] 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 | 4 ++++ lib/email/sender.rb | 19 +++++++++++++++-- lib/email/styles.rb | 40 ++++++++++++++++++++++------------- spec/lib/email/sender_spec.rb | 8 +++++++ spec/lib/email/styles_spec.rb | 2 +- spec/models/upload_spec.rb | 26 +++++++++++++++++++++++ 6 files changed, 81 insertions(+), 18 deletions(-) diff --git a/app/models/upload.rb b/app/models/upload.rb index cea39988f8e..60b39315c3c 100644 --- a/app/models/upload.rb +++ b/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) diff --git a/lib/email/sender.rb b/lib/email/sender.rb index fceb484322b..abf39dc919b 100644 --- a/lib/email/sender.rb +++ b/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 diff --git a/lib/email/styles.rb b/lib/email/styles.rb index d1976f3c517..c248e9cc595 100644 --- a/lib/email/styles.rb +++ b/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 diff --git a/spec/lib/email/sender_spec.rb b/spec/lib/email/sender_spec.rb index b88f95712e5..fb79a136873 100644 --- a/spec/lib/email/sender_spec.rb +++ b/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 diff --git a/spec/lib/email/styles_spec.rb b/spec/lib/email/styles_spec.rb index 0363358a7f5..549d94701ca 100644 --- a/spec/lib/email/styles_spec.rb +++ b/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 diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 6ac71da0f8c..e1fab6b426b 100644 --- a/spec/models/upload_spec.rb +++ b/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")