Check version is actually a proper semver version

[JamesSingleton]

This is in regards to my comment here: https://github.com/lirantal/lockfile-lint/pull/53#issuecomment-579500899

We should check that the version matches a semver version instead of something like file:../lockfile-lint/packages/lockfile-lint/lockfile-lint-3.0.8.tgz

Expected Behavior

lockfile-lint should check that the version is an actual semver version instead of something else.

Current Behavior

It is currently caught with the code in Master, however, with the changes that were required with https://github.com/lirantal/lockfile-lint/pull/53 it will not longer be caught. The reason that it is currently caught is because resolved: depMetadata.resolved ? depMetadata.resolved : depMetadata.version. However, this caused an issue now that we truly go through all dependencies as not all dependencies have a resolved and doing new URL('1.0.0') results in an error from url itself.

Possible Solution

Create a new validator that checks for proper semver version. This would call for a regex pattern but currently eslint-plugin-security's detect-unsafe-regex flags all regex patterns for semver including the ones provided on semver's website.

Steps to Reproduce (for bugs)

1. Check out the code in https://github.com/lirantal/lockfile-lint/pull/53
2. Make this line https://github.com/JamesSingleton/lockfile-lint/blob/bugfix/dependency-depth/packages/lockfile-lint-api/src/ParseLockfile.js#L132 resolved: depMetadata.resolved ? depMetadata.resolved : depMetadata.version
3. In the 3 validator files remove the if statement that surrounds the try...catch...

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

4. npm pack lockfile-lint-api
5. Install that tgx file into lockfile-lint
6. npm pack lockfile-lint
7. Install lockfile-lint into a repo
8. Run lockfile-lint

Context

This will make sure that no locally installed .tgz files in the package-lock.json make it in.

Your Environment

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

[lirantal]

James I'm still struggling with the basic problem case of

We should check that the version matches a semver version instead of something like file:../lockfile-lint/packages/lockfile-lint/lockfile-lint-3.0.8.tgz

where it seems that there are two issues you that are being discussed here:

1. The resolved field sometimes gets its information from the version field because there is no other resolved field that exists, and so the version fields points to the actual source. In your example it is a file location but it can also be git+ssh:// URL
2. The expectation of lockfile-lint should check that the version is an actual semver version instead of something else. in the description makes it sounds like you'd want to also run an additional check that the version field indeed includes a semver range. If so, I'm not convinced that ensuring this is the scope of lockfile-lint, especially when in cases of ad-hoc git URLs you will not really have a semver anyway.

[JamesSingleton]

So the current code tries this packageResolvedURL = new URL(packageMetadata.resolved) which currently is either an actual URL or potentially a file path if you installed a local tarball. However, now that we are looking to go through all of the dependencies, you run into an issue where there isn't a resolved and instead only a version. When that happens packageResolvedURL = new URL(packageMetadata.resolved) will throw a ERR_INVALID_URL TypeError.

For example, if you look at all the dependencies for fsevents you will see none of them have a resolved, only version. They look something like the following (obviously shortened)

"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
    },
    "aproba": {
      "version": "1.2.0",
      "bundled": true,
      "dev": true,
      "optional": true
    },
  }
}

If you keep resolved: depMetadata.resolved ? depMetadata.resolved : depMetadata.version, this would make resolved: "1.1.1" for abbrev and running that through that try catch block will throw and error and exit the process.

[lirantal]

@JamesSingleton got it. what about doing something like this as a solution:

1. resolved takes resolved if exists and if not then version (as originally it did)
2. we try to new URL() it and if it fails we catch the exception and ignore because then we assume that it was indeed an actual version and ignore the exception

would this work?

[lirantal]

@JamesSingleton how about we put a small PR to fix this issue first, based on the above proposed solution, or another if you care to suggest a better way. Land that. And then land #53 with the updated changes as proposed here? this way #53 can land without regression from previous way it worked.

[JamesSingleton]

I mean what you are asking is this correct?

let packageResolvedURL = {}
try {
  packageResolvedURL = new URL(packageMetadata.resolved)
} catch (error) {
// swallow the error
}

If so, I can just make those changes in my current PR. And then add

resolved: depMetadata.resolved ? depMetadata.resolved : depMetadata.version,

Back in.

[lirantal]

@JamesSingleton yes that's what I meant. that works too if you want to push it into the current PR.

[JamesSingleton]

Just an FYI, this will remove the tests that say validator should throw a descriptive error when one is encounterd in a package as it will no longer throw. Is that fine?

[lirantal]

@JamesSingleton correct because that's the only case covered by that test code. After your PR lands I'll think of a more elegant way of solving the resolved-version pair of fields so that we could perhaps maintain that URL check.

[JamesSingleton]

@lirantal it was taken care of in 989d9a2

[lirantal]

Saw it, thanks!

[lirantal]

Fixed in #53