FIX: prevent duplicate attachments in incoming emails - take 2

This is a follow up of 5fcb7c262d

It was missing the case where secure uploads is enabled, which creates a copy of the upload no matter what.

So this checks for the original_sha1 of the uploads as well when checking for duplicates.
This commit is contained in:
Régis Hanol 2024-04-29 16:56:16 +02:00
parent 8704499b5a
commit bfc0f3f4cd
2 changed files with 22 additions and 7 deletions

View File

@ -1359,11 +1359,12 @@ module Email
def add_attachments(raw, user, options = {}) def add_attachments(raw, user, options = {})
raw = raw.dup raw = raw.dup
previous_upload_ids = upload_ids =
UploadReference UploadReference.where(
.where(target_id: Post.where(topic_id: options[:topic_id]).select(:id)) target_id: Post.where(topic_id: options[:topic_id]).select(:id),
.pluck(:upload_id) ).pluck("DISTINCT upload_id")
.uniq
upload_shas = Upload.where(id: upload_ids).pluck("DISTINCT COALESCE(original_sha1, sha1)")
rejected_attachments = [] rejected_attachments = []
attachments.each do |attachment| attachments.each do |attachment|
@ -1374,6 +1375,7 @@ module Email
# create the upload for the user # create the upload for the user
opts = { for_group_message: options[:is_group_message] } opts = { for_group_message: options[:is_group_message] }
upload = UploadCreator.new(tmp, attachment.filename, opts).create_for(user.id) upload = UploadCreator.new(tmp, attachment.filename, opts).create_for(user.id)
upload_sha = upload.original_sha1.presence || upload.sha1
if upload.errors.empty? if upload.errors.empty?
# try to inline images # try to inline images
if attachment.content_type&.start_with?("image/") if attachment.content_type&.start_with?("image/")
@ -1390,10 +1392,10 @@ module Email
end end
elsif raw[/\[image:[^\]]*\]/i] elsif raw[/\[image:[^\]]*\]/i]
raw.sub!(/\[image:[^\]]*\]/i, UploadMarkdown.new(upload).to_markdown) raw.sub!(/\[image:[^\]]*\]/i, UploadMarkdown.new(upload).to_markdown)
elsif !previous_upload_ids.include?(upload.id) elsif !upload_ids.include?(upload.id) && !upload_shas.include?(upload_sha)
raw << "\n\n#{UploadMarkdown.new(upload).to_markdown}\n\n" raw << "\n\n#{UploadMarkdown.new(upload).to_markdown}\n\n"
end end
elsif !previous_upload_ids.include?(upload.id) elsif !upload_ids.include?(upload.id) && !upload_shas.include?(upload_sha)
raw << "\n\n#{UploadMarkdown.new(upload).to_markdown}\n\n" raw << "\n\n#{UploadMarkdown.new(upload).to_markdown}\n\n"
end end
else else

View File

@ -766,6 +766,19 @@ RSpec.describe Email::Receiver do
expect(topic.posts.last.raw).not_to match %r{upload://} expect(topic.posts.last.raw).not_to match %r{upload://}
end end
it "tries not to repeat duplicate secure attachments" do
setup_s3
stub_s3_store
SiteSetting.secure_uploads = true
SiteSetting.authorized_extensions = "jpg"
expect { process(:logo_1) }.to change { UploadReference.count }.by(1)
expect(topic.posts.last.raw).to match %r{upload://}
expect { process(:logo_2) }.not_to change { UploadReference.count }
expect(topic.posts.last.raw).not_to match %r{upload://}
end
it "works with removed attachments" do it "works with removed attachments" do
SiteSetting.authorized_extensions = "jpg" SiteSetting.authorized_extensions = "jpg"