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

@@ -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

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!

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

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"