lirantal / lockfile-lint

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

Bug: CLI Argument parsing typos invokes other validates #145

Closed wingy3181 closed 1 year ago

wingy3181 commented 2 years ago

Expected Behavior

Not to validate the integrity by default via CLI command e.g.)

npx lockfile-lint --validate-https -validate-package-names --validate-integrity --path yarn.lock

Current Behavior

npx lockfile-lint --validate-https -validate-package-names --validate-integrity false --path yarn.lock

Always seems to validate the integrity is sha512

Possible Solution

Steps to Reproduce (for bugs)

  1. Run npx lockfile-lint --validate-https -validate-package-names --validate-integrity --path yarn.lock with a lockfile that is using sha1 integrity image

Have also tried passing false as an aargument to --validate-integrity which also did not work

Context

Your Environment

wingy3181 commented 2 years ago

Ok so just an update......

I realised i'm missing an extra hyphen for validate-package-names

So if I remove that, then it does not validate the integrity

unfortunately cannot test it with the validate package names as I am using artifactory and face the following issue (https://github.com/lirantal/lockfile-lint/issues/112) which i needed to turn off.........

Just interesting the typo still forced it to validate integrity unless its a subset of validate package names.........or an issue with yargs

Anyway...just to add a bit more information

lirantal commented 2 years ago

@yoavain want to look into it if there's something specifically to do to improve that?

@wingy3181 alright, well, sounds like if you don't have a typo then this isn' really an outstanding issue though, right?

yoavain commented 2 years ago

Reproduced, so I'm assuming it's something I missed when renaming the flag. Will check the code...

yoavain commented 2 years ago

I think this is a general issue. I believe the bug is in the following logic:

if (supportedValidators.has(commandArgument)) {
    const validatorItem = supportedValidators.get(commandArgument)
    validators.push({
      name: validatorItem,
      values: commandValue,
      options: {
        emptyHostname: config['empty-hostname'],
        allowedHosts: config['allowed-hosts'],
        allowedUrls: config['allowed-urls']
      }
    })
  }

The first line means that if a boolean parameter exists, the validator is added, regardless of the value true/false. (The commandValue is passed as values to the validator, but the validator will be executed)

If I have time, I'll try to open a PR during the weekend

yoavain commented 2 years ago

Also, regarding the original question, removing the --validate-integrity should work. There seems to be a typo in the command above, Should be a double - in the --validate-package-names.

yoavain commented 2 years ago

@lirantal You can assign this to me, but please consider renaming the title to clear my name 🤨

lirantal commented 1 year ago

Thanks @yoavain Made the updates!

wingy3181 commented 1 year ago

Also, regarding the original question, removing the --validate-integrity should work. There seems to be a typo in the command above, Should be a double - in the --validate-package-names.

yep there was a typo which i found a bit later......... thanks @yoavain !

wingy3181 commented 1 year ago

@yoavain want to look into it if there's something specifically to do to improve that?

@wingy3181 alright, well, sounds like if you don't have a typo then this isn' really an outstanding issue though, right?

yep correct...well kind of...... obviously there was a typo with missing - and i needed to turn that off anyway because of the use of artifactory where i am at....... but as @yoavain found there was still an issue with the fact it was validating the integrity with the typo which was a bit weird...

anyway thanks to you both for looking at it...

lirantal commented 1 year ago

Closing for merged PR. @wingy3181 feel free to comment with any more background if this still needs more attention.