Skip to content

Fix: update minimist version#213

Closed
vileppanen wants to merge 2 commits intostandard:masterfrom
vileppanen:fix/update_minimist
Closed

Fix: update minimist version#213
vileppanen wants to merge 2 commits intostandard:masterfrom
vileppanen:fix/update_minimist

Conversation

@vileppanen
Copy link
Copy Markdown

@vileppanen vileppanen commented Mar 17, 2020

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[ ] New feature
[ X ] Other, please explain:

There's a reported prototype pollution vulnerability in minimist, versions under 1.2.2 https://app.snyk.io/vuln/SNYK-JS-MINIMIST-559764

What changes did you make? (Give an overview)

  • fixed failing tests
  • updated minimist version to ^1.2.2

Which issue (if any) does this pull request address?

This might resolve this autogenerated issue #211
The build seems to fail due to the failing tests, which should be fixed in this PR

Is there anything you'd like reviewers to focus on?

Relied on running tests, and dummy local cli setup to smoke test this. No failing tests, nor crashing cli, therefore assumed, the update didn't break anything. If someone has more detailed insight on this, whether I overlooked something, would gladly hear 👍

Ville Leppänen added 2 commits March 17, 2020 20:38
Version below 1.2.2 vulnerable to prototype pollution
@vileppanen
Copy link
Copy Markdown
Author

Ping @feross, any chance to look into this? It seems the greenkeeper was already trying to update the minimist version, but failed due to those failing tests, which should be corrected in this one.

Copy link
Copy Markdown
Contributor

@toddbluhm toddbluhm left a comment

Choose a reason for hiding this comment

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

This looks good to me. Hopefully, I am not overstepping my bounds, but @feross is very busy so approving a simple test case fix to get CI working again hopefully will not be an issue. I also reviewed the minimist code changes and saw nothing that would break our usage. @LinusU you want to take a look at this too?

I was concerned about the const usage, but when I npm installed this locally using a node v4 environment everything broke anyways as many of the dependencies already require node >=8.10.
The current travis CI setup just uses the latest node LTS build (currently v12) anyways.

@LinusU
Copy link
Copy Markdown
Member

LinusU commented Apr 30, 2020

Hi @vileppanen, very sorry for the delay here. It seems like we arrived in the same fix for the tests, I didn't see this when I fixed it in #219 so sorry for not cherry-picking your commit! 🙈 ❤️

I did a large PR which bumps all dependencies and a bunch of other cleanup here: #220

Again, sorry for just making the changes myself, I was doing all the other little changes and didn't want the package-lock committed since it won't be used when this package is installed by someone else, so I figured it was easier to just roll it in to everything else.

Thank you for opening the PR! I promise to be quicker to respond next time 😉

@LinusU LinusU closed this Apr 30, 2020
@vileppanen
Copy link
Copy Markdown
Author

Hi @vileppanen, very sorry for the delay here. It seems like we arrived in the same fix for the tests, I didn't see this when I fixed it in #219 so sorry for not cherry-picking your commit! 🙈 ❤️

I did a large PR which bumps all dependencies and a bunch of other cleanup here: #220

Again, sorry for just making the changes myself, I was doing all the other little changes and didn't want the package-lock committed since it won't be used when this package is installed by someone else, so I figured it was easier to just roll it in to everything else.

Thank you for opening the PR! I promise to be quicker to respond next time 😉

No worries 👍, main goal achieved anyways 😄

@feross
Copy link
Copy Markdown
Member

feross commented Oct 27, 2020

Just seeing this now. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants