lirantal / lockfile-lint

Lint an npm or yarn lockfile to analyze and detect security issues
Apache License 2.0
780 stars 35 forks source link

Error could be more descriptive #115

Closed vipulgupta2048 closed 1 year ago

vipulgupta2048 commented 2 years ago

Is your feature request related to a problem? Please describe.

Using this command lockfile-lint --type npm --allowed-hosts npm github.com --allowed-schemes 'https:' 'git+https:' resulted in error

Usage: lockfile-lint --path <path-to-lockfile> --allowed-hosts yarn npm

Options:
      --version          Show version number                           [boolean]
  -h, --help             Show help                                     [boolean]
  -p, --path             path to the lockfile                [string] [required]
  -t, --type             lockfile type, options are "npm" or "yarn"     [string]
  -s, --validate-https   validates the use of HTTPS as protocol schema for all
                         resources                                     [boolean]
  -e, --empty-hostname   allows empty hostnames, or set to false if you wish for
                         a stricter policy             [boolean] [default: true]
  -a, --allowed-hosts    validates a whitelist of allowed hosts to be used for
                         resources in the lockfile                       [array]
  -o, --allowed-schemes  validates a whitelist of allowed schemes to be used for
                         resources in the lockfile                       [array]
  -u, --allowed-urls     validates a whitelist of allowed URLs to be used for
                         resources in the lockfile                       [array]

Examples:
  lockfile-lint --path yarn.lock --validate-https
  lockfile-lint --path yarn.lock --validate-https --allowed-hosts npm yarn
  verdaccio
  lockfile-lint --path yarn.lock --allowed-schemes "https:" "git+ssh:"
  --allowed-hosts npm yarn verdaccio

curated by Liran Tal at https://github.com/lirantal/lockfile-lint

Missing required argument: p

Describe the solution you'd like

  1. The error should be the first thing the users should see. Getting this multiple times resulted in confusion over what exactly is the error.
  2. The error's intention is a bit shaky. Reading about it in the tests,

https://github.com/lirantal/lockfile-lint/blob/f1ae11ef18b1229360d7bb835cc1d1c931087211/packages/lockfile-lint/__tests__/config.test.js#L39-L45

I think what can be improved is the message which can be: This error is shown when the required parameter (path) is missing. I am already providing parameters, it's just the path parameter that is missing.

  1. I think instead of just mentioning

Missing required argument: p, how about we show Missing required argument: path

Mentioning just p probably was confusing.

I am happy to help create a PR but I can't find where the error for this is thrown. Let me know what you feel about this issue and I can create a PR. Thanks!

lirantal commented 2 years ago

Hi @vipulgupta2048, thank you for raising this issue. I agree that the issue can be more descriptive. Maybe we even style the error message in red color or such?

Let me look into what sort of fix should be applied to help you create a Pull Request to fix this.

lirantal commented 2 years ago

Ok so if we switch the order of the arguments definition so that the long form text becomes the key of the object and the alias becomes the short, then when yargs (the command line args parser lib) will show the long form.

The change required is kind of like this:

diff --git a/packages/lockfile-lint/src/config.js b/packages/lockfile-lint/src/config.js
index fe619d4..8f43f2d 100644
--- a/packages/lockfile-lint/src/config.js
+++ b/packages/lockfile-lint/src/config.js
@@ -29,8 +29,8 @@ module.exports = (argv, exitProcess = false, searchFrom = process.cwd()) => {
     .help('help')
     .alias('help', 'h')
     .options({
-      p: {
-        alias: ['path'],
+      'path': {
+        alias: ['p'],
         type: 'string',
         describe: 'path to the lockfile',
         demandOption: true

Do you want to submit a pull request that fixes this? don't forget to update the tests too.