lockfile-lint icon indicating copy to clipboard operation
lockfile-lint copied to clipboard

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

Open bozdoz opened this issue 1 year ago • 2 comments

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

bozdoz avatar Sep 17 '22 03:09 bozdoz

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

lirantal avatar Sep 17 '22 21:09 lirantal

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 [email protected], so maybe we can just ignore that it seems to happen on older npm versions.

bozdoz avatar Sep 17 '22 22:09 bozdoz

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 ❤️

lirantal avatar Sep 26 '22 17:09 lirantal