github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2024-11-23 02:19:27 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

FIX: Maintain HTML <img when downloading remote images (#16278)

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

Context at https://meta.discourse.org/t/152801
This commit is contained in:
David Taylor 2022-03-29 10:55:10 +01:00 committed by GitHub
parent 8e5614b1bf
commit b2a8dc4c0f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 48 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
app/services/inline_uploads.rb
View File

@ -204,16 +204,10 @@ class InlineUploads
if src && (external_src || matched_uploads(src).present?) if src && (external_src || matched_uploads(src).present?)
upload = uploads&.[](src) upload = uploads&.[](src)
node["src"] = upload&.short_url || PLACEHOLDER
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
spaces_before = match[1].present? ? match[1][/ +$/].size : 0 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? yield(match[2], src, replacement, $~.offset(0)[0]) if block_given?
end end

24
spec/jobs/pull_hotlinked_images_spec.rb
View File

@ -67,7 +67,7 @@ describe Jobs::PullHotlinkedImages do
end.to change { Upload.count }.by(1) & end.to change { Upload.count }.by(1) &
change { UserHistory.count }.by(0) # Should not add to the staff log 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 end
it 'removes downloaded images when they are no longer needed' do it 'removes downloaded images when they are no longer needed' do
@ -91,7 +91,7 @@ describe Jobs::PullHotlinkedImages do
post.rebake! post.rebake!
end.to change { Upload.count }.by(1) 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 raw is updated back to the old value (e.g. by wordpress integration)
post.update(raw: "<img src='#{image_url}'>") post.update(raw: "<img src='#{image_url}'>")
@ -100,7 +100,7 @@ describe Jobs::PullHotlinkedImages do
post.rebake! post.rebake!
end.to change { Upload.count }.by(0) # We alread have the upload 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 end
it 'replaces encoded image urls' do it 'replaces encoded image urls' do
@ -110,10 +110,10 @@ describe Jobs::PullHotlinkedImages do
Jobs::PullHotlinkedImages.new.execute(post_id: post.id) Jobs::PullHotlinkedImages.new.execute(post_id: post.id)
end.to change { Upload.count }.by(1) 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 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 # Skipped pending https://meta.discourse.org/t/152801
# This spec was previously passing, even though the resulting markdown was invalid # This spec was previously passing, even though the resulting markdown was invalid
# Now the spec has been improved, and shows the issue # Now the spec has been improved, and shows the issue
@ -124,7 +124,7 @@ describe Jobs::PullHotlinkedImages do
post = Fabricate(:post, raw: <<~MD) post = Fabricate(:post, raw: <<~MD)
<h1></h1> <h1></h1>
<a href="https://somelink.com"> <a href="https://somelink.com">
<img alt="somelink" src="#{image_url}" /> <img alt="somelink" src="#{image_url}">
</a> </a>
MD MD
@ -137,7 +137,7 @@ describe Jobs::PullHotlinkedImages do
expect(post.reload.raw).to eq(<<~MD.chomp) expect(post.reload.raw).to eq(<<~MD.chomp)
<h1></h1> <h1></h1>
<a href="https://somelink.com"> <a href="https://somelink.com">
![somelink](#{upload.short_url}) <img alt="somelink" src="#{upload.short_url}">
</a> </a>
MD MD
end end
@ -163,7 +163,7 @@ describe Jobs::PullHotlinkedImages do
Jobs::PullHotlinkedImages.new.execute(post_id: post.id) Jobs::PullHotlinkedImages.new.execute(post_id: post.id)
end.to change { Upload.count }.by(1) 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 end
it 'replaces images without extension' do it 'replaces images without extension' do
@ -176,7 +176,7 @@ describe Jobs::PullHotlinkedImages do
Jobs::PullHotlinkedImages.new.execute(post_id: post.id) Jobs::PullHotlinkedImages.new.execute(post_id: post.id)
end.to change { Upload.count }.by(1) 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 end
it 'replaces optimized images' do it 'replaces optimized images' do
@ -195,7 +195,7 @@ describe Jobs::PullHotlinkedImages do
upload = Upload.last upload = Upload.last
post.reload 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) expect(post.uploads).to contain_exactly(upload)
end end
@ -385,7 +385,7 @@ describe Jobs::PullHotlinkedImages do
post.reload post.reload
expect(post.raw).to eq(<<~MD.chomp) 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 https://commons.wikimedia.org/wiki/File:Brisbane_May_2013201.jpg
<img src='#{broken_image_url}'> <img src='#{broken_image_url}'>
<a href='#{url}'><img src='#{large_image_url}'></a> <a href='#{url}'><img src='#{large_image_url}'></a>
@ -525,7 +525,7 @@ describe Jobs::PullHotlinkedImages do
post.reload 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) expect(post.uploads.count).to eq(1)
end end

6
spec/lib/email/receiver_spec.rb
View File

@ -614,7 +614,7 @@ describe Email::Receiver do
expect(post.raw).to eq(<<~MD.chomp) expect(post.raw).to eq(<<~MD.chomp)
**Before** **Before**
![#{upload.original_filename}|#{upload.width}x#{upload.height}](#{upload.short_url}) <img src="#{upload.short_url}" alt="内嵌图片 1">
*After* *After*
MD MD
@ -648,7 +648,7 @@ describe Email::Receiver do
<details class='elided'> <details class='elided'>
<summary title='Show trimmed content'>&#183;&#183;&#183;</summary> <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> </details>
MD MD
@ -673,7 +673,7 @@ describe Email::Receiver do
expect(post.raw).to eq(<<~MD.chomp) expect(post.raw).to eq(<<~MD.chomp)
Picture below. 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. Picture above.
MD MD

6
spec/services/inline_uploads_multisite_spec.rb
View File

@ -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} #{Discourse.base_url}#{upload2.short_path}
#{Discourse.base_url}#{upload2.short_path} #{Discourse.base_url}#{upload2.short_path}
![some image](#{upload.short_url}) <img src="#{upload.short_url}" alt="some image">
![some image](#{upload2.short_url}) <img src="#{upload2.short_url}" alt="some image">
![](#{upload3.short_url}) <img src="#{upload3.short_url}">
MD MD
end end
end end

56
spec/services/inline_uploads_spec.rb
View File

@ -46,7 +46,7 @@ RSpec.describe InlineUploads do
MD MD
expect(InlineUploads.process(md)).to eq(<<~MD) expect(InlineUploads.process(md)).to eq(<<~MD)
![](#{upload.short_url}) <img src="#{upload.short_url}">
This is an invalid `<img ...>` tag This is an invalid `<img ...>` tag
MD MD
@ -102,7 +102,7 @@ RSpec.describe InlineUploads do
![](#{upload3.short_url}) ![](#{upload3.short_url})
[/quote] [/quote]
![](#{upload2.short_url}) <img src="#{upload2.short_url}">
MD MD
end end
@ -173,7 +173,7 @@ RSpec.describe InlineUploads do
expect(InlineUploads.process(md)).to eq(<<~MD) expect(InlineUploads.process(md)).to eq(<<~MD)
[img]http://some.external.img[/img] [img]http://some.external.img[/img]
![](#{upload.short_url}) ![](#{upload.short_url})
![](#{upload3.short_url}) <img src="#{upload3.short_url}">
![](#{upload2.short_url}) ![](#{upload2.short_url})
@ -202,7 +202,7 @@ RSpec.describe InlineUploads do
This is a [link1][1] test [link2][2] something This is a [link1][1] test [link2][2] something
![](#{upload.short_url}) <img src="#{upload.short_url}">
[1]: #{Discourse.base_url}#{upload.short_path} [1]: #{Discourse.base_url}#{upload.short_path}
[2]: #{Discourse.base_url}#{upload2.short_path} [2]: #{Discourse.base_url}#{upload2.short_path}
@ -218,7 +218,7 @@ RSpec.describe InlineUploads do
expect(InlineUploads.process(md)).to eq(<<~MD) expect(InlineUploads.process(md)).to eq(<<~MD)
![](#{upload.short_url}) ![](#{upload.short_url})
![](#{upload2.short_url}) <img src="#{upload2.short_url}">
[Text|attachment](#{upload3.short_url}) [Text|attachment](#{upload3.short_url})
MD MD
end end
@ -237,11 +237,11 @@ RSpec.describe InlineUploads do
MD MD
expect(InlineUploads.process(md)).to eq(<<~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}) ![](#{upload.short_url})
@ -262,7 +262,7 @@ RSpec.describe InlineUploads do
MD MD
expect(InlineUploads.process(md)).to eq(<<~MD) expect(InlineUploads.process(md)).to eq(<<~MD)
![](#{upload.short_url}) <img src="#{upload.short_url}">
![](#{upload.short_url}) ![](#{upload.short_url})
MD MD
@ -319,7 +319,7 @@ RSpec.describe InlineUploads do
MD MD
expect(InlineUploads.process(md)).to eq(<<~MD) expect(InlineUploads.process(md)).to eq(<<~MD)
test![](#{upload.short_url}) test<img src="#{upload.short_url}">
MD MD
end end
@ -371,13 +371,13 @@ RSpec.describe InlineUploads do
![image](#{upload2.short_url}) ![image](#{upload2.short_url})
![image|100x100](#{upload3.short_url}) ![image|100x100](#{upload3.short_url})
![some image](#{upload.short_url} "some title") <img src="#{upload.short_url}" alt="some image" title="some title">
![some image](#{upload2.short_url})![some image](#{upload3.short_url}) <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} #{Discourse.base_url}#{upload3.short_path} #{Discourse.base_url}#{upload3.short_path}
![|5x4](#{upload.short_url}) <img src="#{upload.short_url}" width="5" height="4">
![](#{upload.short_url}) <img src="#{upload.short_url}" width="5px" height="auto">
`<img src="#{upload.url}" alt="image inside code quotes">` `<img src="#{upload.url}" alt="image inside code quotes">`
@ -421,7 +421,7 @@ RSpec.describe InlineUploads do
expect(InlineUploads.process(md)).to eq(<<~MD) expect(InlineUploads.process(md)).to eq(<<~MD)
<h1></h1> <h1></h1>
<a href="http://somelink.com"> <a href="http://somelink.com">
![test|500x500](#{upload2.short_url}) <img src="#{upload2.short_url}" alt="test" width="500" height="500">
</a> </a>
<a href="http://somelink.com"> <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>" 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 end
it "should correctly update image sources within anchor or paragraph tags" do 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) expect(InlineUploads.process(md)).to eq(<<~MD)
<a href="http://somelink.com"> <a href="http://somelink.com">
![test|500x500](#{upload.short_url}) <img src="#{upload.short_url}" alt="test" width="500" height="500">
</a> </a>
<p> <p>
![test](#{upload2.short_url}) <img src="#{upload2.short_url}" alt="test">
</p> </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"> <a href="http://somelink.com">
![test|500x500](#{upload.short_url}) <img src="#{upload.short_url}" alt="test" width="500" height="500">
</a> </a>
<p>Test ![test|500x500](#{upload2.short_url})</p> <p>Test <img src="#{upload2.short_url}" alt="test" width="500" height="500"></p>
<hr/> <hr/>
![test|500x500](#{upload2.short_url}) <img src="#{upload2.short_url}" alt="test" width="500" height="500">
MD MD
end end
@ -508,9 +508,9 @@ RSpec.describe InlineUploads do
This is some <img src=" and <a href=" 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 > some quote
@ -671,9 +671,9 @@ RSpec.describe InlineUploads do
expect(InlineUploads.process(md)).to eq(<<~MD) expect(InlineUploads.process(md)).to eq(<<~MD)
![](#{upload.short_url}) ![](#{upload.short_url})
![some image](#{upload.short_url}) <img src="#{upload.short_url}" alt="some image">
test![some image](#{upload2.short_url})test test<img src="#{upload2.short_url}" alt="some image">test
![some image](#{upload2.short_url}) <img src="#{upload2.short_url}" alt="some image">
MD MD
end end