resolved field change between v5.0.7 and v5.0.10 for API package

obartra commented 4 years ago

Updating to lockfile-lint-api@5.0.10 the resolved metadata field doesn't always resolve to a URL or undefined (sometimes it's contains a package a version)

The following illustrates the issue:

import { ParseLockfile } from 'lockfile-lint-api';

const packages = new ParseLockfile({
  lockfilePath,
  lockfileType
}).parseSync().object;

const results = Object.entries(packages).map(([packageName, packageMetadata]) => ({
  packageName,
  resolved: packageMetadata.resolved
}))

console.log(results);

Expected Behavior

All packages return a URL that looks like (or undefined):

{
  "packageName": "abbrev@1.1.1-8f0b92d6fc7847f731964a4f3ab3b7c1e53edee5",
  "resolved": "https://npm.corp.indeed.com/repository/npm/fsevents/-/fsevents-1.2.11.tgz"
}

Instead, on 5.0.10 some packages return the version only:

{
  "packageName": "abbrev@1.1.1-8f0b92d6fc7847f731964a4f3ab3b7c1e53edee5",
  "resolved": "1.1.1"
}

Seems to happen for any packages that are listed as dependencies of another package which don't contain this information on the package-lock. e.g.:

        "fsevents": {
            "version": "1.2.11",
            "resolved": "https://my.registry.com/repository/npm/fsevents/-/fsevents-1.2.11.tgz",
            "integrity": "sha512-+ux3lx6peh0BpvY0JebGyZoiR4D+oYzdPZMKJwkZ+sFkNJzpL7tXc/wehS49gUAxg3tmMHPHZkA8JU2rhhgDHw==",
            "optional": true,
            "requires": {
                "bindings": "^1.5.0",
                "nan": "^2.12.1",
                "node-pre-gyp": "*"
            },
            "dependencies": {
                "abbrev": {
                    "version": "1.1.1",
                    "bundled": true,
                    "optional": true
                },

Current Behavior

resolved field contains package version when resolved is not available

Possible Solution

omit the resolved value when not available

Steps to Reproduce (for bugs)

install 5.0.7
Run snippet above
install 5.0.10
Run snippet above
compare output

Context

It makes a little bit harder to filter out packages without registry specified since it has to also account for package versions
If this is the culprit it looks like a breaking change

Your Environment

Library Version used: lockfile-lint-api@5.0.7 and lockfile-lint-api@5.0.10
Node.js version (e.g. Node.js 5.4): Node 12.13.0
Operating System and version (desktop or mobile): OSX 10.146 (desktop)

obartra commented 4 years ago

As a side note, my solution to the update has been to change the code from:

 const protocolMatch = registries.some(
        (registry) =>
          // if not defined
          !packageMetadata.resolved ||
          // or is a valid registry
          packageMetadata.resolved.startsWith(registry)
      );

to:

 const protocolMatch = registries.some(
        (registry) =>
          // if not defined
          !packageMetadata.resolved ||
          // or it's only a semver
          /^(\d+\.)?(\d+\.)?(\*|\d+)$/.test(packageMetadata.resolved) ||
          // or is a valid registry
          packageMetadata.resolved.startsWith(registry)
      );

I haven't tried it but I suspect this line would be problematic if receiving something like 1.2.3 instead of a URL

lirantal commented 4 years ago

The change we introduced didn't really make an update to the resolved field but rather now we iterate through all the tree, which we didn't before, so it is possible that you just didn't have a lockfile structure before that had a resolved key set to a version number.

As you correctly wrote, some packages return the version only:

{
  "packageName": "abbrev@1.1.1-8f0b92d6fc7847f731964a4f3ab3b7c1e53edee5",
  "resolved": "1.1.1"
}

With regards to your workaround. Wouldn't it be better to try to new URL() it and handle the exception thrown when the value detected is not a URL? That's kind of what we do in the line number you linked to here https://github.com/lirantal/lockfile-lint/blob/master/packages/lockfile-lint-api/src/validators/ValidateScheme.js#L32-L43 which if you'll take a close look we swallow that error, which is why it never throws.

p.s. totally agree that ideally this should've been a breaking change in semver too but I had thought this to be an important enough gap in terms of security implications to ship the change in a smaller version range to allow everyone to receive the security fix and hoping for as little impact as possible. Sincere apologies if this had caused you some headaches.

obartra commented 4 years ago

Oh interesting yeah that makes sense. Is the "resolved" field something specific to the lock files or part of lockfile-lint's API? If the later it may make sense to add dedicated fields (e.g. "version" or something like that) instead so that resolved is always a URL or undefined.

And I totally missed the try catch block 🤦‍♂ yeah what you have seems fine. I just wanted to avoid swallowing all errors, I would have missed this otherwise! But I guess it was benign so may be worth updating to match.

Thanks for all the context, I think it's safe to close this issue then.

lirantal commented 4 years ago

@obartra the resolved field is something that stems from the lockfiles indeed. We just standardize the interface to it because it is referenced in a different way between npm and yarn's lockfiles.

Cool thanks, and you're welcome to re-open if you wanted to discuss further 🤗

lirantal / lockfile-lint