add escape_filter_interpolations option #984

wjordan · 2018-03-01T00:24:06Z

Addresses points raised in (closed issue) #940, by allowing optional backwards compatibility to work around breaking changes introduced by #770 (released in 5.0.0).

This PR is mostly intended for migrating large/legacy applications written against the v4 API, that want to upgrade to v5 while avoiding time-intensive application refactoring (e.g., #940 (comment)).

My particular use-case is a need to upgrade haml to >= 5.0.4 for Ruby 2.5 support without refactoring large amounts of existing application code.

Introduces a new escape_interpolated_html option (defaults to the current value of escape_html for backwards compatibility with existing 5.x default behavior), which can be used to override the behavior for escaping HTML in interpolated strings separately from other html-escaping behavior.

With this option, it's possible to set Haml::Template.options[:escape_interpolated_html] = false (e.g., in a Rails initializer) for full backwards compatibility with v4 behavior, without any further changes to existing application code.

Set `escape_interpolated_html` to `false` for backwards compatibility with haml 4 defaults.

k0kubun · 2018-03-10T10:03:31Z

lib/haml/temple_engine.rb

@@ -13,6 +13,7 @@ class TempleEngine < Temple::Engine
      encoding:             nil,
      escape_attrs:         true,
      escape_html:          false,
+      escape_interpolated_html: nil,


To make sure it's a boolean flag, the default value should be false instead of nil.

Ah, okay I understood your intention. So probably you intends to mean absence of this option here. Never mind.

k0kubun · 2018-03-10T10:24:18Z

While I understand why you would need this, as it is just a migration path from 4 to 5 and there is no reason to recommend keeping the vulnerable behavior, I don't like to introduce this as official option, which we need to maintain forever.

I even think having both escape_attrs and escape_html is crazy. Why do we add the third one? Having this would be convenient for some people, but people who don't know the context would be confused by them.

My particular use-case is a need to upgrade haml to >= 5.0.4 for Ruby 2.5 support without refactoring large amounts of existing application code.

Given the situation, I would like to have Ruby 2.5 support in https://github.com/haml/haml/tree/stable. Obviously that's the best path to avoid "refactoring large amounts of existing application code", right?

Also, the bug which was fixed for Ruby 2.5 at v5.0.4 was introduced at Haml v5. In my understanding, it doesn't affect Haml v4. Thus I can't think of any reason to upgrade your Haml to 5.0.4+ only for using Ruby 2.5. For your safe Ruby upgrade, please use Haml 4 as it is. If it doesn't work with Ruby 2.5, please report it separately.

k0kubun · 2018-06-23T09:41:59Z

closing due to inactivity

bughit · 2019-01-19T01:27:13Z

there is no reason to recommend keeping the vulnerable behavior

@k0kubun

The original behavior had safe usage patterns (.to_json), so there was no need to force everyone to search and edit all their interpolations. An option should be there.

k0kubun · 2019-01-19T02:15:45Z

Clarify which filter and what Haml template you're talking about. I can't understand why you mention .to_json here at all since filter interpolation could be related to not only JavaScript filter, but also a plain text, CSS, Sass, markdown, ... whatever
Just use .html_safe or raw if you're talking about Rails. It should be the intended way of embedded dynamic script even in the default template engine Erubi (.erb).
Why not use escape_html: false?
to_json is not HTML-safe and completely vulnerable when it's interpolated to the plain text part of HTML. Do not consider only what you care about. The following string is not HTML-escaped properly.

{a: "<script>alert('hello');</script>"}.to_json #=> "{\"a\":\"<script>alert('hello');</script>\"}"

bughit · 2019-01-19T06:55:19Z

rails activesupport version of to_json escapes by default

https://github.com/rails/rails/blob/master/activesupport/lib/active_support/json/encoding.rb#L42

in a rails console your example outputs:
"{\"a\":\"\\u003cscript\\u003ealert('hello');\\u003c/script\\u003e\"}"

in the following example (with html encoding turned off) the .inspect version injects, the .to_json one doesn't.

- script_injector_string = "</script><script>alert('injected')</script>"

:javascript
  #{script_injector_string.to_json};

:javascript
  #{script_injector_string.inspect};

  <body>
    <script>
  "\u003c/script\u003e\u003cscript\u003ealert('injected')\u003c/script\u003e";
</script>
<script>
  "</script><script>alert('injected')</script>";
</script>
  </body>

Why not use escape_html: false

To avoid porting old code on upgrade.

bughit · 2019-01-19T07:01:32Z

Why not use escape_html: false

To avoid porting old code on upgrade.

This was intended for the html_safe/raw suggestion

bughit · 2019-01-19T08:02:08Z

Just use .html_safe

if to_json did not escape, calling html_safe/raw on it would not be good advice, because you would end up where you started, with a theoretical possibility of script injection. The following injects.

- script_injector_string = "</script><script>alert('injected')</script>"

:javascript
  #{JSON.dump(script_injector_string).html_safe};

If you did not want to depend on the protection of rails to_json, the proper workaround would be to let haml html escape the to_json output, wrap it in quotes and then on the javascript side, html decode it, then JSON.parse the result. Considering that html decoding is not even a js builtin, this is getting pretty cumbersome.

I hope this underscores the need for this option. If you are already using to_json, having to port to .to_json.html_safe is entirely pointless, as you are still protected by to_json. Much better to set an option and leave the already safe and simple code alone.

bughit · 2019-01-22T01:21:14Z

@k0kubun so would you consider adding an escape_filter_interpolations option, in light of

#984 (comment)

and

#984 (comment)

k0kubun · 2019-01-22T04:40:32Z

rails activesupport version of to_json escapes by default
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/json/encoding.rb#L42
in a rails console your example outputs:
"{"a":"\u003cscript\u003ealert('hello');\u003c/script\u003e"}"

Interesting. So this is really Rails-specific issue. Ideally this discussion should go to haml-rails repository but Haml's extension for Rails still exists in haml for now, so I'll talk about it here for now.

Could you confirm the following parts?

In a :javascript filter, you want to embed Rails-specific-.to_json-ed value which is NOT .html_safe-ed even while it's actually HTML-safe by the .to_json's special escape.
Your final goal is to pass a JSON-compatible value to JavaScript script in :javascript filter. Why don't you use data attribute to do that, which is known to be the safest way?

To avoid porting old code on upgrade.

The affected templates are only Haml templates with :javascript filter embedding .to_json-values, right? Do you have so many such templates?

Ultimately, I would recommend using data attribute to pass values to JavaScript safely, so I don't think this is a reasonable use case and I'm reluctant to support such feature to let people move towards that way.

I hope gives the answers to both comments. I'm actually not recommending raw / .html_safe but suggesting to use the safer way to pass values to JavaScript (which works also on Haml 5) regardless of the legacy Haml 4 feature you want to resurrect. In that sense, I reject introducing escape_interpolated_html for your use case. Please provide another one if you need.

bughit · 2019-01-22T05:22:35Z

In a :javascript filter, you want to embed Rails-specific-.to_json-ed value which is NOT .html_safe-ed even while it's actually HTML-safe by the .to_json's special escape.

yes

Why don't you use data attribute to do that, which is known to be the safest way?

we are talking about years of existing code
as already demonstrated, there's nothing unsafe about
```
:javascript
  var jsObject = #{ruby_rails_object.to_json};
```
so why do you keep implying it's somehow less safe?
furthermore this approach is simpler, terser, clearer and more direct than data attributes, so even for new code I would prefer that, even if forced to use html_safe.

The affected templates are only Haml templates with :javascript filter embedding .to_json-values, right?

I also have a bunch of :plain filters inside %script tags, I think to be able set attributes of the script tag, which :javascript does not let you do

Do you have so many such templates?

In one particular project there were ~ 250.

Ultimately, I would recommend using data attribute to pass values to JavaScript safely, so I don't think this is a reasonable use case

Since the safety concern has been dismissed, you are presenting no argument for why it's unreasonable, simply asserting it, whereas I gave you two compelling ones, here they are again:

Years of legacy code up to haml 4.x
The terseness, clarity, simplicity and directness of var jsObject = #{rails_object.to_json};
both the serialization and de-serialization are in one compact expression. This seems superior to the pedantic, more verbose, data attributes approach.

bughit · 2019-01-22T05:41:28Z

I reject introducing escape_interpolated_html for your use case. Please provide another one if you need.

Small aside, escape_interpolated_html is not a good name, it reads the same as the exiting escape_html, the new one would need to be called escape_filter_interpolations or just escape_filters.

I am not clear on what you are saying I could provide? Another use-case?

k0kubun · 2019-01-22T11:54:47Z

Hmm, fine. Your use case makes sense as an easy migration step of Haml 4 -> 5. I'll merge this patch and refine the name.

I'll use the name escape_filter_interpolations (I feel escape_filters is misleaing). I don't like to eliminate _html but first of all Haml is all about HTML and missing _html makes sense for the consistency with escape_attrs too.

bughit · 2019-01-22T18:14:59Z

thanks, confirming that it's working

Update ruby-haml to 5.1.2. pkgsrc change: add "USE_LANGUAGES= # none". ## 5.1.2 Released on August 6, 2019 ([diff](haml/haml@v5.1.1...v5.1.2)). * Fix crash in some environments such as New Relic by unfreezing string literals for ParseNode#inspect. [#1016](haml/haml#1016) (thanks [Jalyna](https://github.com/jalyna)) ## 5.1.1 Released on May 25, 2019 ([diff](haml/haml@v5.1.0...v5.1.1)). * Fix NameError bug for that happens on ruby 2.6.1-2.6.3 + haml 5.1.0 + rails 4.2.x + erubi. (Akira Matsuda) ## 5.1.0 Released on May 16, 2019 ([diff](haml/haml@v5.0.4...v5.1.0)). * Rails 6 support [#1008](haml/haml#1008) (thanks [Seb Jacobs](https://github.com/sebjacobs)) * Add `escape_filter_interpolations` option for backwards compatibility with haml 4 defaults [#984](haml/haml#984) (thanks [Will Jordan](https://github.com/wjordan)) * Fix error on empty :javascript and :css filter blocks [#986](haml/haml#986) (thanks [Will Jordan](https://github.com/wjordan)) * Respect changes in Haml::Options.defaults in `Haml::TempleEngine` options (Takashi Kokubun) * Un-freeze TempleEngine precompiled string literals [#983](haml/haml#983) (thanks [Will Jordan](https://github.com/wjordan)) * Various performance/memory improvements [#965](haml/haml#965), [#966](haml/haml#966), [#963](haml/haml#963) (thanks [Dillon Welch](https://github.com/oniofchaos)) * Enable `frozen_string_literal` magic comment for all .rb files [#967](haml/haml#967) (thanks [Dillon Welch](https://github.com/oniofchaos))

add escape_interpolated_html option

450f9c7

Set `escape_interpolated_html` to `false` for backwards compatibility with haml 4 defaults.

wjordan mentioned this pull request Mar 1, 2018

Javascript escaping invalid, generates SyntaxError: expected expression, got '&' #940

Closed

k0kubun reviewed Mar 10, 2018

View reviewed changes

k0kubun closed this Jun 23, 2018

curious725 mentioned this pull request Dec 19, 2018

:escape_html option overridden in Rails 'production' environment #980

Closed

k0kubun reopened this Jan 22, 2019

k0kubun merged commit 877bbea into haml:master Jan 22, 2019

k0kubun changed the title ~~add escape_interpolated_html option~~ add escape_filter_interpolations option Jan 22, 2019

faelsoto mentioned this pull request Dec 4, 2019

escape_attrs wraps incorrectly? #1019

Closed

marcandre mentioned this pull request Apr 22, 2020

HTML escape interpolated code in filters #770

Merged

This was referenced Mar 16, 2021

Bump haml from 4.0.5 to 5.2.1 in /compass-style.org krisfrenz/compass#5

Open

Bump haml from 4.0.7 to 5.2.1 dshaneg/keep-a-changelog#3

Open

Bump haml from 4.0.5 to 5.2.1 ryanlabouve/pixelperfect#1

Open

Hamms mentioned this pull request Feb 23, 2022

Update haml gem to 5.2.0 code-dot-org/code-dot-org#45007

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add escape_filter_interpolations option #984

add escape_filter_interpolations option #984

wjordan commented Mar 1, 2018

k0kubun Mar 10, 2018

k0kubun Mar 10, 2018 •

edited

Loading

k0kubun commented Mar 10, 2018

k0kubun commented Jun 23, 2018

bughit commented Jan 19, 2019 •

edited

Loading

k0kubun commented Jan 19, 2019

bughit commented Jan 19, 2019

bughit commented Jan 19, 2019

bughit commented Jan 19, 2019 •

edited

Loading

bughit commented Jan 22, 2019

k0kubun commented Jan 22, 2019 •

edited

Loading

bughit commented Jan 22, 2019 •

edited

Loading

bughit commented Jan 22, 2019

k0kubun commented Jan 22, 2019

bughit commented Jan 22, 2019

add escape_filter_interpolations option #984

add escape_filter_interpolations option #984

Conversation

wjordan commented Mar 1, 2018

k0kubun Mar 10, 2018

Choose a reason for hiding this comment

k0kubun Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

k0kubun commented Mar 10, 2018

k0kubun commented Jun 23, 2018

bughit commented Jan 19, 2019 • edited Loading

k0kubun commented Jan 19, 2019

bughit commented Jan 19, 2019

bughit commented Jan 19, 2019

bughit commented Jan 19, 2019 • edited Loading

bughit commented Jan 22, 2019

k0kubun commented Jan 22, 2019 • edited Loading

bughit commented Jan 22, 2019 • edited Loading

bughit commented Jan 22, 2019

k0kubun commented Jan 22, 2019

bughit commented Jan 22, 2019

k0kubun Mar 10, 2018 •

edited

Loading

bughit commented Jan 19, 2019 •

edited

Loading

bughit commented Jan 19, 2019 •

edited

Loading

k0kubun commented Jan 22, 2019 •

edited

Loading

bughit commented Jan 22, 2019 •

edited

Loading