Vulnerability checking in PC restore

[nkolev92]

Design for #12307

[zivkan]

I disagree with this.

Firstly, as you already listed in the unanswered questions section, should PackageReference projects use this config?

Secondly, from a customer point of view, it means they'll have to duplicate the config (when not using defaults), and will have (at least) two places to update when changing values. This will have even higher impact when suppressing specific advisories is implemented, as that feature will have to design a packages.config specific alternative, and customers may need to duplicate the suppressions.

packages.config already reads MSBuild properties for lock files, so configuring and using lock files is the same, regardless if the project uses packages.config or PackageReference. I think NuGetAudit for packages.config should be the same.

[nkolev92]

Usage of packages.config lock is minimal, so I'm not sure it's necessarily a well established pattern.
Maybe we make the nuget.config apply to everything?

I'm also wondering if the technical overhead of adding individual project support in packages.config is worth it in V1 of the feature.

[nkolev92]

fwiw, I'll schedule a meeting to discuss this.

[JonDouglas]

is there any technical or feasibility issues with using the same pattern we've established earlier w/ MSBuild properties?

I like re-using the same pattern if possible.

[nkolev92]

My argument is that it's not a pattern we've adopted for packages.config projects.
Technically, they're both feasible, but for packages.config projects using MSBuild property is not really a thing like it is in PackageReference.

[donnie-msft]

In VS, we have the Vulnerability InfoBar that appears directing customers to the PM UI.

Wondering if we need to ensure customers are aware this feature has become enabled for their packages.config projects. Is VS' "What's New" page enough?

Scenario: a legacy project has been used internally for years with a vulnerable package. Suddenly, VS vNext begins showing errors/infobars popping up, despite no change in vulnerabilities. As a customer, would it be obvious this is a new feature lighting up, or would it appear as a new vulnerability in my project and cost me time verifying and potentially raising false alarms in my org?

[nkolev92]

Thanks for the comments @donnie-msft

In VS, we have the Vulnerability InfoBar that appears directing customers to the PM UI.
Wondering if we need to ensure customers are aware this feature has become enabled for their packages.config projects. Is VS' "What's New" page enough?
Scenario: a legacy project has been used internally for years with a vulnerable package. Suddenly, VS vNext begins showing errors/infobars popping up, despite no change in vulnerabilities. As a customer, would it be obvious this is a new feature lighting up, or would it appear as a new vulnerability in my project and cost me time verifying and potentially raising false alarms in my org?

I think this is something we should figure out in general for new features. I think we can do that separately from this particular feature and not block on the spec.
Do you know if anyone started a discussion on this topic? If not, I can do it.

[donnie-msft]

Thanks for the comments @donnie-msft

In VS, we have the Vulnerability InfoBar that appears directing customers to the PM UI.
Wondering if we need to ensure customers are aware this feature has become enabled for their packages.config projects. Is VS' "What's New" page enough?
Scenario: a legacy project has been used internally for years with a vulnerable package. Suddenly, VS vNext begins showing errors/infobars popping up, despite no change in vulnerabilities. As a customer, would it be obvious this is a new feature lighting up, or would it appear as a new vulnerability in my project and cost me time verifying and potentially raising false alarms in my org?

I think this is something we should figure out in general for new features. I think we can do that separately from this particular feature and not block on the spec. Do you know if anyone started a discussion on this topic? If not, I can do it.

Sure, feel free. Not aware of anyone else asking this question. It is just a thought I had reading the spec.

FYI, I'm not blocking on this spec (hence I didn't use the Changes Requested state). It's probably fine to call out that we expect customers to see What's New or we're just not concerned with any jarring effects caused by lighting it up at this time.

EDIT: this comment highlights the "no new warnings for minor releases" idea. Wonder how that applies to this spec.
dotnet/sdk#38037 (comment)

[JonDouglas]

👍

[JonDouglas]

Not sure I like the idea of nuget.config config being introduced here nor affecting PR.

[nkolev92]

I recommend reading: https://github.com/NuGet/Home/blob/dev-nkolev92-vulnerabilityCheckingInPCRestore/accepted/2024/vulnerabilities-in-packages.config-restore.md#msbuild-properties-vs-nugetconfig-for-packagesconfig-restore-configuration.

It covers the in depth thought process.
Let me know what you think.

[JonDouglas]

This might get confusing between having this and also a MSBuild property.

Is the rationale here that this would be used for legacy project system?

[nkolev92]

I detail that in https://github.com/NuGet/Home/blob/dev-nkolev92-vulnerabilityCheckingInPCRestore/accepted/2024/vulnerabilities-in-packages.config-restore.md#msbuild-properties-vs-nugetconfig-for-packagesconfig-restore-configuration .

We have places where we have duplicate configurations, one via nuget.config and another one via property. The property tends to override the nuget.config value.

The reasoning here is that in packages.config world, for those customers, nuget.config makes more sense.

[nkolev92]

TLDR from the meeting:

• We'll go with properties for now, but the concerns about nuget.config vs msbuild are greater for the suppressions work, so if the suppressions work leans nuget.config, we might do the same for the vulnerability checking.