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 1 commit
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.
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.

We should also tell folks to take care when doing this that they don't change the deployment behavior of their project. I've seen folks hoist a dependency that was marked with PrivateAssets and ExcludeAssets and forget to add those same markings.

Copy link
Contributor Author

@JonDouglas JonDouglas Aug 28, 2024

Choose a reason for hiding this comment

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

Something like?

Note: This does not change the deployment behavior in any way.

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.

No, I mean the user needs to take care to make sure that they don't do this. So if they were previously doing:

<PackageRefence Include="Direct" Version="1.0.0" PrivateAssets="All" ExcludeAssets="Runtime" />

They should take care when adding a new reference to give it the same:

<PackageRefence Include="Direct" Version="1.0.0" PrivateAssets="All" ExcludeAssets="Runtime" />
<!-- Note: Apply the same values for PrivateAssets and ExcludeAssets as Direct --> 
<PackageRefence Include="Indirect" Version="1.1.0" PrivateAssets="All" ExcludeAssets="Runtime"/>

It's not automatic here. It is for CPM - that's a major benefit it gives.

Copy link
Contributor Author

@JonDouglas JonDouglas Aug 28, 2024

Choose a reason for hiding this comment

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

ah i see. you're talking about the attribute behavior.

@jeffkl @zivkan should we explicitly mention this?

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.

Maybe we say: Take care when adding the dependency to match the Assets settings of the existing dependency. When doing so it should behave in the same way as if your dependency itself was updated. This is not required when using CPM.

Copy link
Contributor Author

@JonDouglas JonDouglas Aug 28, 2024

Choose a reason for hiding this comment

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

yep. "whne adding a explicit reference, ensure you maintain your packagereference attributes for the asset behavior" etc

Copy link
Member

@zivkan zivkan Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm confused, or I think we have inconsistent behaviour (which now would be a breaking change to fix). Today, so coincidence it's on the same day, I was pinged on Discord because someone claims that a package reference with ExcludeAssets="runtime" does not exclude that package's dependencies runtime assets, and they had to manually list every transitive package as direct references and add the same ExcludeAssets="runtime".

If CPM's central pinning does flow through include/private/exclude assets, then it sounds like it changes behaviour compared to the same package not being pinned, which feel like a bug to me.

I just did a super quick test, and it looks like excludeassets does flow, so the person on discord who pinged me has a more complex scenario.

I think yes, it is worth mentioning. Include/Private/Exclude Assets might not be used by a high percentage of customers, but I would be surprised if the people using it will remember that asset group selection flows when they promote a transitive package to a direct reference (especially since none of our tools (VS, or dotnet add package) provide a way to view or edit it.

Copy link
Contributor

@ericstj ericstj Aug 30, 2024

Choose a reason for hiding this comment

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

Here's a good example of the sort of thing that makes this complicated: dotnet/aspnetcore#57560 (comment)

- 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 a package version update becomes available.
- 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