Permalink
Browse files

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.
  • Loading branch information...
1 parent 1ffa95c commit aea2d608fe592a665741087a8d6d31fcf26848ec @rtomayko rtomayko committed Jan 15, 2009
Showing with 30 additions and 10 deletions.
  1. +9 −5 lib/rack/request.rb
  2. +21 −5 test/spec_rack_request.rb
View
14 lib/rack/request.rb
@@ -90,7 +90,6 @@ def head?; request_method == "HEAD" end
# one of the media types presents in this list will not be eligible
# for form-data / param parsing.
FORM_DATA_MEDIA_TYPES = [
- nil,
'application/x-www-form-urlencoded',
'multipart/form-data'
]
@@ -104,12 +103,17 @@ def head?; request_method == "HEAD" end
]
# Determine whether the request body contains form-data by checking
- # the request media_type against registered form-data media-types:
- # "application/x-www-form-urlencoded" and "multipart/form-data". The
+ # the request Content-Type for one of the media-types:
+ # "application/x-www-form-urlencoded" or "multipart/form-data". The
# list of form-data media types can be modified through the
# +FORM_DATA_MEDIA_TYPES+ array.
+ #
+ # A request body is also assumed to contain form-data when no
+ # Content-Type header is provided and the request_method is POST.
def form_data?
- FORM_DATA_MEDIA_TYPES.include?(media_type)
+ type = media_type
+ meth = env["rack.methodoverride.original_method"] || env['REQUEST_METHOD']
+ (meth == 'POST' && type.nil?) || FORM_DATA_MEDIA_TYPES.include?(type)
end
# Determine whether the request body contains data by checking
@@ -158,7 +162,7 @@ def POST
# The union of GET and POST data.
def params
- self.put? ? self.GET : self.GET.update(self.POST)
@rtomayko
rtomayko Dec 22, 2009

The issue is primarily with this line. It makes it impossible to use the body for params on PUT requests. My main issue with it is that it's one of the least correct pieces of code in Rack. Can't have that :)

+ self.GET.update(self.POST)
rescue EOFError => e
self.GET
end
View
26 test/spec_rack_request.rb
@@ -66,9 +66,11 @@
lambda { req.POST }.should.raise(RuntimeError)
end
- specify "can parse POST data" do
+ specify "can parse POST data when method is POST and no Content-Type given" do
req = Rack::Request.new \
- Rack::MockRequest.env_for("/?foo=quux", :input => "foo=bar&quux=bla")
+ Rack::MockRequest.env_for("/?foo=quux",
+ "REQUEST_METHOD" => 'POST',
+ :input => "foo=bar&quux=bla")
req.content_type.should.be.nil
req.media_type.should.be.nil
req.query_string.should.equal "foo=quux"
@@ -77,7 +79,7 @@
req.params.should.equal "foo" => "bar", "quux" => "bla"
end
- specify "can parse POST data with explicit content type" do
+ specify "can parse POST data with explicit content type regardless of method" do
req = Rack::Request.new \
Rack::MockRequest.env_for("/",
"CONTENT_TYPE" => 'application/x-www-form-urlencoded;foo=bar',
@@ -92,6 +94,7 @@
specify "does not parse POST data when media type is not form-data" do
req = Rack::Request.new \
Rack::MockRequest.env_for("/?foo=quux",
+ "REQUEST_METHOD" => 'POST',
"CONTENT_TYPE" => 'text/plain;charset=utf-8',
:input => "foo=bar&quux=bla")
req.content_type.should.equal 'text/plain;charset=utf-8'
@@ -102,6 +105,16 @@
req.body.read.should.equal "foo=bar&quux=bla"
end
+ specify "can parse POST data on PUT when media type is form-data" do
+ req = Rack::Request.new \
+ Rack::MockRequest.env_for("/?foo=quux",
+ "REQUEST_METHOD" => 'PUT',
+ "CONTENT_TYPE" => 'application/x-www-form-urlencoded',
+ :input => "foo=bar&quux=bla")
+ req.POST.should.equal "foo" => "bar", "quux" => "bla"
+ req.body.read.should.equal "foo=bar&quux=bla"
+ end
+
specify "rewinds input after parsing POST data" do
input = StringIO.new("foo=bar&quux=bla")
req = Rack::Request.new \
@@ -114,7 +127,8 @@
specify "cleans up Safari's ajax POST body" do
req = Rack::Request.new \
- Rack::MockRequest.env_for("/", :input => "foo=bar&quux=bla\0")
+ Rack::MockRequest.env_for("/",
+ 'REQUEST_METHOD' => 'POST', :input => "foo=bar&quux=bla\0")
req.POST.should.equal "foo" => "bar", "quux" => "bla"
end
@@ -173,7 +187,9 @@
specify "can cache, but invalidates the cache" do
req = Rack::Request.new \
- Rack::MockRequest.env_for("/?foo=quux", :input => "foo=bar&quux=bla")
+ Rack::MockRequest.env_for("/?foo=quux",
+ "CONTENT_TYPE" => "application/x-www-form-urlencoded",
+ :input => "foo=bar&quux=bla")
req.GET.should.equal "foo" => "quux"
req.GET.should.equal "foo" => "quux"
req.env["QUERY_STRING"] = "bla=foo"

6 comments on commit aea2d60

@mislav

I've lost track of why this was such a big deal, but OK—I'm happy some cleaning has been made.

@gtd

@rtomayko Is there any objective reasoning for the last part of the change other than not regressing on Eric Wong's patch? It seems to me that if POST requests parse without a content type then PUT should too. This bit us bad (especially because PUT requests are much less common than POSTs in the API in question, and so it failed slooowly) and now we are stuck maintaining a fork of rack just to forward port a fix for this which was submitted as a pull request #118 and closed unceremoniously by Josh citing this commit.

@rtomayko

Can you clarify on "the last part of the change"? It's been a while since I've had my head fully wrapped around this.

POSTs with no content type were fairly common. The assumption was that if you're writing clients that use PUT, you'd be able to provide a content-type whereas many tools POST things without any real ability to configure a content-type. I'm curious where you're seeing PUT used without a content-type.

Another thing you could do is insert a middleware early in the chain that added a default Content-Type header to requests that don't have it.

@gtd

I meant specifically the change to not parse PUT requests without content-type.

The reason we're seeing PUT requests without a content type is because we're working with a Sony PS3 application that is using some byzantine corporate libraries. Needless to say, requesting a fix for this requires mobilizing corporate development teams across 3 continents and a handful of multinational subsidiaries.

Your suggestion of a middleware is actually a very smart one that hadn't occurred to me, so we'll probably move in that direction.

Nevertheless, what do you think of my thesis that PUT/POST should be consistent as they used to be?

@rtomayko

The reason we're seeing PUT requests without a content type is because we're working with a Sony PS3 application that is using some byzantine corporate libraries. Needless to say, requesting a fix for this requires mobilizing corporate development teams across 3 continents and a handful of multinational subsidiaries.

Heh. Yeah, this is exactly why the POST exception was made. If PUT is effected too we should consider it I guess.

Nevertheless, what do you think of my thesis that PUT/POST should be consistent as they used to be?

Makes sense to me. Although I'd almost rather remove the POST special case at this point. Maybe in the next major rev of Rack. The huge issue here is that parsing into this body when its not actually form encoded could fail or have massive memory implications (think of a file upload without content-type being parsed into by the param parser).

@gtd

Yes, I would be happy with that as well as it seems like The Right Thing™ though I suspect you'll get a lot more blowback since POST requests are so common. However in our case this would have caused the app to fail fast and we could have demanded Sony fix the problem back when resources were allocated ;)

I'll give a +1 and defend you against the hordes if you decide to go that way. Thanks for your time.

Please sign in to comment.