#79 ✓ resolved
Mark Goris

Multipart handling incorrectly assuming file upload

Reported by Mark Goris | November 10th, 2009 @ 10:06 PM | in 1.1

I tried posting this (twice) in the Rack developer group, but the posts aren't showing up. Not sure if their getting mod rejected or not. So I apologize in advance if this should have started as a discussion first, issue (or non-issue) later.

I'm running into a problem with Rack 1.0.1 caused by this commit: http://github.com/rack/rack/commit/6674f3652ed19136802a0b84f1923f0b...

The problem is that in 1.0.0 (and prior) Rack was using the filename attribute to decide it was a file upload. As of 1.0.1 it now is using the filename or a content-type attribute. As far as I can tell from reading RFC 2388 that is not correct. RFC 2388 doesn't state that. As a matter of fact, Section 4.5 states that "Each part of a multipart/form-data is supposed to have a content-type." So this change in behavior appears to go against the relevant RFC.

From my experimentation I can see that most browsers don't send content-type, except for file-upload fields. But there are libraries (like the often using Apache HttpClient library) that are more strict and do send content-type for each part of a multipart. That is where this became an issue for me. I'm was using Jmeter to drive load into my Rails app, and as of 1.0.1 that no longer works. Rack assumes file upload for fields that aren't file uploads, and I get tempfiles for every field, not just the uploaded file.

So my proposal is that this should go back to using filename to decide when something is a file. If there is some reason why that was deemed insufficient (or incorrect), I'd like to understand that too.

FYI, the thread on the developer group I tried initiating this proposal on is http://groups.google.com/group/rack-devel/browse_frm/thread/24a0fa8...

Comments and changes to this ticket

  • Jonathan del Strother

    Jonathan del Strother November 11th, 2009 @ 03:25 PM

    FWIW, I still consider this a bug & my comments in the google groups page still stand. Would be great to get it fixed, but in the meantime, we're hacking around it in our Rails app using something like the following:

    class ActionController::Request
      private
      def normalize_parameters_with_stringified_files(params)
        if params.is_a?(Hash) and params[:tempfile] and params[:type]=~/^text\/plain(?:; charset=([^;]+))/
          param = params[:tempfile].read
          if encoding = $1
            param = Iconv.iconv('utf-8', encoding, param).to_s
          end
          param
        else
          normalize_parameters_without_stringified_files(params)
        end
      end
      alias_method_chain :normalize_parameters, :stringified_files
    end
    
  • Joshua Peek

    Joshua Peek December 11th, 2009 @ 03:20 AM

    • Assigned user set to “chneukirchen (at gmail)”
    • State changed from “new” to “open”
    • Milestone set to 1.1
  • Paul

    Paul January 8th, 2010 @ 04:09 PM

    the version of http://jupload.sourceforge.net/ i am using is sending content-types with parameters too / so this bug (i think this is a bug) appears there too.
    for another example you might look at http://blogs.sun.com/mandy/entry/rack_and_content_type_for

    +1 for 'if filename' and nothing more...

  • chneukirchen (at gmail)

    chneukirchen (at gmail) January 8th, 2010 @ 04:56 PM

    Proposal 1: remove the check for parsable_data?, only check for filename.
    Proposal 2: (parsable_data? && content_type) || (form_data? && filename)

    The second option allows this parser to be used for ws-* stuff, which
    might be useful to some people.

  • chneukirchen (at gmail)

    chneukirchen (at gmail) January 8th, 2010 @ 06:00 PM

    Proposal 1: remove the check for parsable_data?, only check for filename.
    Proposal 2: (parsable_data? && content_type) || (form_data? && filename)

    The second option allows this parser to be used for ws-* stuff, which
    might be useful to some people.

  • chneukirchen (at gmail)

    chneukirchen (at gmail) January 8th, 2010 @ 06:01 PM

    Proposal 1: remove the check for parsable_data?, only check for filename.
    Proposal 2: (parsable_data? && content_type) || (form_data? && filename)

    The second option allows this parser to be used for ws-* stuff, which
    might be useful to some people.

  • Mark Goris

    Mark Goris January 9th, 2010 @ 01:02 AM

    I haven't looked at all the code in detail, but I'm a little confused about how the "parsable_data?" check factors in here. In looking at the specific commit I referenced, it looked like lib/rack/utils.rb was make a decision about using tempfiles based on the presences of either a content_type field or a filename. So I think the other check that would be removed is "content_type", not "parsable_data?" (which is what you stated in Proposal 1). That's the way it reads in today's code as well.

    As for Proposal 2, obviously there is some other consideration you want to make regarding WS-* stuff that I don't have insight into, so it's hard for me to judge it's merit. At the highest level, I would mostly likely be satisfied as long as the presence of a content_type attribute didn't immediately trigger Rack to save off data in a tempfile. If Proposal 2 would satisfy that (it appears like it might), then that is fine by me.

  • Mateo Murphy

    Mateo Murphy February 18th, 2010 @ 10:45 PM

    I've just been bit by this behavior myself, and coded a work around in my application; I can submit a patch if it would save someone else the time.

    Regarding the proposal 2; Yes, it would fix the bug, but I'm not certain that this behavior would be correct for the ws-* cases either. I'm not familiar with those specs to say for sure, but I think it would be wise to be absolutely certain of this before implementing it.

  • chneukirchen (at gmail)
  • Mateo Murphy

    Mateo Murphy February 19th, 2010 @ 03:48 PM

    Great! Correct me if I'm wrong though, but shouldn't the following else statement be removed as well?

    http://github.com/rack/rack/blob/acffe8ef5ea6de74fe306f2dd908b7681a...

    I'm pretty sure body will not exist at this point, resulting in an exception thrown

  • Jeremy Kemper

    Jeremy Kemper June 28th, 2010 @ 12:17 AM

    • Milestone order changed from “0” to “0”

    Mateo, yes. Body will be a string at that point, which can't be rewound.

    This fix also breaks apps that expect to use the Content-Type of the MIME part, such as an app parsing multipart/mixed.

    I think it should be reverted.

  • Jeremy Kemper

    Jeremy Kemper June 28th, 2010 @ 12:28 AM

    A better choice would be to skip the Tempfile for text/plain only

  • Jeremy Kemper
  • Jeremy Kemper

    Jeremy Kemper June 28th, 2010 @ 12:44 AM

    Losing the metadata for these MIME parts is painful, though. I think the right fix is to treat text/plain specially, given that's the content type in all the examples above.

  • Jeremy Kemper

    Jeremy Kemper September 21st, 2010 @ 11:57 PM

    Fix has been pending here for a few months. Could someone please reopen this ticket until it's addressed?

  • Jeremy Kemper

    Jeremy Kemper May 17th, 2011 @ 08:49 PM

    The multipart parser was recently refactored, fixing this issue.

    Here's a test verifying it (against master).

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Pages