bug in recently fixed Array.to_query?

rubyonrails-core
1

Hello,

I posted a message about this previously on the prototype core board (http://groups.google.com/group/prototype-core/browse_thread/thread/107f1dafddb773d7), but I'm now thinking the issues lies in Rails.

This issue came about with recent fixes to Array.to_query. One of the related tickets is http://dev.rubyonrails.org/changeset/6343.

I think the issue lies with a call to CGI::escape somewhere within rails. Basically the issue is the following...

<%= url_for(:controller => 'test', :action => 'auto_complete_for_test', :field => %w(a b c)) %> generates... '/test/auto_complete_for_test?field%5B%5D=a&amp;field%5B%5D=b&amp;field%5B%5D=c'

The &amp;'s get substituted to & in the html normally producing the correct url for rails to parse. This is fine for urls generated within "normal" html, but when used within a <script> tag, the &amp; doesn't get substituted back into a single &. So for urls generated within a script tag like those with auto_complete_field for example, the url is incorrect.

This is how rails receives the data when the url resides in a script tag (note the "field" and "amp;field" params)...

Processing TestController#auto_complete_for_test (for 127.0.0.1 at 2007-10-14 18:21:22) [POST]   Parameters: {"action"=>"auto_complete_for_test", "field"=>["a"], "controller"=>"test", "amp;field"=>["b", "c"], "test"=>"test"}

I would have expected an issue like this to have popped up before (and maybe it has and been fixed), but it's occuring in the latest 1.2.5 release at least now.

So to summarize...

no issues with this alone... url_for(:controller => 'test', :action => 'auto_complete_for_test', :field => %w(a b c))

but this causes a problem... <%= javascript_tag url_for(:controller => 'test', :action => 'auto_complete_for_test', :field => %w(a b c)) %>

Is this an issue that has come up before?

Thanks, Andrew

2

In response to my own question, further testing has shown that this problem appears at least as far back as 1.2.2.(likely further) with any parameters used in a script tag.

As this example shows, it's not specific to arrays, it's just applies in general to a url with more than one parameter.

A url like this in a script tag... url_for(:action => 'auto_complete_for_test', :test1 => 'a', :test2 => 'b')

Is received by rails like so... Parameters: {"amp;test2"=>"b", "action"=>"auto_complete_for_test", "test1"=>"a"}

I searched for a ticket on this and couldn't find anything. Am I simply doing something dumb here, which is why I'm not seeing much on this in Trac?

Thanks, Andrew

3

I searched for a ticket on this and couldn't find anything. Am I simply doing something dumb here, which is why I'm not seeing much on this in Trac?

I'm pretty sure this has come up before and I'm not sure we ever got a 'real' resolution.

It certainly seems messed up that we're generating URLs and escaping & in url_for rather than in link_to, form_for etc.

4

I was just responding to my own post....

I see that simply passing :escape => false to any urls used within script tags will solve the problem.

The only thing I see now that comes out of this I suppose is that by default, methods like auto_complete_field should perhaps use :escape => false as well as any other methods that are wrapped in a javascript_tag call and deal with urls. Perhaps the documentation for url_for needs to stress :escape => false usage for urls generated in script tags.

I'll try to create a patch shortly for the docs and at least the auto_complete_field.

Sound acceptable?

5

I see that simply passing :escape => false to any urls used within script tags will solve the problem.

But, why do we even have that option? Couldn't we just not escape, then do that in link_to, url_for etc.?

Sound acceptable?

Only if we really need escaping in url_for. This will mess up things like urls in mailers, etc. etc.

6

Aren't the urls escaped to produce valid HTML? A & that isn't escaped in a url is not valid. But this escaping causes problems in script tags, and mailers like you mention.

Basically it looks like a case of most urls occuring within normal HTML, so escape it, otherwise manual override is necessary for the other cases.

Couldn't url_for be overridden with in various modules to prevent the escaping? Add a url_for to ActionMailer::Base, ActionView::Helpers::JavaScriptMacrosHelper and any other places where escaping should not be done... simply set :escape => false before passing the url off to the main url_for method?

Just some thoughts.

Andrew

7

Aren't the urls escaped to produce valid HTML? A & that isn't escaped in a url is not valid.

I agree, an '&' should normally be escaped as &amp; when rendering the url in html.

8

>Aren't the urls escaped to produce valid HTML? A & that isn't escaped >in a url is not valid.

I agree, an '&' should normally be escaped as &amp; when rendering the url in html.

Sure, but shouldn't *link_to* do this escaping. It's the thing that knows the URL is destined for html.

9

The top level remote methods (such as remote_form_for, link_to_remote) would also require the escaping. I'm sure there are other methods which would require it as well. This couldn't be specified in remote_function though as this method can be used in a script tag where escaping should be false, so setting :escape would have to be set higher in the call stack where remote_function is invoked.

Methods like auto_complete_field and in_place_editor would set escaping to false since it's context for the url is always within a script tag.

So to do what you propose seems doable, but it looks like it would require checking and modification to any method that wraps url_for.

The changes would still require the :escape option to exist though for the cases (like remote_function) where escaping depends on it's context.