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

feat(lockfile-lint-api): adds validation for resolved fields (#120) #136

Closed bozdoz closed 1 year ago

bozdoz commented 1 year ago

Description

Adds a validator for when resolved fields are missing in lockfiles.

Types of changes

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

Related Issue

120

Motivation and Context

If a resolved field doesn't exist, npm may have to fetch a packument, which is a large file, and can cause timeouts in CI/CD.

How Has This Been Tested?

I ran yarn test from the root, copied the existing test suite from validate package names, and got 100% coverage

Checklist:

[x] 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

lirantal commented 1 year ago

Thanks boz. So effectively you're looking at using this flag to find cases in the lockfile for which npm would spend more time during install processes if I understood the issue correctly? If so, I'm a bit hesitant to add this as something we check for because this seems like an issue with npm side of things. If they fix it in the next minor version and you'd be using lockfile-lint, then we'd effectively show you an error for it but it would actually be a false positive. In the first place, this should only be missing for bundled dependencies according to the docs here: https://docs.npmjs.com/cli/v6/configuring-npm/package-lock-json

bozdoz commented 1 year ago

Fair enough. It does seem to be an issue, but perhaps npm itself should be resolving this issue. Quickly testing this, I can't actually reproduce this with npm@8.15.0, so maybe we can just ignore that it seems to happen on older npm versions.

lirantal commented 1 year ago

To put it a different way, if there is no resolved field, does that open the door for security issues? if so, then there's room for it. Otherwise, it's more of a functional issue at npm's side of things.

So I'll close for now, but happy to re-open and land if we can show there's a potential security hole with having no resolved URL field. Thanks a bunch @bozdoz, I appreciate the effort put into this PR regardless ❤️