Add project properties for NuGetAudit

[zivkan]

Summary

Sorry about the late notice. We've been so busy implementing, testing, and fixing bugs, this slipped my mind 😞. Anyway, in VS17.8, NuGet is shipping a feature called NuGetAudit: https://learn.microsoft.com/en-us/nuget/concepts/auditing-packages

If it's too late to get project properties added for this in 17.8, that's fine with me, though I didn't actually talk to any managers so I don't know if they share my feeling.

Also, while project properties already has a section for "Package", all the settings there are related to making a package out of the project. NuGetAudit is for referenced packages, and is therefore relevant for all projects, including ones that do not get packed into nupkgs.

Anyway, NuGetAudit has 3 properties that customers can set:

Property name	possible values	description
NuGetAudit	true (default), false	Enables or disables the feature
NuGetAuditMode	direct (default), all	direct checks only directly referenced packages, whereas all will check transitive packages as well
NuGetAuditLevel	low (default), moderate, high, critical	The minimum vulnerability severity level to report when a package has a known vulnerability. Known vulnerabilities with a lower severity level will not be reported.

I haven't actually looked at project properties in a long time (too used to hand editing the csproj). If it's possible to have a group heading, with a "Learn more" link, I think a link to the docs would be great.

User Impact

Increases discoverability into NuGetAudit, for customers who are unaware. Makes it easier to set and change values avoiding typos.

[drewnoakes]

I took a look at this and had a chat with @zivkan. I'll try to summarize that here.

Turns out the NuGetAudit property is tri-state:

• "" is enabled but not enforced
• "true" is enabled and enforced (errors if vuln data cannot be found)
• "false" is disabled

This means we cannot add a simple checkbox. I suggested that instead of having a combo box, we add a checkbox for "Enable NuGet package auditing" and have another to control "Error when vuln data cannot be obtained". It's too late in the cycle for 17.8 to ship this.

The other issue we have in tooling this is that the values for NuGetAuditMode and NuGetAuditLevel don't evaluate to their default values when unspecified. This means the comboboxes appear blank rather than having their defaults populated. While we can intercept the value and replace an empty value with the known default, this moves NuGet logic into the project system where it becomes hard to change that default.

@zivkan is going to explore the tooling side of this with the team and may revisit in 17.9. It's too late for us to realistically ship properties for 17.8 now anyway, unfortunately. We missed the window by a day sadly.

[zivkan]

The other issue we have in tooling this is that the values for NuGetAuditMode and NuGetAuditLevel don't evaluate to their default values when unspecified.

From a basic customer point of view, I get that having a checkbox or dropdown is the simplest. From an intermediate or advanced .NET developer, it means it's not possible to specify "remove from my csproj and use feature default" vs "write this value in my csproj, so I'm protected from changing defaults". Maybe it's acceptable to require customers who want to do this to hand edit the csproj as XML.

However, the problem from a NuGet team point of view, if we evaluate the default value in our props/targets, then it becomes difficult for us to determine the difference between "project did not specify a value" vs "project explicitly set the value the same as the default", meaning in our telemetry, we can't tell the difference. Knowing this might be useful for understanding product usage and making decisions about education, but it would be very useful if we ever want to change defaults, and need to know the impact on how many customers it will impact vs how many customers have explicitly set the value to the old default value and will therefore not be affected.

Is there some other way we can tell the project properties windows what the default is, while still getting the semantic difference between "not set" and "set to the default value"? Or alternatively, get that semantic difference through NuGet's project nomination, if we do set the default value in evaluation via NuGet.targets?

[drewnoakes]

From an intermediate or advanced .NET developer, it means it's not possible to specify "remove from my csproj and use feature default" vs "write this value in my csproj, so I'm protected from changing defaults".

This is a general issue with the property pages. Fixing it would require quite a bit of work, including to the APIs that CPS provides for setting project properties.

Is there some other way we can tell the project properties windows what the default is, while still getting the semantic difference between "not set" and "set to the default value"?

Thinking out loud here, perhaps in a .targets file (that runs after the project file (e.g. .csproj) you could copy the property you want to set a default for to another property, then set the default. Something like:

<PropertyGroup>
  <_OriginalNuGetAuditMode>$(NuGetAuditMode)</_OriginalNuGetAuditMode>
  <NuGetAuditMode Condition="'$(NuGetAuditMode)' == ''">direct</NuGetAuditMode>
</PropertyGroup>

[zivkan]

We removed the NU1905 (no vulnerability data) warning, so NuGetAudit is no longer a tri-state. It's strictly true or false now: https://github.com/NuGet/NuGet.Client/pull/5398

I haven't yet done anything about setting defaults in props/targets though.

[drewnoakes]

I haven't yet done anything about setting defaults in props/targets though.

@zivkan is this on your radar for 17.9? Let's work together to provide a great tooling experience in VS for this feature.

[zivkan]

Thanks for the reminder. I created a tracking issue to get this done, and have already reached out to people who care more about telemetry than I do to see if they want to keep this "explicitly set" telemetry or not. If not, it becomes a trivial change to make. If they want to keep it, then it's work that will need to be scheduled, hopefully next month.

[zivkan]

The properties have been merged into NuGet just now. Hopefully tonight it will be merged into VS main branch, but it might take a few days depending on who is on support and when they click the button (auto-insertions still need manual approval)

[steve-torchia]

This audit feature needs to be OFF by default. Having it ON by default is breaking automated builds out from underneath folks, giving them no heads-up whatsoever.

[slang25]

The value of this feature comes from being on by default, it's to raise awareness of vulnerabilities, which 99% of users will not go out their way to enable.

A major version is the right way to introduce this change. A breaking build following an SDK major version update is expected behaviour.

[steve-torchia]

Fair enough. However, a breaking change without any sort of advanced notification is not the right way to introduce it. The feature itself is great. My primary concern is that this functionality was introduced in a build image (see: https://github.com/actions/runner-images/issues/8836) without any sort of advanced notice. This new build image with breaking changes and no heads-up took down an entire engineering organization's CI/CD pipelines, affecting release schedules/dates.

[zivkan]

Given that NuGetAudit is a NuGet feature, I think a discussion or issue over a https://github.com/NuGet/Home would be more appropriate than here dotnet/project-system. Especially since this issue is specifically about adding a few settings to the project properties window, and is not a general NuGetAudit feature issue.