'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 January 14th, 2009 @ 03:02 PM
- Assigned user set to “chneukirchen (at gmail)”
-
Mark Bates January 14th, 2009 @ 04:11 PM
- Assigned user changed from “chneukirchen (at gmail)” to “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:
-
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.
-
The EOFError rescue should be moved into the #POST method.
-
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 January 14th, 2009 @ 05:29 PM
Agreed, Ryan. The content type should determine whether the body is parseable, not the request method.
-
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".
-
-
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. -
-
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.
-
josh August 3rd, 2009 @ 04:11 PM
- State changed from “new” to “open”
- Milestone changed from “1.0” to “1.1”
-
chneukirchen (at gmail) August 3rd, 2009 @ 10:15 PM
+1 for checking the Content-Type header
But Rack 1.1 because it is incompatible.
-
josh December 11th, 2009 @ 03:11 AM
- Milestone changed from “1.1” to “1.x”
-
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.
-
-
-
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?
-
-
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.
Create new ticket
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป
chneukirchen (at gmail)
josh
Mislav
Ryan Tomayko