From 8b5e42ea160d0b068ecb6817d6310d7be09d572b Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 20 Aug 2018 12:18:49 +1000 Subject: [PATCH] FIX: always test and coerce to image on upload In the past the filename of the origin was used as the source for the extension of the file when optimizing on upload. We now use the actual calculated extension based on upload data. --- app/models/optimized_image.rb | 16 +++---- lib/upload_creator.rb | 39 +++++++++++++----- .../images/{png_as.jpg => png_as.bin} | Bin spec/lib/upload_creator_spec.rb | 6 +-- spec/models/optimized_image_spec.rb | 26 ++++++++++++ 5 files changed, 66 insertions(+), 21 deletions(-) rename spec/fixtures/images/{png_as.jpg => png_as.bin} (100%) diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index 72b37912eed..a344d4f5fc5 100644 --- a/app/models/optimized_image.rb +++ b/app/models/optimized_image.rb @@ -139,8 +139,8 @@ class OptimizedImage < ActiveRecord::Base IM_DECODERS ||= /\A(jpe?g|png|tiff?|bmp|ico|gif)\z/i - def self.prepend_decoder!(path, ext_path = nil) - extension = File.extname(ext_path || path)[1..-1] + def self.prepend_decoder!(path, ext_path = nil, opts = nil) + extension = File.extname((opts && opts[:filename]) || ext_path || path)[1..-1] raise Discourse::InvalidAccess if !extension || !extension.match?(IM_DECODERS) "#{extension}:#{path}" end @@ -153,8 +153,8 @@ class OptimizedImage < ActiveRecord::Base ensure_safe_paths!(from, to) # note FROM my not be named correctly - from = prepend_decoder!(from, to) - to = prepend_decoder!(to, to) + from = prepend_decoder!(from, to, opts) + to = prepend_decoder!(to, to, opts) # NOTE: ORDER is important! %W{ @@ -190,8 +190,8 @@ class OptimizedImage < ActiveRecord::Base def self.crop_instructions(from, to, dimensions, opts = {}) ensure_safe_paths!(from, to) - from = prepend_decoder!(from, to) - to = prepend_decoder!(to, to) + from = prepend_decoder!(from, to, opts) + to = prepend_decoder!(to, to, opts) %W{ convert @@ -225,8 +225,8 @@ class OptimizedImage < ActiveRecord::Base def self.downsize_instructions(from, to, dimensions, opts = {}) ensure_safe_paths!(from, to) - from = prepend_decoder!(from, to) - to = prepend_decoder!(to, to) + from = prepend_decoder!(from, to, opts) + to = prepend_decoder!(to, to, opts) %W{ convert diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index f25f7ae677e..3bb50448382 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -34,7 +34,13 @@ class UploadCreator end DistributedMutex.synchronize("upload_#{user_id}_#{@filename}") do - if FileHelper.is_image?(@filename) + # test for image regardless of input + @image_info = FastImage.new(@file) rescue nil + + is_image = FileHelper.is_image?(@filename) + is_image ||= @image_info && FileHelper.is_image?("test.#{@image_info.type}") + + if is_image extract_image_info! return @upload if @upload.errors.present? @@ -51,6 +57,7 @@ class UploadCreator optimize! if should_optimize? end + # conversion may have switched the type image_type = @image_info.type.to_s end @@ -69,17 +76,27 @@ class UploadCreator # return the previous upload if any return @upload unless @upload.nil? + fixed_original_filename = nil + if is_image + # we have to correct original filename here, no choice + # otherwise validation will fail and we can not save + # TODO decide if we only run the validation on the extension + if File.extname(@filename) != ".#{image_type}" + fixed_original_filename = "#{@filename}.fixed.#{image_type}" + end + end + # create the upload otherwise @upload = Upload.new @upload.user_id = user_id - @upload.original_filename = @filename + @upload.original_filename = fixed_original_filename || @filename @upload.filesize = filesize @upload.sha1 = sha1 @upload.url = "" @upload.origin = @opts[:origin][0...1000] if @opts[:origin] @upload.extension = image_type || File.extname(@filename)[1..10] - if FileHelper.is_image?(@filename) + if is_image @upload.width, @upload.height = ImageSizer.resize(*@image_info.size) end @@ -101,7 +118,7 @@ class UploadCreator end end - if @upload.errors.empty? && FileHelper.is_image?(@filename) && @opts[:type] == "avatar" + if @upload.errors.empty? && is_image && @opts[:type] == "avatar" Jobs.enqueue(:create_avatar_thumbnails, upload_id: @upload.id, user_id: user_id) end @@ -141,7 +158,7 @@ class UploadCreator OptimizedImage.ensure_safe_paths!(from, to) - from = OptimizedImage.prepend_decoder!(from) + from = OptimizedImage.prepend_decoder!(from, nil, filename: "image.#{@image_info.type}") to = OptimizedImage.prepend_decoder!(to) begin @@ -229,7 +246,7 @@ class UploadCreator path = @file.path OptimizedImage.ensure_safe_paths!(path) - path = OptimizedImage.prepend_decoder!(path) + path = OptimizedImage.prepend_decoder!(path, nil, filename: "image.#{@image_info.type}") Discourse::Utils.execute_command('convert', path, '-auto-orient', path) @@ -243,20 +260,22 @@ class UploadCreator def crop! max_pixel_ratio = Discourse::PIXEL_RATIOS.max + filename_with_correct_ext = "image.#{@image_info.type}" + case @opts[:type] when "avatar" width = height = Discourse.avatar_sizes.max - OptimizedImage.resize(@file.path, @file.path, width, height, filename: @filename, allow_animation: allow_animation) + OptimizedImage.resize(@file.path, @file.path, width, height, filename: filename_with_correct_ext, allow_animation: allow_animation) when "profile_background" max_width = 850 * max_pixel_ratio width, height = ImageSizer.resize(@image_info.size[0], @image_info.size[1], max_width: max_width, max_height: max_width) - OptimizedImage.downsize(@file.path, @file.path, "#{width}x#{height}\>", filename: @filename, allow_animation: allow_animation) + OptimizedImage.downsize(@file.path, @file.path, "#{width}x#{height}\>", filename: filename_with_correct_ext, allow_animation: allow_animation) when "card_background" max_width = 590 * max_pixel_ratio width, height = ImageSizer.resize(@image_info.size[0], @image_info.size[1], max_width: max_width, max_height: max_width) - OptimizedImage.downsize(@file.path, @file.path, "#{width}x#{height}\>", filename: @filename, allow_animation: allow_animation) + OptimizedImage.downsize(@file.path, @file.path, "#{width}x#{height}\>", filename: filename_with_correct_ext, allow_animation: allow_animation) when "custom_emoji" - OptimizedImage.downsize(@file.path, @file.path, "100x100\>", filename: @filename, allow_animation: allow_animation) + OptimizedImage.downsize(@file.path, @file.path, "100x100\>", filename: filename_with_correct_ext, allow_animation: allow_animation) end extract_image_info! diff --git a/spec/fixtures/images/png_as.jpg b/spec/fixtures/images/png_as.bin similarity index 100% rename from spec/fixtures/images/png_as.jpg rename to spec/fixtures/images/png_as.bin diff --git a/spec/lib/upload_creator_spec.rb b/spec/lib/upload_creator_spec.rb index 4238918225d..9c23672eeea 100644 --- a/spec/lib/upload_creator_spec.rb +++ b/spec/lib/upload_creator_spec.rb @@ -26,7 +26,7 @@ RSpec.describe UploadCreator do end describe 'when image has the wrong extension' do - let(:filename) { "png_as.jpg" } + let(:filename) { "png_as.bin" } let(:file) { file_from_fixtures(filename) } it 'should store the upload with the right extension' do @@ -41,7 +41,7 @@ RSpec.describe UploadCreator do expect(upload.extension).to eq('png') expect(File.extname(upload.url)).to eq('.png') - expect(upload.original_filename).to eq('png_as.jpg') + expect(upload.original_filename).to eq('png_as.bin.fixed.png') end end @@ -65,7 +65,7 @@ RSpec.describe UploadCreator do expect(upload.extension).to eq('jpeg') expect(File.extname(upload.url)).to eq('.jpeg') - expect(upload.original_filename).to eq('logo.png') + expect(upload.original_filename).to eq('logo.png.fixed.jpeg') end end end diff --git a/spec/models/optimized_image_spec.rb b/spec/models/optimized_image_spec.rb index beb94b02ff6..0b7fbd63869 100644 --- a/spec/models/optimized_image_spec.rb +++ b/spec/models/optimized_image_spec.rb @@ -27,6 +27,32 @@ describe OptimizedImage do end describe '.resize' do + it 'should work correctly when extension is bad' do + + original_path = Dir::Tmpname.create(['origin', '.bin']) { nil } + + begin + FileUtils.cp "#{Rails.root}/spec/fixtures/images/logo.png", original_path + + # we use "filename" to get the correct extension here, it is more important + # then any other param + + OptimizedImage.resize( + original_path, + original_path, + 5, + 5, + filename: "test.png" + ) + + expect(File.read(original_path)).to eq( + File.read("#{Rails.root}/spec/fixtures/images/resized.png") + ) + ensure + File.delete(original_path) if File.exists?(original_path) + end + end + it 'should work correctly' do tmp_path = "/tmp/resized.png"