Adding a proposal for the new CLI command to fix vulnerabilities

[OliaG]

No description provided.

[nkolev92]

Mid-way through I realized this was still a WIP, but I finished my review.

hopefully these are helpful.
Happy to chat more as needed.

[nkolev92]

Packages are installed on a per project basis, so this would probably be updating projects.

Solution updates & project updates can be different.
There's some mechanics to work out potentially here, especially in non-CPM scenario.

For example, an update to package A from 1.0.0 to 2.0.0 in project 1 may succeed, but may fail in project 2.

[nkolev92]

Does this address direct only or both direct & transitive?
Is the audit mode considered at all?

[martinrrm]

Since this command is going to be used in solutions too, I think we need to be more detailed in this json format. For example, let's say that dotnet audit fix resolves to different package version for different projects, or for different target frameworks, I think we need to reflect that in the JSON output.

Another edge case I can think of is when customer has project references. Let's say that ProjectA has a reference to ProjectB that contains a vulnerable package, since those packages are considered transitive will the solution be updating the package in ProjectB and install it in ProjectA?

With those considerations I was thinking something like this:

{
	"version": "string",
	"feasible": true,
	"metadata": 
	{
		"added": "number",
		"removed": "number",
		"updated": "number",
		"promotedToTopLevel": "number",
	},
	"vulnerabilities":
	{
		"projectA":
		{
			"net8.0":
			{
				"total": "number",
				"newtonsoft.json": 
				{
					"originalVersion": "12.0.1", // Could be ranges too
					"newVersion": "13.0.1",
					"promotedToTopLevel": true,
					"projectReference": true, // Could be a path to the project reference
					...
				},
				"castle.core":
				{
					...
				},
			},
			"net7.0":
			{
				...
			},	
		},
		"projectB":
		{
			...
		},
	},
}

@OliaG @zivkan What do you think about this JSON proposal?

[zivkan]

I have a more comments than I thought I would:

1. The proposed schema doesn't provide a place to output warning or error messages. If the command fails, the error should still be in a valid JSON, to make it easier to script. I believe that having an errors or messages array removes the need for a feasible bool, since any result without any errors is by definition feasible.
2. Some languages make it hard to use json properties that are not valid language variable names, so property keys like "net8.0" is problematic, as is using project names. Even when your language allows you to parse it, it can be harder to use since property/key names are dynamic. In some languages it's much easier when the schema is static. For example, consider trying to use json query to search the result, or use Convert-FromJson in PowerShell.
3. On the topic of project names, it's possible to have multiple projects in a solution have the same name, as long as they're in different subdirectories, so we have to use the full path to the project, not just the name, otherwise in edge cases they won't be unique.
4. In the case of a package being transitive through a project reference, I think the tool should be smart enough to not update the package. In that case, since the package isn't being updated, there's no need for a "projectReference" property.
5. dotnet package update will be used for all kinds of updates, not just vulnerable packages. I think Olia's "added":, "updated":, and "removed": properties make more sense than a vulnerabilities property key. Although in that property you have a collection of projects, so projects is a more appropriate name.
6. I haven't been looking at JSON other than NuGet's own for years, so I'm quite out of touch with JSON best practises. But I don't understand the purpose of putting the counts within a metadata object. Is there a benefit over putting the counts in the parent object?
7. I feel like the counts should be per-project, not at the root object level. Having it at the root object level raises questions, like if multiple projects have to promote the same package to top level, does that count as 1, or is the promote to top level count going to equal the number of projects? However, I don't understand the point of the count properties, since you can look at look at the length of the array of package updates.
8. JSON numbers aren't quoted. Some languages/parsers might be loosey-goosey, but strict parsers will fail. I feel like NuGet had this problem when migrating one of our parsers from Newtonsoft.Json (using JToken) to System.Text.Json.
9. While nuget.org doesn't have a package with id total, it is a valid package ID, so it should not be used in a place where package IDs are also expressed. However, I already mentioned we shouldn't have a dynamic schema, so this concern should automatically be solved when addressing where to put package ids.
10. Similar to the "I'm not familiar with JSON best practises" comment I made earlier, I'm not sure if long property names originalVersion, newVersion, promotedToTopLevel are commonly used. from and to I think are easy enough to understand, as are original and new. Assuming we have some docs somewhere that explains what it means, I think promoted might be a good enough name. promotedToTopLevel might be guessable without docs. However, I just had the thought that since it's possible to tell the difference between an "addition" vs an "upgrade", there could be a property wasTransitive, and I think that will be more clear to people than promoted

[zivkan]

Suggested change

	This improvement will, in turn, bolster the overall security and reliability of .NET applications. The feature will build upon the existing NuGet Audit functionality.
	This improvement will, in turn, bolster the overall security and reliability of .NET applications.
	The feature will build upon the existing NuGet Audit functionality.

[zivkan]

Suggested change

dotnet

[zivkan]

I think the comparison between VS and the dotnet CLI woud be easier to understand as a table. For example:

Capability	VS	CLI
do thing	✔️	✔️
something else	✔️	❌

[zivkan]

Suggested change

	C:\> dotnet package update [<SOLUTION_PATH>|<PROJECT_PATH_>]
	C:\> dotnet package update [<SOLUTION_PATH>|<PROJECT_PATH>]

[zivkan]

Suggested change

	- 1 - The command will exit with a 1 exit code if changes were successful and the PR is submitted.
	- 2 - The command will exit with a 2 exit code if any error has occurred, PR will not be submitted.
	- 1 - The command will exit with a 1 exit code if changes were successful.
	- 2 - The command will exit with a 2 exit code if any error has occurred.

dotnet package update won't automate creating pull requests, just modifying local files. The developer, or other tools that use dotnet package update, will be responsible for creating the pull request.

[zivkan]

I'm not sure where in the document it should be written, but while talking to an internal partner, they suggested that packages that are promoted to top level in order to remove a vulnerable package should have some additional metadata to signal this. For example <PackageReference Include="SomePackage" Version="1.2.3" PromotedBecauseOfVulnerability="true" />. A much, much better name is needed though, this is just an example.

If in the future the top level package that originally caused SomePackage to be included in the package graph has a new version that removes SomePackage from the package graph, this extra metadata will allow a future improvement to dotnet update package to detect that the PackageReference can now be removed.

I think this is a great idea for projects that aren't using CPM with transitive pinning. Although it's also a signal that transitive pinning without CPM would be a useful feature, but very much out of scope for this feature 😁

[zivkan]

I think it would be nice to have a summary of all features, similar to what you'd expect to see when you run dotnet package update -h. The sub-feature examples below are good, but I had an idea for something, and a ctrl-f can't find it in the document. Without re-reading the whole thing top to bottom, I don't have an easy way to figure out if I'm searching for the wrong keyword or if it's something I need to suggest.

And that is that I think a --dry-run option would be good. But now I feel like this was originally proposed. Did we remove it for some reason that I can't remember?

[zivkan]

Suggested change

	The `dotnet package update` command will update all the NuGet dependencies to the latest versions.
	The `dotnet package update` command without any other arguments or options will update all the NuGet dependencies to the latest versions.

[zivkan]

above, the json option was --format json, but here it's just --json. As someone else commented, we already have prior art with --format json

[zivkan]

Suggested change

	This flag will try to update only packages that have direct or transitive vulnerabilities. The remediation is calculated with an implicit dotnet audit to then apply directly to a resulting package graph. It can add packages, remove packages, and update packages depending on the problem it's attempting to resolve. It does not take into consideration downgrading to a compatible version if a higher one has already been specified.
	This flag will try to update only packages that have direct or transitive vulnerabilities.
	The remediation is calculated with an implicit dotnet audit to then apply directly to a resulting package graph.
	It can add packages, remove packages, and update packages depending on the problem it's attempting to resolve.
	It does not take into consideration downgrading to a compatible version if a higher one has already been specified.

I like one sentence per line, because it makes it easier to comment on specific things.

In particular:

1. In another comment, I wrote that if we tag packages promoted to top level, then later on we can remove it as a top level package when we detect it's no longer needed as top level. Without this feature, I'm not sure I can think of any scenarios where we will remove a package reference.
2. I shouldn't be working this late at night, but at the moment, I don't understand the last sentence. Is this in reference to packages where new versions no longer support target frameworks that the current project is using, so while the higher package version no longer has a known vulnerability, it's not compatible with the project? The "if a higher one has already been specified" part also throws me off. At this time of night, I can't guess what this means.