Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add escape_filter_interpolations option #984

Merged
merged 1 commit into from Jan 22, 2019

Conversation

wjordan
Copy link
Contributor

@wjordan wjordan commented Mar 1, 2018

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.
@@ -13,6 +13,7 @@ class TempleEngine < Temple::Engine
encoding: nil,
escape_attrs: true,
escape_html: false,
escape_interpolated_html: nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@k0kubun k0kubun Mar 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@k0kubun
Copy link
Member

k0kubun commented Mar 10, 2018

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
Copy link
Member

k0kubun commented Jun 23, 2018

closing due to inactivity

@bughit
Copy link

bughit commented Jan 19, 2019

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
Copy link
Member

k0kubun commented Jan 19, 2019

  1. 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
  2. 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).
  3. Why not use escape_html: false?
  4. 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
Copy link

bughit commented Jan 19, 2019

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
Copy link

bughit commented Jan 19, 2019

Why not use escape_html: false

To avoid porting old code on upgrade.

This was intended for the html_safe/raw suggestion

@bughit
Copy link

bughit commented Jan 19, 2019

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
Copy link

bughit commented Jan 22, 2019

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

#984 (comment)

and

#984 (comment)

@k0kubun
Copy link
Member

k0kubun commented Jan 22, 2019

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?

  1. 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.
  2. 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
Copy link

bughit commented Jan 22, 2019

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?

  1. we are talking about years of existing code
  2. 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?
  3. 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:

  1. Years of legacy code up to haml 4.x
  2. 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
Copy link

bughit commented Jan 22, 2019

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
Copy link
Member

k0kubun commented Jan 22, 2019

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.

@k0kubun k0kubun reopened this Jan 22, 2019
@k0kubun k0kubun merged commit 877bbea into haml:master Jan 22, 2019
@k0kubun k0kubun changed the title add escape_interpolated_html option add escape_filter_interpolations option Jan 22, 2019
@bughit
Copy link

bughit commented Jan 22, 2019

thanks, confirming that it's working

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 24, 2020
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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants