Added :sanitize_javascript template option

[johnkchow]

In production code we had experienced XSS issue when we bootstrap JS data within haml. An example is if we have the following code for a .html.haml file:

- user_input = "</script><script>alert('you've been xss-ed!');</script>"
:javascript
  var userInput = #{user_input.to_json};

While you could argue that the server-side code should be sanitizing data, I think this PR will add another safeguard and help a lot of folks.

I realize that this isn't a proper PR, since I'm missing a lot of things (updates to CHANGELOG, unit tests, etc). I wanted to open this PR to start a discussion to get your feedback and make any adjustments if you guys think it's a good idea. I'll go ahead and polish up the PR to get it checked in.

I'm also having trouble running the test/filters_test.rb tests (bundle exec ruby -Itest test/filters_test.rb), it just say I'm running 0 assertions. Executing bundle exec rake doesn't seem to be executing any of the test cases within test/filters_test.rb. So any help with that would be great!

[mattwildig]

I’m not sure that this is the right way to address this, but looking into it I realised interpolation in filters doesn’t use Rails’ XSS protection.

It looks like all filter content used to be escaped if the global escape_html was set until 6efc9ba. Obviously that was no good since some filters produce HTML that you want. Since 83f8b20 we can now be more specific with what gets escaped (i.e. only escape interpolated bits), and with Rails XSS mods the user can determine whether or not an individual string needs escaping at all.

I propose that we add an option to HTML escape interpolation in filters, and enable it by default in Rails. I suspect Rails users may relying on this protection, and also they can use html_safe for any strings that shouldn’t be escaped. Outside of Rails there is no expectation that strings would be escaped, and there is no easy way to turn it off for an individual string, so it doesn’t make sense to enable it there.

6efc9ba includes an explicit test that interpolated code isn’t escaped (and that test still exists although it has been moved and renamed), but I think that just represents the code at the time (there wasn’t the flexibility to escape interpolated sections but leave the rest).

The actual code changes seem pretty straightforward – if everyone agrees I’ll put a patch together.

@norman, @teeparham any thoughts? Any reason interpolation in filters should be treated differently in Rails?

@johnkchow do you think HTML escaping the string rather than stripping <script> tags would address your issue?

[johnkchow]

@mattwildig After re-reading the Rails security guide, I agree that my solution isn't ideal. HTML escaping the string interpolations should work for my use case. If you can release that patch, I'd much appreciated it! 👍

For my two cents, I think this should be enabled by default in Rails. I just assumed it was enabled by default because Rails by default escapes strings in views, and I imagine many Rails users are thinking the same thing. Only problem is it may break existing codebases, but IMO it'd make the internet a whole lot safer and worth it.

[teeparham]

@mattwildig What about filters like :plain? I suppose we should escape interpolated text there too (so you're right that all filters should escape interpolated text). Thanks for the careful analysis. I like your proposed solution.

[mattwildig]

See #770.

This affects all filters using the base compile method, which is everything except the Erb And Ruby filters in Haml itself, and all but builder and Nokogiri filters in Haml contrib.

[teeparham]

Closing in favor of #770.

[johnkchow]

@mattwildig @teeparham thanks guys! 👍

[johnkchow]

For those with existing codebases and is experiencing breakage from #770 (i.e. when interpolating JSON strings), take a look at my forked 4.0.5 branch. It only HTML escapes '<' and '>' characters for :javascript and :coffee filters instead of /[\"><&]/, so it should protect you from XSS w/ less impact. Not exactly the cleanest code but gets the job done.