lockfile-lint does not lint the entire tree for package-lock.json

JamesSingleton commented 4 years ago

When running "test:lockfile": "lockfile-lint -p package-lock.json -t npm -a npm -o https: -c -i", it does not check past the first resolved URL for a dependency.

Expected Behavior

lockfile-lint should check the entire tree.

Current Behavior

If we take the below snippet from a package-lock.json and change the semver resolved URL to http or a URL other than registry.npmjs.org, lockfile-lint will not catch this.

"@babel/core": {
  "version": "7.7.5",
  "resolved": "https://registry.npmjs.org/@babel/core/-/core-7.7.5.tgz",
  "integrity": "sha512-M42+ScN4+1S9iB6f+TL7QBpoQETxbclx+KNoKJABghnKYE+fMzSGqst0BZJc8CpI625bwPwYgUyRvxZ+0mZzpw==",
  "dev": true,
  "requires": {
    "@babel/code-frame": "^7.5.5",
    "@babel/generator": "^7.7.4",
    "@babel/helpers": "^7.7.4",
    "@babel/parser": "^7.7.5",
    "@babel/template": "^7.7.4",
    "@babel/traverse": "^7.7.4",
    "@babel/types": "^7.7.4",
    "convert-source-map": "^1.7.0",
    "debug": "^4.1.0",
    "json5": "^2.1.0",
    "lodash": "^4.17.13",
    "resolve": "^1.3.2",
    "semver": "^5.4.1",
    "source-map": "^0.5.0"
  },
  "dependencies": {
    "semver": {
      "version": "5.7.1",
      "resolved": "https://registry.npmjs.org/semver/-/semver-5.7.1.tgz",
      "integrity": "sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ==",
      "dev": true
    },
    "source-map": {
      "version": "0.5.7",
      "resolved": "https://registry.npmjs.org/source-map/-/source-map-0.5.7.tgz",
      "integrity": "sha1-igOdLRAh0i0eoUyA2OpGi6LvP8w=",
      "dev": true
    }
  }
},

Possible Solution

At the moment, my current possible solution is to find and loop over all the resolved URLs.

Steps to Reproduce (for bugs)

If you don't have a project with lockfile-lint in it already, create one and install lockfile-lint
Add a script that uses lockfile-lint (e.x. "test:lockfile": "lockfile-lint -p package-lock.json -t npm -a npm -o https: -c -i")
Go into the package-lock.json and change the resolved URL to a dependency that is under "dependencies"
Run lockfile-lint

Context

I cannot fully lint my entire package-lock.json file due to this.

Your Environment

Library Version used: ^3.0.8
Node.js version (e.g. Node.js 5.4): v10.13.0
Operating System and version (desktop or mobile): macOS Mojave v10.14.5

JamesSingleton commented 4 years ago

It is because it takes the last dependency and version and adds it to the flattened lockfile. If we take something like

"ansi-regex": {
  "version": "2.1.1",
  "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-2.1.1.tgz",
  "integrity": "sha1-w7M6te42DYbg5ijwRorn7yfWVN8=",
  "dev": true
},

This dependency is used in a lot of packages, if I change the first one to be malicious but the rest are fine, then lockfile-lint will miss it.

lirantal commented 4 years ago

@JamesSingleton can you perhaps provide a more complete lock-file with the semver or ansi-regex problem? The problem isn't that lockfile-lint doesn't lints the dependencies but rather that it perhaps builds them incorrectly and then misses something.

lirantal commented 4 years ago

Alrighty, I'm able to reproduce. Thanks for the heads up on this 👌

lirantal commented 4 years ago

@JamesSingleton if you wanted to take a stab at fixing it and providing a PR I'm happy to guide you through on the change that I had in mind. It's not a very complicated one but the refactor will touch a few places.

In specific I was thinking of:

parse functions in ParseLockfile.js will return a flat array list instead of an object
all validators and tests will accept the list and will iterate that, instead of an object
updating the constructor signature for the validators to use packagesList instead of packages which hints an array list, and opens the possibility in the future to provide a hierarchy structure or otherwise.

JamesSingleton commented 4 years ago

@lirantal yeah, I was trying to keep everything an object so there wouldn't have to be much refactoring of the validators. However, as I dug more into the code I don't think that would be possible.

lirantal commented 4 years ago

Right. The problem with keeping it as an object is that the same package/version would be equal (which is what happening today) so it would be the same entry and not a different one. I think converting to an array would be ok.

Are you gonna send off a PR for this? (no rush)

JamesSingleton commented 4 years ago

This is what I was thinking of:

_flattenNpmDepsTree (npmDepsTree, npmDepMap = {}) {
    for (const [depName, depMetadata] of Object.entries(npmDepsTree)) {
      const depMetadataShortend = {
        version: depMetadata.version,
        resolved: depMetadata.resolved ? depMetadata.resolved : depMetadata.version,
        integrity: depMetadata.integrity,
        requires: depMetadata.requires
      }
      const hashedDepValues = hash(depMetadataShortend)

      npmDepMap[`${depName}@${depMetadata.version}-${hashedDepValues}`] = depMetadataShortend

      const nestedDepsTree = depMetadata.dependencies

      if (nestedDepsTree && Object.keys(nestedDepsTree).length !== 0) {
        this._flattenNpmDepsTree(nestedDepsTree, npmDepMap)
      }
    }

    return npmDepMap
  }

However, I now run into another issue Encountered error Invalid URL: 1.1.1 in package: "abbrev@1.1.1-8f0b92d6fc7847f731964a4f3ab3b7c1e53edee5"

This is because the dependency looks like this:

"fsevents": {
  "version": "1.2.11",
  "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-1.2.11.tgz",
  "integrity": "sha512-+ux3lx6peh0BpvY0JebGyZoiR4D+oYzdPZMKJwkZ+sFkNJzpL7tXc/wehS49gUAxg3tmMHPHZkA8JU2rhhgDHw==",
  "dev": true,
  "optional": true,
  "requires": {
    "bindings": "^1.5.0",
    "nan": "^2.12.1",
    "node-pre-gyp": "*"
  },
  "dependencies": {
    "abbrev": {
      "version": "1.1.1",
      "bundled": true,
      "dev": true,
      "optional": true
    },
    "ansi-regex": {
      "version": "2.1.1",
      "bundled": true,
      "dev": true,
      "optional": true
    },
  }
}

I shortened it as this one is roughly 500 lines long.

The error points to this line

let packageResolvedURL = {}
      try {
        packageResolvedURL = new URL(packageMetadata.resolved)
      } catch (error) {
        throw new PackageError(packageName, error)
      }

Not sure how this is now just coming up now though. Looks like this will happen with any dependency that does not have a true URL for resolved since resolved is optional and doing new URL("1.1.1") is not a valid URL.

JamesSingleton commented 4 years ago

@lirantal, let me know your thoughts on that solution and if you can think of a decent solution for the new URL() issue as I noticed you do resolved: depMetadata.resolved ? depMetadata.resolved : depMetadata.version to probably catch if someone npm install a .tgz file.

JamesSingleton commented 4 years ago

So I have it working minus the above issue. Also, there are tests like

it('validator should throw a descriptive error when one is encounterd in a package', () => {
    const mockedPackages = {
      '@babel/code-frame': {
        resolved: 'debug-4.1.1.tgz#3b72260255109c6b589cee050f1d516139664791'
      }
    }
    const validator = new ValidatorHost({packages: mockedPackages})

    expect(() => validator.validate(['npm'])).toThrow(PackageError)
  })

That yes this throws, but it would throw TypeError [ERR_INVALID_URL]: Invalid URL: debug-4.1.1.tgz#3b72260255109c6b589cee050f1d516139664791 because new URL('debug-4.1.1.tgz#3b72260255109c6b589cee050f1d516139664791'') is not a valid URL.

The issue with throwing here is that the entire process stops for something that is potentially valid.

One solution I was thinking of was

if (
        packageMetadata.resolved.startsWith('file') ||
        packageMetadata.resolved.startsWith('http') ||
        packageMetadata.resolved.startsWith('git+ssh')
      ) {
        try {
          packageResolvedURL = new URL(packageMetadata.resolved)
        } catch (error) {
          throw new PackageError(packageName, error)
        }
      }

But not sure about the tests.

JamesSingleton commented 4 years ago

If you want to see the changes I had in mind, I have pushed them here: https://github.com/JamesSingleton/lockfile-lint/commits/bugfix/dependency-depth

JamesSingleton commented 4 years ago

@lirantal, when do you think this fix will be published?

lirantal / lockfile-lint