Fix media attachment size validation not correctly accounting for file type (#16819)

* Fix media attachment size validation not correctly accounting for file type

Fixes a regression introduced in #16724 caused by the fact that kt-paperclip
now correctly runs validations before processing, meaning that file size
verification could not rely on our before_post_processing hook.

Moved the `before_post_processing` hooks to `before_validate` to make sure
the media attachment type is set correctly before the file gets validated.

* Add tests

diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb
index cc48f65ed347f87ac494f7f04c016cbc5f354a41..97d619f35f21c4ae0c3ad79d203e2119587738b9 100644 (file)
--- a/app/models/media_attachment.rb
+++ b/app/models/media_attachment.rb
@@ -167,12 +167,11 @@ class MediaAttachment < ApplicationRecord
                     processors: ->(f) { file_processors f },
                     convert_options: GLOBAL_CONVERT_OPTIONS
 
-  before_file_post_process :set_type_and_extension
-  before_file_post_process :check_video_dimensions
+  before_file_validate :set_type_and_extension
+  before_file_validate :check_video_dimensions
 
   validates_attachment_content_type :file, content_type: IMAGE_MIME_TYPES + VIDEO_MIME_TYPES + AUDIO_MIME_TYPES
-  validates_attachment_size :file, less_than: IMAGE_LIMIT, unless: :larger_media_format?
-  validates_attachment_size :file, less_than: VIDEO_LIMIT, if: :larger_media_format?
+  validates_attachment_size :file, less_than: ->(m) { m.larger_media_format? ? VIDEO_LIMIT : IMAGE_LIMIT }
   remotable_attachment :file, VIDEO_LIMIT, suppress_errors: false, download_on_assign: false, attribute_name: :remote_url
 
   has_attached_file :thumbnail,

diff --git a/spec/models/media_attachment_spec.rb b/spec/models/media_attachment_spec.rb
index edab67d4769cf2fb3ca66259c633108e23d0030a..5dbb3d20607b5e2f023b5886dde435bec88c6b30 100644 (file)
--- a/spec/models/media_attachment_spec.rb
+++ b/spec/models/media_attachment_spec.rb
@@ -181,4 +181,32 @@ RSpec.describe MediaAttachment, type: :model do
       expect(media.description.size).to be <= 1_500
     end
   end
+
+  describe 'size limit validation' do
+    it 'rejects video files that are too large' do
+      stub_const 'MediaAttachment::IMAGE_LIMIT', 100.megabytes
+      stub_const 'MediaAttachment::VIDEO_LIMIT', 1.kilobyte
+      expect { MediaAttachment.create!(account: Fabricate(:account), file: attachment_fixture('attachment.webm')) }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+
+    it 'accepts video files that are small enough' do
+      stub_const 'MediaAttachment::IMAGE_LIMIT', 1.kilobyte
+      stub_const 'MediaAttachment::VIDEO_LIMIT', 100.megabytes
+      media = MediaAttachment.create!(account: Fabricate(:account), file: attachment_fixture('attachment.webm'))
+      expect(media.valid?).to be true
+    end
+
+    it 'rejects image files that are too large' do
+      stub_const 'MediaAttachment::IMAGE_LIMIT', 1.kilobyte
+      stub_const 'MediaAttachment::VIDEO_LIMIT', 100.megabytes
+      expect { MediaAttachment.create!(account: Fabricate(:account), file: attachment_fixture('attachment.jpg')) }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+
+    it 'accepts image files that are small enough' do
+      stub_const 'MediaAttachment::IMAGE_LIMIT', 100.megabytes
+      stub_const 'MediaAttachment::VIDEO_LIMIT', 1.kilobyte
+      media = MediaAttachment.create!(account: Fabricate(:account), file: attachment_fixture('attachment.jpg'))
+      expect(media.valid?).to be true
+    end
+  end
 end