Fix #3917: Uploads: validate file before resizing/distributing.

app/logical/upload_service/utils.rb

@@ -66,9 +66,15 @@ class UploadService
         w, h = calculate_ugoira_dimensions(file.path)
         yield(w, h)
 
-      else
+      elsif upload.is_image? || upload.is_flash?
         image_size = ImageSpec.new(file.path)
         yield(image_size.width, image_size.height)
+
+      elsif upload.file_ext == "bin"
+        yield(0, 0)
+
+      else
+        raise ArgumentError, "unhandled file type (#{upload.file_ext})" # should not happen
       end
     end
 
@@ -141,6 +147,7 @@ class UploadService
         upload.image_height = height
       end
 
+      upload.validate!(:file)
       upload.tag_string = "#{upload.tag_string} #{Utils.automatic_tags(upload, file)}"
 
       preview_file, crop_file, sample_file = Utils.generate_resizes(file, upload)

app/models/upload.rb

@@ -3,16 +3,20 @@ require "tmpdir"
 class Upload < ApplicationRecord
   class Error < Exception ; end
 
-  class Validator < ActiveModel::Validator
+  class FileValidator < ActiveModel::Validator
     def validate(record)
-      if record.new_record?
+      validate_file_ext(record)
-        validate_md5_uniqueness(record)
+      validate_md5_uniqueness(record)
-        validate_video_duration(record)
+      validate_video_duration(record)
-      end
-
       validate_resolution(record)
     end
 
+    def validate_file_ext(record)
+      if record.file_ext == "bin"
+        record.errors[:file_ext] << "is invalid (only JPEG, PNG, GIF, SWF, MP4, and WebM files are allowed"
+      end
+    end
+
     def validate_md5_uniqueness(record)
       if record.md5.nil?
         return
@@ -36,6 +40,10 @@ class Upload < ApplicationRecord
 
       if resolution > Danbooru.config.max_image_resolution
         record.errors[:base] << "image resolution is too large (resolution: #{(resolution / 1_000_000.0).round(1)} megapixels (#{record.image_width}x#{record.image_height}); max: #{Danbooru.config.max_image_resolution / 1_000_000} megapixels)"
+      elsif record.image_width > Danbooru.config.max_image_width
+        record.errors[:image_width] << "is too large (width: #{record.image_width}; max width: #{Danbooru.config.max_image_width})"
+      elsif record.image_height > Danbooru.config.max_image_height
+        record.errors[:image_height] << "is too large (height: #{record.image_height}; max height: #{Danbooru.config.max_image_height})"
       end
     end
 
@@ -55,12 +63,9 @@ class Upload < ApplicationRecord
   before_validation :assign_rating_from_tags
   validate :uploader_is_not_limited, on: :create
   # validates :source, format: { with: /\Ahttps?/ }, if: ->(record) {record.file.blank?}, on: :create
-  validates :image_height, numericality: { less_than_or_equal_to: Danbooru.config.max_image_height }, allow_nil: true
-  validates :image_width, numericality: { less_than_or_equal_to: Danbooru.config.max_image_width }, allow_nil: true
   validates :rating, inclusion: { in: %w(q e s) }, allow_nil: true
   validates :md5, confirmation: true, if: -> (rec) { rec.md5_confirmation.present? }
-  validates :file_ext, format: { with: /jpg|gif|png|swf|webm|mp4|zip/ }, allow_nil: true
+  validates_with FileValidator, on: :file
-  validates_with Validator
   serialize :context, JSON
   scope :preprocessed, -> { where(status: "preprocessed") }
 

test/models/upload_service_test.rb

@@ -108,9 +108,7 @@ class UploadServiceTest < ActiveSupport::TestCase
       context "for an ugoira" do
         setup do
           @file = File.open("test/files/valid_ugoira.zip", "rb")
-          @upload = mock()
+          @upload = Upload.new(file_ext: "zip")
-          @upload.stubs(:is_video?).returns(false)
-          @upload.stubs(:is_ugoira?).returns(true)
         end
 
         teardown do
@@ -129,8 +127,7 @@ class UploadServiceTest < ActiveSupport::TestCase
       context "for a video" do
         setup do
           @file = File.open("test/files/test-300x300.mp4", "rb")
-          @upload = mock()
+          @upload = Upload.new(file_ext: "mp4")
-          @upload.stubs(:is_video?).returns(true)
         end
 
         teardown do
@@ -148,9 +145,7 @@ class UploadServiceTest < ActiveSupport::TestCase
       context "for an image" do 
         setup do
           @file = File.open("test/files/test.jpg", "rb")
-          @upload = mock()
+          @upload = Upload.new(file_ext: "jpg")
-          @upload.stubs(:is_video?).returns(false)
-          @upload.stubs(:is_ugoira?).returns(false)
         end
 
         teardown do
@@ -498,15 +493,9 @@ class UploadServiceTest < ActiveSupport::TestCase
       end
 
       context "for an invalid content type" do
-        setup do
-          @source = "http://www.example.com"
-          @service = subject.new(source: @source)
-        end
-
         should "fail" do
-          upload = @service.start!
+          upload = subject.new(source: "http://www.example.com").start!
-          upload.reload
+          assert_match(/\Aerror:.*File ext is invalid/, upload.status)
-          assert_match(/error:/, upload.status)
         end
       end
     end