carrierwaveuploader / carrierwave

Classier solution for file uploads for Rails, Sinatra and other Ruby web frameworks
https://github.com/carrierwaveuploader/carrierwave
8.78k stars 1.66k forks source link

【Version3.0.0】`check_content_type_allowlist!` doesn't work the same as v2 #2673

Closed macera closed 1 year ago

macera commented 1 year ago

check_content_type_allowlist! doesn't work the same as v2.

class SampleUser < ApplicationRecord
  mount_uploader :filename, SampleAvatarUploader
end

class SampleAvatarUploader < CarrierWave::Uploader::Base
  storage :file
  def store_dir
    "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
  end

  def default_url(*args)
    "/images/fallback/" + [version_name, "default.png"].compact.join('_')
  end

  def content_type_allowlist
    [%r{image/}]
  end

  def extension_allowlist
    %w(jpg jpeg gif png)
  end
end
RSpec.describe SampleAvatarUploader do

  describe '#upload' do
    subject { SampleUser.create!(filename: fixture_file_upload('photos/sample.png', media_type)) }

    describe 'image/png' do
      let(:media_type) { 'image/png' }
      it { expect { subject }.not_to raise_error }
    end

    describe 'text/plain' do
      let(:media_type) { 'text/plain' }
      it { expect { subject }.to raise_error(ActiveRecord::RecordInvalid, /You\sare\snot\sallowed\sto\supload\stext\/plain\sfiles/) }
    end
  end
end

v3.0.0

Failures:

  1) SampleAvatarUploader#upload text/plain is expected to raise ActiveRecord::RecordInvalid with message matching /You\sare\snot\sallowed\sto\supload\stext\/plain\sfiles/
     Failure/Error: it { expect { subject }.to raise_error(ActiveRecord::RecordInvalid, /You\sare\snot\sallowed\sto\supload\stext\/plain\sfiles/) }
       expected ActiveRecord::RecordInvalid with message matching /You\sare\snot\sallowed\sto\supload\stext\/plain\sfiles/ but nothing was raised
     # ./spec/uploaders/sample_avatar_uploader_spec.rb:17:in `block (4 levels) in <top (required)>'
     ...
Finished in 0.11192 seconds (files took 8.54 seconds to load)
2 examples, 1 failure

v2.2.4

SampleAvatarUploader
  #upload
    image/png
      is expected not to raise Exception
    text/plain
      is expected to raise ActiveRecord::RecordInvalid with message matching /You\sare\snot\sallowed\sto\supload\stext\/plain\sfiles/

Finished in 0.44736 seconds (files took 10.85 seconds to load)
2 examples, 0 failures

But If overriding the following function, it will work the same as in v2. lib/carrierwave/sanitized_file.rb

def content_type
    @content_type ||=
      #identified_content_type ||
      declared_content_type || 
      identified_content_type || #moved
      guessed_safe_content_type ||
      Marcel::MimeType::BINARY
end

Caused by not being assigned to @content_type at initialization and being assigned to @content_type at check_content_type_allowlist!.

lib/carrierwave/sanitized_file.rb

def initialize(file)
    self.file = file
-    @content = nil
+    @content = @content_type = nil
end
def file=(file)
  if file.is_a?(Hash)
     @file = file["tempfile"] || file[:tempfile]
     @original_filename = file["filename"] || file[:filename]
-    @content_type = file["content_type"] || file[:content_type] || file["type"] || file[:type]
+    @declared_content_type = file["content_type"] || file[:content_type] || file["type"] || file[:type]

And I think it's because existing_content_type(declared_content_type) and identified_content_type(marcel_magic_content_type) were swapped.

def content_type
    @content_type ||=
-        existing_content_type ||
-        marcel_magic_content_type ||
-        mini_mime_content_type
+        identified_content_type ||
+        declared_content_type ||
+        guessed_safe_content_type ||
+       Marcel::MimeType::BINARY
end
mshibuya commented 1 year ago

This behavior is correct. Please refer to #2570.