Comments (28)
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.
from carrierwave-bombshelter.
Thanks for answer and gem maintaining.
So, do you think the reason of such server behaviour (crash) is the kind of mime-type faking?
from carrierwave-bombshelter.
Done. Now it checks if the file is an actual image. Plz check.
from carrierwave-bombshelter.
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
from carrierwave-bombshelter.
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
from carrierwave-bombshelter.
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 gem
s "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.
from carrierwave-bombshelter.
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?
from carrierwave-bombshelter.
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
from carrierwave-bombshelter.
Does this bug apper with unpacked spark.png?
from carrierwave-bombshelter.
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"}
`
from carrierwave-bombshelter.
What version of CarrierWave do you use?
from carrierwave-bombshelter.
Using carrierwave 0.10.0 from git://github.com/carrierwaveuploader/carrierwave.git
The line in Gemfile:
gem 'carrierwave', github: 'carrierwaveuploader/carrierwave'
from carrierwave-bombshelter.
Here is the stack of calls when the code is beginning the processing
from carrierwave-bombshelter.
Now it's time to ask: are you sure you're running the last version of your code? Because this looks totally strange.
from carrierwave-bombshelter.
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
from carrierwave-bombshelter.
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?
from carrierwave-bombshelter.
Still weird. I can't even reproduce this bug. CarrierWave runs processing after caching.
from carrierwave-bombshelter.
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 }
from carrierwave-bombshelter.
Tried to exclude file_validations gem, no changes
from carrierwave-bombshelter.
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"]
from carrierwave-bombshelter.
I am confused too... For some reason my "processing" command in uploader is performed without your gem`s before :cache...
from carrierwave-bombshelter.
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 ?"
from carrierwave-bombshelter.
Ahh, I reproduced the bug. It appears on the edge version of CarrierWave. Please use stable version until I find the solution.
from carrierwave-bombshelter.
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?
from carrierwave-bombshelter.
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.
from carrierwave-bombshelter.
Looks like the problem is solved. Check, please.
from carrierwave-bombshelter.
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 :)
from carrierwave-bombshelter.
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 :)
При отрицании в данном случае правильнее использовать родительный падеж.
from carrierwave-bombshelter.
Related Issues (7)
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from carrierwave-bombshelter.