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

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

Closed
wants to merge 1 commit into from

Conversation

cjbarth
Copy link
Contributor

@cjbarth cjbarth commented Sep 26, 2019

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
Copy link

jsf-clabot commented Sep 26, 2019

CLA assistant check
All committers have signed the CLA.

@cjbarth cjbarth force-pushed the master branch 2 times, most recently from 0f225b4 to ba623b3 Compare September 26, 2019 14:56
@cjbarth cjbarth changed the title Update dependencies; fix tests; Update dependencies (fix npm audit failures); fix tests; update Travis build Sep 26, 2019
@cjbarth
Copy link
Contributor Author

cjbarth commented Sep 27, 2019

@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
Copy link
Member

Well, the PR has unrelated changes in it.

Also, 4.3.3 might not be the latest handlebars-lang/handlebars.js#1563 (comment)

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

@cjbarth
Copy link
Contributor Author

cjbarth commented Sep 27, 2019

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
Copy link
Contributor Author

cjbarth commented Sep 27, 2019

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
Copy link
Member

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

@cjbarth
Copy link
Contributor Author

cjbarth commented Sep 27, 2019

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
Copy link
Member

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 gruntjs/grunt-contrib-internal#40 is solved. The handlebars update can still be applied with npm audit.

@cjbarth
Copy link
Contributor Author

cjbarth commented Sep 27, 2019

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
Copy link
Member

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

"chalk": "^1.0.0",
"handlebars": "^4.3.1",
Copy link
Member

Choose a reason for hiding this comment

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

This should be 4.3.3 at the time of writing

@XhmikosR XhmikosR mentioned this pull request Sep 27, 2019
@XhmikosR
Copy link
Member

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.

@cjbarth cjbarth closed this Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants