Enable AuditSource during dotnet list package command #6206

Nigusu-Allehu · 2024-12-21T00:03:38Z

Bug

Description

This PR updates dotnet list package --vulnerable to use user configured <AuditSources>.

Currently, the command only looks into <PackageSources> to load vulnerability data. However, with the introduction of NuGet Audit, other commands now support <AuditSources> to specify vulnerability data sources. This PR makes sure dotnet list package --vulnerable is also up to date and supports <AuditSources>

In order to do a manual test, I specified a package that has only one vulnerability data source. That source is only specified as an Audit source. This is what running dotnet list package --vulnerable results in before and after this PR

Before

After

PR Checklist

Meaningful title, helpful description and a linked NuGet/Home issue
Added tests
Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc. Update doc: dotnet list package --vulnerable uses AuditSources Home#14021

zivkan

Good work!

I particularly like test calling GetReportDataAsync and asserting on the object, rather than a test that tries to parse console output.

There are some changes needed to make dothet list package work more similarly to restore with regards to audit sources. And considering audit sources introduces multiple behaviours, it means multiple tests are needed to test all the scenarios.

I also made some suggestions about code readability/maintainability, which while subjective, I hope that I've justified sufficiently well to convince you that it's changes we should make, not just an opinion I'm trying to force onto others.

zivkan · 2024-12-27T23:23:34Z

...ore/NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageArgs.cs

@@ -17,6 +17,7 @@ internal class ListPackageArgs
        public ILogger Logger { get; }
        public string Path { get; }
        public List<PackageSource> PackageSources { get; }
+        public List<PackageSource> AuditSources { get; }


The pattern that most NuGet.CommandLine and NuGet.CommandLine.XPlat commands use is that these *Args classes hold the inputs, and business logic goes in the *Runner classes.

Since no CLI argument/option was added to dotnet list package for audit sources (should there be?), I feel like this AuditSources property doesn't belong here.

I had a quick look, and I see that ListCommand reads nuget.config and gets the sources at the same time as it's parsing the other CLI arguments and options into the ListPakcageArgs class. But I think this is not good software design. Following the Single Responsibility Principal, I think easier to understand code if ListPackageArgs is only parses CLI commands and arguments and puts them in the ListPackageArgs class, and then ListCommandRunner has all business logic regarding whether CLI sources are added to nuget.config sources, or used as a replacement instead, as well all the other work that list package does.

zivkan · 2024-12-27T23:29:37Z

...ore/NuGet.CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageArgs.cs

            Logger = logger ?? throw new ArgumentNullException(nameof(logger));
            CancellationToken = cancellationToken;
            ArgumentText = GetReportParameters();
        }

+        public ListPackageArgs(


My comment isn't as relevant given my other suggestion that AuditSources shouldnt be defined in this class (unless a CLI option is added), but this class isn't public, so it's not part of the public API, and therefore we can modify the existing constructor rather than adding a new overload.

If this was already understood, and this overload was added instead to avoid needing to modify existing tests that call the existing constructor, a trade-off needs to be made between ease of finishing your current work (this PR) vs ongoing tech debt in having more methods, risks of bugs because tests are using the production class differently to how production code is being used.

There will be times where adding an overload is the better choice, and other times when making the effort to update tests is the better choice to reduce risk. I haven't looked in detail at how many tests create instances of this class, but my gut feeling is that I would expect it not to be too difficult to update all tests, so we probably don't need a constructor overload.

zivkan · 2024-12-27T23:49:02Z

....CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommandRunner.cs

@@ -176,6 +179,18 @@ private static void WarnForHttpSources(ListPackageArgs listPackageArgs, ListPack
                }
            }

+            foreach (PackageSource packageSource in listPackageArgs.AuditSources)


I would have liked the PR description to have addressed my question, but from https://learn.microsoft.com/en-us/nuget/concepts/auditing-packages#audit-sources

If auditSources is not defined or is cleared without adding any sources, then packageSources will be used and warning NU1905 is suppressed.

So, for restore, when nuget.config defines one or more audit sources, then vulnerability information is not retrieved from the package sources, only from the audit sources.

This PR does not appear to follow this behaviour for dotnet list package.

In theory, if a package source lists a package as vulnerable, but the audit source does not, this will cause dotnet list package --vulnerable to have different results to dotnet restore.

If it's intentional that dotnet list package --vulnerable will work differently, I'm curious why

zivkan · 2024-12-27T23:50:23Z

....CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommandRunner.cs

@@ -176,6 +179,18 @@ private static void WarnForHttpSources(ListPackageArgs listPackageArgs, ListPack
                }
            }

+            foreach (PackageSource packageSource in listPackageArgs.AuditSources)
+            {
+                if (packageSource.IsHttp && !packageSource.IsHttps && !packageSource.AllowInsecureConnections)


There's no else block, so if any sources defined in nuget.config are not used because it's using insecure http, it will be silently ignored. I believe all or other commands report an error, so this should as well.

zivkan · 2024-12-28T00:05:17Z

....CommandLine.XPlat/Commands/PackageReferenceCommands/ListPackage/ListPackageCommandRunner.cs

+                foreach (var source in listPackageArgs.AuditSources)
+                {
+                    var repository = Repository.Factory.GetCoreV3(source);
+                    var vulnerabilityProvider = new VulnerabilityInfoResourceV3Provider();
+                    var result = await vulnerabilityProvider.TryCreate(repository, listPackageArgs.CancellationToken);


Package sources can provide the VulnerabilityInfoResource as well as audit sources, and that's actually how audit during restore works. An issue was created for this before auditSources was implemented, so maybe some people would consider it scope creep because the issue to add support for auditSources didn't explicitly call it out, but it's very related to the work currently being done, so I feel like it's a good time to implement it at the same time:

dotnet list package --vulnerable should use new server VulnerabilityInfo resource Home#13026

Implementing this will give dotnet list package --vulnerable a significant performance boost when there are many packages to check, because NuGet can download the database once and then do a whole lot of lookups, instead of making an HTTP request to every source for every package id.

zivkan · 2024-12-28T05:43:48Z