pgscan: lockfileVersion 3 for npm dependencies not supported

caterina

We noticed that pgscan does not list any dependencies for one of our npm-projects.
After debugging into it and comparing it with other npm-projects we noticed that there is a difference in the lockfileVersion of the package-lock.json files. The "problem-project" has lockfileVersion 3 while the others have lockfileVersion 2.
pgscan tries to read the dependencies from the property "dependencies" which is a legacy-property from lockfileVersion 1. lockfileVerson 2 was downward compatible, but lockfileVersion 3 (used by npm v9) is not. The newest package-lock.json no longer has the property "dependencies" and all dependencies are part of the "packages"-property.
Here is the official documentation about it: https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json/#lockfileversion

Have you already noticed this breaking change in the package-lock.json files?

I already opened an issue for this topic on ghithub:
https://github.com/Inedo/pgscan/issues/33

atripp

Hi @caterina

Sorry that issue fell off our radar; we're not great at keeping track of GitHub issues.

We haven't noticed this issue in our testing/environment yet, but I'm guessing that's b/c we haven't gone to v9?

Looking at the code, I guess it seems easy enough to support? Just a matter of iterating packages/dependencies instead of dependencies perhaps??

https://github.com/Inedo/pgscan/blob/master/Inedo.DependencyScan/NpmDependencyScanner.cs#L23

The hardest part for us is getting this tested/verified. We don't have a "problem project" ourselves yet, so we have to repro, study, fix, test, etc.

Any help here would be appreciated :)

Thanks,
Alana

caterina

This post is deleted!

caterina

Hi @atripp

I think packages has to be iterated instead. I haven't seen dependencies beneath packages.
Further, the "empty" Key has to be ignored as it stands for the root project:

Maybe a little bit parsing would be necessary.
In lockfileVersion 2 a dependency was listed like this:

In lockfileVersion 3 it looks like this:

If desired, we can also upload package-lock.json files for testing via MyInedo.

shayde

Per https://github.com/Inedo/pgscan/issues/34, I am open to contributing a fix for this to support lockfileversion 3, if I can get confirmation from @atripp and co that such a contribution would be acceptable / worthwhile. You okay if I spend an afternoon taking a crack at this?

sebastian...

@shayde
I can't speak for @atripp and the folks at Inedo, but I can tell you that we have submitted PRs for pgscan via GitHub in the past and they have all been accepted. Caterina and I actually already discussed implementing this, because it doesn't look too complicated, but we probably won't get a chance to look into it closer for a few more days. So if you want to go ahead, I think this would be much appreciated

shayde

I have a PR open on GitHub here that is working locally targeting a lockfileVersion 2 and a lockfileVersion 3. I'm sure it could use some improvements if anyone has the chance to review.

atripp

Thanks so much @shayde, on a quick glance the code looks good :)

From here we can do the easy part early next week - internal reviewed, merge, build, test, deploy!

sebastian...

@shayde
I added two comments to your PR. I think transitive dependencies need to be handled differently, otherwise you'll get concatenate package names like @angular-devkit/build-angular/jsonc-parser 3.2.0, @angular-devkit/core/jsonc-parser 3.2.0, @angular-devkit/schematics/jsonc-parser 3.2.0, [...] when it should probably really just be jsonc-parser 3.2.0.

sebastian...

Hi again

Two more things I noted while looking into this: It seems that the current implementation (reading the dependencies property) doesn't consider transitive dependencies at all. Take the package chalk as a example. I have one package that has a requirement of the form "chalk": "^2.0.0", which is resolved to the following dependency

    "chalk": {
      "version": "2.4.2",
      "resolved": "https://registry.npmjs.org/chalk/-/chalk-2.4.2.tgz",
      "integrity": "sha512-Mti+f9lpJNcwF4tWV8/OrTTtF1gZi+f8FqlyAdouralcFWFQWF2+NgCHShjkCb+IFBLq9buZwE1xckQU4peSuQ==",
      "requires": {
        "ansi-styles": "^3.2.1",
        "escape-string-regexp": "^1.0.5",
        "supports-color": "^5.3.0"
      }

However, I have some more packages with requirements like "chalk": "^4.1.0", which are resolved to sub-dependencies like this:

        "chalk": {
          "version": "4.1.2",
          "resolved": "https://registry.npmjs.org/chalk/-/chalk-4.1.2.tgz",
          "integrity": "sha512-oKnbhFyRIXpUuez8iBMmyEa4nbj4IOQyuhc/wy9kY7/WVPcwIO9VA668Pu8RkO7+0G76SLROeyw9CpQ061i4mA==",
          "dev": true,
          "requires": {
            "ansi-styles": "^4.1.0",
            "supports-color": "^7.1.0"
          }

Those sub-dependencies are not scanned at all, if I'm reading the current implementation of NpmDependencyScanner correctly. This results to only version 2.4.2 being reported into our SCA reports. Version 4.1.2 is not listed.

The packagesproperty of the new file versions 2 and 3 seems to list all packages in a flat list, putting information about relations between packages on the top level. This leads to entries like "node_modules/chalk" (resolving the version 2.4.2), "node_modules/critters/node_modules/chalk" (resolving the version 4.1.2), "node_modules/eslint/node_modules/chalk" (4.1.2 as well), ...

So if we extract the correct name of the package and add to corresponding version to it, our SCA reports should now (correctly?) report both versions of the chalk package.

The second thing I noticed here: we will get multiple entries of the same package/version combination. This probably won't bother ProGet, as I guess those entries will be considered identical, but I guess it wouldn't hurt to put a Distinct()in somewhere on the client side. Like, maybe changing the line

            return new[] { new ScannedProject(projectName, ReadDependencies(doc)) };

to

            return new[] { new ScannedProject(projectName, ReadDependencies(doc).Distinct()) };

or maybe changing to code of the ScannedProject constructor from

            this.Dependencies = dependencies.ToList();

to

            this.Dependencies = dependencies.Distinct().ToList();

Cheers,
Sebastian

shayde

@sebastian Thank you for your thorough review and thoughts. I've updated the code to address your concerns, I believe. Please review when you have a moment? I'll clean up commit history when things are good to go :)

sebastian...

@shayde

The code looks good to me

apxltd

@shayde @sebastian really appreciate the help, we'll get this incorporated ASAP !!

atripp

And I'm sure you noticed but looks like this was released :)

sebastian...

Hi everyone,

just created a PR to improve support for both the old and the new file format.

File format 1 & 2 ('dependencies'): Added code to also read sub-dependencies.

File Format 2 & 3 ('packages'): Added code to deal with the new way of handling package alias. Apparently, there is an optional 'name' property. Couldn't find anything about that in the format's specification, so this is purely based on observation of our test projects.

gdivis

Thanks! Merged and released.