pgscan: lockfileVersion 3 for npm dependencies not supported

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