rails 3.0.4 broke yield :javascript ?

[Joaquin Rivera Padron]

hello, 

I have today updated my rails app to 3.0.4 security release but now this 

yield :javascripts

fails in the layout and I get my custom js escaped as text in the view.

anybody seeing this also?

tia,

jk

--
www.least-significant-bit.com

[Brian Morearty]

Yes, I saw something similar when I upgraded to 3.0.4 this morning. I
didn't have a chance to debug it so for the moment I went back to
3.0.1. I wasn't sure if it was my doing so I didn't say anything on
this list.

I have a helper function that returns an HTML string. The function
calls .html_safe before returning. That worked in 3.0.1 but in 3.0.4
it is being escaped in the output.

I also tried adding .html_safe to the .html.erb file (double-safe it)
but to no avail.

I was not able to reproduce it in a simple case though, even in very
same function.

Brian

[Joaquin Rivera Padron]

for me are broken also versions 3.0.4, 3.0.4.rc1, 3.0.3 and 3.0.2

ok is 3.0.1, will keep digging then

jk

2011/2/9 Brian Morearty <bmor...@gmail.com>

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To post to this group, send email to rubyonra...@googlegroups.com.
To unsubscribe from this group, send email to rubyonrails-co...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.

--
www.least-significant-bit.com

[Santiago Pastorino]

Great, ping me if I can help you.
BTW did you tried 3-0-stable?

[Joaquin Rivera Padron]

yes, if by 3-0-stable you mean 3.0.0, yes it works

thanks for the "ping offer", I'll let you know if anything, but I won't (can't) be full time chasing the bug :-(

jk

2011/2/10 Santiago Pastorino <sant...@wyeworks.com>

--
www.least-significant-bit.com

[Joaquin Rivera Padron]

hi, 

I diff-ed 3.0.0 with 3.0.1 and I got this

diff --git a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb

index 142cd08..fb2118a 100644

--- a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb

+++ b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb

@@ -17,7 +17,7 @@ module ActionDispatch

     #

...skipping...

         buffer = with_output_buffer { value = yield(*args) }

         if string = buffer.presence || value and string.is_a?(String)

-          NonConcattingString.new(string)

+          NonConcattingString.new(ERB::Util.html_escape(string))

         end

       end

if I put bac k the NonConcattingString.new(string) it works (at least for me)

don't know the implications though, wdyt?

jk

2011/2/10 Joaquin Rivera Padron <joah...@gmail.com>

--
www.least-significant-bit.com

[Joaquin Rivera Padron]

commit 2c8bff3513b17a8ad55595a61601bfba14ad40bf

Author: Santiago Pastorino <sant...@wyeworks.com>

Date:   Tue Nov 2 20:18:22 2010 -0200

    Call as ERB::Util.html_escape since is not the module is not included here

--
www.least-significant-bit.com

[Joaquin Rivera Padron]

2011/2/10 Joaquin Rivera Padron <joah...@gmail.com>

hi, 

I diff-ed 3.0.0 with 3.0.1 and I got this

sorry I meant diff-ed 3.0.1 to 3.0.2

--
www.least-significant-bit.com

[Joaquin Rivera Padron]

I'll try to run the tests in 3.0.2 with that change to see if (what) breaks

--
www.least-significant-bit.com

[Santiago Pastorino]

This commit https://github.com/rails/rails/commit/2c8bff3513b17a8ad55595a61601bfba14ad40bf
just changes from html_escape string to ERB::Util.html_escape(string)
so both are calling the same method.

You're talking about this one
https://github.com/rails/rails/commit/bb9c58eb4aa637fa75c69c705a9918d6322ff834
and this fix a security issue. I'd say that you're missing a html_safe
some where.

On Thu, Feb 10, 2011 at 10:54 AM, Joaquin Rivera Padron

[Joaquin Rivera Padron]

I'll check, thanks for the reply

[Joaquin Rivera Padron]

you are totally right, I was missing a html_safe :-)

this is wrong as now:

    content_for :js do

<<JS

<script type="text/javascript">

        alert('hello');

      </script>

JS

    end

this is ok:

    js = <<JS

<script type="text/javascript">

        alert('hello');

      </script>

JS

    content_for :js do

      js.html_safe

    end

thanks a lot

--
www.least-significant-bit.com

[Brian Morearty]

The change that Santiago mentioned (https://github.com/rails/rails/
commit/bb9c58eb4aa637fa75c69c705a9918d6322ff834) also causes
content_tag always to escape its output, even when the 'escape'
parameter is set to false. However, from my experiments it seems the
'escape' parameter wasn't working before the change either. Instead of
always escaping the output, content_tag was never escaping the output.

In a 3.0.1 Rails project the output is never escaped but html_safe?
always returns true:

rails console
Loading development environment (Rails 3.0.1)
ruby-1.8.7-p330 :001 > helper.content_tag :div do '<b>hello</b>'
end
=> "<div><b>hello</b></div>"
ruby-1.8.7-p330 :002 > (helper.content_tag :div do '<b>hello</b>'
end).html_safe?
=> true
ruby-1.8.7-p330 :003 > helper.content_tag :div,nil,nil,true do
'<b>hello</b>' end
=> "<div><b>hello</b></div>"
ruby-1.8.7-p330 :004 > (helper.content_tag :div,nil,nil,true do
'<b>hello</b>' end).html_safe?
=> true
ruby-1.8.7-p330 :005 > helper.content_tag :div,nil,nil,false do
'<b>hello</b>' end
=> "<div><b>hello</b></div>"
ruby-1.8.7-p330 :006 > (helper.content_tag :div,nil,nil,false do
'<b>hello</b>' end).html_safe?
=> true

In a Rails 3.0.2 project the content is always escaped and html_safe?
always returns true:

rails console
Loading development environment (Rails 3.0.2)
ruby-1.8.7-p330 :001 > helper.content_tag :div do '<b>hello</b>' end
=> "<div>&lt;b&gt;hello&lt;/b&gt;</div>"
ruby-1.8.7-p330 :002 > (helper.content_tag :div do '<b>hello</b>'
end).html_safe?
=> true
ruby-1.8.7-p330 :003 > helper.content_tag :div,nil,nil,true do
'<b>hello</b>' end
=> "<div>&lt;b&gt;hello&lt;/b&gt;</div>"
ruby-1.8.7-p330 :004 > (helper.content_tag :div,nil,nil,true do
'<b>hello</b>' end).html_safe?
=> true
ruby-1.8.7-p330 :005 > helper.content_tag :div,nil,nil,false do
'<b>hello</b>' end
=> "<div>&lt;b&gt;hello&lt;/b&gt;</div>"
ruby-1.8.7-p330 :006 > (helper.content_tag :div,nil,nil,false do
'<b>hello</b>' end).html_safe?
=> true

So at least in Rails 3.0.2 the html_safe? function is reporting the
truth. But content_tag escapes the output even when escape=false (see
the last two lines).

I would submit a patch but I'm not sure what the right fix is.

Brian Morearty

On Feb 10, 5:36 am, Joaquin Rivera Padron <joahk...@gmail.com> wrote:
> you are totally right, I was missing a html_safe :-)
>
> this is wrong as now:
>
>     content_for :js do
> <<JS
> <script type="text/javascript">
>         alert('hello');
>       </script>
> JS
>     end
>
> this is ok:
>
>     js = <<JS
> <script type="text/javascript">
>         alert('hello');
>       </script>
> JS
>
>     content_for :js do
>       js.html_safe
>     end
>
> thanks a lot
> jk
>

> 2011/2/10 Joaquin Rivera Padron <joahk...@gmail.com>

>
>
>
> > I'll check, thanks for the reply
>
> > jk
>

> > 2011/2/10 Santiago Pastorino <santi...@wyeworks.com>
>
> >> This commit
> >>https://github.com/rails/rails/commit/2c8bff3513b17a8ad55595a61601bfb...

> >> just changes from html_escape string to ERB::Util.html_escape(string)
> >> so both are calling the same method.
>
> >> You're talking about this one
>

> >>https://github.com/rails/rails/commit/bb9c58eb4aa637fa75c69c705a9918d...

> >> and this fix a security issue. I'd say that you're missing a html_safe
> >> some where.
>
> >> On Thu, Feb 10, 2011 at 10:54 AM, Joaquin Rivera Padron
> >> <joahk...@gmail.com> wrote:
>
> >> > commit 2c8bff3513b17a8ad55595a61601bfba14ad40bf

> >> > Author: Santiago Pastorino <santi...@wyeworks.com>

> >> > Date:   Tue Nov 2 20:18:22 2010 -0200
> >> >     Call as ERB::Util.html_escape since is not the module is not
> >> included
> >> > here
>

> >> > 2011/2/10 Joaquin Rivera Padron <joahk...@gmail.com>

>
> >> >> hi,
> >> >> I diff-ed 3.0.0 with 3.0.1 and I got this
> >> >> diff --git
> >> a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
> >> >> b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
> >> >> index 142cd08..fb2118a 100644
> >> >> --- a/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
> >> >> +++ b/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb
> >> >> @@ -17,7 +17,7 @@ module ActionDispatch
> >> >>      #
> >> >> ...skipping...
> >> >>          buffer = with_output_buffer { value = yield(*args) }
> >> >>          if string = buffer.presence || value and string.is_a?(String)
> >> >> -          NonConcattingString.new(string)
> >> >> +          NonConcattingString.new(ERB::Util.html_escape(string))
> >> >>          end
> >> >>        end
> >> >> if I put bac k the NonConcattingString.new(string) it works (at least
> >> for
> >> >> me)
> >> >> don't know the implications though, wdyt?
> >> >> jk

> >> >> 2011/2/10 Joaquin Rivera Padron <joahk...@gmail.com>

>
> >> >>> yes, if by 3-0-stable you mean 3.0.0, yes it works
> >> >>> thanks for the "ping offer", I'll let you know if anything, but I
> >> won't
> >> >>> (can't) be full time chasing the bug :-(
> >> >>> jk
>

> >> >>> 2011/2/10 Santiago Pastorino <santi...@wyeworks.com>

>
> >> >>>> Great, ping me if I can help you.
> >> >>>> BTW did you tried 3-0-stable?
>
> >> >>>> On Thu, Feb 10, 2011 at 9:51 AM, Joaquin Rivera Padron
> >> >>>> <joahk...@gmail.com> wrote:
> >> >>>> > for me are broken also versions 3.0.4, 3.0.4.rc1, 3.0.3 and 3.0.2
> >> >>>> > ok is 3.0.1, will keep digging then
> >> >>>> > jk
>

> >> >>>> > 2011/2/9 Brian Morearty <bmorea...@gmail.com>

[Santiago Pastorino]

Hey Brian,

I think we should remove the escape parameter at least here
https://github.com/rails/rails/blob/e8c870726a67a27965b2a5333a5ecf450d4f458f/actionpack/lib/action_view/helpers/form_tag_helper.rb#L303.
I haven't checked it in other places but I that guess could be removed
too, so if you want to go ahead do it and deprecate it for 3-0-stable.
We should use html_safe when you need to not escape.

Best,
Santiago.

[Brian Morearty]

Thanks, Santiago. I will submit a patch to deprecate the escape param/
option.

I've created a ticket to track it:

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6421-deprecate-escape-parameteroption-in-three-helper-functions

Brian

On Feb 12, 7:43 am, Santiago Pastorino <santi...@wyeworks.com> wrote:
> Hey Brian,
>

>   I think we should remove the escape parameter at least herehttps://github.com/rails/rails/blob/e8c870726a67a27965b2a5333a5ecf450....

> I haven't checked it in other places but I that guess could be removed
> too, so if you want to go ahead do it and deprecate it for 3-0-stable.
> We should use html_safe when you need to not escape.
>
> Best,
> Santiago.
>

> ...
>
> read more »

[Brian Morearty]

The patch is done, tested by me, and ready to be vetted by others. If
any of you are interested in looking at it, here it is:

https://rails.lighthouseapp.com/projects/8994/tickets/6421-deprecate-escape-parameteroption-in-three-helper-functions#ticket-6421-5

The 3 helper functions mentioned above (FormTagHelper#text_area_tag,
TagHelper#content_tag, and TagHelper#tag ) will have the following
behavior after these patches are applied:

* In 3.0.5, using the 'escape' parameter or option will show a
deprecation message.
* In 3.1 the 'escape' parameter or option to these 3 methods is
removed. The correct way to specify escaping behavior is to call (or
not call) html_safe on the parameters. Both the rdoc and the tests are
updated.

Please respond if anything looks amiss or if it all looks good.

Brian

On Feb 12, 9:30 am, Brian Morearty <bmorea...@gmail.com> wrote:
> Thanks, Santiago. I will submit a patch to deprecate the escape param/
> option.
>
> I've created a ticket to track it:
>

> https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6...

> ...
>
> read more »

[Brian Morearty]

whoops:

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6421-deprecate-escape-parameteroption-in-three-helper-functions#ticket-6421-5

On Feb 12, 9:30 am, Brian Morearty <bmorea...@gmail.com> wrote:

> Thanks, Santiago. I will submit a patch to deprecate the escape param/
> option.
>
> I've created a ticket to track it:
>

> https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6...

> ...
>
> read more »