From b2a8dc4c0fb80a6ef7721b310f2c59eebb218670 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Tue, 29 Mar 2022 10:55:10 +0100
Subject: [PATCH] FIX: Maintain HTML `<img` when downloading remote images
 (#16278)

Under some conditions, replacing an `<img` with `![]()` can break rendering, and make the image disappear.

Context at https://meta.discourse.org/t/152801
---
 app/services/inline_uploads.rb                | 10 +---
 spec/jobs/pull_hotlinked_images_spec.rb       | 24 ++++----
 spec/lib/email/receiver_spec.rb               |  6 +-
 .../services/inline_uploads_multisite_spec.rb |  6 +-
 spec/services/inline_uploads_spec.rb          | 56 +++++++++----------
 5 files changed, 48 insertions(+), 54 deletions(-)

diff --git a/app/services/inline_uploads.rb b/app/services/inline_uploads.rb
index 598fdc9e360..55a162a8ea4 100644
--- a/app/services/inline_uploads.rb
+++ b/app/services/inline_uploads.rb
@@ -204,16 +204,10 @@ class InlineUploads
 
       if src && (external_src || matched_uploads(src).present?)
         upload = uploads&.[](src)
-
-        text = upload&.original_filename || node.attributes["alt"]&.value
-        width = (node.attributes["width"]&.value || upload&.width).to_i
-        height = (node.attributes["height"]&.value || upload&.height).to_i
-        title = node.attributes["title"]&.value
-        text = "#{text}|#{width}x#{height}" if width > 0 && height > 0
-        url = upload&.short_url || PLACEHOLDER
+        node["src"] = upload&.short_url || PLACEHOLDER
 
         spaces_before = match[1].present? ? match[1][/ +$/].size : 0
-        replacement = +"#{" " * spaces_before}![#{text}](#{url}#{title.present? ? " \"#{title}\"" : ""})"
+        replacement = +"#{" " * spaces_before}#{node.to_s}"
 
         yield(match[2], src, replacement, $~.offset(0)[0]) if block_given?
       end
diff --git a/spec/jobs/pull_hotlinked_images_spec.rb b/spec/jobs/pull_hotlinked_images_spec.rb
index 28082f01d22..d41b9d31d89 100644
--- a/spec/jobs/pull_hotlinked_images_spec.rb
+++ b/spec/jobs/pull_hotlinked_images_spec.rb
@@ -67,7 +67,7 @@ describe Jobs::PullHotlinkedImages do
       end.to change { Upload.count }.by(1) &
              change { UserHistory.count }.by(0) # Should not add to the staff log
 
-      expect(post.reload.raw).to eq("![](#{Upload.last.short_url})")
+      expect(post.reload.raw).to eq("<img src=\"#{Upload.last.short_url}\">")
     end
 
     it 'removes downloaded images when they are no longer needed' do
@@ -91,7 +91,7 @@ describe Jobs::PullHotlinkedImages do
         post.rebake!
       end.to change { Upload.count }.by(1)
 
-      expect(post.reload.raw).to eq("![](#{Upload.last.short_url})")
+      expect(post.reload.raw).to eq("<img src=\"#{Upload.last.short_url}\">")
 
       # Post raw is updated back to the old value (e.g. by wordpress integration)
       post.update(raw: "<img src='#{image_url}'>")
@@ -100,7 +100,7 @@ describe Jobs::PullHotlinkedImages do
         post.rebake!
       end.to change { Upload.count }.by(0) # We alread have the upload
 
-      expect(post.reload.raw).to eq("![](#{Upload.last.short_url})")
+      expect(post.reload.raw).to eq("<img src=\"#{Upload.last.short_url}\">")
     end
 
     it 'replaces encoded image urls' do
@@ -110,10 +110,10 @@ describe Jobs::PullHotlinkedImages do
         Jobs::PullHotlinkedImages.new.execute(post_id: post.id)
       end.to change { Upload.count }.by(1)
 
-      expect(post.reload.raw).to eq("![](#{Upload.last.short_url})")
+      expect(post.reload.raw).to eq("<img src=\"#{Upload.last.short_url}\">")
     end
 
-    xit 'replaces images in an anchor tag with weird indentation' do
+    it 'replaces images in an anchor tag with weird indentation' do
       # Skipped pending https://meta.discourse.org/t/152801
       # This spec was previously passing, even though the resulting markdown was invalid
       # Now the spec has been improved, and shows the issue
@@ -124,7 +124,7 @@ describe Jobs::PullHotlinkedImages do
       post = Fabricate(:post, raw: <<~MD)
       <h1></h1>
                                 <a href="https://somelink.com">
-                                    <img alt="somelink" src="#{image_url}" />
+                                    <img alt="somelink" src="#{image_url}">
                                 </a>
       MD
 
@@ -137,7 +137,7 @@ describe Jobs::PullHotlinkedImages do
       expect(post.reload.raw).to eq(<<~MD.chomp)
       <h1></h1>
                                 <a href="https://somelink.com">
-                                    ![somelink](#{upload.short_url})
+                                    <img alt="somelink" src="#{upload.short_url}">
                                 </a>
       MD
     end
@@ -163,7 +163,7 @@ describe Jobs::PullHotlinkedImages do
         Jobs::PullHotlinkedImages.new.execute(post_id: post.id)
       end.to change { Upload.count }.by(1)
 
-      expect(post.reload.raw).to eq("![test](#{Upload.last.short_url})")
+      expect(post.reload.raw).to eq("<img alt=\"test\" src=\"#{Upload.last.short_url}\">")
     end
 
     it 'replaces images without extension' do
@@ -176,7 +176,7 @@ describe Jobs::PullHotlinkedImages do
         Jobs::PullHotlinkedImages.new.execute(post_id: post.id)
       end.to change { Upload.count }.by(1)
 
-      expect(post.reload.raw).to eq("![](#{Upload.last.short_url})")
+      expect(post.reload.raw).to eq("<img src=\"#{Upload.last.short_url}\">")
     end
 
     it 'replaces optimized images' do
@@ -195,7 +195,7 @@ describe Jobs::PullHotlinkedImages do
       upload = Upload.last
       post.reload
 
-      expect(post.raw).to eq("![](#{upload.short_url})")
+      expect(post.raw).to eq("<img src=\"#{Upload.last.short_url}\">")
       expect(post.uploads).to contain_exactly(upload)
     end
 
@@ -385,7 +385,7 @@ describe Jobs::PullHotlinkedImages do
         post.reload
 
         expect(post.raw).to eq(<<~MD.chomp)
-        ![](upload://z2QSs1KJWoj51uYhDjb6ifCzxH6.gif)
+        <img src="upload://z2QSs1KJWoj51uYhDjb6ifCzxH6.gif">
         https://commons.wikimedia.org/wiki/File:Brisbane_May_2013201.jpg
         <img src='#{broken_image_url}'>
         <a href='#{url}'><img src='#{large_image_url}'></a>
@@ -525,7 +525,7 @@ describe Jobs::PullHotlinkedImages do
 
       post.reload
 
-      expect(post.raw).to eq("![](#{Upload.last.short_url})")
+      expect(post.raw).to eq("<img src=\"#{Upload.last.short_url}\">")
       expect(post.uploads.count).to eq(1)
     end
 
diff --git a/spec/lib/email/receiver_spec.rb b/spec/lib/email/receiver_spec.rb
index b690e6d26de..eb441e5cff5 100644
--- a/spec/lib/email/receiver_spec.rb
+++ b/spec/lib/email/receiver_spec.rb
@@ -614,7 +614,7 @@ describe Email::Receiver do
       expect(post.raw).to eq(<<~MD.chomp)
       **Before**
 
-      ![#{upload.original_filename}|#{upload.width}x#{upload.height}](#{upload.short_url})
+      <img src="#{upload.short_url}" alt="内嵌图片 1">
 
       *After*
       MD
@@ -648,7 +648,7 @@ describe Email::Receiver do
       <details class='elided'>
       <summary title='Show trimmed content'>&#183;&#183;&#183;</summary>
 
-      ![logo.png|300x200](upload://qUm0DGR49PAZshIi7HxMd3cAlzn.png)
+      <img src="upload://qUm0DGR49PAZshIi7HxMd3cAlzn.png" width="300" height="200">
 
       </details>
       MD
@@ -673,7 +673,7 @@ describe Email::Receiver do
       expect(post.raw).to eq(<<~MD.chomp)
       Picture below.
 
-      ![#{upload.original_filename}|#{upload.width}x#{upload.height}](#{upload.short_url})
+      <img apple-inline="yes" id="06C04C58-783E-4753-9B6B-D57403903060" src="#{upload.short_url}" class="">
 
       Picture above.
       MD
diff --git a/spec/services/inline_uploads_multisite_spec.rb b/spec/services/inline_uploads_multisite_spec.rb
index 0c29dc0eb71..042479e583a 100644
--- a/spec/services/inline_uploads_multisite_spec.rb
+++ b/spec/services/inline_uploads_multisite_spec.rb
@@ -31,9 +31,9 @@ RSpec.describe InlineUploads, type: :multisite do
         #{Discourse.base_url}#{upload2.short_path} #{Discourse.base_url}#{upload2.short_path}
         #{Discourse.base_url}#{upload2.short_path}
 
-        ![some image](#{upload.short_url})
-        ![some image](#{upload2.short_url})
-        ![](#{upload3.short_url})
+        <img src="#{upload.short_url}" alt="some image">
+        <img src="#{upload2.short_url}" alt="some image">
+        <img src="#{upload3.short_url}">
         MD
       end
     end
diff --git a/spec/services/inline_uploads_spec.rb b/spec/services/inline_uploads_spec.rb
index f47f3fa7203..152b1b20537 100644
--- a/spec/services/inline_uploads_spec.rb
+++ b/spec/services/inline_uploads_spec.rb
@@ -46,7 +46,7 @@ RSpec.describe InlineUploads do
         MD
 
         expect(InlineUploads.process(md)).to eq(<<~MD)
-        ![](#{upload.short_url})
+        <img src="#{upload.short_url}">
 
         This is an invalid `<img ...>` tag
         MD
@@ -102,7 +102,7 @@ RSpec.describe InlineUploads do
         ![](#{upload3.short_url})
         [/quote]
 
-        ![](#{upload2.short_url})
+        <img src="#{upload2.short_url}">
         MD
       end
 
@@ -173,7 +173,7 @@ RSpec.describe InlineUploads do
         expect(InlineUploads.process(md)).to eq(<<~MD)
         [img]http://some.external.img[/img]
         ![](#{upload.short_url})
-        ![](#{upload3.short_url})
+        <img src="#{upload3.short_url}">
 
         ![](#{upload2.short_url})
 
@@ -202,7 +202,7 @@ RSpec.describe InlineUploads do
 
         This is a [link1][1] test [link2][2] something
 
-        ![](#{upload.short_url})
+        <img src="#{upload.short_url}">
 
         [1]: #{Discourse.base_url}#{upload.short_path}
         [2]: #{Discourse.base_url}#{upload2.short_path}
@@ -218,7 +218,7 @@ RSpec.describe InlineUploads do
 
         expect(InlineUploads.process(md)).to eq(<<~MD)
         ![](#{upload.short_url})
-        ![](#{upload2.short_url})
+        <img src="#{upload2.short_url}">
         [Text|attachment](#{upload3.short_url})
         MD
       end
@@ -237,11 +237,11 @@ RSpec.describe InlineUploads do
         MD
 
         expect(InlineUploads.process(md)).to eq(<<~MD)
-        ![](#{upload.short_url})
+        <img src="#{upload.short_url}">
 
-        ![](#{upload.short_url})
+        <img src="#{upload.short_url}">
 
-        ![](#{upload.short_url})
+        <img src="#{upload.short_url}">
 
         ![](#{upload.short_url})
 
@@ -262,7 +262,7 @@ RSpec.describe InlineUploads do
           MD
 
           expect(InlineUploads.process(md)).to eq(<<~MD)
-            ![](#{upload.short_url})
+            <img src="#{upload.short_url}">
 
             ![](#{upload.short_url})
           MD
@@ -319,7 +319,7 @@ RSpec.describe InlineUploads do
         MD
 
         expect(InlineUploads.process(md)).to eq(<<~MD)
-        test![](#{upload.short_url})
+        test<img src="#{upload.short_url}">
         MD
       end
 
@@ -371,13 +371,13 @@ RSpec.describe InlineUploads do
         ![image](#{upload2.short_url})
         ![image|100x100](#{upload3.short_url})
 
-        ![some image](#{upload.short_url} "some title")
-        ![some image](#{upload2.short_url})![some image](#{upload3.short_url})
+        <img src="#{upload.short_url}" alt="some image" title="some title">
+        <img src="#{upload2.short_url}" alt="some image"><img src="#{upload3.short_url}" alt="some image">
 
         #{Discourse.base_url}#{upload3.short_path} #{Discourse.base_url}#{upload3.short_path}
 
-        ![|5x4](#{upload.short_url})
-        ![](#{upload.short_url})
+        <img src="#{upload.short_url}" width="5" height="4">
+        <img src="#{upload.short_url}" width="5px" height="auto">
 
         `<img src="#{upload.url}" alt="image inside code quotes">`
 
@@ -421,7 +421,7 @@ RSpec.describe InlineUploads do
         expect(InlineUploads.process(md)).to eq(<<~MD)
         <h1></h1>
                         <a href="http://somelink.com">
-                          ![test|500x500](#{upload2.short_url})
+                          <img src="#{upload2.short_url}" alt="test" width="500" height="500">
                         </a>
 
                         <a href="http://somelink.com">
@@ -431,7 +431,7 @@ RSpec.describe InlineUploads do
 
         md = "<h1></h1>\r\n<a href=\"http://somelink.com\">\r\n        <img src=\"#{upload.url}\" alt=\"test\" width=\"500\" height=\"500\">\r\n</a>"
 
-        expect(InlineUploads.process(md)).to eq("<h1></h1>\r\n<a href=\"http://somelink.com\">\r\n        ![test|500x500](#{upload.short_url})\r\n</a>")
+        expect(InlineUploads.process(md)).to eq("<h1></h1>\r\n<a href=\"http://somelink.com\">\r\n        <img src=\"#{upload.short_url}\" alt=\"test\" width=\"500\" height=\"500\">\r\n</a>")
       end
 
       it "should correctly update image sources within anchor or paragraph tags" do
@@ -463,28 +463,28 @@ RSpec.describe InlineUploads do
 
         expect(InlineUploads.process(md)).to eq(<<~MD)
         <a href="http://somelink.com">
-          ![test|500x500](#{upload.short_url})
+          <img src="#{upload.short_url}" alt="test" width="500" height="500">
         </a>
 
         <p>
-          ![test](#{upload2.short_url})
+          <img src="#{upload2.short_url}" alt="test">
         </p>
 
-        <a href="http://somelink.com">![test|500x500](#{upload3.short_url})</a>
+        <a href="http://somelink.com"><img src="#{upload3.short_url}" alt="test" width="500" height="500"></a>
 
-        <a href="http://somelink.com">  ![test|500x500](#{upload.short_url})  </a>
+        <a href="http://somelink.com">  <img src="#{upload.short_url}" alt="test" width="500" height="500">  </a>
 
         <a href="http://somelink.com">
 
 
-        ![test|500x500](#{upload.short_url})
+        <img src="#{upload.short_url}" alt="test" width="500" height="500">
 
         </a>
 
-        <p>Test ![test|500x500](#{upload2.short_url})</p>
+        <p>Test <img src="#{upload2.short_url}" alt="test" width="500" height="500"></p>
 
         <hr/>
-        ![test|500x500](#{upload2.short_url})
+        <img src="#{upload2.short_url}" alt="test" width="500" height="500">
         MD
       end
 
@@ -508,9 +508,9 @@ RSpec.describe InlineUploads do
         This is some <img src=" and <a href="
         ```
 
-        ![test](#{upload.short_url})
+        <img src="#{upload.short_url}" alt="test">
 
-        ![test](#{upload2.short_url})
+        <img src="#{upload2.short_url}" alt="test" height="150<img">
 
         > some quote
 
@@ -671,9 +671,9 @@ RSpec.describe InlineUploads do
 
         expect(InlineUploads.process(md)).to eq(<<~MD)
         ![](#{upload.short_url})
-        ![some image](#{upload.short_url})
-        test![some image](#{upload2.short_url})test
-        ![some image](#{upload2.short_url})
+        <img src="#{upload.short_url}" alt="some image">
+        test<img src="#{upload2.short_url}" alt="some image">test
+        <img src="#{upload2.short_url}" alt="some image">
         MD
       end