Rails 3 no longer URI escaping [ and ]

rubyonrails-core
1

Is there a reason Rails 3 no longer URI escapes [ and ]? Example:

# Rails 2.3.5 $ script/console Loading development environment (Rails 2.3.5)

app.url_for(:controller=>'sequel', 'a[b]'=>1)

=> "http://www.example.com/sequel?a[b]=1"

app.url_for(:controller=>'sequel', Rack::Utils.escape('a[b]')=>1)

=> "http://www.example.com/sequel?a%255Bb%255D=1"

# Rails 3.0.0 $ rails console Loading development environment (Rails 3.0.0) irb(main):001:0> app.url_for(:controller=>'sequel', 'a[b]'=>1) => "http://www.example.com/sequel?a[b]=1" irb(main):002:0> app.url_for(:controller=>'sequel', Rack::Utils.escape('a[b]')=>1) => "http://www.example.com/sequel?a%255Bb%255D=1"

I'm not sure it this is intentional or if this is a bug. RFC 3986 (http://tools.ietf.org/rfc/rfc3986.txt) implies that [ and ] are reserved characters in the query part of URIs.

   reserved = gen-delims / sub-delims    gen-delims = ":" / "/" / "?" / "#" / "[" / "]" / "@"

In Section 2.2 it says:

   URI producing applications should percent-encode data octets that    correspond to characters in the reserved set unless these characters    are specifically allowed by the URI scheme to represent data in that    component.

The HTTP RFC (RFC 2616) references the older URI RFC (RFC 2396: http://www.ietf.org/rfc/rfc2396.txt), which doesn't state that [ and ] are reserved characters in the query. But the HTTP RFC doesn't specifically allow them either. As it predates RFC 3986, I'm not sure what is considered the best practice.

Obviously Rails is escaping some characters and not others, and it isn't using Rack::Utils.escape to do the escaping.

Jeremy

2

Obviously Rails is escaping some characters and not others, and it isn't using Rack::Utils.escape to do the escaping.

I don't believe this was done deliberately. A quick glance over the git history didn't point anything obvious out. Do you want to take a go at rationalising the escaping code a bit?

3

I already filed this "bug" around two weeks ago at lighthouse: https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/5690-should-be-escaped-in-urls

I must admit that my suggested fix (use CGI.escape) does not really work as it escaped the slah (/) too.

Corin

4

After some digging, I can say it was done delibritely: http://github.com/rails/rails/commit/856d2fd874d72dd9f83204affff4edfef3308361

Prehaps josh can enlighten us as to the reason why the change was made, since the commit message does not do so.

Reversing that commit and http://github.com/rails/rails/commit/1ee9b40b18a0bed5bb10a0785f7e2730bac983f6 would probably fix things, if you decide to go that route.

Jeremy

5

After some digging, I can say it was done delibritely: Quick fix for not escaping []s (not ideal) · rails/rails@856d2fd · GitHub

Prehaps josh can enlighten us as to the reason why the change was made, since the commit message does not do so.

Nor does the message here:

Those commits don't really have any justification, so unless josh pipes up I'd be tempted to reverse them.

However to play devil's advocate, the spec isn't exactly clear that they *should* be escaped, and nothing seems to break with them *not* being escaped. What makes you want them that way? Is it breaking your app some how?

6

After some digging, I can say it was done delibritely: Quick fix for not escaping []s (not ideal) · rails/rails@856d2fd · GitHub

Prehaps josh can enlighten us as to the reason why the change was made, since the commit message does not do so.

Nor does the message here: Stop escaping "[]" in query string · rails/rails@4dee277 · GitHub

Those commits don't really have any justification, so unless josh pipes up I'd be tempted to reverse them.

However to play devil's advocate, the spec isn't exactly clear that they *should* be escaped, and nothing seems to break with them *not* being escaped. What makes you want them that way?

As RFC 3986 is more recent and implies they should be escaped, and rack and cgi both escape them, that seems like the more logical choice. The current code specifically reverses the escaping done by cgi, so it's actually doing more work. I can't see a benefit for escaping the URI with rack and then unescaping just the brackets with a gsub, other than the unescaped URI looks nicer. If a nicer look is more important, why unescape just the brackets, why not unescape other characters where it wouldn't break things?

Is it breaking your app some how?

It breaks some test code, that's all. Test code that works on 5 other web frameworks and previous versions of Rails.

It's not a big deal either way, but I wanted to know rails-core opinion before I decide whether to update my test code.

Thanks, Jeremy

7

Is it breaking your app some how?

It breaks some test code, that's all. Test code that works on 5 other web frameworks and previous versions of Rails.

It's not a big deal either way, but I wanted to know rails-core opinion before I decide whether to update my test code.

I'll see if I can get something a bit more definitive from josh as to why this was changed. Seems a toss up either way, but the current method is certainly a bit messy and at the very least we shouldn't be escaping / unescaping

8

It's been a month since this was last discussed, and there doesn't appear to have been any related changes made. Should I add a issue to lighthouse so that it's easier to keep track of this?

Jeremy

9

Fixed here https://github.com/rails/rails/commit/52b71c01fd3c8a87152f55129a8cb3234190734a Thanks you Jeremy ;).