Fixes #322 - Autoescaping doesen't work in <script> tag context

[patrick-steele-idem]

This PR changes out the escape function that is used when generating code within the context of the <script> tag. The escape function used within the context of the <script> tag will only escape '</' to prevent an ending script tag.

[mlrawlings]

Functionally, everything looks good.

However, the fact that you had to change the script-tag-entities test highlights that technically this is a breaking change. In the case of the document.write in that test, the <script> tag will now get inserted into the DOM and executed.

That said, it's no less secure (the user could still run arbitrary code) and I think this is how things should work. This change is less about security and more about making sure <script> blocks continue to execute properly.

[mlrawlings]

I wonder if this change would be better applied to $!{} and and then we deprecate use of ${} inside <script> tags. That may seem a bit weird at first, but it communicates that ${} is not secure inside a <script> tag as it is outside one. And I cannot envision any case where the escape wouldn't be desired.

Assuming it is in a string, '</' === '\u003C/' so they'll be functionally equivalent and if it's not in a string, it wouldn't be valid JS (unless you're doing a comparison against a regex literal if(x</A-Z/) which I'm willing to bet no one is doing).

We could take this farther and look for the full ending </script> tag before escaping as other tags shouldn't matter.

[patrick-steele-idem]

Your comments are correct @mlrawlings. This is more about fixing bad escaping since HTML entities within the <script> are not decoded by the browser so we should have never been using HTML entity encoding with the <script> tag. I'm only concerned about the ${JSON.stringify(...)} being safe within the <script> tag because anything else would be inherently unsafe. The only reason JSON.stringify is not safe without additional escaping is because a JSON string could contain an ending </script> tag, hence the need to escape that ending sequence.

[patrick-steele-idem]

I wonder if this change would be better applied to $!{} and and then we deprecate use of ${} inside <script> tags. That may seem a bit weird at first, but it communicates that ${} is not secure inside a <script> tag as it is outside one. And I cannot envision any case where the escape wouldn't be desired.

${JSON.stringify()} should be safe within the context of a <script> tag and that is the only thing we need to ensure in my opinion.

Assuming it is in a string, '</' === '\u003C/' so they'll be functionally equivalent and if it's not in a string, it wouldn't be valid JS (unless you're doing a comparison against a regex literal if(x</A-Z/) which I'm willing to bet no one is doing).

Since we are only concerned within the text within the ${} placeholder, I am not concerned about the expression within ${} containing </ outside of a string.

We could take this farther and look for the full ending </script> tag before escaping as other tags shouldn't matter.

I'm not quite sure what you mean here.

[mlrawlings]

To me, ${}, communicates that Marko is handling escaping and will likely result in people assuming things like <script>var x = "${ data }"</script> are safe (see #322).

Where as $!{} communicates you're on your own to escape things if it's necessary, making it more clear that you should do something like $!{JSON.stringify(data)}

I think the basic script injection is something that's pretty obvious, but the danger of a string that includes </script> is not immediately obvious. I didn't even realize until today that this is what you've been talking about when you said JSON.stringify isn't completely safe.

We could take this farther and look for the full ending </script> tag before escaping as other tags shouldn't matter.

Adding the functionality to $!{} means that it doesn't have to be a thought for developers. It also means there's no way to actually have something that isn't escaped, but I don't see that being an issue. However, looking for the full script tag would further reduce the chance of it impacting something.

The other thing I'm concerned about is I know that we don't want to bump to 4.0 on this change, but there's the (small) possibility that someone is relying on the current behavior of ${}.

[mlrawlings]

On second thought, let's merge it.