Skip to content

Add transitive guidance #3336

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 3, 2024
Merged

Add transitive guidance #3336

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ceec92d
Add transitive guidance
JonDouglas Aug 28, 2024
061cc2c
Update Auditing-Packages.md
JonDouglas Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions docs/concepts/Auditing-Packages.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,15 @@ If security vulnerabilities are found and updates are available for the package,
- Use the NuGet package manager user interface in Visual Studio to update the individual package.
- Run the `dotnet add package` command with the respective package ID to update to the latest version.

#### Transitive Packages

If a known vulnerability exists in a top-level package's transitive dependencies, you have these options:

- Add the fixed package version as a direct package reference. **Note:** Be sure to remove this reference when a new package version update becomes available and be sure to maintain the defined attributes for the expected behavior.
- Use [Central Package Management with the transitive pinning functionality](https://learn.microsoft.com/nuget/consume-packages/Central-Package-Management#transitive-pinning).
Copy link
Member

@zivkan zivkan Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should explicitly warn people that if they pack the project into a package, the pinned package will become a package dependency. The linked doc mentions that it promotes a package to a top level dependency, but the very first two customers who tested the first CPM prototype didn't understand this, complained, and stopped using CPM.

Copy link
Contributor

@ericstj ericstj Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That probably belongs in the CPM docs. Remind folks that rolling up dependencies doesn't actually add any new dependencies than what you already have. Maybe NuGet gallery could even help here by showing authored dependencies visually different than transitive ones?

Copy link
Member

@zivkan zivkan Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nuget.org (NuGetGallery) only show dependencies listed in the nuspec, so only direct dependencies, not transitive dependencies.

Any package that CPM promotes via transitive pinning becomes a direct dependency (the page on CPM explicitly states this), and therefore it will be listed in the nuspec as a dependency. NuGet Gallery has no way to know that "this was a transitive package that was pinned by the project that it was packed from". The very first 2 customers who tried CPM (both package authors) when it was in preview and transitive pinning was default with no option to disable, immediately said they hated it and stopped using CPM. So, I think it's valuable to set expectations.

I still see transitive pinning (or direct references) as a valid way for library authors to remove transitive packages with known vulnerabilities. But if a single repo is building both packages and apps, the developer might not want to cause a transitive package like System.Text.Json to become a direct dependency of their package, when they upgrade the package for the web/gui/console apps.

Copy link
Member

@nkolev92 nkolev92 Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's another challenge here that the CPM docs aren't really complete in this scenario.
Separate problem: NuGet/Home#13811.

Copy link
Member

@zivkan zivkan Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what I was thinking with my last reply. Let me try again to make it more clear:

Even if it's documented elsewhere, the first set of customers who tried transitive pinning hated it because it caused additional package dependencies, and told us they immediately stopped using it. I'm sure those two people were not unique and other customers will feel similarly. Therefore, if we're going to advise people to use the feature, I think we should set expectations so they're not surprised.

Copy link
Member

@nkolev92 nkolev92 Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah fair, I was just pointing out that we have another gap to close, which is document it for CPM in the first place.

- [Suppress the advisory](https://learn.microsoft.com/nuget/concepts/auditing-packages#excluding-advisories) until it can be addressed.
- File an issue in the top-level package's tracker to request an update.
Copy link
Contributor

@kartheekp-ms kartheekp-ms Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could add a link to the dotnet nuget why command or Visual Studio transitive dependencies feature documentation to identify the top-level package. I am okay with addressing this comment in a follow-up PR.

Copy link
Contributor Author

@JonDouglas JonDouglas Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jebriede solution explorer ships what release again?

Copy link
Contributor

@jebriede jebriede Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JonDouglas do you mean transitive dependencies in Solution-level PM UI? If so, 17.12.


### Security vulnerabilities found with no updates

In the case that a known vulnerability exists in a package without a security fix, you can do the following.
Expand Down