lirantal / lockfile-lint

Lint an npm or yarn lockfile to analyze and detect security issues

Apache License 2.0

781 stars 35 forks source link

fix(cli): conflicting arguments will error #64

Closed lirantal closed 4 years ago

lirantal commented 4 years ago

Description

When incompatible CLI arguments are provided they shouldn't just be ignored and lead to failure but rather a proper message should be provided to users.

Types of changes

Detecting the conflicting use of both --allowed-schemes and --validate-https and error'ing out with specifying that only one of those should be used.

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Related Issue

Fixes #63

Motivation and Context

Provides a better CLI experience for users.

How Has This Been Tested?

Added a CLI test.

Screenshots (if appropriate):

Checklist:

[ ] I have updated the documentation (if required).
[x] I have read the CONTRIBUTING document.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.
[ ] I added a picture of a cute animal cause it's fun

codecov-io commented 4 years ago

Codecov Report

Merging #64 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #64   +/-   ##
=======================================
  Coverage   95.08%   95.08%           
=======================================
  Files          11       11           
  Lines         183      183           
  Branches       29       29           
=======================================
  Hits          174      174           
  Misses          8        8           
  Partials        1        1

Impacted Files	Coverage Δ
packages/lockfile-lint/src/cli.js	`0% <ø> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1e76c88...53d57ba. Read the comment docs.

XhmikosR commented 4 years ago

BTW @lirantal maybe this is considered a breaking change for some :)

That being said, the error message isn't 100% correct IMO.

I was using this command:

lockfile-lint --allowed-hosts npm --allowed-schemes https: --empty-hostname false --validate-https --type npm --path package-lock.json

and the error message I got was:

Arguments o and validate-https are mutually exclusive

But I didn't use the -o flag, I used --allowed-schemes

lirantal commented 4 years ago

@XhmikosR correct about the possible breaking-change so I'm reverting #65 and will merge as a breaking change.

about your case: -o and --allowed-schemes are the same options, it's the shortname and the alias for it:

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

Options:
  --version              Show version number                           [boolean]
  --help, -h             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]

XhmikosR commented 4 years ago

I know but it's a little confusing since I didn't pass -o :)

lirantal commented 4 years ago

Totally understood :-) Will tweak out yargs as much as I can :)