pgscan: lockfileVersion 3 for npm dependencies not supported

[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.