Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip vulnerable package versions in dependency resolution during nuget restore #13771

Open
Matheos96 opened this issue Sep 5, 2024 · 3 comments

Comments

@Matheos96
Copy link

Matheos96 commented Sep 5, 2024

NuGet Product(s) Involved

NuGet.exe, MSBuild.exe, dotnet.exe

The Elevator Pitch

This feature request is based on the discussion in the dotnet/runtime repo for issue dotnet/runtime#105028. The copy pasted quote below describes well the requested feature. It basically tries to address the issue where we are using a certain NuGet dependency which has its own dependency (a transitive dependency of ours) which has a known vulnerability AND a newer version addressing this vulnerability. The current behaviour is to always pick the minimum version that fullfills all dependency requirements, which would be the vulnerable in this case, instead of the minimum non-vulnerable one. We wish this behaviour to change accordingly:

Quote by @ibruynin from dotnet/runtime#105028 (comment)

The nuget resolution goes for minimal version that fulfills all the dependencies in the project tree, which - in our case - upgrades 4.7.2 (not affected) to 8.0.0 (affected) but not to 8.0.4 which is available but does not match minimal version requirements.

Suppose, we thought, that nuget keeps on doing what it does today but takes known vulnerabilities into account.
When looking at the versions of System.Text.Json only 8.0.4 is a good one now. If nuget restore skips all vulnerable versions, it will automatically result in 8.0.4 and all problems are gone automagically.

Additionally, I personally suggest this skip feature should respect Major boundaries and never skip "on" to the next Major. We should probably only skip to the latest X.Minor.Patch to ensure no surprise breaking changes are introduced by the implicit NuGet package upgrade. This of course assumes that developers have respected the SemVer 2.0 standard. In cases where this feature would cause an issue, due to breaking changes in Minor or Patch, perhaps simply an exact PackageReference to a specific version could be used.

Additional Context and Details

This is an all too common occurence, where we have transitive dependencies with vulnerabilities and the only way to solve them, besides waiting for the package we depend on to be updated, is to add an "unnecessary" direct reference to that package. These direct references are a mess to maintain and easily they get out of control when you in the future forget what is what, and why some reference ever was a direct reference etc. We want to just depend on the packages we actually directly depend on, and let them manage their own dependencies (along with the proposed package resolve improvement).

@ibruynin
Copy link

ibruynin commented Sep 6, 2024

Additional info
The minimal repro steps are documented in this issue dotnet/runtime#107342

@Matheos96
Copy link
Author

Matheos96 commented Sep 8, 2024

An additional thought of concern came to mind...

As a developer, I would expect that building the exact same code base one day, would produce the exact same result if I built it the next day. However, that may not necessarily be the case with the proposed solution as the next day, a Minor or Patch version update may have gotten released and therefore picked for the new build. This is probably not an issue most of the time, and also probably exactly what we would want. That being said, IF the developer of said library accidentally or knowingly had introduced a "breaking" change in the the update (agains the SemVer 2.0 standard, but not impossible), our software could suddenly contain unwanted behaviour or bugs. By "breaking", I mean a change that does not cause a compile-time error, but something which changes the way that a certain function works etc. For example, two boolean parameters have for some reason changed order (dumb example but not impossible), the code compiles but the booleans values are swapped. Another example would be an exiciting method, which we are using, now suddenly throws an exception wheras earlier it never did. We would never know that we need to update our code and handle it...
Unexpected changes like these being implicitly introduced may be a risk if it happens for example close to a release. Imagine the testers testing for a week thorougly on the latest builds one week, but the next week we finally publish the final RC and signs it and ships it - no matter how well we have tested it before, hidden stuff like this (unlikely but possible) scenario may well slip through.
Of course, if everyone sticks to respecting SemVer standards there is no issue 😄

Having written the above paragraph, I just now found kind of exactly what I was going to propose... Enable repeatable package restores using a lock file
If feature requested in this issue is introduced, I believe that it should almost force users to make use of a lock file in their projects (if using the requested feature). At the very least, in some way strongly hint towards its usage and its importance.

@JonDouglas
Copy link
Contributor

I think many of these challenges would best be solved by allowing developers to choose if they'd like a SemVer compatible resolution. NuGet is one of the only modern package managers that does not do this. Other package managers typically install the latest version available within a SemVer-compatible range. This includes both top-level AND transitive-level dependencies.

I'm going to file a new issue given that #5553 is extremely old and unsustainable comment wise.

Go upvote this if this sounds like it would tackle the problem here instead of needing to work around it with skipping vulnerable versions.

#13779

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants