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

Fix handlebars upstream vulnerability #178

Closed
wants to merge 1 commit into from
Closed

Fix handlebars upstream vulnerability #178

wants to merge 1 commit into from

Conversation

janjongboom
Copy link

@janjongboom janjongboom commented Sep 26, 2019 •
edited

This fixes the upstream prototype pollution vulnerability in handlebars that was fixed in handlebars 4.3.0. Similar to #175 but with tests passing.

Test result against Node.js v10.16.0:

  ✓ express 3.x index: 42ms
  ✓ express 3.x partials: 7ms
  ✓ express 3.x html extension: 15ms
  ✓ express 3.x syntax error: 3ms
  ✓ express 3.x escape for frontend: 3ms
  ✓ express 3.x response locals: 3ms
  ✓ express 3.x response locals cached: 4ms
  ✓ express 3.x response globals: 5ms
  ✓ express 3.x app.render: 2ms
  ✓ express 3.x async helpers index: 4ms
  ✓ express 3.x async helpers async: 3ms
  ✓ express 3.x view engine index: 3ms
  ✓ express 3.x view engine index w/layout: 2ms
  ✓ express 3.x no layout index: 2ms
  ✓ express 3.x no layout index w/layout: 3ms
  ✓ express 3.x no layout index layout cache: 2ms
  ✓ express 4.x index: 12ms
  ✓ express 4.x partials: 3ms
  ✓ express 4.x html extension: 8ms
  ✓ express 4.x syntax error: 3ms
  ✓ express 4.x escape for frontend: 3ms
  ✓ express 4.x response locals: 3ms
  ✓ express 4.x response locals cached: 12ms
  ✓ express 4.x response globals: 4ms
  ✓ express 4.x multiple views directories: 3ms
  ✓ express 4.x app.render: 2ms
  ✓ express 4.x async helpers index: 4ms
  ✓ express 4.x async helpers async: 2ms
  ✓ express 4.x async helpers async-with-params: 2ms
  ✓ express 4.x view engine index: 2ms
  ✓ express 4.x view engine index w/layout: 3ms
  ✓ express 4.x no layout index: 2ms
  ✓ express 4.x no layout index w/layout: 3ms
  ✓ express 4.x no layout index layout cache: 2ms

  34 passing (3s)

=============================================================================
Writing coverage object [/Users/janjongboom/repos/hbs/coverage/coverage.json]
Writing coverage reports at [/Users/janjongboom/repos/hbs/coverage]
=============================================================================

=============================== Coverage summary ===============================
Statements   : 89.66% ( 156/174 )
Branches     : 73.13% ( 49/67 )
Functions    : 92.31% ( 36/39 )
Lines        : 89.66% ( 156/174 )

The patch in lib/hbs.js is not strictly related to this patch, but is to get the tests running in the current version of Node.js. Master tests are also broken against Node v10.

dougwilson
dougwilson reviewed Sep 26, 2019
View reviewed changes
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

I just have a general comment. The same concern was raised in #176 (comment) so hoping there is a way.

lib/hbs.js
@@ -69,8 +69,7 @@ function middleware(filename, options, cb) {
cb(null, res);
});
} catch (err) {
err.message = filename + ': ' + err.message;
cb(err);
cb(new Error(filename + ': ' + err.message));
Copy link
Contributor

Choose a reason for hiding this comment

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

I looked into this change too, but was hoping for a backwards compatible one so this didn't need to be a 5.0 release. It drops all custom properties from the original Error.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm... What kind of custom properties are present here? The stack trace will be maintained through this pattern (with an extra step, but that should be fine). If handlebars set any other properties we could just copy them over.

Copy link
Contributor

Choose a reason for hiding this comment

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

Syntax error from handlebars have all those properties which show how the syntax was invalid. For example err.hash.line is the line the error was on, err.hash.text was the bad text it encountered, etc. Node.js errors coming through here have their standard err.code property, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

The last time there was a release (in April) the feature was working. It seems like something has changed and it is no longer working for some reason. I'm trying to track down what is causing it to fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, haha, I forgot I was running handlebars 4.3.1 locally for testing. Yes, the feature still works fine on 4.0.14. So I'm trying to determine what changed in handlebars.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is something in the 4.2.1 release, as it works with handlebars 4.2.0 but does not work with 4.2.1

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed a bug in handlebars.js project: handlebars-lang/handlebars.js#1562

Copy link
Author

Choose a reason for hiding this comment

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

@dougwilson Good find.

@dougwilson dougwilson mentioned this pull request Sep 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougwilson dougwilson dougwilson left review comments

Assignees
No one assigned
Labels
deps pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@janjongboom @dougwilson