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(api): lint all dependencies in package-lock #53

Closed JamesSingleton closed 4 years ago

JamesSingleton commented 4 years ago

Will not override in the object if there is a matching dep and version and will lint all the dependencies of dependencies.

Fixes #49

Description

Types of changes

Related Issue

https://github.com/lirantal/lockfile-lint/issues/49

Motivation and Context

It solved the issue where if multiple dependencies had the same subdependencies (name and version) only one was added to the object. This was an issue if the first one was corrupt but the last one was fine, it would not be caught.

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

JamesSingleton commented 4 years ago

One thing I want to mention is this now will not longer catch something like

 "lockfile-lint": {
      "version": "file:../lockfile-lint/packages/lockfile-lint/lockfile-lint-3.0.8.tgz",
      "integrity": "sha512-rS+Xbf31aGpj/vXAokFNjCobPEWT0IkeGpWY6ZwPdtlW80WAF13gZfkAIvxWvbl4yORhFvCXDOmSWzHCnBT78Q==",
      "dev": true,
      "requires": {
        "debug": "^4.1.1",
        "lockfile-lint-api": "file:../lockfile-lint/packages/lockfile-lint-api/lockfile-lint-api-5.0.7.tgz",
        "yargs": "^15.0.2"
      },

However, this I believe is a different issue all together and am willing to open an issue in regards to this.

codecov-io commented 4 years ago

Codecov Report

Merging #53 into master will decrease coverage by 2.8%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   97.88%   95.08%   -2.81%     
==========================================
  Files          11       11              
  Lines         189      183       -6     
  Branches       29       29              
==========================================
- Hits          185      174      -11     
- Misses          4        8       +4     
- Partials        0        1       +1
Impacted Files Coverage Δ
.../lockfile-lint-api/src/validators/ValidateHttps.js 100% <ø> (ø) :arrow_up:
...s/lockfile-lint-api/src/validators/ValidateHost.js 100% <ø> (ø) :arrow_up:
...lockfile-lint-api/src/validators/ValidateScheme.js 100% <100%> (ø) :arrow_up:
packages/lockfile-lint-api/src/ParseLockfile.js 100% <100%> (ø) :arrow_up:
...kages/lockfile-lint-api/src/common/PackageError.js 0% <0%> (-100%) :arrow_down:
packages/lockfile-lint/src/cli.js 0% <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 c7671ac...989d9a2. Read the comment docs.

lirantal commented 4 years ago

@JamesSingleton can you elaborate your last comment about why it would not catch that kind of structure and what exactly will be missed?

lirantal commented 4 years ago

@JamesSingleton I see you've taken the approach of keeping the object structure but with a modification for it being a hash. Did you find this solution better than just changing it to an array structure?

lirantal commented 4 years ago

You've also referred to an issue with one of the tests in the last comment in the issue, here: https://github.com/lirantal/lockfile-lint/issues/49#issuecomment-579380305

Is this PR solving that or is that still something we need to fix?

JamesSingleton commented 4 years ago

@JamesSingleton I see you've taken the approach of keeping the object structure but with a modification for it being a hash. Did you find this solution better than just changing it to an array structure?

@lirantal, I felt that it was a better solution in my opinion.

lirantal commented 4 years ago

@JamesSingleton in any specific way? I mean, an object is nice because it's a dictionary structure that you can easily get the package if you know it's name/version but with the hash that's not possible anymore. I guess I could argue that it is much of a breaking change as just using arrays.

JamesSingleton commented 4 years ago

I keep the name and version along with the hash. The hash was just to make it unique and not have to redo all the validators.

lirantal commented 4 years ago

@JamesSingleton alright. I'm not entirely against doing as you proposed either but can assume that in the long run we will probably move to an array structure.

I'm ok to move forward with this

JamesSingleton commented 4 years ago

@lirantal can we move forward with merging this?

Francois-Esquire commented 4 years ago

Thank you @lirantal for taking time to work through and merge the PR! Question for you, when can we expect a release?

lirantal commented 4 years ago

New versions released 🎉

lirantal commented 4 years ago

thanks @JamesSingleton for your contribution on this 💜