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"