From b2481adb40d584e2bd01eab0afcb4336880f187b Mon Sep 17 00:00:00 2001
From: Krzysztof Kotlarek <kotlarek.krzysztof@gmail.com>
Date: Thu, 22 Oct 2020 13:25:09 +1100
Subject: [PATCH] FIX: persist secure image width and height if is given
 (#10994)

`max-width: 50%; max-height: 400px;` is a good fallback, however, if width and height are given and are smaller than fallback -  we should persist that smaller size.
---
 lib/email/styles.rb                  | 16 ++++++++++++++--
 lib/pretty_text.rb                   | 12 ++++++++----
 spec/components/email/styles_spec.rb |  3 ++-
 spec/components/pretty_text_spec.rb  |  4 +++-
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/lib/email/styles.rb b/lib/email/styles.rb
index 9beebe02a3b..8e79a475767 100644
--- a/lib/email/styles.rb
+++ b/lib/email/styles.rb
@@ -6,6 +6,8 @@
 #
 module Email
   class Styles
+    MAX_IMAGE_DIMENSION = 400
+
     @@plugin_callbacks = []
 
     attr_accessor :fragment
@@ -220,7 +222,7 @@ module Email
       onebox_styles
       plugin_styles
 
-      style('.post-excerpt img', "max-width: 50%; max-height: 400px;")
+      style('.post-excerpt img', "max-width: 50%; max-height: #{MAX_IMAGE_DIMENSION}px;")
 
       format_custom
     end
@@ -257,7 +259,7 @@ module Email
           url = attachments[original_filename].url
 
           div.add_next_sibling(
-            "<img src=\"#{url}\" data-embedded-secure-image=\"true\" style=\"max-width: 50%; max-height: 400px;\" />"
+            "<img src=\"#{url}\" data-embedded-secure-image=\"true\" style=\"#{calculate_width_and_height_style(div)}\" />"
           )
           div.remove
         end
@@ -320,6 +322,16 @@ module Email
       end
     end
 
+    def calculate_width_and_height_style(div)
+      width = div['data-width']
+      height = div['data-height']
+      if width.present? && height.present? && height.to_i < MAX_IMAGE_DIMENSION && width.to_i < MAX_IMAGE_DIMENSION
+        "width: #{width}px; height: #{height}px;"
+      else
+        "max-width: 50%; max-height: #{MAX_IMAGE_DIMENSION}px;"
+      end
+    end
+
     def replace_secure_media_urls
       # strip again, this can be done at a lower level like in the user
       # notification template but that may not catch everything
diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb
index da60a75436f..30e567ea3f6 100644
--- a/lib/pretty_text.rb
+++ b/lib/pretty_text.rb
@@ -409,21 +409,25 @@ module PrettyText
       if Upload.secure_media_url?(a["href"])
         target = %w(video audio).include?(a&.parent&.name) ? a.parent : a
         next if target.to_s.include?("stripped-secure-view-media")
-        target.add_next_sibling secure_media_placeholder(doc, a['href'])
+        width = a.xpath("//*[@width]").attr("width")&.value
+        height = a.xpath("//*[@height]").attr("height")&.value
+        target.add_next_sibling secure_media_placeholder(doc, a['href'], width: width, height: height)
         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.add_next_sibling secure_media_placeholder(doc, img['src'], width: img['width'], height: img['height'])
         img.remove
       end
     end
   end
 
-  def self.secure_media_placeholder(doc, url)
+  def self.secure_media_placeholder(doc, url, width: nil, height: nil)
+    data_width = width ? "data-width=#{width}" : ''
+    data_height = height ? "data-height=#{height}" : ''
     <<~HTML
-    <div class="secure-media-notice" data-stripped-secure-media="#{url}">
+    <div class="secure-media-notice" data-stripped-secure-media="#{url}" #{data_width} #{data_height}>
       #{I18n.t('emails.secure_media_placeholder')} <a class='stripped-secure-view-media' href="#{url}">#{I18n.t("emails.view_redacted_media")}</a>.
     </div>
     HTML
diff --git a/spec/components/email/styles_spec.rb b/spec/components/email/styles_spec.rb
index e42a44db78f..db57d7715cf 100644
--- a/spec/components/email/styles_spec.rb
+++ b/spec/components/email/styles_spec.rb
@@ -212,7 +212,7 @@ describe Email::Styles do
     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>"
+      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>"
 
       # strip out the secure media
       styler = Email::Styles.new(html)
@@ -230,6 +230,7 @@ describe Email::Styles 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
+      expect(@frag.children.attr('style').value).to eq("width: 20px; height: 30px;")
     end
 
     it "does not inline anything if the upload cannot be found" do
diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb
index 6d03f362fdf..887dcfba9c1 100644
--- a/spec/components/pretty_text_spec.rb
+++ b/spec/components/pretty_text_spec.rb
@@ -966,12 +966,14 @@ describe PrettyText do
       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}\">
+        <img src=\"#{url}\" width=\"20\" height=\"20\">
         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}\"")
+        expect(md).to include("data-width=\"20\"")
+        expect(md).to include("data-height=\"20\"")
       end
     end
   end