This project is archived and is in readonly mode.
Empty file uploads should not come through as empty Tempfiles
We're using Paperclip for file uploads and recent edge Rails rendered our user profile forms unusable.
File uploads don't break our application when there was an actual file upload; what breaks Paperclip is the case when nothing was selected in the file input. The form is still sent with multipart encoding and parsed by Rack, which creates a Tempfile regardless of whether some data was received or not.
The result of Rack processing a single file field is a hash with these keys: {:filename, :type, :name, :tempfile, :head}.
Rails further processes this in ActionController::UrlEncodedPairParser.get_typed_value. When it sees the above formatted hash, it replaces it with the Tempfile object it references and applies other metadata, like filename, as properties of this object.
In short, when a "user[avatar]" file field was sent empty, older Rails version would receive nothing:
params[:user][:avatar] # => nil
Now a Tempfile is received in any case:
params[:user][:avatar] # => #<File:/tmp/RackMultipart.xxxyyy>
So naturally Paperclip thinks a file was uploaded and explodes
because this object has nil value original_filename
and a size of 0.
A solution is to check for Tempfile size and not let it through if it's zero.
-
rack-multipart.patch
1.04 KB
-
josh- Milestone cleared.
- State changed from new to open
- Assigned user set to josh
-
Xavier NoriaAn uploaded file could have size 0... Wouldn't be possible to test something else like value[:filename].nil? ?
Not really into it so don't know if it is a valid alternative indeed.
-
Mislav- Assigned user cleared.
Theoretically it could, but what good is storing a 0-byte file? I say ignore it
-
Repository- State changed from open to resolved
(from [01f06fc7f4dda52035d5a2273d402d8555a897a5]) Don't let empty Tempfiles come through as uploaded files [#1785 state:resolved]
Signed-off-by: Joshua Peek josh@joshpeek.com http://github.com/rails/rails/co...
-
Xavier Noria
I think it doesn't matter if you cannot come up with a use case if you can be correct by the same price.
The patch should test whether a file was uploaded, and it doesn't test that. That's my point. If it could it should.
-
josh
- Assigned user set to josh
I created the test fixture by dumping the raw POST body from an empty file upload in Safari.
http://github.com/rails/rails/bl...
Am I missing something else?
-
Mislav
Xavier, would you say that this is a better test for when a file was not uploaded?
if value.has_key?(:tempfile) and not (value[:tempfile].size == 0 and value[:filename].blank?)This check allows 0-sized files to be uploaded if they have a filename.
-
josh
- State changed from resolved to open
Now I'm confused? Xavier, I guess you were referring the to test conditional not test fixture :)
After some more testing, it seems like we could just check for value[:filename].blank? This always seems to be the case when the user does not select a file. This should allow users to upload a text file with no contents.
-
josh
Related Rack Ticket: http://rack.lighthouseapp.com/pr...
-
Xavier Noria
Exactly, my untested guess is that to test if the file was uploaded you just check value[:filename].blank?.
If that's correct checking the size would not be necessary.
-
-
MatthewRudy
Great
Thanks.
I hit this problem on Monday, but didn't have time to debug it.
Nice one.
-
Thijs
I'm stil seeing the first reported behaviour with Paperclip master and Rails master. Am I missing something?
-
Mislav
I didn't experience it anymore after the fix.
The best bet is to test this in your browser: submit a form with empty file field and inspect the parameters you've got in the controller. Is it a Tempfile? If so, can you inspect its properties like
original_filenameandsize?