transitive packages added by `dotnet package update --vulnerable` should be marked as such

[zivkan]

NuGet Product(s) Affected

dotnet.exe

Current Behavior

The initial implementation of dotnet package update --vulnerable will add PackageReferences to the project to promote transitive packages to direct package references when the transitive package has a security advisory.

Desired Behavior

The PackageReference item added to the project should have some matadata to signal that it was added only to mitigate a security advisory. This will allow both people and automated tools to understand this context and remove the package reference when it's no longer needed.

The exact metadata name and value is open for suggestions. Please comment with yours. Some initial ideas:

audit="mitigate security advisory"
pinned="security"
resolution="audit"

Additional Context

Most developers don't want their projects to reference unnecessary packages. When updating the package that originally caused the transitive package to be included either removes the dependency, or upgrades it to a version that no longer has security advisories, then it is usually preferable to remove the package reference and let the restore graph sort it out automatically.

The first time I heard the idea to add metadata so that tooling could automatically clean it up later was suggested to me by one of the Dependabot developers. Thanks!

[Frulfump]

I added a similar comment here #13372 (comment) and #11549 (comment) a while back that suggested a comment being added, where is the metadata being discussed in this issue visible? Is it directly on the PackageReference itself like an attribute? If so that is probably better than a comment (For projects I oversee today we ask the developers to manually add a comment like "Added to fix vulnerable version x brought in by y" but sometimes y is a bit of a large list of packages and then we allow it to be truncated since it's available in the PR.). And we do this for the reasons you outline in the additional context.

Of the initial ideas I liked audit="mitigate security advisory" (a bit unsure of the exact meaning of mitigate but I thought it could also mean to lessen an issue and not fully avoid it but in this case the issue is fully avoided (fully mitigated?)) and pinned="security". The last one "resolution="audit" I thought was a bit too vague (depending on where it would show up). Is it worthwhile to include the advisory(ies) in the first suggestion? I guess that could potentially be a bit too verbose. (In the first comment I also wished for a mechanism to automatically remove references that are no longer needed, I guess we'd need the advisories stored then? But thinking some more about that it seems like it would be a bit error prone to support something like that)

As I'm not fully sure where this would show up I might change my mind but my suggestion for now if it's tied directly to the PackageReference would be something like "reason=Avoid transitive vulnerable version", reason (purpose?) could be something you add for other purposes as well is that a benefit or is it better if the category is more tailored for the usecase here? If so audit is probably better.

[zivkan]

Apparently in Go a comment // indirect is added, so something like Indirect="true" would be similar.

Is it directly on the PackageReference itself like an attribute?

Yes, in MSBuild nomenclature it's called metadata. MSBuild's syntax allows it to be defined either as a child element, or as an xml attribute.

I was also thinking about taking advantage of the Label attribute on ItemGroups, but:

• It splits PackageReferences into multiple groups. When a project has a modest number of package references, it's nice to keep them all together to make it easy to see the full list when viewing the project file's xml.
• Programmatically accessing this information is much harder (especially given the technical constraints of the VS architecture and how project system needs to pass the information to NuGet)

[baronfel]

Also I'd nitpick that Label has no actual semantics - it'd be like putting directives in comments. Definitely agree that MSBuild Metadata are the way to go here.

[zivkan]

I just had a meeting with some teammates just to talk about naming 😆 here are some of my favorites from that discussion:

ToolManaged="true"
TransitiveFix="true"
IsPromotedDependency="true"

some criteria/concerns:

• The intent is for tooling to be able to automatically clean it up in the future, so something with a true/false value is better than something like reason="some text". The reason is that developers might think they're free to write anything in and it will affect the product in other ways (for example, be displayed in VS's PM UI or dotnet list package)
• When someone is hand editing a csproj to add a PackageReference, it should be something they're discouraged from copy-pasting
• .NET 10 is already introducing a feature called PrunePackageReference, and CPM already has the concept of "pinned" packages, so avoid the words pin and prune, to reduce confusion with those features
• We can imagine deprecated packages having the same feature in the future. So, we want this attribute to be re-usable, not security vulnerability specific, to avoid multiple attributes with the same behaviour.

ToolManaged is my favorite, except for the concern that we'll only be adding these references for now, and who knows what the future will bring, so even if we intend now on implementing tooling to automatically remove them when no longer needed, if we don't implement it within a year or two it'll be confusing what "managed" even means.

Another idea was don't add any attribute, and instead create tooling (something like dotnet package update --simplify ) that will analyze the graph and remove package references that come in transitively. If the command only removes PackageReferences where the version is the same as the transitive dependency, then it could work. But if a customer wants to use this command to minimize PackageReference items, and accepts that some packages will resolve to a lower version, then the command will become a lot more complex because it now has to figure out if a package was upgraded to a higher version because of a known vulnerability or deprecation, or maybe even a bug in the transitive dependency version. While I like the idea of a package reference cleanup command, I don't think we have time to think it through for .NET 10, and I need to get dotnet package update --vulnerable ready for .NET 10.