From c1f3bd6a1c4b9f0b4df0b0da7032e38b9641709d Mon Sep 17 00:00:00 2001
From: Penar Musaraj <pmusaraj@gmail.com>
Date: Wed, 4 Nov 2020 15:45:50 -0500
Subject: [PATCH] FIX: secure_media stripping on lightboxes, non-image links
 (#11121)

- Fixes stripping of lightboxes with empty srcset attribute
- Does not fail when email has links with secure media URLs but no child image elements
---
 lib/pretty_text.rb                   | 14 ++++++++++----
 spec/components/email/styles_spec.rb | 25 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb
index ba25ed4921a..0ed10099efd 100644
--- a/lib/pretty_text.rb
+++ b/lib/pretty_text.rb
@@ -414,12 +414,18 @@ module PrettyText
       target = non_image_media ? a.parent : a
       next if target.to_s.include?('stripped-secure-view-media')
 
+      next if a.css('img[src]').empty? && !non_image_media
+
       if a.classes.include?('lightbox')
-        # we are using the first image from the srcset here so we get the
-        # optimized image instead of the possibly huge original
         img = a.css('img[src]').first
-        srcset = img.attributes['srcset'].value
-        url = srcset.split(',').first
+        srcset = img&.attributes['srcset']&.value
+        if srcset
+          # if available, use the first image from the srcset here
+          # so we get the optimized image instead of the possibly huge original
+          url = srcset.split(',').first
+        else
+          url = img['src']
+        end
         a.add_next_sibling secure_media_placeholder(doc, url, width: img['width'], height: img['height'])
         a.remove
       else
diff --git a/spec/components/email/styles_spec.rb b/spec/components/email/styles_spec.rb
index 84a1e3e204b..8776d5019a4 100644
--- a/spec/components/email/styles_spec.rb
+++ b/spec/components/email/styles_spec.rb
@@ -210,6 +210,31 @@ describe Email::Styles do
       frag = html_fragment("<a href=\"#{Discourse.base_url}\/t/secure-media-uploads/235723\">Visit Topic</a>")
       expect(frag.to_s).not_to include("Redacted")
     end
+
+    it "works in lightboxes with missing srcset attribute" do
+      frag = html_fragment("<a href=\"#{Discourse.base_url}\/secure-media-uploads/original/1X/testimage.png\" class=\"lightbox\"><img src=\"/secure-media-uploads/original/1X/testimage.png\"></a>")
+      expect(frag.at('img')).not_to be_present
+      expect(frag.to_s).to include("Redacted")
+    end
+
+    it "works in lightboxes with srcset attribute set" do
+      frag = html_fragment(
+        <<~HTML
+          <a href="#{Discourse.base_url}/secure-media-uploads/original/1X/testimage.png" class="lightbox">
+            <img src="/secure-media-uploads/original/1X/testimage.png" srcset="/secure-media-uploads/optimized/1X/testimage.png, /secure-media-uploads/original/1X/testimage.png 1.5x" />
+          </a>
+        HTML
+      )
+
+      expect(frag.at('img')).not_to be_present
+      expect(frag.to_s).to include("Redacted")
+    end
+
+    it "skips links with no images as children" do
+      frag = html_fragment("<a href=\"#{Discourse.base_url}\/secure-media-uploads/original/1X/testimage.png\"><span>Clearly not an image</span></a>")
+      expect(frag.to_s).to include("not an image")
+    end
+
   end
 
   context "inline_secure_images" do