Inedo / pgscan

Dependency scanner for ProGet.
MIT License
11 stars 5 forks source link

Scan for npm package-lock.json when scanning a NuGet solution/project #27

Closed crotondo-dap closed 1 year ago

crotondo-dap commented 1 year ago

Hi,

within Issue #25 you added a support to scan for npm package-lock-json when scanning a NuGet solution. Unfortunately, we do not want parts of this behavior. I can't see a switch to control this behavior. If I call 'pgscan identify' on a solution it automatically takes npm-dependencies into account, even if I say the type is "nuget". Am I missing something here? Or is it not possible to control this? Further, all package-lock-json files are read recursively. Until now we were able to give pgscan a specific path to a specific package-lock-json. We don't want every file to be scanned. It doesn't seem to give a switch here neither?

So maybe adding a switch would be an option - which controls if all package-lock.json files should be taken into account automatically or not. Or an additional parameter to give the specific path to the package-lock.json if one wants to do it in one call but with only one specific package-lock.json file.

Let me know if I am missing something and what you think about this suggestions.

rhessinger commented 1 year ago

Hello,

Thanks for catching this. It looks to be a bug in the code. It was supposed to only scan npm and nuget when the type was auto, but it looks like I forgot to include that when I made the change. I'll get that fixed up and a new release out later today.

The plan is if you specify a type (nuget, npm, etc...), it will work as it did before. If you leave the type as auto (don't include a type), then it will automatically scan for npm when a NuGet type is auto-detected.

Then to handle the case where you want specific files scanned, you will just need to specify the type and file path (package-lock.json, MyProject.csproj, etc...) and just run it for each type you want to scan.

Thanks, Rich

rhessinger commented 1 year ago

Hi @crotondo-dap,

I just release 1.4.3 that implements the fix and now will not scan NPM dependencies when the type is specified.

aunverdorben-dap commented 1 year ago

Hi @rhessinger,

thank you for fixing this so quickly. Regarding this:

Further, all package-lock-json files are read recursively.

We've seen that package-lock.json files from npm packages consumed by our project are also getting processed when no specific package-lock.json is specified on the command line. We think that all package-lock.json files under \node_modules should be ignored.

Do you agree or do you think these files should also be processed?

rhessinger commented 1 year ago

Hi @aunverdorben-dap,

That is a fair point to make. My thought was that including the node_modules folder in the recursive search would allow us to include the child dependencies used by installed packages that were not marked as dependencies in the npm package. But in my research and testing, I have found the package-lock.json at the root of the node_modules folder includes a subset of the data in the main package-lock.json. So no extra information was added. Do your package-lock.json files under the node-modules folder have additional information the parent doesn't? Also, do your packages in that folder have package-lock.json outside of the root of that folder?

Looking at the hidden lock file documentation. The information in that file should be redundant as it is only used to improve performance, but if there is manual change in the node_modules tree by something other than npm, then the lock file is ignored (and should probably be removed anyways). I'm inclined to just exclude files from the node_modules folder as you suggest.

Thanks, Rich

crotondo-dap commented 1 year ago

Hi @rhessinger,

I can confirm your observation. There is no extra information in our package-lock.json files under the node-modules folder. Further, we do not have additional package-lock.json outside of the root folder.

rhessinger commented 1 year ago

Hi @crotondo-dap,

Thanks for confirming that for me. I'll add a new issue to remove the scanning of the node_modules, but it will be a low-priority change since most cases will not yield any different results.

Thanks, Rich