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.

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

resolve handlebars security risk #179

Closed
wants to merge 1 commit into from
Closed

resolve handlebars security risk #179

wants to merge 1 commit into from

Conversation

mcandre
Copy link

@mcandre mcandre commented Sep 26, 2019

This resolves #176 .

Of course, npm audit complains about additional security risks that should be addressed in another ticket.

@mcandre
Copy link
Author

mcandre commented Sep 26, 2019

Related:

#180

@@ -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

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.

@mcandre
Copy link
Author

mcandre commented Sep 26, 2019

Related:

#181

@mcandre
Copy link
Author

mcandre commented Sep 26, 2019

See #182

@dougwilson
Copy link
Contributor

Duplicate of #178

@dougwilson dougwilson marked this as a duplicate of #178 Sep 26, 2019
@dougwilson dougwilson closed this Sep 26, 2019
dougwilson added a commit that referenced this pull request Sep 27, 2019
fixes #176
closes #175
closes #178
closes #179
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

npm audit security issue with handlebars 4.0.x
2 participants