Rails core - Jan 2009 - Switching to Rack possibly breaks user code dealing with file uploads

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

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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.

On Tue, Jan 20, 2009 at 9:35 AM, Mislav Marohnić
<mislav.marohnic@gmail.com> wrote:
> 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

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Tue, Jan 20, 2009 at 16:41, Joshua Peek <josh@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?

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Tue, Jan 20, 2009 at 9:48 AM, Mislav Marohnić
<mislav.marohnic@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

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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.


On Tue, Jan 20, 2009 at 16:56, Joshua Peek <josh@joshpeek.com> wrote:
>
> On Tue, Jan 20, 2009 at 9:48 AM, Mislav Marohnić
> <mislav.marohnic@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.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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.
>
> On Tue, Jan 20, 2009 at 16:56, Joshua Peek <j...@joshpeek.com> wrote:
>
> > On Tue, Jan 20, 2009 at 9:48 AM, Mislav Marohnić
> > <mislav.maroh...@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.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---