Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding interface, implementation, and tests for vulnerability capabilities for a package domain model #6279

Open
wants to merge 2 commits into
base: dev-feature-PackageDomainModel
Choose a base branch
from
+160 −0
Open

Adding interface, implementation, and tests for vulnerability capabilities for a package domain model #6279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0901c77
Adding interface, implementation, and tests for vulnerability capabil…
jebriede Feb 19, 2025
a4e0382
Enable nullable and enforce required paramaters
jebriede Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/NuGet.Clients/NuGet.PackageManagement.UI/Models/Package/IVulnerable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

#nullable enable

using System.Collections.Generic;
using NuGet.Protocol;
using NuGet.VisualStudio.Internal.Contracts;

namespace NuGet.PackageManagement.UI
{
interface IVulnerable
{
public IEnumerable<PackageVulnerabilityMetadataContextInfo> Vulnerabilities { get; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use more concrete types to avoid paying the enumerator costs?


public bool IsVulnerable { get; }

public PackageVulnerabilitySeverity VulnerabilityMaxSeverity { get; }
}
}
48 changes: 48 additions & 0 deletions src/NuGet.Clients/NuGet.PackageManagement.UI/Models/Package/VulnerableCapability.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

#nullable enable

using System;
using System.Collections.Generic;
using System.Linq;
using NuGet.Protocol;
using NuGet.VisualStudio.Internal.Contracts;

namespace NuGet.PackageManagement.UI
{
public class VulnerableCapability : IVulnerable
{
private IEnumerable<PackageVulnerabilityMetadataContextInfo> _vulnerabilities = [];

public IEnumerable<PackageVulnerabilityMetadataContextInfo> Vulnerabilities
{
get => _vulnerabilities;
private set => _vulnerabilities = value.OrderByDescending(v => v?.Severity ?? -1);
}

public bool IsVulnerable => Vulnerabilities.Any(v => v != null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so you're saying it's possible to have a list of vulnerabilities where all entries are null?

Copy link
Contributor Author

@jebriede jebriede Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, programmatically. It doesn't make much sense to have that, but there a few strategies I can think of:

  1. Prevent the construction of a VulnerableCapability if the collection of vulnerabilities contains any nulls. This would require iterating over the collection upon construction to validate if any of the entries are null. With big lists this could take a performance hit for constructing many Packages with IVulnerable and I would not favor this.
  2. Allow the collection to contain nulls because it's just a list of objects, but handle them gracefully so we essentially treat them as if they did not exist. This is what I've done in this PR and made that behavior explicit by adding a test around it and to ensure that it is handled gracefully.

Can you think of another way you'd suggest instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I think iterating through them to validate there aren't any nulls seems excessive. I guess I didn't realize we had been accounting for this in the old code too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgonz120 We aren't accounting for it explicitly in the ViewModels today which presents a potential flaw. The goal here is to make this code more robust than what we had before and put it under test. By handling this edge case gracefully, we avoid any NRE's in case a null makes its way into the list at any point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has #nullable enable, and the definition of the Vulnerabilities property doesn't allow nulls, so either the definition is wrong, or this not null check shouldn't be needed.

Copy link
Contributor Author

@jebriede jebriede Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has #nullable enable, and the definition of the Vulnerabilities property doesn't allow nulls, so either the definition is wrong, or this not null check shouldn't be needed.

I see what you mean. Both the tests and the VulnerabilityCapability classes themselves have #nullable enable yet the Test allows a null value to be used. As such, it proves that with nullable enabled on the caller, we can still end up with nulls making their way into the list. While a test can show that it's possible, we should have code that handles this edge case and a test that proves it's being handled. If I wrote the test incorrectly though, please let me know how and I'll fix it!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this class immutable and avoid paid the cost of Any.
In general, I'd avoid properties that force a compute that's not cached.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, the current implementation of calculating the IsVulnerable property gets the Vulnerabilities property, which in turn uses LINQ's OrderByDescending API. Therefore, in order to determine if the current package is vulnerable, the code is going to allocate a list of equal or greater capacity to the _vulnerabilities enumerable, spend CPU cycles sorting it, then just check if there's a first item, without caring about the contents of it. The .Any() call causes an enumerator to be allocated on the stack, as well as the OrderByDescending call. Both of these methods pass in a predicate, and unless the C# compiler has improved since I last checked, they both cause heap allocations as well (could be avoided by creating class methods and passing it without using an anonymous function, but that makes the code less pretty).

Referring to Nikolche's other comment about using a concrete class, all these heap allocations could be avoided by using a concrete class, and making this property's body => _vulnerabilities.Count > 0

Copy link
Contributor Author

@jebriede jebriede Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, the current implementation of calculating the IsVulnerable property gets the Vulnerabilities property, which in turn uses LINQ's OrderByDescending API. Therefore, in order to determine if the current package is vulnerable, the code is going to allocate a list of equal or greater capacity to the _vulnerabilities enumerable, spend CPU cycles sorting it, then just check if there's a first item, without caring about the contents of it.

The .Any() call causes an enumerator to be allocated on the stack, as well as the OrderByDescending call

This is not correct. The OrderByDescending is in the setter of the Vulnerabilities property, not in the getter. Therefore, it will only be sorted once when assigned privately, not upon each access.


public PackageVulnerabilitySeverity VulnerabilityMaxSeverity
{
get
{
// Vulnerabilities are ordered so the first element is always the highest severity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great for performance, not sure if we are currently iterating. More context: https://learn.microsoft.com/en-us/nuget/api/vulnerability-info#vulnerability-page.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is not officially part of the contract, we shouldn't depend on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, there's no way to guarantee the list will be ordered from any and all vulnerability sources, both current and future. This comment points out that the list is ordered. It's ordered in this class (see the setter) explicitly to avoid making assumptions about the data source. Does that help? Do you have any suggested edits?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, right? The list of known vulnerabilities for a package should be sorted in descending order of the max version of the version range.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't that talk about the way the version ranges are sorted and not the list of vulnerabilities?

int? severity = Vulnerabilities.FirstOrDefault()?.Severity;
if (severity != null && Enum.IsDefined(typeof(PackageVulnerabilitySeverity), severity))
{
return (PackageVulnerabilitySeverity)severity;
}
else
{
return PackageVulnerabilitySeverity.Unknown;
}
}
}

public VulnerableCapability(IEnumerable<PackageVulnerabilityMetadataContextInfo> vulnerabilities)
{
Vulnerabilities = vulnerabilities ?? throw new ArgumentNullException(nameof(vulnerabilities));
}
}
}
92 changes: 92 additions & 0 deletions ...Clients.Tests/NuGet.PackageManagement.UI.Test/Models/Package/VulnerableCapabilityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

#nullable enable

using System;
using System.Collections.Generic;
using System.Linq;
using NuGet.Protocol;
using NuGet.VisualStudio.Internal.Contracts;
using Xunit;

namespace NuGet.PackageManagement.UI.Test
{
public class VulnerableCapabilityTests
{
[Theory]
[InlineData(1, true)]
[InlineData(0, false)]
public void IsVulnerable_VariousVulnerabilities_ReturnsExpected(int severity, bool expected)
{
// Arrange
IEnumerable<PackageVulnerabilityMetadataContextInfo> vulnerabilities = severity > 0
? [new(new Uri("http://example.com"), severity)]
: [];
var vulnerableCapability = new VulnerableCapability(vulnerabilities);

// Act
var result = vulnerableCapability.IsVulnerable;

// Assert
Assert.Equal(expected, result);
}

[Fact]
public void IsVulnerable_NullVulnerabilityInCollection_ReturnsFalse()
{
// Arrange
List<PackageVulnerabilityMetadataContextInfo> vulnerabilities = [null];
var vulnerableCapability = new VulnerableCapability(vulnerabilities);

// Act
var result = vulnerableCapability.IsVulnerable;

// Assert
Assert.False(result);
}

[Theory]
[InlineData(new int[] { (int)PackageVulnerabilitySeverity.Low, (int)PackageVulnerabilitySeverity.High }, PackageVulnerabilitySeverity.High)]
[InlineData(new int[] { (int)PackageVulnerabilitySeverity.Critical, (int)PackageVulnerabilitySeverity.Unknown }, PackageVulnerabilitySeverity.Critical)]
[InlineData(new int[] { }, PackageVulnerabilitySeverity.Unknown)]
[InlineData(new int[] { -2 }, PackageVulnerabilitySeverity.Unknown)]
public void VulnerabilityMaxSeverity_VariousVulnerabilities_ReturnsExpected(int[] severities, PackageVulnerabilitySeverity expected)
{
// Arrange
var vulnerabilities = severities.Select(severity => new PackageVulnerabilityMetadataContextInfo(new Uri("http://example.com"), severity)).ToList();
var vulnerableCapability = new VulnerableCapability(vulnerabilities);

// Act
var result = vulnerableCapability.VulnerabilityMaxSeverity;

// Assert
Assert.Equal(expected, result);
}

[Fact]
public void Constructor_OrdersVulnerabilitiesBySeverity_DescendingOrder()
{
// Arrange
var vulnerabilities = new List<PackageVulnerabilityMetadataContextInfo>
{
new(new Uri("http://example.com/high"), (int)PackageVulnerabilitySeverity.High),
new(new Uri("http://example.com/low"), (int)PackageVulnerabilitySeverity.Low),
new(new Uri("http://example.com/moderate"), (int)PackageVulnerabilitySeverity.Moderate),
new(new Uri("http://example.com/critical"), (int)PackageVulnerabilitySeverity.Critical)
};

// Act
var vulnerableCapability = new VulnerableCapability(vulnerabilities);
var orderedVulnerabilities = vulnerableCapability.Vulnerabilities.ToList();

// Assert
Assert.Collection(orderedVulnerabilities,
item => Assert.Equal((int)PackageVulnerabilitySeverity.Critical, item.Severity),
item => Assert.Equal((int)PackageVulnerabilitySeverity.High, item.Severity),
item => Assert.Equal((int)PackageVulnerabilitySeverity.Moderate, item.Severity),
item => Assert.Equal((int)PackageVulnerabilitySeverity.Low, item.Severity)
);
}
}
}