Looks like this does not work

[darthsim]

Looks like this does not work

[DarthSim]

Hi.
Yep, it looks like a bad idea to ignore non-image files while checking. Will be fixed soon.

I also found the words in FastImage gem description:
'But take care to sanitise the strings passed to FastImage; it will try to read from whatever is passed.'
Is it safe to send FastImage.size(new_file.path) directly without any sanitizing?

new_file is provided by CarrierWave, it's definitely your uploaded file and not some secret system stuff. So yes, it's safe.

[sintro]

Thanks for answer and gem maintaining.
So, do you think the reason of such server behaviour (crash) is the kind of mime-type faking?

[DarthSim]

Done. Now it checks if the file is an actual image. Plz check.

[sintro]

Looks like it is still trying to modify the file after uploading the faked .png (which previously was bz2)... Fix does not work for me.
Do you sure the code is correct in your last commit:
return if image.type

[DarthSim]

Yep, I'm totally sure, look at the tests. It looks like the bug is somewhere outside bombshelter.

[1] pry(main)> UserAvatarUploader.new.cache!(File.open("tmp/spark.png"))
CarrierWave::IntegrityError: File is not an image

[sintro]

I just tried to repeat this bug in my local machine within more real environment: I have working RoR application, with avatar uploader. So I tried to upload the fake spark.png in debug mode, and got the error:
[here is russian translation of error main description, like "Error communicate with Mini_Magick, maybe this is not the picture? The original error:"]
And then:
Command ("mogrify -format jpg -resize 260 -gravity North -background rgba(255,255,255,0.0) /var/folders/cr/b4_bf02s1739dcjlgjdwzzdm0000gn/T/mini_magick20151024-13647-9m3f00.png[0]") failed: {:status_code=>1, :output=>"mogrify: unable to extend cache /var/tmp/magick-13668eDoaViyfKAMD': No space left on device @ error/cache.c/OpenPixelCache/3722.\nmogrify: Memory allocation failed /var/tmp/magick-13668eDoaViyfKAMD' @ error/png.c/MagickPNGErrorHandler/1630.\nmogrify: corrupt image /var/tmp/magick-13668eDoaViyfKAMD' @ error/png.c/ReadPNGImage/3958.\n"} What I got from this message is: mini_magick is still trying to process the image, and your gems "before :cache" hook does not fire at all (I made the break point at the file type checking line of your gem).
So I have assumption now:
My uploader set like this:

 # Process files as they are upsaded:
  process :efficient_conversion => [260, 260, 'North']

  # Create different versions of your uploaded files:
  version :thumb do
    process resize_to_fill: [80, 80, 'North']
  end

You can see, that it processes the original image, resizing it 260x260. This is what error message contains: Command ("mogrify -format jpg -resize 260 -gravity North -background rgba(255,255,255,0.0) /var/folders/cr/b4_bf02s1739dcjlgjdwzzdm0000gn/T/mini_magick20151024-13647-9m3f00.png[0]") failed.
So CarrierWave actually tried to perform this processing before you gem was hit.
May be the reason is that such images processing without versions performed even before before :cache hook? If so, can we fix it, or I should make something like "normal" version of uploaded image, without original image processing? I am really don't want to do it, because see no reasons to keep original images on server or perform any post-operations on deleting it.

[DarthSim]

before :cache fires every time you're trying to upload. All native CarrierWave validators are bound on before :cache.
Can I see a whole code of your uploader?

[sintro]

Here is it:

# encoding: utf-8

class AvatarUploader < CarrierWave::Uploader::Base

  # Include RMagick or MiniMagick support:
  # include CarrierWave::RMagick
  include CarrierWave::BombShelter
  include CarrierWave::MiniMagick

  CarrierWave::SanitizedFile.sanitize_regexp = /[^a-zA-Zа-яА-ЯёЁ0-9\.\-\+_]/

  #TODO!!! https://net.dirty.ru/png-bomba-828390/ FIX?

  FILE_FORMAT = 'jpg'
  ALLOWED_TYPES = %w(jpg jpeg gif png bmp tif tiff)

  # Choose what kind of storage to use for this uploader:
  storage :file
  # storage :fog

  # Override the directory where uploaded files will be stored.
  # This is a sensible default for uploaders that are meant to be mounted:
  def store_dir
    "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
  end

  # Provide a default URL as a default if there hasn't been a file uploaded:
  def default_url(*args)
    ActionController::Base.helpers.asset_path("uploads/#{model.class.to_s.underscore}/#{mounted_as}/" + [version_name, "default.png"].compact.join('_'))
  end

  # Process files as they are uploaded:
  process :efficient_conversion => [260, 260, 'North']

  # Create different versions of your uploaded files:
  version :thumb do
    process resize_to_fill: [80, 80, 'North']
  end

  version :navbar_thumb, from_version: :thumb do
    process resize_to_fill: [36, 36, 'North']
  end

  def image_name
    uri = URI.parse(url)
    name = File.basename(uri.path)
    if name.split('.').first == 'default'
      I18n.t('common.didnt_upload_avatar')
    else
      name
    end
  end

  # Add a white list of extensions which are allowed to be uploaded.
  # For images you might use something like this:
  def extension_white_list
    ALLOWED_TYPES
  end

  # Override the filename of the uploaded files:
  # Avoid using model.id or version_name here, see uploader/store.rb for details.
  def filename
    # if original_filename
      # if model && model.read_attribute(mounted_as).present?
      #   model.read_attribute(mounted_as)
      # else
      #   Russian.translit(super.chomp(File.extname(super))).gsub('-', '.').downcase + '_'
      #       secure_token +
      #       '.' + FILE_FORMAT
      # end
    # end
    Russian.translit(super.chomp(File.extname(super))).gsub('-', '.').downcase + '_' +
        "#{secure_token(10)}.#{FILE_FORMAT}" if original_filename.present?
  end

  protected

  def secure_token(length=16)
    var = :"@#{mounted_as}_secure_token"
    model.instance_variable_get(var) or model.instance_variable_set(var, SecureRandom.hex(length/2))
  end

  private

  def efficient_conversion(width, height, gravity = 'Center')
    manipulate! do |img|
      cols, rows = img[:dimensions]
      img.format(FILE_FORMAT) do |cmd|
        if width != cols || height != rows
          scale_x = width/cols.to_f
          scale_y = height/rows.to_f
          if scale_x >= scale_y
            cols = (scale_x * (cols + 0.5)).round
            rows = (scale_x * (rows + 0.5)).round
            cmd.resize "#{cols}"
          else
            cols = (scale_y * (cols + 0.5)).round
            rows = (scale_y * (rows + 0.5)).round
            cmd.resize "x#{rows}"
          end
        end
        cmd.gravity gravity
        cmd.background "rgba(255,255,255,0.0)"
        cmd.extent "#{width}x#{height}" if cols != width || rows != height
      end
      img.quality("75")
      img = yield(img) if block_given?
      img
    end
  end


end

[DarthSim]

Does this bug apper with unpacked spark.png?

[sintro]

Checked it, and yes, got the same error:
Command ("mogrify -format jpg -resize 260 -gravity North -background rgba(255,255,255,0.0) /var/folders/cr/b4_bf02s1739dcjlgjdwzzdm0000gn/T/mini_magick20151024-13834-1xiujp7.png[0]") failed: {:status_code=>1, :output=>"mogrify: unable to extend cache/var/folders/cr/b4_bf02s1739dcjlgjdwzzdm0000gn/T/mini_magick20151024-13834-1xiujp7.png': No space left on device @ error/cache.c/OpenPixelCache/3722.\nmogrify: Memory allocation failed /var/folders/cr/b4_bf02s1739dcjlgjdwzzdm0000gn/T/mini_magick20151024-13834-1xiujp7.png' @ error/png.c/MagickPNGErrorHandler/1630.\nmogrify: corrupt image /var/folders/cr/b4_bf02s1739dcjlgjdwzzdm0000gn/T/mini_magick20151024-13834-1xiujp7.png' @ error/png.c/ReadPNGImage/3958.\n"}
`

[DarthSim]

What version of CarrierWave do you use?

[sintro]

Using carrierwave 0.10.0 from git://github.com/carrierwaveuploader/carrierwave.git

The line in Gemfile:
gem 'carrierwave', github: 'carrierwaveuploader/carrierwave'

[sintro]

Here is the stack of calls when the code is beginning the processing

[DarthSim]

Now it's time to ask: are you sure you're running the last version of your code? Because this looks totally strange.

[sintro]

Just changed the processing command to resize on 261x261 (not 260x260), and yes, I got the error

 Command ("mogrify -format jpg -resize 261 ...

So it is 100% tries to execute exact this file

[sintro]

Ok, I just commented the

 process :efficient_conversion => [261, 260, 'North']

and bingo, now your plugin was hit! So, this string is performed even BEFORE the hooks... Any suggestions on how to workaround this behavior?

[DarthSim]

Still weird. I can't even reproduce this bug. CarrierWave runs processing after caching.

[sintro]

You can try to include the file_validations gem, I am using it in User model to validate the size of included file, by this validation:

  AVATAR_FILE_TYPES =  AvatarUploader::ALLOWED_TYPES.map {|type| 'image/'+type}
  validates :avatar, file_size: { less_than_or_equal_to: 2.megabytes },
            file_content_type: { allow: AVATAR_FILE_TYPES }

[sintro]

Tried to exclude file_validations gem, no changes

[DarthSim]

Sorry, still can't reproduce the bug even by using your uploader.

[1] pry(main)> u = User.last
  User Load (1.8ms)  SELECT  "users".* FROM "users" WHERE "users"."deleted_at" IS NULL  ORDER BY "users"."id" DESC LIMIT 1
=> #<User:0x007fe78f0c8f68...>
[2] pry(main)> u.update(avatar: File.open("tmp/spark2.png"))
   (0.2ms)  BEGIN
   (0.3ms)  ROLLBACK
=> false
[3] pry(main)> u.errors[:avatar]
=> ["Image size should be less than or equal to 4096x4096"]

[sintro]

I am confused too... For some reason my "processing" command in uploader is performed without your gem`s before :cache...

[sintro]

Interesting commet carrierwaveuploader/carrierwave#1320 (comment)
"Anything happened with this? I notice that the process! is called with before :cache, so maybe it should be moved to before :store ?"

[DarthSim]

Ahh, I reproduced the bug. It appears on the edge version of CarrierWave. Please use stable version until I find the solution.

[sintro]

Weird... I noticed that "process" method is called also by "before :cache" hook, so it can be can be called before any other registered callback if there is no priority system?
I think the possible solution of this problem is to override the "process" method?

[DarthSim]

There is no priority system, just FIFO.

I think the possible solution of this problem is to override the "process" method?

This is what plugin definitely should not do.

[DarthSim]

Looks like the problem is solved. Check, please.

[sintro]

Thanks! Now everything works fine!
Some more questions and suggestions. Do you sure that
_before_callbacks[:cache].unshift(:protect_from_image_bomb!) is the reliable way to keep this gem working?
And some suggestions about i18n of your plugin: rails messaging system is not intended to use nouns in "messages", but the description should be stackable with attribute: format: "%{attribute} %{message}".
So I think the better way to do messages is something like:

      pixel_dimensions_error: "size should be less than or equal to %{x_size}x%{y_size}"
      not_image: "is not an image"

Also, there is a grammar mistake in russian locale:
`pixel_dimensions_error: "Изображение не должно превышать размера %{x_size}x%{y_size}"``
It should be "размер", not "размера". Sorry for nicety :)

[DarthSim]

Do you sure that _before_callbacks[:cache].unshift(:protect_from_image_bomb!) is the reliable way to keep this gem working?

They're using this format for callbacks for a years. Anyway I'm going to make a PR with prepend_before and prepend_after.

And some suggestions about i18n of your plugin: rails messaging system is not intended to use nouns in "messages", but the description should be stackable with attribute: format: "%{attribute} %{message}".

I've done this partn in CarrierWave style.

Also, there is a grammar mistake in russian locale:
pixel_dimensions_error: "Изображение не должно превышать размера %{x_size}x%{y_size}"
It should be "размер", not "размера". Sorry for nicety :)

При отрицании в данном случае правильнее использовать родительный падеж.