From 796164b58c989bbfe5a16e2dff120347a7527950 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 16 Aug 2018 16:32:36 +1000 Subject: [PATCH] FIX: automatically correct bad avatars on access Also start relying on upload extension for optimized images --- app/controllers/user_avatars_controller.rb | 38 ++++++++++++++- app/jobs/regular/create_avatar_thumbnails.rb | 7 ++- app/models/optimized_image.rb | 26 +++++++---- app/models/upload.rb | 1 - lib/wizard/builder.rb | 7 ++- spec/requests/user_avatars_controller_spec.rb | 46 +++++++++++++++++++ 6 files changed, 111 insertions(+), 14 deletions(-) diff --git a/app/controllers/user_avatars_controller.rb b/app/controllers/user_avatars_controller.rb index 03e9e034712..754fa3b0bbc 100644 --- a/app/controllers/user_avatars_controller.rb +++ b/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 diff --git a/app/jobs/regular/create_avatar_thumbnails.rb b/app/jobs/regular/create_avatar_thumbnails.rb index ebc86a4d65f..2d704602815 100644 --- a/app/jobs/regular/create_avatar_thumbnails.rb +++ b/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 diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index 4b3f219a54b..406d59547f5 100644 --- a/app/models/optimized_image.rb +++ b/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) diff --git a/app/models/upload.rb b/app/models/upload.rb index 022d10b160a..517f34c1e00 100644 --- a/app/models/upload.rb +++ b/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 } diff --git a/lib/wizard/builder.rb b/lib/wizard/builder.rb index 1d7a2e5a4b0..8c00c2ae1ba 100644 --- a/lib/wizard/builder.rb +++ b/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) diff --git a/spec/requests/user_avatars_controller_spec.rb b/spec/requests/user_avatars_controller_spec.rb index be10954e8aa..9c0110407e0 100644 --- a/spec/requests/user_avatars_controller_spec.rb +++ b/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