#20 ✓ resolved
Mark Bates

'PUT' requests only get query string parameters

Reported by Mark Bates | January 14th, 2009 @ 03:00 PM | in 1.x

In Rack::Request the params method checks if the request is a'PUT' request, if it is it will only return query string parameters and not form parameters:


# The union of GET and POST data.
def params
  self.put? ? self.GET : self.GET.update(self.POST)
rescue EOFError => e
  self.GET
end

Most 'PUT' requests are done using 'POST', because of this you don't get all the parameters you are supposed to.

The method should probably look like this:


# The union of GET and POST data.
def params
  self.get? ? self.GET : self.GET.update(self.POST)
rescue EOFError => e
  self.GET
end

Comments and changes to this ticket

  • Mark Bates

    Mark Bates January 14th, 2009 @ 03:02 PM

    • Assigned user set to “chneukirchen (at gmail)”
  • Mark Bates

    Mark Bates January 14th, 2009 @ 04:11 PM

    • Assigned user changed from “chneukirchen (at gmail)” to “Ryan Tomayko”
  • Ryan Tomayko

    Ryan Tomayko January 14th, 2009 @ 04:40 PM

    • Milestone set to 1.0

    The original "put?" condition was added because the body input stream was being slurped on PUT requests when no Content-Type request header was present. Here's the ML discussion:

    http://groups.google.com/group/r...

    IMO, if the request has a form-data Content-Type, it should be parsed into the POST params unconditionally. There's no reason you can't use PUT with a application/x-www-form-encoded body.

    The other thing that bothers me about the way this method is implemented is 1.) it calls GET.update instead of GET.merge so the GET hash is modified with the POST params, and 2.) it's not memoized so the merge/update happens every time "#params" is called.

    This area of the code has suffered from multiple band-aid patches. Here's what I'd suggest to clean it up:

    1. The #POST method should not assume a application/x-www-form-encoded body when there's no Content-Type header unless the request method is POST (i.e., don't assume application/x-www-form-encoded on PUT). Any request (including POST and PUT) that has an explicit Content-Type header with a form-data type encoding should be parsed.

    2. The EOFError rescue should be moved into the #POST method.

    3. The #params method is implemented something like this:

    
    def params
      @params ||= self.GET.merge(self.POST)
    end
    

    (The memoization should probably be done in the env instead of on the request object.)

    I'll try to put a patch together.

  • Jeremy Kemper

    Jeremy Kemper January 14th, 2009 @ 05:29 PM

    Agreed, Ryan. The content type should determine whether the body is parseable, not the request method.

  • Ryan Tomayko

    Ryan Tomayko January 14th, 2009 @ 05:35 PM

    Yeah. I think the only special case should be when the request method is POST and no Content-Type request header is present. In that case, we should act as if the Content-Type header is "application/x-www-form-encoded".

  • Joshua Peek

    Joshua Peek January 14th, 2009 @ 06:29 PM

    +1 for checking the Content-Type header

  • Ryan Tomayko

    Ryan Tomayko January 15th, 2009 @ 02:26 PM

    • Tag changed from params, request to params, request, review

    This should be a bit better while still fixing the issue with large PUT bodies:

    http://github.com/rtomayko/rack/...

    I'd still like to change the way we update the #GET hash but that's actually a separate issue so I can open another ticket for it. This is murky enough as it is.

    Commit message:

    
    Use Content-Type to determine POST params parsing [#20]
    
    Reverts the hard test for a 'PUT' request method (8d01dc0) and
    uses the Content-Type to determine whether to read into the
    request body. The Request#POST method parses the request body
    if (and only if) either of the following conditions are met:
    
    1. The request's Content-Type is application/x-www-form-urlencoded
       or multipart/form-data. Note: the REQUEST_METHOD is ignored in
       this case.
    
    2. The original REQUEST_METHOD is 'POST' and no Content-Type header
       was specified in the request. Note that we use the REQUEST_METHOD
       value before any modifications by the MethodOverride middleware.
    
    This is very similar to how this worked prior to 8d01dc0 but
    narrows the 'no Content-Type' special case to apply only to
    POST requests. A PUT request with no Content-Type header would
    trigger parsing before - with this change only POST requests
    with no Content-Type trigger parsing.
    
  • Mislav

    Mislav January 15th, 2009 @ 02:29 PM

    What about memoization?

  • Ryan Tomayko

    Ryan Tomayko January 15th, 2009 @ 02:39 PM

    Yeah. We need to tackle memoization, too. I tried with an earlier rev of this patch and it introduced a few legitimate test failures that probably need to be considered independently from the PUT issue. I don't think memoization should hold this up. I'll split that out into a separate ticket.

  • Joshua Peek

    Joshua Peek August 3rd, 2009 @ 04:11 PM

    • State changed from “new” to “open”
    • Milestone changed from 1.0 to 1.1
  • chneukirchen (at gmail)

    chneukirchen (at gmail) August 3rd, 2009 @ 10:15 PM

    +1 for checking the Content-Type header

    But Rack 1.1 because it is incompatible.

  • Joshua Peek

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

    • Milestone changed from 1.1 to 1.x
  • Ryan Tomayko

    Ryan Tomayko December 22nd, 2009 @ 10:58 PM

    I've rebased this on top of master (proposed 1.1 release) and pushed to rack/putparsing. Not sure if we want to try to put this in for 1.1 yet but it was getting stale so I figured I'd clean the patch up a bit.

  • chneukirchen (at gmail)
  • Joshua Peek
  • Ryan Tomayko

    Ryan Tomayko December 24th, 2009 @ 03:54 AM

    Josh do you want me to hold off until after 1.1 or are you still accepting stuff into master?

  • Joshua Peek

    Joshua Peek December 24th, 2009 @ 05:23 AM

    Please merge into master.

  • Ryan Tomayko

    Ryan Tomayko December 24th, 2009 @ 06:34 AM

    • State changed from “open” to “resolved”

    Merged. Thanks!

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