Switching to Rack possibly breaks user code dealing with file uploads

[Mislav Marohnić]

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.

Do you think this should be handled in Rails or Rack?

In the meantime, I've monkeypatched our app: http://gist.github.com/49519

[Joshua Peek]

I'm trying to make Rails 2.3 w/ Rack as backwords compat with the CGI
API. So this is definitely something we need to fix in Rails.

Please create a ticket on LH with some tests (so we can catch this
issue if it happens again) and I'll pull it in ASAP.

Thanks for finding this.

--
Joshua Peek

[Mislav Marohnić]

On Tue, Jan 20, 2009 at 16:41, Joshua Peek <jo...@joshpeek.com> wrote:

I'm trying to make Rails 2.3 w/ Rack as backwords compat with the CGI
API. So this is definitely something we need to fix in Rails.

Please create a ticket on LH with some tests (so we can catch this
issue if it happens again) and I'll pull it in ASAP.

In my workaround I detect there was no file if the `filename` property is blank. Are there cases a valid file could be uploaded without an original filename? Is a better check to see if the size of Tempfile is 0, also?

[Joshua Peek]

On Tue, Jan 20, 2009 at 9:48 AM, Mislav Marohnić
<mislav....@gmail.com> wrote:
> In my workaround I detect there was no file if the `filename` property is
> blank. Are there cases a valid file could be uploaded without an original
> filename? Is a better check to see if the size of Tempfile is 0, also?

I think a filename is always provided. Could probably check
params[:tempfile].length as you suggested too.

hrm, maybe this is a Rack issue too :)
I don't think Rack's multipart parser should be creating empty tempfiles.

--
Joshua Peek

[Mislav Marohnić]

Ticket created http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/1785-empty-file-uploads-should-not-come-through-as-empty-tempfiles

I would like to ask someone to create a failing tests for this. I didn't figure out how to simulate a browser submitting an empty form field -- I don't even know what it sends. An empty byte stream with Content-Type: application/octet-stream; Content-Transfer-Encoding: binary? I tried, but ordinary String comes through instead of Tempfile.

[Joshua Peek]

Applied the fix to Rails.

I also submitted this patch to Rack:
http://rack.lighthouseapp.com/projects/22435/tickets/

On Jan 20, 11:21 am, "Mislav Marohnić" <mislav.maroh...@gmail.com>
wrote:
> Ticket createdhttp://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/17...

> I would like to ask someone to create a failing tests for this. I didn't
> figure out how to simulate a browser submitting an empty form field -- I
> don't even know what it sends. An empty byte stream with Content-Type:
> application/octet-stream; Content-Transfer-Encoding: binary? I tried, but
> ordinary String comes through instead of Tempfile.
>