FIX: automatically correct bad avatars on access

Also start relying on upload extension for optimized images

app/controllers/user_avatars_controller.rb

@@ -179,13 +179,49 @@ class UserAvatarsController < ApplicationController
     send_file path, disposition: nil
   end
 
+  protected
+
+  # consider removal of hacks some time in 2019
+
   def get_optimized_image(upload, size)
+    if (!upload.extension || upload.extension.length == 0)
+      fix_extension(upload)
+    end
+
+    begin
+      try_optimize(upload, size, true)
+    rescue
+      if fix_extension(upload)
+        try_optimize(upload, size, false)
+        # TODO decide if we want to detach faulty avatar here?
+      else
+        nil
+      end
+    end
+  end
+
+  def fix_extension(upload)
+    # this is relatively cheap
+    original_path = Discourse.store.path_for(upload)
+    if original_path.blank?
+      external_copy = Discourse.store.download(upload) rescue nil
+      original_path = external_copy.try(:path)
+    end
+
+    image_info = FastImage.new(original_path) rescue nil
+    if image_info && image_info.type.to_s != upload.extension
+      upload.update_columns(extension: image_info.type.to_s)
+      true
+    end
+  end
+
+  def try_optimize(upload, size, raise_on_error)
     OptimizedImage.create_for(
       upload,
       size,
       size,
-      filename: upload.original_filename,
       allow_animation: SiteSetting.allow_animated_avatars,
+      raise_on_error: raise_on_error
     )
   end
 

app/jobs/regular/create_avatar_thumbnails.rb

@@ -13,7 +13,12 @@ module Jobs
       return unless user = User.find_by(id: args[:user_id] || upload.user_id)
 
       Discourse.avatar_sizes.each do |size|
-        OptimizedImage.create_for(upload, size, size, filename: upload.original_filename, allow_animation: SiteSetting.allow_animated_avatars)
+        OptimizedImage.create_for(
+          upload,
+          size,
+          size,
+          allow_animation: SiteSetting.allow_animated_avatars
+        )
       end
     end
 

app/models/optimized_image.rb

@@ -48,7 +48,12 @@ class OptimizedImage < ActiveRecord::Base
         Rails.logger.error("Could not find file in the store located at url: #{upload.url}")
       else
         # create a temp file with the same extension as the original
-        extension = File.extname(original_path)
+        extension = ".#{upload.extension}"
+
+        if extension.length == 1
+          return nil
+        end
+
         temp_file = Tempfile.new(["discourse-thumbnail", extension])
         temp_path = temp_file.path
 
@@ -120,8 +125,8 @@ class OptimizedImage < ActiveRecord::Base
 
   IM_DECODERS ||= /\A(jpe?g|png|tiff?|bmp|ico|gif)\z/i
 
-  def self.prepend_decoder!(path)
-    extension = File.extname(path)[1..-1]
+  def self.prepend_decoder!(path, ext_path)
+    extension = File.extname(ext_path)[1..-1]
     raise Discourse::InvalidAccess unless extension.present? && extension[IM_DECODERS]
     "#{extension}:#{path}"
   end
@@ -133,8 +138,9 @@ class OptimizedImage < ActiveRecord::Base
   def self.resize_instructions(from, to, dimensions, opts = {})
     ensure_safe_paths!(from, to)
 
-    from = prepend_decoder!(from)
-    to = prepend_decoder!(to)
+    # note FROM my not be named correctly
+    from = prepend_decoder!(from, to)
+    to = prepend_decoder!(to, to)
 
     # NOTE: ORDER is important!
     %W{
@@ -170,8 +176,8 @@ class OptimizedImage < ActiveRecord::Base
   def self.crop_instructions(from, to, dimensions, opts = {})
     ensure_safe_paths!(from, to)
 
-    from = prepend_decoder!(from)
-    to = prepend_decoder!(to)
+    from = prepend_decoder!(from, to)
+    to = prepend_decoder!(to, to)
 
     %W{
       convert
@@ -205,8 +211,8 @@ class OptimizedImage < ActiveRecord::Base
   def self.downsize_instructions(from, to, dimensions, opts = {})
     ensure_safe_paths!(from, to)
 
-    from = prepend_decoder!(from)
-    to = prepend_decoder!(to)
+    from = prepend_decoder!(from, to)
+    to = prepend_decoder!(to, to)
 
     %W{
       convert
@@ -240,7 +246,7 @@ class OptimizedImage < ActiveRecord::Base
 
   def self.optimize(operation, from, to, dimensions, opts = {})
     method_name = "#{operation}_instructions"
-    if !!opts[:allow_animation] && (from =~ /\.GIF$/i || opts[:filename] =~ /\.GIF$/i)
+    if !!opts[:allow_animation] && (from =~ /\.GIF$/i)
       method_name += "_animated"
     end
     instructions = self.send(method_name.to_sym, from, to, dimensions, opts)

app/models/upload.rb

@@ -36,7 +36,6 @@ class Upload < ActiveRecord::Base
     return unless SiteSetting.create_thumbnails?
 
     opts = {
-      filename: self.original_filename,
       allow_animation: SiteSetting.allow_animated_thumbnails,
       crop: crop
     }

lib/wizard/builder.rb

@@ -165,7 +165,12 @@ class Wizard
             if upload && upload.width > dimensions && upload.height > dimensions
               updater.update_setting(:large_icon_url, updater.fields[:apple_touch_icon_url])
 
-              apple_touch_icon_optimized = OptimizedImage.create_for(upload, dimensions, dimensions, filename: upload.original_filename)
+              apple_touch_icon_optimized = OptimizedImage.create_for(
+                upload,
+                dimensions,
+                dimensions
+              )
+
               original_file = File.new(Discourse.store.path_for(apple_touch_icon_optimized)) rescue nil
               if original_file
                 apple_touch_icon_upload = UploadCreator.new(original_file, upload.original_filename).create_for(@wizard.user.id)

spec/requests/user_avatars_controller_spec.rb

@@ -16,6 +16,52 @@ describe UserAvatarsController do
   end
 
   context 'show' do
+
+    context 'invalid' do
+      after do
+        FileUtils.rm(Discourse.store.path_for(upload))
+      end
+      # travis is not good here, no image magick
+      if !ENV["TRAVIS"]
+        let :upload do
+          File.open("#{Rails.root}/spec/fixtures/images/cropped.png") do |f|
+            UploadCreator.new(
+              f,
+              "test.png"
+            ).create_for(-1)
+          end
+        end
+
+        let :user do
+          user = Fabricate(:user)
+          user.user_avatar.update_columns(custom_upload_id: upload.id)
+          user.update_columns(uploaded_avatar_id: upload.id)
+          user
+        end
+
+        it 'automatically corrects bad avatar extensions' do
+          orig = Discourse.store.path_for(upload)
+
+          upload.update_columns(
+            original_filename: 'bob.jpg',
+            extension: 'jpg',
+            url: upload.url + '.jpg'
+          )
+
+          # at this point file is messed up
+          FileUtils.mv(orig, Discourse.store.path_for(upload))
+
+          SiteSetting.avatar_sizes = "50"
+
+          get "/user_avatar/default/#{user.username}/50/#{upload.id}.png"
+
+          expect(OptimizedImage.where(upload_id: upload.id).count).to eq(1)
+          expect(response.status).to eq(200)
+        end
+      end
+
+    end
+
     it 'handles non local content correctly' do
       SiteSetting.avatar_sizes = "100|49"
       SiteSetting.enable_s3_uploads = true