HAML+Rails can lead to doubly escaped attributes

[gurgeous]

This behavior was introduced with #1028, which is related to CVE-2016-6316. Here is a test case:

# in rails console
Haml::Engine.new("%p{title: html_escape('hi &')}").render
=> "<p title='hi &amp;amp;'></p>\n"

In our Rails app it's common to write code like so:

%meta{ property: "og:title", content: content_for(:og_title) || "App Name" }

This code used to work great, because HAML helpfully determined whether or not the value is html_safe and escaped as necessary. The new behavior escapes regardless, resulting in double escaping. Potential fixes:

1. Fix HAML to only escape quotes in this case (this is what Rails does). See rails/rails@v4.2.7...v4.2.7.1
2. Encourage Rails users to use Haml::Template.options[:escape_attrs] = :once. (is this correct?)
3. Encourage Rails users to manually unescape HTML before passing to HAML. This seems dangerous.
4. Encourage Rails users not to co-mingle safe/unsafe attribute values. Difficult to enforce or detect.

Do I have that correct? I've spent a few hours investigating but I could easily have missed something important.

[k0kubun]

Here is a test case

For that code, there should be no problem to write %p{title: 'hi &'}, so let me talk about your real use case:

In our Rails app it's common to write code like so:

Could you also show the code that sets the value returned from content_for(:og_title)? Why is it already escaped?

Encourage Rails users to use Haml::Template.options[:escape_attrs] = :once. (is this correct?)

Seems like this is what you want. Because I haven't seen a case where it's needed and the underlying escape method used by true is way faster than one for :once (ruby/ruby#1164 ruby/ruby#2226), I'm reluctant to encourage every Rails user to use it. However, maybe we could improve the documentation for use cases like yours.

[gurgeous]

Thanks Takashi. content_for always escapes, and we use it inside our templates to supply content to the layout. og:title is one example where attribute escaping is causing problems, but we have several others. I think this is the ActionView code that illustrates the behavior of content_for:

https://github.com/rails/rails/blob/main/actionview/lib/action_view/helpers/capture_helper.rb
https://github.com/rails/rails/blob/main/actionview/lib/action_view/flows.rb

I wonder if anyone else is encountering this issue. content_for is a common pattern, and the behavior is subtle. Escaping issues are difficult to diagnose. Maybe it would be better to only escape quotes the way that Rails does.

[k0kubun]

content_for always escapes, and we use it inside our templates to supply content to the layout. og:title is one example where attribute escaping is causing problems, but we have several others.

Thanks.

Maybe it would be better to only escape quotes the way that Rails does.

Now I get your use case, but I don't think we should escape quotes in Haml's case. The most important question is, do you really want to doubly escape ' and " in og:title content? I think you would like to escape it only once, meaning you need to either use Haml::Template.options[:escape_attrs] = :once or unescape it if you use content_for to get og:title content.

Either way, could you try Haml::Template.options[:escape_attrs] = :once? I think it solves your problem, while escaping only quotes may not solve it perfectly.

[gurgeous]

We are experimenting with :once and it fixes the issue. Should we include a suggestion in the HAML docs somewhere? This content_for approach is probably pretty common.

Ideally HAML wouldn't html_escape attribute values if they were already marked safe. I think the CVE was intended to address things like sanitize or manually calling html_safe. The Rails fix uses value.gsub(/"/, '"'), which avoids the XSS issue without accidentally double escaping.

Regardless, I'm quite happy with the fix :) Totally up to you if you want to take further action. Thank you for your help and your work on HAML!

[k0kubun]

👍 Updated the document 77ccce6.