FIX: Ensure oneboxed secure images which are optimized and also lightboxed optimized images are embedded in email (#11061)
We had an issue where onebox thumbnail was too large and thus was optimized, and we are using the image URLs in post to redact and re-embed, based on the sha1 in the URL. Optimized image URLs have extra stuff on the end like _99x99 so we were not parsing out the sha1 correctly. Another issue I found was for posts that have giant images, the original was being used to embed in the email and thus would basically never get included because it is huge.
For example the URL 787b17ea61_2_690x335.jpeg was not parsed correctly; we would end up with 787b17ea6140f4f022eb7f1509a692f2873cfe35_2_690x335.jpeg as the sha1 which would not find the image to re-embed that was already attached to the email.
This fix will use the first optimized image of the detected upload when we are redacting and then re-embedding to make sure we are not sending giant things in email. Also, I detect if it is a onebox thumbnail or the site icon and force appropriate sizes and styles.
This commit is contained in:
Martin Brennan
GitHub
parent
b6aaff74be
commit
3655062c60
|
|
@ -272,22 +272,25 @@ module Email
|
|||
return if max_email_size == 0
|
||||
|
||||
email_size = 0
|
||||
post.uploads.each do |upload|
|
||||
if FileHelper.is_supported_image?(upload.original_filename) &&
|
||||
!should_attach_image?(upload)
|
||||
post.uploads.each do |original_upload|
|
||||
optimized_1X = original_upload.optimized_images.first
|
||||
|
||||
if FileHelper.is_supported_image?(original_upload.original_filename) &&
|
||||
!should_attach_image?(original_upload, optimized_1X)
|
||||
next
|
||||
end
|
||||
|
||||
next if email_size + upload.filesize > max_email_size
|
||||
attached_upload = optimized_1X || original_upload
|
||||
next if email_size + original_upload.filesize > max_email_size
|
||||
|
||||
begin
|
||||
path = if upload.local?
|
||||
Discourse.store.path_for(upload)
|
||||
path = if attached_upload.local?
|
||||
Discourse.store.path_for(attached_upload)
|
||||
else
|
||||
Discourse.store.download(upload).path
|
||||
Discourse.store.download(attached_upload).path
|
||||
end
|
||||
|
||||
@message.attachments[upload.original_filename] = File.read(path)
|
||||
@message.attachments[original_upload.original_filename] = File.read(path)
|
||||
email_size += File.size(path)
|
||||
rescue => e
|
||||
Discourse.warn_exception(
|
||||
|
|
@ -295,8 +298,8 @@ module Email
|
|||
message: "Failed to attach file to email",
|
||||
env: {
|
||||
post_id: post.id,
|
||||
upload_id: upload.id,
|
||||
filename: upload.original_filename
|
||||
upload_id: original_upload.id,
|
||||
filename: original_upload.original_filename
|
||||
}
|
||||
)
|
||||
end
|
||||
|
|
@ -305,9 +308,9 @@ module Email
|
|||
fix_parts_after_attachments!
|
||||
end
|
||||
|
||||
def should_attach_image?(upload)
|
||||
def should_attach_image?(upload, optimized_1X = nil)
|
||||
return if !SiteSetting.secure_media_allow_embed_images_in_emails || !upload.secure?
|
||||
return if upload.filesize > SiteSetting.secure_media_max_email_embed_image_size_kb.kilobytes
|
||||
return if (optimized_1X&.filesize || upload.filesize) > SiteSetting.secure_media_max_email_embed_image_size_kb.kilobytes
|
||||
true
|
||||
end
|
||||
|
||||
|
|
|
|||
|
|
@ -7,6 +7,8 @@
|
|||
module Email
|
||||
class Styles
|
||||
MAX_IMAGE_DIMENSION = 400
|
||||
ONEBOX_IMAGE_BASE_STYLE = "max-height: 80%; max-width: 20%; height: auto; float: left; margin-right: 10px;"
|
||||
ONEBOX_IMAGE_THUMBNAIL_STYLE = "width: 60px;"
|
||||
|
||||
@@plugin_callbacks = []
|
||||
|
||||
|
|
@ -125,8 +127,8 @@ module Email
|
|||
style('aside.onebox header img.site-icon', "width: 16px; height: 16px; margin-right: 3px;")
|
||||
style('aside.onebox header a[href]', "color: #222222; text-decoration: none;")
|
||||
style('aside.onebox .onebox-body', "clear: both")
|
||||
style('aside.onebox .onebox-body img:not(.onebox-avatar-inline)', "max-height: 80%; max-width: 20%; height: auto; float: left; margin-right: 10px;")
|
||||
style('aside.onebox .onebox-body img.thumbnail', "width: 60px;")
|
||||
style('aside.onebox .onebox-body img:not(.onebox-avatar-inline)', ONEBOX_IMAGE_BASE_STYLE)
|
||||
style('aside.onebox .onebox-body img.thumbnail', ONEBOX_IMAGE_THUMBNAIL_STYLE)
|
||||
style('aside.onebox .onebox-body h3, aside.onebox .onebox-body h4', "font-size: 1.17em; margin: 10px 0;")
|
||||
style('.onebox-metadata', "color: #919191")
|
||||
style('.github-info', "margin-top: 10px;")
|
||||
|
|
@ -244,7 +246,8 @@ module Email
|
|||
stripped_media.each do |div|
|
||||
url = div['data-stripped-secure-media']
|
||||
filename = File.basename(url)
|
||||
sha1 = filename.gsub(File.extname(filename), "")
|
||||
filename_bare = filename.gsub(File.extname(filename), "")
|
||||
sha1 = filename_bare.partition('_').first
|
||||
upload_shas[url] = sha1
|
||||
end
|
||||
uploads = Upload.select(:original_filename, :sha1).where(sha1: upload_shas.values)
|
||||
|
|
@ -258,9 +261,15 @@ module Email
|
|||
if attachments[original_filename]
|
||||
url = attachments[original_filename].url
|
||||
|
||||
div.add_next_sibling(
|
||||
"<img src=\"#{url}\" data-embedded-secure-image=\"true\" style=\"#{calculate_width_and_height_style(div)}\" />"
|
||||
)
|
||||
style = if div['data-oneboxed']
|
||||
"#{ONEBOX_IMAGE_THUMBNAIL_STYLE} #{ONEBOX_IMAGE_BASE_STYLE}"
|
||||
else
|
||||
calculate_width_and_height_style(div)
|
||||
end
|
||||
|
||||
div.add_next_sibling(<<~HTML)
|
||||
<img src="#{url}" data-embedded-secure-image="true" style="#{style}" />
|
||||
HTML
|
||||
div.remove
|
||||
end
|
||||
end
|
||||
|
|
|
|||
|
|
@ -406,29 +406,60 @@ module PrettyText
|
|||
end
|
||||
|
||||
def self.strip_secure_media(doc)
|
||||
doc.css("a[href]").each do |a|
|
||||
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")
|
||||
width = a.xpath("//*[@width]").attr("width")&.value
|
||||
height = a.xpath("//*[@height]").attr("height")&.value
|
||||
# images inside a lightbox or other link
|
||||
doc.css('a[href]').each do |a|
|
||||
next if !Upload.secure_media_url?(a['href'])
|
||||
|
||||
non_image_media = %w(video audio).include?(a&.parent&.name)
|
||||
target = non_image_media ? a.parent : a
|
||||
next if target.to_s.include?('stripped-secure-view-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
|
||||
a.add_next_sibling secure_media_placeholder(doc, url, width: img['width'], height: img['height'])
|
||||
a.remove
|
||||
else
|
||||
width = non_image_media ? nil : a.at_css('img').attr('width')
|
||||
height = non_image_media ? nil : a.at_css('img').attr('height')
|
||||
target.add_next_sibling secure_media_placeholder(doc, a['href'], width: width, height: height)
|
||||
target.remove
|
||||
end
|
||||
end
|
||||
|
||||
# images by themselves or inside a onebox
|
||||
doc.css('img[src]').each do |img|
|
||||
if Upload.secure_media_url?(img['src'])
|
||||
img.add_next_sibling secure_media_placeholder(doc, img['src'], width: img['width'], height: img['height'])
|
||||
url = if img.parent.classes.include?("aspect-image")
|
||||
|
||||
# we are using the first image from the srcset here so we get the
|
||||
# optimized image instead of the original, because an optimized
|
||||
# image may be used for the onebox thumbnail
|
||||
srcset = img.attributes["srcset"].value
|
||||
srcset.split(",").first
|
||||
else
|
||||
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')
|
||||
|
||||
if Upload.secure_media_url?(url)
|
||||
img.add_next_sibling secure_media_placeholder(doc, url, oneboxed: oneboxed, width: width, height: height)
|
||||
img.remove
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def self.secure_media_placeholder(doc, url, width: nil, height: nil)
|
||||
def self.secure_media_placeholder(doc, url, oneboxed: false, width: nil, height: nil)
|
||||
data_width = width ? "data-width=#{width}" : ''
|
||||
data_height = height ? "data-height=#{height}" : ''
|
||||
data_oneboxed = oneboxed ? "data-oneboxed=true" : ''
|
||||
<<~HTML
|
||||
<div class="secure-media-notice" data-stripped-secure-media="#{url}" #{data_width} #{data_height}>
|
||||
<div class="secure-media-notice" data-stripped-secure-media="#{url}" #{data_oneboxed} #{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
|
||||
|
|
|
|||
|
|
@ -424,7 +424,7 @@ describe Email::Sender do
|
|||
context "when secure media enabled" do
|
||||
before do
|
||||
setup_s3
|
||||
stub_s3_store
|
||||
store = stub_s3_store
|
||||
|
||||
SiteSetting.secure_media = true
|
||||
SiteSetting.login_required = true
|
||||
|
|
@ -436,7 +436,8 @@ describe Email::Sender do
|
|||
FileStore::S3Store.any_instance.expects(:has_been_uploaded?).returns(true).at_least_once
|
||||
CookedPostProcessor.any_instance.stubs(:get_size).returns([244, 66])
|
||||
|
||||
@secure_image = UploadCreator.new(file_from_fixtures("logo.png", "images"), "logo.png")
|
||||
@secure_image_file = file_from_fixtures("logo.png", "images")
|
||||
@secure_image = UploadCreator.new(@secure_image_file, "logo.png")
|
||||
.create_for(Discourse.system_user.id)
|
||||
@secure_image.update_secure_status(secure_override_value: true)
|
||||
@secure_image.update(access_control_post_id: reply.id)
|
||||
|
|
@ -470,6 +471,7 @@ describe Email::Sender do
|
|||
expect(message.attachments.length).to eq(4)
|
||||
expect(message.attachments.map(&:filename))
|
||||
.to contain_exactly(*[small_pdf, large_pdf, csv_file, @secure_image].map(&:original_filename))
|
||||
expect(message.attachments["logo.png"].body.raw_source.force_encoding("UTF-8")).to eq(File.read(@secure_image_file))
|
||||
expect(message.html_part.body).to include("cid:")
|
||||
expect(message.html_part.body).to include("embedded-secure-image")
|
||||
expect(message.attachments.length).to eq(4)
|
||||
|
|
@ -481,6 +483,25 @@ describe Email::Sender do
|
|||
expect(message.html_part.body).to include("It’s")
|
||||
expect(message.html_part.charset.downcase).to eq("utf-8")
|
||||
end
|
||||
|
||||
context "when the uploaded secure image has an optimized image" do
|
||||
let!(:optimized) { Fabricate(:optimized_image, upload: @secure_image) }
|
||||
let!(:optimized_image_file) { file_from_fixtures("logo-dev.png", "images") }
|
||||
|
||||
it "uses the email styles and the optimized image to inline secure images and attaches the secure image upload to the email" do
|
||||
Discourse.store.store_optimized_image(optimized_image_file, optimized)
|
||||
optimized.update(url: Discourse.store.absolute_base_url + '/' + optimized.url)
|
||||
Discourse.store.cache_file(optimized_image_file, File.basename("#{optimized.sha1}.png"))
|
||||
Email::Sender.new(message, :valid_type).send
|
||||
expect(message.attachments.length).to eq(4)
|
||||
expect(message.attachments.map(&:filename))
|
||||
.to contain_exactly(*[small_pdf, large_pdf, csv_file, @secure_image].map(&:original_filename))
|
||||
expect(message.attachments["logo.png"].body.raw_source.force_encoding("UTF-8")).to eq(File.read(optimized_image_file))
|
||||
expect(message.html_part.body).to include("cid:")
|
||||
expect(message.html_part.body).to include("embedded-secure-image")
|
||||
expect(message.attachments.length).to eq(4)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
|||
|
|
@ -210,10 +210,9 @@ describe Email::Styles do
|
|||
context "inline_secure_images" do
|
||||
let(:attachments) { { 'testimage.png' => stub(url: 'cid:email/test.png') } }
|
||||
fab!(:upload) { Fabricate(:upload, original_filename: 'testimage.png', secure: true, sha1: '123456') }
|
||||
let(: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>" }
|
||||
|
||||
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\" width=\"20\" height=\"30\"></a>"
|
||||
|
||||
# strip out the secure media
|
||||
styler = Email::Styles.new(html)
|
||||
styler.format_basic
|
||||
|
|
@ -240,5 +239,62 @@ describe Email::Styles do
|
|||
expect(@frag.to_s).not_to include("cid:email/test.png")
|
||||
expect(@frag.css('[data-stripped-secure-media]')).to be_present
|
||||
end
|
||||
|
||||
context "when an optimized image is used instead of the original" do
|
||||
let(:html) { "<a href=\"#{Discourse.base_url}\/secure-media-uploads/optimized/2X/1/123456_2_20x30.png\"><img src=\"/secure-media-uploads/optimized/2X/1/123456_2_20x30.png\" width=\"20\" height=\"30\"></a>" }
|
||||
|
||||
it "inlines attachments where the stripped-secure-media data attr is present" do
|
||||
optimized = Fabricate(:optimized_image, upload: upload, width: 20, height: 30)
|
||||
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
|
||||
end
|
||||
|
||||
context "when inlining an originally oneboxed image" do
|
||||
before do
|
||||
SiteSetting.authorized_extensions = "*"
|
||||
end
|
||||
|
||||
let(:siteicon) { Fabricate(:upload, original_filename: "siteicon.ico") }
|
||||
let(:attachments) do
|
||||
{
|
||||
'testimage.png' => stub(url: 'cid:email/test.png'),
|
||||
'siteicon.ico' => stub(url: 'cid:email/test2.ico')
|
||||
}
|
||||
end
|
||||
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">
|
||||
<div class="aspect-image" style="--aspect-ratio:20/30;"><img src="#{Discourse.base_url}/secure-media-uploads/optimized/2X/1/123456_2_20x30.png" class="thumbnail d-lazyload" width="20" height="30" srcset="#{Discourse.base_url}/secure-media-uploads/optimized/2X/1/123456_2_20x30.png"></div>
|
||||
|
||||
<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 site icon width and height and onebox styles" do
|
||||
optimized = Fabricate(:optimized_image, upload: upload, width: 20, height: 30)
|
||||
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]')[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
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
|||
Loading…
Reference in New Issue