Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
macera opened this issue Jul 12, 2023 · 1 comment
Closed

Comments

@macera
Copy link

macera commented Jul 12, 2023

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
@macera macera changed the title 【Version3.0.0】check_extension_allowlist! doesn't work the same as v2 【Version3.0.0】check_extension_allowlist! doesn't work the same as v2 Jul 12, 2023
@macera macera changed the title 【Version3.0.0】check_extension_allowlist! doesn't work the same as v2 【Version3.0.0】check_content_type_allowlist! doesn't work the same as v2 Jul 12, 2023
@mshibuya
Copy link
Member

This behavior is correct. Please refer to #2570.

@mshibuya mshibuya closed this as not planned Won't fix, can't repro, duplicate, stale Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants