Update dependencies (fix `npm audit` failures); fix tests; update Travis build

gruntjs / grunt-contrib-handlebars

Precompile Handlebars templates to JST file.

http://gruntjs.com/

MIT License

282 stars 126 forks source link

Update dependencies (fix `npm audit` failures); fix tests; update Travis build #169

Closed cjbarth closed 5 years ago

cjbarth commented 5 years ago

This package had insecure and deprecated dependencies. This PR corrects that and fixes the test to keep working. One test (the Node module test) had to be disabled because I couldn't get it working across all versions of Node. I'm open to suggestions on that. Otherwise, perhaps we can land this and iterate on that as this package is causing npm audit failures for projects the depend on it.

jsf-clabot commented 5 years ago

All committers have signed the CLA.

cjbarth commented 5 years ago

@XhmikosR I wanted to ping you specifically on this PR because of your recent involvement with this project. This PR addresses a critical security vulnerability found in this package and I'd like to get some eyes on it. If you are the wrong person, might you suggest someone who I could get to help with this matter?

XhmikosR commented 5 years ago

Well, the PR has unrelated changes in it.

Also, 4.3.3 might not be the latest https://github.com/wycats/handlebars.js/issues/1563#issuecomment-535888536

So, just revert your changes and update only the needed stuff.

cjbarth commented 5 years ago

If you are talking about JSCS being unrelated, that is actually part of the problem. That is no longer maintained and eslint is the replacement. To get this package to clear npm audit I couldn't use JSCS. Are there other parts you're referring to? I really tried to keep this to a minimum change and get it to pass a build.

cjbarth commented 5 years ago

It does occur to me that with updating semver major versions of dependencies, perhaps this package should get a semver major bump too. Please let me know your thoughts.

XhmikosR commented 5 years ago

I can't review a PR with so many unrelated changes in it.

cjbarth commented 5 years ago

Ok, let's start this way; I'll make a different PR that just get's the build working again, so those will be a separate PR. I can't get anything to pass Travis without that working.

XhmikosR commented 5 years ago

First of all there's a procedure for things.

We have https://github.com/gruntjs/grunt-contrib-internal for some common stuff. JSCS I don't care about it personally and I would just drop it until we have a proper ESLint solution in the https://github.com/gruntjs/grunt-contrib-internal repo.

So, wait until https://github.com/gruntjs/grunt-contrib-internal/pull/40 is solved. The handlebars update can still be applied with npm audit.

cjbarth commented 5 years ago

Thank you for pointing me to that. I will try to incorporate that in here so we can leverage the common stuff with this grunt-contrib too. Thank you.

XhmikosR commented 5 years ago

I removed jscss in master. Please rebase and only keep the handlebars update + the test files.

XhmikosR commented 5 years ago

I went ahead and made #171, which updates everything and drops support for Node.js < 8.x. It will be a major version bump due to this.