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

Move from optimist to yargs to address security vulnerabilities #63

Conversation

bunnymatic
Copy link

@bunnymatic bunnymatic commented Jun 15, 2020

Problem

sifter depends on optimist which depends on an old version of
minimist which has a security vulnerability
(https://snyk.io/vuln/SNYK-JS-MINIMIST-559764).

Additionally, optimist the package is no longer supported. The
author suggests just using minimist directly. After some
investigation, it looks like yargs is basically a drop in replacement
for optimist.

Solution

Replace optimist with yargs. This removes the vulnerabilty
and requires almost no code changes.

Demo after the move to yargs

$ bin/sifter.js --help
Usage: sifter.js --query="search query" --fields=a,b

Options:
  --help       Show help                                               [boolean]
  --version    Show version number                                     [boolean]
  --fields     Search fields (comma separated)                     [default: ""]
  --query      Search query                                        [default: ""]
  --sort       Sort field                                          [default: ""]
  --direction  Sort direction                                   [default: "asc"]
  --file       CSV or JSON dataset

$ bin/sifter.js --version
0.6.0

vulnerabilities

Problem
-------

`sifter` depends on `optimist` which depends on an old version of
`minimist` which has a security vulnerability
(https://snyk.io/vuln/SNYK-JS-MINIMIST-559764).

Additionally, `optimist` the package is no longer supported.  The
author suggests just using `minimist` directly.  After some
investigation, it looks like `yargs` is basically a drop in replacement
for `optimist`.

Solution
--------

Replace `optimist` with `yargs`.  This removes the vulnerabilty
and requires almost no code changes.

```bash
$ bin/sifter.js --help
Usage: sifter.js --query="search query" --fields=a,b

Options:
  --help       Show help                                               [boolean]
  --version    Show version number                                     [boolean]
  --fields     Search fields (comma separated)                     [default: ""]
  --query      Search query                                        [default: ""]
  --sort       Sort field                                          [default: ""]
  --direction  Sort direction                                   [default: "asc"]
  --file       CSV or JSON dataset

$ bin/sifter.js --version
0.6.0
```
@bunnymatic
Copy link
Author

This should address #62.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.467% when pulling 23ca2a4 on rcode5:chore/move-from-optimist-to-yargs-for-security into 97270b4 on brianreavis:master.

@bytestream
Copy link

I think the only thing to point out is the Node.js support difference between optimist 0.6.1 and yargs 15.

@jangya
Copy link

jangya commented Oct 21, 2020

Any idea when this will be merged. @Yanchek99 @brianreavis I wonder why it's left open since long days..

@bytestream
Copy link

Probably never, all Brian's work should be archived as it's unmaintained and has been for a number of years

@bunnymatic
Copy link
Author

closing for inactivity

@bunnymatic bunnymatic closed this Mar 5, 2023
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

5 participants