search

lirantal / lockfile-lint

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

Adds more user friendly error messages in ParseLockfile #22

Closed emimuresan closed 5 years ago

emimuresan commented 5 years ago

Closes: #16

Description

Added enhanced and more informative error messages in ParseLockfile.js. The following error types are supported:

NO_OPTIONS:

Programatically generated the scenario below, this isn't something that would happen normally:

$ lockfile-lint -p __tests__/fixtures/yarn-only-http.lock --allowed-hosts yarn --validate-https
ABORTING lockfile lint process due to error exceptions

Did not recive options for lockfile path or type.

Error at new ParsingError (stack trace...)

error: command failed with exit code 1

NO_PARSER_FOR_TYPE:

$ lockfile-lint -p __tests__/fixtures/yarn-only-http.lock --allowed-hosts yarn --validate-https --type pnpm
ABORTING lockfile lint process due to error exceptions

Unable to find relevant lockfile parser for type "pnpm", the currently available options are npm,yarn.

Error at new ParsingError (stack trace...)

error: command failed with exit code 1

NO_PARSER_FOR_PATH:

$ lockfile-lint -p __tests__/fixtures/yarn-only-http.lock --allowed-hosts yarn --validate-https
ABORTING lockfile lint process due to error exceptions

Unable to find relevant lockfile parser for "__tests__/fixtures/yarn-only-http.lock", consider passing the --type option.

Error at new ParsingError (stack trace...)

error: command failed with exit code 1

READ_FAILED:

$ lockfile-lint -p __tests__/fixtures/non-existing-yarn.lock --allowed-hosts yarn --validate-https --type yarn
ABORTING lockfile lint process due to error exceptions

Unable to read lockfile "__tests__/fixtures/non-existing-yarn.lock"

Error: ENOENT: no such file or directory, open '__tests__/fixtures/non-existing-yarn.lock'
    at Object.openSync (fs.js:443:3)
    at Object.readFileSync (fs.js:343:35) (stack trace...)

error: command failed with exit code 1

PARSE_NPMLOCKFILE_FAILED:

$ lockfile-lint -p __tests__/fixtures/bad-package-lock.json --allowed-hosts yarn --validate-https --type npm
ABORTING lockfile lint process due to error exceptions

Unable to parse npm lockfile "__tests__/fixtures/bad-package-lock.json"

SyntaxError: Unexpected token m in JSON at position 23 (stack trace...)

error: command failed with exit code 1

PARSE_YARNLOCKFILE_FAILED

$ lockfile-lint -p __tests__/fixtures/bad-yarn.lock --allowed-hosts yarn --validate-https --type yarn
ABORTING lockfile lint process due to error exceptions

Unable to parse yarn lockfile "__tests__/fixtures/bad-yarn.lock"

SyntaxError: Unknown token: { line: 1, col: 0, type: 'INVALID', value: undefined } 1:0 in lockfile (stack trace...)

error: command failed with exit code 1

More Details

These error types are defined in packages/lockfile-lint-api/src/common/ParsingError.js and are used in packages/lockfile-lint-api/src/ParseLockfile.js.

Types of changes

Related Issue

16

Motivation and Context

More descriptive/informative error messages when parsing a lockfile fails.

How Has This Been Tested?

Added/Updated unit tests for all scenarios.

Checklist:

giphy-cat-type

emimuresan commented 5 years ago

Not sure why the tests in TravisCI are failing with:

Expected substring: "expecting options object"
lockfile-lint-api:     Received message:   "Did not recive options for lockfile path or type"

I modified parseLockfile.test, so instead of "expecting options object" it actually should be "Did not recive options for lockfile path or type", and that is what it asserts on in the test file.

lirantal commented 5 years ago

@emimuresan I'm unable to reproduce the build failure locally either and I also tried resetting the build cache which didn't help. What about if you just open a new PR with the exact same code? I think you can just use a new branch to trigger a new PR

lirantal commented 5 years ago

@emimuresan maybe you can rebase with master? checking out locally I see that I'm finding the test file being unstaged. This is probably why it's not getting through to the CI.

codecov-io commented 5 years ago

Codecov Report

Merging #22 into master will increase coverage by 1.11%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
+ Coverage    96.1%   97.22%   +1.11%     
==========================================
  Files           9       11       +2     
  Lines         154      180      +26     
  Branches       21       25       +4     
==========================================
+ Hits          148      175      +27     
+ Misses          6        5       -1
Impacted Files Coverage Δ
...kages/lockfile-lint-api/src/common/PackageError.js 100% <ø> (ø) :arrow_up:
...s/lockfile-lint-api/src/validators/ValidateHost.js 100% <100%> (ø) :arrow_up:
...kages/lockfile-lint-api/src/common/ParsingError.js 100% <100%> (ø)
packages/lockfile-lint-api/src/ParseLockfile.js 100% <100%> (+2.63%) :arrow_up:
packages/lockfile-lint-api/src/common/constants.js 100% <100%> (ø)

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 ff32753...8e9d43c. Read the comment docs.

emimuresan commented 5 years ago

@emimuresan maybe you can rebase with master? checking out locally I see that I'm finding the test file being unstaged. This is probably why it's not getting through to the CI.

Yep, it was strange. Upon further investigation i discovered that there were 2 duplicate test files for ParseLockfile, i checked on master and it was the same:

parseLockFile.test.js
parseLockfile.test.js

I deleted parseLockFile.test.js in this PR. Hopefully it should be ok in CI now.

lirantal commented 5 years ago

Great work here @emimuresan ✨, thank you ❤️