Enable advisory suppressions in CLI restore for PackageReference projects

[advay26]

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/2720
Fixes: NuGet/Home#13313

Regression? Last working version:

Description

Design spec: https://github.com/NuGet/Home/blob/dev/accepted/2023/NuGetAudit-supress-advisory.md

We're adding a new MSBuild item called NuGetAuditSuppress that allows users to suppress individual advisories during restore. These items will have the advisory URL in the Include= field, looking something like this:

<NuGetAuditSuppress Include="https://github.com/advisories/GHSA-5crp-9r3c-p9vr" />.

This PR implements suppressions for CLI restore paths (dotnet/MSBuild restore, NuGet.exe restore, Static graph restore) for PackageReference projects. The packages.config implementation, as well as the VS restore implementation for PackageReference projects, will follow later.

These are the main pieces:

• To read the new items, we added a new 'Collect' target called CollectNuGetAuditSuppressions (src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets)
• We also added a new target called _CollectRestoreInputs, which will run all the individual 'Collect' item targets. This is meant to reduce duplication in our code. Going forward, instead of adding new 'Collect' targets to lists in multiple places, we will be able to add it to the DependsOnTargets= attribute in _CollectRestoreInputs, and use this target everywhere. (See _CollectRestoreInputs target)
• The new GetRestoreNuGetAuditSuppressionsTask parses the 'NuGetAuditSuppress' items into the _RestoreGraphEntry items that we ultimately read in restore.
• src/NuGet.Core/NuGet.ProjectModel/ holds all the changes related to reading and writing the dgspec.
• A new HashSet<string> SuppressedAdvisories member was added src/NuGet.Core/NuGet.ProjectModel/RestoreAuditProperties.cs to store the suppression inputs.
• The business logic for suppressions is in the src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/AuditUtility.cs file.
• I've added tests too, but I'm not sure if they cover everything. Let me know if I need to add tests anywhere else, and I'll go back and make the changes.

PR Checklist

• PR has a meaningful title

• PR has a linked issue.

• Described changes

• Tests

  • Automated tests added
  • OR
  • Test exception
  • OR
  • N/A

• Documentation

  • Documentation PR or issue filled: Add docs for NuGetAuditSuppress Home#13493
  • OR
  • N/A

[zivkan]

Great job!

Since you're new to the client team, you're not aware of our performance requirements, especially on restore, so there's a bunch of unnecessary memory allocations that should be removed.

[nkolev92]

This an API breaking change.
Sounds like no one is concerned about it, since this is arguably something that ideally wasn't public, but has to be, so I think it's ok to make the breaking change.

However, we should make sure we disclose the API breaking change in our release notes.
You can do that by creating a specific issue with a label Category:BreakingChange that talks about the breaking change and is fixed by this PR (alongside the other one).

[zivkan]

From memory, it's a public API so that both static graph restore, as well as "normal" CLI restore, can share the same code. But the method name, the method parameter type and return type are so specific, I just don't see it being useful to anyone else.

I wish we had some way to mark an API as "public because NuGet needs it internally, but we're going to make no effort to avoid breaking changes". InternalsVisibleTo feels like a bad solution. But yes, I noticed this when reviewing the PR, but because of the above justification, I just don't see it impacting anyone.

[advay26]

Sounds good. I've created an issue here: NuGet/Home#13313, and referenced it in this PR as you mentioned. Let me know if I need to change anything else here

[zivkan]

Looks great to me!

Just one thing where my gut reaction makes me feel it's a correctness issue, although testing a bunch of popular websites, and all the CVE websites I can find, they're all case insensitive, so maybe my suggestion doesn't actually matter.

[nkolev92]

Removing the label since the target branch is the feature branch anyways

The base branch was changed.