Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Convert lack of value to an empty string, not nil. #398

Closed
wants to merge 2 commits into from

6 participants

@homakov

Pros:
first - if we send "?admin" query then we are able to make conditions like this:

if params[:admin]
 ...
end

w/ nil we needed to check existance of key using has_key? and that is not nice.

second - we fix some CVE, known and possibly unknown.

Cons:
I don't see anything bad. Seems @tenderlove @steveklabnik don't see either.

@homakov

also slight fix
next unless k || v
weird string, it will never happen. next if p.empty? above will skip iteration if string is empty but if string is not empty k will have some value thus that line is pointless

@rkh
Owner
rkh commented

It would be really cool if you could provide tests with patches. Eases the process a lot.

@homakov

@rkh sure but why to add tests if patch is not accepted yet?
I ask here about acceptance of the change and if you are ready to merge I provide anything.

@rkh
Owner
rkh commented

Right, no, that's two different discussions, really. I see you wanna start a discussion first. A similar question would be: Why write an initial patch if the feature wasn't accepted yet.

@rkh rkh closed this
@rkh rkh reopened this
@homakov

@rkh haha yes :) Sorry, if you haven't noticed, but discussion was kinda started https://twitter.com/steveklabnik/status/208993016134897664 https://twitter.com/tenderlove/status/208991607570182144
Writing the patch is easier than describing how it should work here, this is why. Could you describe why you are against of it?

@rkh
Owner
rkh commented

Right. You have a link to the CVE?

@rkh
Owner
rkh commented

Thanks.

@chneukirchen
Owner

I disagree. IMO, ?admin should just be accessible with QUERY_STRING, and not result in parameters at all.

But given how it is now, we should be able to differ "foo", "foo=" and "foo=&foo=".

@homakov

@chneukirchen removing from parameters at all definitely not a way to go
why should we differ foo and foo= - we never need nil values and empty string is more comfortable default for developer. GET params are always must be k=v. just "k" is a bug in params and IMO to handle it we should turn it into "k="
also we never differ "foo=&foo=" do we?

@chneukirchen
Owner

There is a big semantic difference between ?foo (which is from an ISINDEX document) and ?foo= (which is an empty field in a form). And we differ on foo=&foo= which is ["", ""].

@steveklabnik

(which is an empty field in a form)

This is not necessarily true. Neither is your assertion about what ?foo means. It is technically free-form, though key=value is often used to indicate key/value pairs. See RFC 3986 Sec 3.4 for more.

@chneukirchen
Owner
@steveklabnik

Do you know of any case where FORMs are sent without =?

I meant the opposite; just because it's a key value doesn't mean that it was generated by a form.

@chneukirchen
Owner

It doesn't have to come from a form, but if you possibly send foo= bar, you'll never send just foo, you send foo=. Thus they should be different.

@steveklabnik

You don't know that.

@chneukirchen
Owner

You also don't know that foo= is supposed to mean the same as foo.

@homakov

@chneukirchen leaving it as is is not good, you was agreed in first post, right?
two options:
1. do not include keys w/o value, ?foo will not create params[:foo] at all.
2. create something different from nil, obviously empty string.
if both don't suit let's close it?

@chneukirchen
Owner

I think it should have been different in first place, but now we got to keep it as it is. Note its the same as in cgi.rb:

% irb -r cgi

CGI.parse("foo")
=> {"foo"=>[]}
CGI.parse("foo=")
=> {"foo"=>[""]}

@travisbot

This pull request passes (merged 1571308 into edc8b92).

@raggi
Owner

I'll review this again ASAP. I'm concerned that we've gone back and forward over this code for security reasons plenty of times in the past, and the solution doesn't seem to be "interpret the parameters differently", the solution is "write your application code safely".

The last change in this area was made to ensure that parameters round-trip correctly through our existing encoders/decoders. At minimum this commit breaks that.

Whilst I'm considering that we should move away from CGI as a baseline for Rack 2, I'm not so sure that it's the right thing to do today, with Rack 1.x. CGI compliments HTTP (and other RFCs) to provide a more precise set of specifications, although still far from perfect (and far from up to date), the IDB in HTTP (and other RFCs) make all of this very complicated for the wider expanse of users we have.

Please try to remember that our users are not just using Rails/Sinatra on Unicorn/Puma/Mongrel/Thin.

@homakov

@raggi

"interpret the parameters differently", the solution is "write your application code safely".

telling "write code safely" sounds funny to me. is it your only advice? :)

At minimum this commit breaks that

Really "breaks" or just requires some fixes? :)

@raggi
Owner

References:

6127c81

5c00dd6

c9b4159

There are a few more, many of which were introduced / changed by Josh in 2009.

My largest concern with this is code the presently relies upon having a nil value in these cases. In the rails world, most people will be using blank? or present?, but in the rest of the ruby world, many folks will be using plain conditionals. Due to this I'm hesitant to introduce this any earlier than 1.5, if at all. It's also possible that adding another parse method with these kinds of semantics could be more safe, and Rails could migrate to using that instead.

@homakov

BTW I can close this (marked with milestone?)
Problem was a security concern about find_by_*(nil). But eventually we found a way to send nil with JSON and XML too. Seems I have changed my mind and started to think ''x&y' are both nils. :) Up to you

@raggi raggi closed this
@homakov homakov deleted the homakov:patch-1 branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 4, 2012
  1. @homakov
Commits on Jun 5, 2012
  1. @homakov

    Update patch-1

    homakov authored
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 4 deletions.
  1. +3 −1 lib/rack/utils.rb
  2. +3 −3 test/spec_request.rb
View
4 lib/rack/utils.rb
@@ -69,7 +69,8 @@ def parse_query(qs, d = nil)
(qs || '').split(d ? /[#{d}] */n : DEFAULT_SEP).each do |p|
next if p.empty?
k, v = p.split('=', 2).map { |x| unescape(x) }
- next unless k || v
+ v = '' if v.nil?
+ next unless k
if cur = params[k]
if cur.class == Array
@@ -102,6 +103,7 @@ def parse_nested_query(qs, d = nil)
def normalize_params(params, name, v = nil)
name =~ %r(\A[\[\]]*([^\[\]]+)\]*)
k = $1 || ''
+ v = '' if v.nil?
after = $' || ''
return if k.empty?
View
6 test/spec_request.rb
@@ -118,11 +118,11 @@
end
should "parse the query string" do
- req = Rack::Request.new(Rack::MockRequest.env_for("/?foo=bar&quux=bla"))
+ req = Rack::Request.new(Rack::MockRequest.env_for("/?foo=bar&quux=bla&empty"))
req.query_string.should.equal "foo=bar&quux=bla"
- req.GET.should.equal "foo" => "bar", "quux" => "bla"
+ req.GET.should.equal "foo" => "bar", "quux" => "bla", "empty" => ""
req.POST.should.be.empty
- req.params.should.equal "foo" => "bar", "quux" => "bla"
+ req.params.should.equal "foo" => "bar", "quux" => "bla", "empty" => ""
end
should "limit the keys from the GET query string" do
Something went wrong with that request. Please try again.