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

Upgrade argparse dependency #337

Closed
wants to merge 1 commit into from
Closed

Upgrade argparse dependency #337

wants to merge 1 commit into from

Conversation

stefanobaghino-da
Copy link

Running npm audit reveals that this package is affected by a security
vulnerability that is described in the following advisory:

Upgrading the dependency to a new version removes the vulnerability for
this package and transitively for all its dependees. I'm addressing this
as part of an effort to get rid of security advisories as advertised by
npm audit from https://github.com/digital-asset/daml-js. I believe the
security risk is very low, but I hoped getting this low hanging fruit
could have been a worthy contribution.

Running npm test seems to reveal no regression despite the potentially
breaking changes introduced by a new major release of the upgraded
dependency.

This contribution also introduces a lockfile. You can read more details
at the following link:

Running `npm audit` reveals that this package is affected by a security
vulnerability that is described in the following advisory:

* https://nodesecurity.io/advisories/745

Upgrading the dependency to a new version removes the vulnerability for
this package and transitively for all its dependees. I'm addressing this
as part of an effort to get rid of security advisories as advertised by
`npm audit` from https://github.com/digital-asset/daml-js. I believe the
security risk is very low, but I hoped getting this low hanging fruit
could have been a worthy contribution.

Running `npm test` seems to reveal no regression despite the potentially
breaking changes introduced by a new major release of the upgraded
dependency.

This contribution also introduces a lockfile. You can read more details
at the following link:

* https://docs.npmjs.com/files/package-locks
@stefanobaghino-da
Copy link
Author

How reliable is the CI? eslint seems to be missing but if that's the case I would not understand how this could ever possibly work. I've run the tests locally installing eslint in the global scope because it was missing in the package manifest and expected the CI to follow the same process.

@DannyDainton
Copy link

Hey @jonschlinkert,

Would it be possible to take a look at this PR and some of the other security-related ones, please?

Thank you for creating this resource but it's starting to flag up more issues now and I would love to see them gone.

Cheers.

@stefanobaghino-da
Copy link
Author

I took some time to go through the recent PRs and opened issues. #312 does the same as this PR. Apologies for opening a duplicate PR. I will close this.

Also related to this issue, #327 would fix the reported advisory by replacing argparse with minimist. This approach has been discussed in #321.

@stefanobaghino-da stefanobaghino-da deleted the upgrade-argparse-dependency branch June 10, 2019 08:04
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.

2 participants