Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

correct handling for incomplete hash/array parameters #615

Merged
merged 2 commits into from

4 participants

@vspy

Two problems:
1. incompete parameters with single bracket caused an exception "Expected Hash, ...", while it is pretty much valid parameter name
2. rack.request.query_hash should be only set after query is parsed. Otherwise, in case of thrown exception, client will get exception only first time, next time execution goes inside first if block and nil is being returned. This leads to absolutely weird and non-descriptive exception in params method (No method merge for Nil)

@vspy

Is there something wrong with this pull request?

@BukhariH

Why have you added this variable?

There are two ways to prevent @env["rack.request.query_string"] written before actually parsing query. One is to parse it beforehand as I did it, another is to flip @env["rack.request.query_string"]=… and @env["rack.request.query_hash"]=....

I just think that this way it is more implicit that parsing is done before caching query string. But I don't mind to just flip following two lines, if you think it would be better.

As I (maybe very briefly) explained in pull request, this whole situation of writing rack.request.query_string before actually parsing query leads to weird, non-descriptive error messages in case parsing itself ends with throwing some exception. This is especially important when you using rack behind some web-framework. I was using rails when I encountered this bug, and what I have seen - first exception were caught by something inside the rails, but when I tried to access parameters again @env["rack.request.query_string"] == query_string was true, obviously, but @env["rack.request.query_hash"] was nil, so it failed somewhere here, in request.rb trying to merge something into query hash, which was nil.

@tenderlove
Owner

@vspy can you add a test for the second behavior you desire? I see tests for the first behavior, but I'm afraid if we don't have tests for the second behavior, people will just say "why do we have this extra variable?" and there will be a regression.

@vspy

done!

@raggi raggi merged commit 23dfded into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
3  lib/rack/request.rb
@@ -192,8 +192,9 @@ def GET
if @env["rack.request.query_string"] == query_string
@env["rack.request.query_hash"]
else
+ p = parse_query(query_string)
@env["rack.request.query_string"] = query_string
- @env["rack.request.query_hash"] = parse_query(query_string)
+ @env["rack.request.query_hash"] = p
end
end
View
2  lib/rack/utils.rb
@@ -109,6 +109,8 @@ def normalize_params(params, name, v = nil)
if after == ""
params[k] = v
+ elsif after == "["
+ params[name] = v
elsif after == "[]"
params[k] ||= []
raise TypeError, "expected Array (got #{params[k].class.name}) for param `#{k}'" unless params[k].is_a?(Array)
View
7 test/spec_request.rb
@@ -1098,6 +1098,13 @@ def params
req2.params.should.equal "foo" => "bar"
end
+ should "raise TypeError every time if request parameters are broken" do
+ broken_query = Rack::MockRequest.env_for("/?foo[]=0&foo[bar]=1")
+ req = Rack::Request.new(broken_query)
+ lambda{req.GET}.should.raise(TypeError)
+ lambda{req.params}.should.raise(TypeError)
+ end
+
(0x20...0x7E).collect { |a|
b = a.chr
c = CGI.escape(b)
View
10 test/spec_utils.rb
@@ -157,6 +157,16 @@ def kcodeu
should.equal "foo" => [""]
Rack::Utils.parse_nested_query("foo[]=bar").
should.equal "foo" => ["bar"]
+ Rack::Utils.parse_nested_query("foo[]=bar&foo").
+ should.equal "foo" => nil
+ Rack::Utils.parse_nested_query("foo[]=bar&foo[").
+ should.equal "foo" => ["bar"], "foo[" => nil
+ Rack::Utils.parse_nested_query("foo[]=bar&foo[=baz").
+ should.equal "foo" => ["bar"], "foo[" => "baz"
+ Rack::Utils.parse_nested_query("foo[]=bar&foo[]").
+ should.equal "foo" => ["bar", nil]
+ Rack::Utils.parse_nested_query("foo[]=bar&foo[]=").
+ should.equal "foo" => ["bar", ""]
Rack::Utils.parse_nested_query("foo[]=1&foo[]=2").
should.equal "foo" => ["1", "2"]
Something went wrong with that request. Please try again.