Detect HTML/JS injection attacks and warn users pro-actively

[joshgoebel]

Is your request related to a specific problem you're having?

#2884
ccampbell/rainbow#249

This comes up again and again (though thankfully not TOO often). Beginners are VERY confused about this whole HTML escaping thing.

The solution you'd prefer / feature you'd like to see added...

While we can't do anything smart about this yet (because we unfortunately allow HTML inside code blocks for "clever" users) I'd like this to change with v11. With v11 we should drop this HTML pass-thru behavior and move it to a plugin (making it very much opt-in). The default behavior should be that HTML is silently dropped and I'd even consider adding some sort of error:

[code block]
WARNING.
Are you missing a bunch of HTML code you expected to see here? 
Your HTML wasn't properly escaped and that can lead to serious
security issues. _Learn More_

Properly escape your code and the highlighting you expect will kick in.
[/code block]

This would of course be a breaking change so we'd need to wait until v11. For 95% of users I can't see the downside to this and it seems we could potentially educate and prevent a lot of harm. Someone wanting the HTML to pass thru would install a plug-in and thus change the behavior, get the old behavior back, etc.

Any alternative solutions you considered...

Silent dropping but no error... but that just leads to support issues... I suppose we could log the error to the console vs actually showing it on the webpage.

[joshgoebel]

@RunDevelopment Would love if you have any thought on this from a Prism.js perspective.

[RunDevelopment]

Adding a console waring is a good idea IMO.
Just make sure you only log once, otherwise constantly re-rendering React projects will get their console spammed. There might also be a problem with SSR in general, so there should be an option to turn off the warning.

---

Honestly, I don't understand this merging but that's probably the HTML pass-thru you mentioned. (Is this behavior documented anywhere?)

For Prism, we have Keep Markup to implement an HTML pass-thru but this can actually cause some issues (PrismJS/prism#2384).
HTML pass-thru seems to on by default in HighlightJS. Did this cause problems with double (SSR+client-side) highlighting in the past?

[joshgoebel]

Just make sure you only log once, otherwise constantly re-rendering React projects will get their console spammed.

This only matters on the client-side when retrieving content from an HTML element... it doesn't have any relevance to SSR at all unless I'm missing something. The only case we're trying to prevent here is client-side:

Here is an example of how script works:

<pre><code>
<script>alert("hi");</script>
</code></pre>

...

<script>
hljs.highlightOnLoad(); // will fire us up and highlight every code block
</script>

Now that code block is VERY dangerous because that code will execute. It should be escaped... and if this was a more complex example then all sorts of harm could result... so I'm saying with a major version release a breaking change that simply DISALLOWS HTML inside the code block... it must be raw text or an error... the idea being that in development people would catch this problem (because of the visible error) and it would never make it out into the wild (where someone is going to exploit it).

so there should be an option to turn off the warning.

We could add allowUnsafeHTML or some such - but I'm not sure why it's needed. But I'm honestly thinking of entirely breaking the highlighting functionality if we see HTML and they haven't enabled that (or aren't using a plugin that allows for it).

[joshgoebel]

Honestly, I don't understand this merging but that's probably the HTML pass-thru you mentioned. (Is this behavior documented anywhere?)

Not really, but the idea is simple. We pass HTML thru without touching it in the same position in the content:

var x;
<span class="important">var y;</span>

We will highlighting this as such (on the browser):

<span class="keyword">var</span> x;
<span class="important"><span class="keyword">var</span> y;</span>

We added keyword wrappers but the HTML passes thru unmolested... making it useful for things like highlighting a line or a piece of code. It's definitely an edge case but one some people really want - but it's a ton of complexity.

---

Occasionally we get an issue here "where did all my HTML go" and this issue was meant to perhaps stave off such issues by building that "education" into the library iself.

[joshgoebel]

The code someone would use to do Highlight.js SSR is very different than the code they'd use on the client. Most people probably do one or the other not both.

[RunDevelopment]

My concern is that the server might pass already highlighted code to the client, the client rehydrates the pages, and that causes re-highlighting. But maybe that's not a problem?