V1.3.x (Retrofit Security Patch...)

[infolock]

granted - this will probably be closed as soon as it is opened - however... this would help teams that may have a legacy build that still uses this old ass version ;)

Original PR this PR is based on can be found here: 83b8e84

Idea is simply to have a 1.3.x branch so that a 1.3.whatever tag can be generated from it with the patch applied...

if you guys could create a v1.3.x branch and update this PR to be merged into it - that would effectively do what we're after here...

[nknapp]

Yes, your probably right, it won't be merged, at least not like that...

I have attempted to fix that in the 3.0.x branch. Shortly after publishing 3.0.4 and 3.0.5 with the fix, I received the first complaints (#1489) that this breaking change should not be done with an patch-version increment. But as I wrote in #1492: The only semver-compatible way to achieve this, is a compiler- or runtime-option that allows you to activate secure escaping. It's definitely not a patch increment or a 1.3.0-something

But I have created a 1.x-branch.

[infolock]

Note: I intentionally left the PR in a conflict state until the wycats team can have an opportunity to decide if they even want to do this...

Updated base to point to 1.x

[infolock]

@nknapp heh, i'm not entirely sure why the thing this breaks would actually be considered a reason not to apply this (isn't it doing exactly what it was intended to do? 🤔 )

Either way, its your all's call =) This PR was created just as a means to help get it applied to that build (if it is even possible)

ty for looking at it tho

[infolock]

@nknapp i dunno - the person's complaint about it breaking is using it in the way that was outlined in the original report : "When using attributes without quotes in a handlebars template, an attacker can manipulate the input to introduce additional attributes, potentially executing code. " - source

so i'm not so sure this should have been reverted. i'll watch the threads related to this though and see where you all go from here.

As an aside though...
imo though - this shouldn't be considered a handlebars vulnerability so much as it is a vulnerability introduced by the developer(s) who aren't applying quotes to values. but hey - that's just me ;)

do a quick search on XSS HTML attributes without quotes and you'll see its been an issue for - quite a long time now ;)

Here is one such reference: https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.md#rule-2---attribute-escape-before-inserting-untrusted-data-into-html-common-attributes

[infolock]

@nknapp Actually - the more I'm reading the reported issue on snyk, the more i'm now convinced this is not a handlebars issue at all.

i'm going to close this PR because there isn't actually anything to fix in handlebars. the developer who reported this issue needs to just fix their own code lol...

The reason is simple: the XSS vulnerability that has been reported is due to the developer not adding quotes around their HTML attributes (again, refer to this OWASP link). However, they could introduce this issue with or without handlebars.

Case in point is the very example they list for this:

Example:

Assume handlebars was used to display user comments and avatar, using the following template: <img src={{avatarUrl}}><pre>{{comment}}</pre>

If an attacker spoofed their avatar URL and provided the following value: http://evil.org/avatar.png onload=alert(document.cookie)

The resulting HTML would be the following, triggering the script once the image loads: <img src=http://evil.org/avatar.png onload=alert(document.cookie)><pre>Gotcha!</pre>

if you replace the image example with this: document.write('<img src=' + blah + '/><pre>blah</pre>') - you effectively create the very XSS vulnerability they are complaining about without a single line of handlebars.

I was also able to show how to reproduce the same vulnerability using the underscorejs _.template.

Therefore, this should be something the handlebars team pushes back on and says they won't fix because it isn't a handlebars issue.

(apparently, this was also mentioned back in 2015: #1083 (comment))

sorry for all the @ mentions - just wanted to keep you informed here...

[infolock]

@nknapp i actually thought maybe handlebars wasn't responsible for this and hoped to get the vulnerability report removed entirely. however, after a few days of back and forth discussions it turns out that it unfortunately isn't that simple since the escape character logic stuff was originally implemented with the intention of actually being something to help thwart an xss attack. (which was even added to the readme back in the day too).

However... is there any way that the escape character logic and be removed completely and the documentation updated to help users understand that inserting HTML without quotes around their attributes would result in an XSS injection, and that handlebars does not (any longer) condone nor attempt to protect against this (or something along those lines)?

maybe an even better solution would to just saying that you do this for whatever feature you need it to be there for to work and tell users that they should not rely on this functionality to sanitize HTML - which would require only a README update and no code changes (saying you make no promises of it providing any XSS protection specifically would remove you from their deps and leave it up to the devs to fix)

If you do - then this would satisfy the report and this can be lifted (which is based on the conversation had with one of the members on the node security advisory team).

Otherwise, I'm afraid you are in for a very long rabbit hole of possible bad characters that could be used as a means to create an XSS Vulnerability (which can be hundreds of possibilities when you start diving into the unicode form of some of these escape characters that would also then have to support).

CC: @wycats (fwiw)