Commit 9e657133 authored by James Fargher's avatar James Fargher

Merge branch...

Merge branch 'qmnguyen0711/239343-exif-metadata-not-stripped-when-creating-new-issue-and-merge-requests-with-image-attachments' into 'master'

Exif metadata not stripped when uploading image attachments via Emails

See merge request gitlab-org/gitlab!80735
parents cdb704ec 5dd9cc8f
...@@ -15,7 +15,9 @@ module Gitlab ...@@ -15,7 +15,9 @@ module Gitlab
filter_signature_attachments(message).each do |attachment| filter_signature_attachments(message).each do |attachment|
tmp = Tempfile.new("gitlab-email-attachment") tmp = Tempfile.new("gitlab-email-attachment")
begin begin
File.open(tmp.path, "w+b") { |f| f.write attachment.body.decoded } content = attachment.body.decoded
File.open(tmp.path, "w+b") { |f| f.write content }
sanitize_exif_if_needed(content, tmp.path)
file = { file = {
tempfile: tmp, tempfile: tmp,
...@@ -55,6 +57,12 @@ module Gitlab ...@@ -55,6 +57,12 @@ module Gitlab
def normalize_mime(content_type) def normalize_mime(content_type)
MIME::Type.simplified(content_type, remove_x_prefix: true) MIME::Type.simplified(content_type, remove_x_prefix: true)
end end
# https://gitlab.com/gitlab-org/gitlab/-/issues/239343
def sanitize_exif_if_needed(content, path)
exif_sanitizer = Gitlab::Sanitizers::Exif.new
exif_sanitizer.clean_existing_path(path, content: content, skip_unallowed_types: true)
end
end end
end end
end end
...@@ -97,6 +97,28 @@ module Gitlab ...@@ -97,6 +97,28 @@ module Gitlab
end end
end end
def clean_existing_path(src_path, dry_run: false, content: nil, skip_unallowed_types: false)
content ||= File.read(src_path)
if skip_unallowed_types
return unless check_for_allowed_types(content, raise_error: false)
else
check_for_allowed_types(content)
end
to_remove = extra_tags(src_path)
if to_remove.empty?
logger.info "#{src_path}: only whitelisted tags present, skipping"
return
end
logger.info "#{src_path}: found exif tags to remove: #{to_remove}"
return if dry_run
exec_remove_exif!(src_path)
end
private private
def extra_tags(path) def extra_tags(path)
...@@ -146,12 +168,15 @@ module Gitlab ...@@ -146,12 +168,15 @@ module Gitlab
filename filename
end end
def check_for_allowed_types(contents) def check_for_allowed_types(contents, raise_error: true)
mime_type = Gitlab::Utils::MimeType.from_string(contents) mime_type = Gitlab::Utils::MimeType.from_string(contents)
unless ALLOWED_MIME_TYPES.include?(mime_type) allowed = ALLOWED_MIME_TYPES.include?(mime_type)
if !allowed && raise_error
raise "File type #{mime_type} not supported. Only supports #{ALLOWED_MIME_TYPES.join(", ")}." raise "File type #{mime_type} not supported. Only supports #{ALLOWED_MIME_TYPES.join(", ")}."
end end
allowed
end end
def upload_ref(upload) def upload_ref(upload)
......
...@@ -8,7 +8,27 @@ RSpec.describe Gitlab::Email::AttachmentUploader do ...@@ -8,7 +8,27 @@ RSpec.describe Gitlab::Email::AttachmentUploader do
let(:message_raw) { fixture_file("emails/attachment.eml") } let(:message_raw) { fixture_file("emails/attachment.eml") }
let(:message) { Mail::Message.new(message_raw) } let(:message) { Mail::Message.new(message_raw) }
before do
allow_next_instance_of(Gitlab::Sanitizers::Exif) do |instance|
allow(instance).to receive(:clean_existing_path).and_call_original
end
end
def expect_exif_sanitizer_called
expect_next_instance_of(Gitlab::Sanitizers::Exif) do |sanitizer|
expect(sanitizer).to receive(:clean_existing_path) do |path, **options|
expect(File.exist?(path)).to be true
file = File.open(path, "rb")
expect(options).to eql(content: file.read, skip_unallowed_types: true)
file.close
end
end
end
it "uploads all attachments and returns their links" do it "uploads all attachments and returns their links" do
expect_exif_sanitizer_called
links = described_class.new(message).execute(upload_parent: project, uploader_class: FileUploader) links = described_class.new(message).execute(upload_parent: project, uploader_class: FileUploader)
link = links.first link = links.first
...@@ -21,6 +41,8 @@ RSpec.describe Gitlab::Email::AttachmentUploader do ...@@ -21,6 +41,8 @@ RSpec.describe Gitlab::Email::AttachmentUploader do
let(:message_raw) { fixture_file("emails/valid_reply_signed_smime.eml") } let(:message_raw) { fixture_file("emails/valid_reply_signed_smime.eml") }
it 'uploads all attachments except the signature' do it 'uploads all attachments except the signature' do
expect_exif_sanitizer_called
links = described_class.new(message).execute(upload_parent: project, uploader_class: FileUploader) links = described_class.new(message).execute(upload_parent: project, uploader_class: FileUploader)
expect(links).not_to include(a_hash_including(alt: 'smime.p7s')) expect(links).not_to include(a_hash_including(alt: 'smime.p7s'))
...@@ -36,6 +58,8 @@ RSpec.describe Gitlab::Email::AttachmentUploader do ...@@ -36,6 +58,8 @@ RSpec.describe Gitlab::Email::AttachmentUploader do
let(:message_raw) { fixture_file("emails/valid_reply_signed_smime_mixed_protocol_prefix.eml") } let(:message_raw) { fixture_file("emails/valid_reply_signed_smime_mixed_protocol_prefix.eml") }
it 'uploads all attachments except the signature' do it 'uploads all attachments except the signature' do
expect_exif_sanitizer_called
links = described_class.new(message).execute(upload_parent: project, uploader_class: FileUploader) links = described_class.new(message).execute(upload_parent: project, uploader_class: FileUploader)
expect(links).not_to include(a_hash_including(alt: 'smime.p7s')) expect(links).not_to include(a_hash_including(alt: 'smime.p7s'))
......
...@@ -131,6 +131,124 @@ RSpec.describe Gitlab::Sanitizers::Exif do ...@@ -131,6 +131,124 @@ RSpec.describe Gitlab::Sanitizers::Exif do
end end
end end
describe '#clean_existing_path' do
let(:dry_run) { false }
let(:tmp_file) { Tempfile.new("rails_sample.jpg") }
subject { sanitizer.clean_existing_path(tmp_file.path, dry_run: dry_run) }
context "no dry run" do
let(:file_content) { fixture_file_upload('spec/fixtures/rails_sample.jpg') }
before do
File.open(tmp_file.path, "w+b") { |f| f.write file_content }
end
it "removes exif from the image" do
expected_args = ["exiftool", "-all=", "-tagsFromFile", "@", *Gitlab::Sanitizers::Exif::EXCLUDE_PARAMS, "--IPTC:all", "--XMP-iptcExt:all", kind_of(String)]
expect(sanitizer).to receive(:extra_tags).and_return(["", 0])
expect(sanitizer).to receive(:exec_remove_exif!).once.and_call_original
expect(Gitlab::Popen).to receive(:popen).with(expected_args) do |args|
File.write("#{args.last}_original", "foo") if args.last.start_with?(Dir.tmpdir)
[expected_args, 0]
end
subject
end
it "ignores image without exif" do
expected_args = ["exiftool", "-all", "-j", "-sort", "--IPTC:all", "--XMP-iptcExt:all", kind_of(String)]
expect(Gitlab::Popen).to receive(:popen).with(expected_args).and_return(["[{}]", 0])
expect(sanitizer).not_to receive(:exec_remove_exif!)
subject
end
it "raises an error if the exiftool fails with an error" do
expect(Gitlab::Popen).to receive(:popen).and_return(["error", 1])
expect { subject }.to raise_exception(RuntimeError, "failed to get exif tags: error")
end
context 'for files that do not have the correct MIME type from file' do
let(:mime_type) { 'text/plain' }
it 'cleans only jpg/tiff images with the correct mime types' do
expect(sanitizer).not_to receive(:extra_tags)
expect { subject }.to raise_error(RuntimeError, %r{File type text/plain not supported})
end
end
context 'skip_unallowed_types is false' do
context 'for files that do not have the correct MIME type from input content' do
let(:mime_type) { 'text/plain' }
it 'raises an error if not jpg/tiff images with the correct mime types' do
expect(sanitizer).not_to receive(:extra_tags)
expect do
sanitizer.clean_existing_path(tmp_file.path, content: file_content)
end.to raise_error(RuntimeError, %r{File type text/plain not supported})
end
end
context 'for files that do not have the correct MIME type from input content' do
let(:mime_type) { 'text/plain' }
it 'raises an error if not jpg/tiff images with the correct mime types' do
expect(sanitizer).not_to receive(:extra_tags)
expect do
sanitizer.clean_existing_path(tmp_file.path, content: file_content)
end.to raise_error(RuntimeError, %r{File type text/plain not supported})
end
end
end
context 'skip_unallowed_types is true' do
context 'for files that do not have the correct MIME type from input content' do
let(:mime_type) { 'text/plain' }
it 'cleans only jpg/tiff images with the correct mime types' do
expect(sanitizer).not_to receive(:extra_tags)
expect do
sanitizer.clean_existing_path(tmp_file.path, content: file_content, skip_unallowed_types: true)
end.not_to raise_error
end
end
context 'for files that do not have the correct MIME type from input content' do
let(:mime_type) { 'text/plain' }
it 'cleans only jpg/tiff images with the correct mime types' do
expect(sanitizer).not_to receive(:extra_tags)
expect do
sanitizer.clean_existing_path(tmp_file.path, content: file_content, skip_unallowed_types: true)
end.not_to raise_error
end
end
end
end
context "dry run" do
let(:dry_run) { true }
it "doesn't change the image" do
expect(sanitizer).to receive(:extra_tags).and_return({ 'foo' => 'bar' })
expect(sanitizer).not_to receive(:exec_remove_exif!)
subject
end
end
end
describe "#extra_tags" do describe "#extra_tags" do
it "returns a list of keys for exif file" do it "returns a list of keys for exif file" do
tags = '[{ tags = '[{
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment