SECURITY: ensure we never accept fake images

app/models/upload.rb

@@ -67,7 +67,7 @@ class Upload < ActiveRecord::Base
   def self.create_for(user_id, file, filename, filesize, options = {})
     DistributedMutex.synchronize("upload_#{user_id}_#{filename}") do
       # do some work on images
-      if FileHelper.is_image?(filename)
+      if FileHelper.is_image?(filename) && system("identify '#{file.path}' >/dev/null 2>&1")
         if filename =~ /\.svg$/i
           svg = Nokogiri::XML(file).at_css("svg")
           w = svg["width"].to_i
@@ -122,7 +122,7 @@ class Upload < ActiveRecord::Base
       upload = find_by(sha1: sha1)
 
       # make sure the previous upload has not failed
-      if upload && upload.url.blank?
+      if upload && (upload.url.blank? || is_dimensionless_image?(filename, upload.width, upload.height))
         upload.destroy
         upload = nil
       end
@@ -141,8 +141,9 @@ class Upload < ActiveRecord::Base
       upload.height            = height
       upload.origin            = options[:origin][0...1000] if options[:origin]
 
-      if FileHelper.is_image?(filename) && (upload.width == 0 || upload.height == 0)
+      if is_dimensionless_image?(filename, upload.width, upload.height)
         upload.errors.add(:base, I18n.t("upload.images.size_not_found"))
+        return upload
       end
 
       return upload unless upload.save
@@ -162,6 +163,10 @@ class Upload < ActiveRecord::Base
     end
   end
 
+  def self.is_dimensionless_image?(filename, width, height)
+    FileHelper.is_image?(filename) && (width.blank? || width == 0 || height.blank? || height == 0)
+  end
+
   def self.get_from_url(url)
     return if url.blank?
     # we store relative urls, so we need to remove any host/cdn

spec/controllers/uploads_controller_spec.rb

@@ -19,6 +19,13 @@ describe UploadsController do
         })
       end
 
+      let(:fake_jpg) do
+        ActionDispatch::Http::UploadedFile.new({
+          filename: 'fake.jpg',
+          tempfile: file_from_fixtures("fake.jpg")
+        })
+      end
+
       let(:text_file) do
         ActionDispatch::Http::UploadedFile.new({
           filename: 'LICENSE.TXT',
@@ -118,6 +125,20 @@ describe UploadsController do
         expect(response).to_not be_success
       end
 
+      it 'returns an error when it could not determine the dimensions of an image' do
+        Jobs.expects(:enqueue).with(:create_thumbnails, anything).never
+
+        message = MessageBus.track_publish do
+          xhr :post, :create, file: fake_jpg, type: "composer"
+        end.first
+
+        expect(response.status).to eq 200
+
+        expect(message.channel).to eq("/uploads/composer")
+        expect(message.data["errors"]).to be
+        expect(message.data["errors"][0]).to eq(I18n.t("upload.images.size_not_found"))
+      end
+
     end
 
   end