Skip to content

Commit

Permalink
Escape = in HTML content
Browse files Browse the repository at this point in the history 
There was a potential XSS exploit when using unquoted attributes that this should help reduce.

Fixes #1083
  • Loading branch information
@kpdecker
kpdecker committed Sep 1, 2015
1 parent b0d217e commit 83b8e84
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 3 deletions.
7 changes: 4 additions & 3 deletions lib/handlebars/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ const escape = {
'>': '>',
'"': '"',
"'": ''',
'`': '`'
'`': '`',
'=': '='
};

const badChars = /[&<>"'`]/g,
possible = /[&<>"'`]/;
const badChars = /[&<>"'`=]/g,
possible = /[&<>"'`=]/;

function escapeChar(chr) {
return escape[chr];
Expand Down
1 change: 1 addition & 0 deletions spec/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ describe('utils', function() {
describe('#escapeExpression', function() {
it('shouhld escape html', function() {
equals(Handlebars.Utils.escapeExpression('foo<&"\'>'), 'foo&lt;&amp;&quot;&#x27;&gt;');
equals(Handlebars.Utils.escapeExpression('foo='), 'foo&#x3D;');
});
it('should not escape SafeString', function() {
var string = new Handlebars.SafeString('foo<&"\'>');
Expand Down

15 comments on commit 83b8e84

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented on 83b8e84 Feb 8, 2016

Choose a reason for hiding this comment

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

Would it be possible to get this backported into a release on the 3.0.x stream?

That way I can do a non-breaking change release of express-hbs which benefits from not having the potential exploit.

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented on 83b8e84 Feb 8, 2016

Choose a reason for hiding this comment

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

In the interest of making this as easy as possible, I tried to make a branch with the right state: https://github.com/ErisDS/handlebars.js/tree/v3-with-fix

Not sure if it really helps 😁

@kpdecker
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is listed in the compatibility changes list for 4.0.0 and I suppose that it's debatable wether it is a fix or a breaking change, but I am somewhat hesitant to do a forked release over urging users to move to the 4.x code line. Do you have any sense of the number of users that would be impacted by this and the number that would be overly impacted by a 4.x upgrade to the extent that the wouldn't otherwise?

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented on 83b8e84 Feb 25, 2016

Choose a reason for hiding this comment

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

I think we're coming at this from two different angles.

From my perspective, this change is a fix for a security vulnerability in two different codebases that I maintain. The vulnerability is listed by both the node security project and snyk.io. Currently every one of handlebars dependencies which use the 3.x branch, and every sub dependency of those projects, is being flagged as insecure when checked against the available tools.

As the fix isn't available on the 3.x branch, the entire ecosystem has to, currently, do a breaking update and handle several other changes in order to do a security release.

E.g. Ghost is dependent on handlebars through express-hbs. The only way that I can update express-hbs is to move to the 4.x branch, meaning I have to also figure out and publish a full set of compatibility notes, and all of the dependents of express-hbs have to do the same work.

It makes sense to do that when moving forward to get new features from the 4.x line. It does not make sense to do that when you want to update your project to get rid of a security vulnerability.

I'm not sure what the use case is in which adding escaping for = might cause a breaking change, but if this is a serious concern, then 3.1.0 makes more sense than 3.0.3. However, I do think that this vulnerability fix needs to be available to the 3.x branch because handlebars is so heavily depended upon.

@kpdecker
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We've never followed branching in the line sense, more of semver as a method to advertise that something may be breaking as the code line progresses forward. There are a few other things since 4.0.0 proper which are flagged in security warnings due to upstream projects (4.0.5).

At this point we don't really have the resources to maintain two code lines, i.e. myself, but if you're willing to maintain a security codeline for 3.x then we should talk to @wycats to make that happen.

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented on 83b8e84 Feb 25, 2016

Choose a reason for hiding this comment

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

As @wycats knows, I'm more than happy to lend a hand wherever needed 👍

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented on 83b8e84 Feb 29, 2016

Choose a reason for hiding this comment

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

@kpdecker & @wycats can we get this ball in motion?

@kpdecker
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ErisDS I don't have admin on this repo, so @wycats will have to do this for you.

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented on 83b8e84 Apr 11, 2016

Choose a reason for hiding this comment

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

Ok, I'm feeling a bit silly 😵. I've got the changes I wanted to release as 3.0.4: https://github.com/wycats/handlebars.js/commits/3.x - I've cherry picked 3 commits from the 4.x branch to get rid of the security warnings for 3.x. Very straightforward changes.

Now I'm ready to do a release, but because the release process is wrapped in the yeoman generator, I am not 100% certain that it will behave correctly for this case? @kpdecker you seem to know the generator-release package pretty well - are there any commands that would incorrectly try to base themselves from the previous 4.0.5 release or is it safe? I've done a notes dry-run and here's my generator-release file 😁

Also I missed that I need permission to do gem push handlebars-source-*.gem @wycats

@kpdecker
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ErisDS generator-release, to my knowledge, has never been used for branching releases. It's certainly a feature that it should have, but I doubt that it will work well right now. I'm in push to get my startup's v1 out the door right now and my 1 month old daughter still believes that sleep is optional for everyone in this house, so I don't have much time for "fun things" :(. Adding a feature like this is on my radar but it might be a little bit before I get to it. Baring that, release generator is mostly just a markdown generator and test executor, so you could do the same with a little bit of manual work.

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented on 83b8e84 Apr 21, 2016

Choose a reason for hiding this comment

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

@kpdecker I totally understand your position - I spent a long time reading what was going on because I'm trying not to be a pain in your arse 😉

It seems this is more work than I had initially hoped, so I've u-turned and am not going to be pursuing this any further. Thanks for your time and patience though.

@nknapp
Copy link
Collaborator

@nknapp nknapp commented on 83b8e84 Apr 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@ErisDS do you still need a 3.x-release of Handlebars? Apart from handlebars-site and AWS, I now have all the permissions I need to do one and I have (imho successfully) created a 4.0.7 release yesterday.
If you need 3.0.4, I could probably do it.

@ErisDS
Copy link
Collaborator

@ErisDS ErisDS commented on 83b8e84 May 1, 2017

Choose a reason for hiding this comment

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

@nknapp I do not, but thank you for offering 😃

@mattolson
Copy link

Choose a reason for hiding this comment

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

@nknapp @kpdecker Any chance we can get a 3.0.4 released with this security fix?

@nknapp
Copy link
Collaborator

@nknapp nknapp commented on 83b8e84 Dec 15, 2018

Choose a reason for hiding this comment

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

3.0.4 and 3.0.5 released today. See #1454

Please sign in to comment.