escape_attrs wraps incorrectly?

[faelsoto]

Regarding the same idea of #984, we're migrating an app to newer Ruby/Rails versions, and it seems that the 4.x behavior isn't complete with just escape_interpolated_html, as the attributes are being a little weird.

On 4.x:

Haml::Engine.new(%{ %a{ :href => '/', :'@click' => "callback('a')" } link }.strip).render
=> <a @click="callback('a')" href='/'>link</a>

On 5.1.2:

Haml::Engine.new(%{ %a{ :href => '/', :'@click' => "callback('a')" } link }.strip).render
=> <a @click='callback(&#39;a&#39;)' href='/'>link</a>

This change breaks the inline templates for libraries like Vue. Digging in the code, I find escape_attrs, which doesn't help on either version, both outputting:

Haml::Engine.new(%{ %a{ :href => '/', :'@click' => "callback('a')" } link }.strip, :escape_attrs => false ).render
=> <a @click='callback('a')' href='/'>link</a>

Is this the expected output? I'm not seeing a case in which I would like to break out of the attribute value, I think this is a security issue.

I found that the attr_wrapper option can be assigned to a symbol that will wrap the attribute (single quotes by default), but this would break whenever the attribute value has the same value.

My proposal is to call inspect for the attribute value:

"callback('a', \"b\")".inspect
=> "callback('a', \"b\")"

Does that make sense? Should I start working on a PR or is it part of the spec or wontfix?

[faelsoto]

First approach in #1020, I think the wrapper should be via inspect in order to escape everything safely, regardless if the attribute will be converted to html entities or not, but for this PR it's only whenever escape_attrs is disabled.

However, if implemented globally, we could avoid concatenating the attr_wrapper value.

[faelsoto]

Even with the patch, I noticed that some attributes were not escaped correctly.

This happens with html_safe strings, which take another different path from the simple string values. I found the function in the attribute_compiler:

def compile_common_attribute(key, values)
  var = unique_name
  [:multi,
   [:code, "#{var} = (#{merged_value(key, values)})"],
   [:case, var,
    ['Hash', runtime_build([AttributeValue.new(:dynamic, key, var)])],
    ['true', true_value(key)],
    ['false, nil', [:multi]],
    [:else, [:multi,
             [:static, " #{key}=#{@attr_wrapper}"],
             [:escape, @escape_attrs, [:dynamic, var]],
             [:static, @attr_wrapper]],
    ]
   ],
  ]
end

This is where the concatenation happens, however it's just preparing it to be passed to Temple, I think, and this is where I got stuck. The issue is getting bigger, it seems.

[k0kubun]

Your use case sounds legit. If you don't mind, I'd be interested to see a minimum project which reproduces the issue using both Haml and Vue so that we can confirm it's fixed while thinking about some options.

Let me ask a few questions about your proposal:

My proposal is to call inspect for the attribute value
"callback('a', "b")".inspect
=> "callback('a', "b")"

• Can we generate 'callback(\'a\', "b")' when attr_wrapper is not " but '?
  • I know Haml 4 used to generate " despite ' attr_wrapper in some conditions, but we intentionally made the semantics simpler in Haml 5 for performance reasons.
• What about callback('&', "><")? Are you okay with "callback('&', \"><\")"?

[faelsoto]

Hey @k0kubun, here's the minimum project you requested, you can clearly see both issues with and without html_safe being called: https://github.com/faelsoto/hamltest (Just install and head to the root)

Regarding both questions, I think both are great since they don't break the HTML.

[k0kubun]

here's the minimum project you requested, you can clearly see both issues with and without html_safe being called: https://github.com/faelsoto/hamltest (Just install and head to the root)

Great, thank you for your help!

In this app, when I click the first click('1', "2", 3) link (rendered as <a @click.prevent='click(&#39;1&#39;, &quot;2&quot;, 3)' href='/'> like your description), it pops up an alert saying "1". So I'm wondering:

• Is it (showing an alert "1" on click) the expected behavior? If so, click('1', "2", 3) escaped as click('1', "2", 3) isn't causing the trouble with Vue, is it?
• Obviously the second link (<a @click.prevent='click('1', "2", 3)' href='/'>) is broken, and the issue is about attribute with safe-buffer containing a apostrophe is not escaped #993. I can prepare a fix of it, but if it's the main problem, this issue is duplicated.

[faelsoto]

While it's not causing an issue on the first link, the html entities might not be what it's expected, and if you ask me, there should be a way to turn them off because if this attribute has a JSON-encoded string, it will be much larger than it needs to be. However, it doesn't cause any issues because the JS parser is very lenient, I guess.

But yeah, the second link is where the issue is obviously seen.

[k0kubun]

OK, so in your hamltest app, we see two different issues:

1. Currently escape_attrs supports only HTML escape and there's no way to escape only a character of attr_wrapper by \. While HTML-escaping both ' and " (escape_attrs: true) doesn't break a Vue application, HTML-escaping an attribute increases the content size and makes code weird.
2. SafeBuffer isn't HTML-escaped and it breaks Vue application.

1 seems the main thing reported by this issue, so I'm not gonna close this issue as duplicated. Because escape_attrs: false is a mode which doesn't perform no escape and escape_attrs: true is one which performs HTML escape, the feature should probably be a new mode like escape_attrs: :wrapper_only (a non-true/false mode like escape_attrs: :once) for backward compatibility.

Do you want both 1 and 2 to be fixed, or is fixing 2 more important than 1 for the time being?

[faelsoto]

Oh, I totally forgot about escape_attrs. To be fair, the HTML entities is a minor issue (a JSON object being bigger than required). The real issue is #2, which totally breaks the HTML.