From 27e94f2f9812ec354901bef6442f2316be88fcc7 Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Tue, 10 Nov 2020 12:55:18 +1000
Subject: [PATCH] FIX: Make secure image onebox check more robust (#11179)

When embedding secure images which have been oneboxed, we checked to see if the image's parent's parent had the class onebox-body. This was not always effective as if the image does not get resized/optimized then it does not have the aspect-image div wrapping it. This would cause the image to embed in the email but be huge.

This PR changes the check to see if any of the image's ancestors have the class onebox-body, or if the image has the onebox-avatar class to account for variations in HTML structure.
---
 lib/pretty_text.rb                   | 13 ++++++++---
 spec/components/email/styles_spec.rb | 32 ++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb
index 0ed10099efd..8dea3107554 100644
--- a/lib/pretty_text.rb
+++ b/lib/pretty_text.rb
@@ -449,9 +449,16 @@ module PrettyText
         img['src']
       end
 
-      width = img.classes.include?('site-icon') ? 16 : img['width']
-      height = img.classes.include?('site-icon') ? 16 : img['height']
-      oneboxed = (img.parent&.parent&.classes || []).include?('onebox-body')
+      width = img['width']
+      height = img['height']
+      oneboxed = img.ancestors.css(".onebox-body").any? || img.classes.include?("onebox-avatar")
+
+      # we always want this to be tiny and without any special styles
+      if img.classes.include?('site-icon')
+        oneboxed = false
+        width = 16
+        height = 16
+      end
 
       if Upload.secure_media_url?(url)
         img.add_next_sibling secure_media_placeholder(doc, url, oneboxed: oneboxed, width: width, height: height)
diff --git a/spec/components/email/styles_spec.rb b/spec/components/email/styles_spec.rb
index 8776d5019a4..382f1a11d64 100644
--- a/spec/components/email/styles_spec.rb
+++ b/spec/components/email/styles_spec.rb
@@ -330,6 +330,38 @@ describe Email::Styles do
         expect(@frag.css('[data-embedded-secure-image]')[0].attr('style')).to eq('width: 16px; height: 16px;')
         expect(@frag.css('[data-embedded-secure-image]')[1].attr('style')).to eq('width: 60px; max-height: 80%; max-width: 20%; height: auto; float: left; margin-right: 10px;')
       end
+
+      context "when inlining a oneboxed image with a direct parent of onebox-body" do
+        let(:html) do
+          <<~HTML
+<aside class="onebox allowlistedgeneric">
+  <header class="source">
+      <img src="#{Discourse.base_url}/secure-media-uploads/original/1X/#{siteicon.sha1}.ico" class="site-icon" width="64" height="64">
+      <a href="https://test.com/article" target="_blank" rel="noopener" title="02:33PM - 24 October 2020">Test</a>
+  </header>
+  <article class="onebox-body">
+    <img src="#{Discourse.base_url}/secure-media-uploads/original/1X/123456.png" class="thumbnail onebox-avatar" width="20" height="30">
+
+<h3><a href="https://test.com/article" target="_blank" rel="noopener">Test</a></h3>
+
+<p>This is a test onebox.</p>
+
+  </article>
+  <div class="onebox-metadata">
+  </div>
+  <div style="clear: both"></div>
+</aside>
+          HTML
+        end
+
+        it "keeps the special onebox styles" do
+          strip_and_inline
+          expect(@frag.to_s).to include("cid:email/test.png")
+          expect(@frag.to_s).to include("cid:email/test2.ico")
+          expect(@frag.css('[data-sripped-secure-media]')).not_to be_present
+          expect(@frag.css('[data-embedded-secure-image]')[1].attr('style')).to eq('width: 60px; max-height: 80%; max-width: 20%; height: auto; float: left; margin-right: 10px;')
+        end
+      end
     end
   end
 end