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

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
3 changes: 1 addition & 2 deletions lib/hbs.js
View file
Open in desktop
Expand Up @@ -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.

}
});
}
Expand Down
10 changes: 5 additions & 5 deletions package.json
View file
Open in desktop
Expand Up @@ -12,13 +12,13 @@
"lib": "./lib"
},
"dependencies": {
"handlebars": "4.0.14",
"walk": "2.3.9"
"handlebars": "^4.3.1",
"walk": "^2.3.9"
},
"devDependencies": {
"istanbul": "0.4.5",
"mocha": "1.21.5",
"supertest": "1.1.0"
"istanbul": "^0.4.5",
"mocha": "^6.2.0",
"supertest": "^1.1.0"
},
"license": "MIT",
"engines": {
Expand Down