Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

resolve handlebars security risk #179

Closed
wants to merge 1 commit into from
Closed

resolve handlebars security risk #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -12,7 +12,7 @@
"lib": "./lib"
},
"dependencies": {
"handlebars": "4.0.14",
"handlebars": "~4.3.0",
"walk": "2.3.9"
},
"devDependencies": {
Expand Down
2 changes: 1 addition & 1 deletion test/3.x/app.js
View file
Open in desktop
Expand Up @@ -195,7 +195,7 @@ test('syntax error', function(done) {
request(app)
.get('/syntax-error')
.expect(500)
.expect(shouldHaveFirstLineEqual('Error: ' + path.join(__dirname, 'views', 'syntax-error.hbs') + ': Parse error on line 1:'))
.expect(shouldHaveFirstLineEqual('Error'))
Copy link
Contributor

Choose a reason for hiding this comment

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

A feature of this module is to add the path if the file to the error message. Is there no way to keep this feature working in the new handlebars?

If not, can you explain what has changed in handlebars that break this (maybe even the commit in handlbars)?

Copy link
Author

@mcandre mcandre Sep 26, 2019 •
edited

Choose a reason for hiding this comment

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

We have a choice: Keep the feature now, and keep handlebars insecure. Or drop the feature for now and revisit once we have a CVE-less release published to NPM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but to make the decision, it would help to understand what the cause is, so we can gauge what the effort is in order to make a 4.x release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to figure out one of three paths is all (maybe there are more)

  1. What can we do to upgrade to latest handlebars but keep this a 4.x release so folks get it automatically with semver ranges.
  2. Maybe even if we can backport the fix to the 4.0.x branch of handlebars (this was done with previous patches) thus we can bump the patch version.
  3. Last case, document the breaking changes and migration guide and release as 5.0 and people will need to migrate to address the issue.

Those are my thoughts, at least, in order of own own preferences.

Copy link
Author

Choose a reason for hiding this comment

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

  1. Cut a new major release now, before people get hacked.

Worry about backwards compatible features later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @martinrue your comments are not constructive and taking away my attention to actually getting a release made. If you are that concerned, the best use of the effort would likely be to actually help address the compatibility issues to get handlebars upgraded or be to get the handlebars project to make a 4.0.x release instead of hounding me.

.end(done)
});

Expand Down
2 changes: 1 addition & 1 deletion test/4.x/app.js
View file
Open in desktop
Expand Up @@ -205,7 +205,7 @@ test('syntax error', function(done) {
request(app)
.get('/syntax-error')
.expect(500)
.expect(shouldHaveFirstLineEqual('Error: ' + path.join(__dirname, 'views', 'syntax-error.hbs') + ': Parse error on line 1:'))
.expect(shouldHaveFirstLineEqual('Error'))
.end(done)
});

Expand Down