Show vulnerability indicators for transitive vulnerable packages#5384
Show vulnerability indicators for transitive vulnerable packages#5384
Conversation
src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageManagerControl.xaml.cs
Show resolved
Hide resolved
|
Your fixes statement in your PR description is broken. You need a link in the |
nkolev92
left a comment
There was a problem hiding this comment.
Seems reasonable.
I have suggestions, but nothing that can't be addressed in a future PR.
src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/DetailControl.xaml.cs
Show resolved
Hide resolved
|
|
||
| ISourceRepositoryProvider sourceRepositoryProvider = await ServiceLocator.GetComponentModelServiceAsync<ISourceRepositoryProvider>(); | ||
| var sourceRepositories = sourceRepositoryProvider.GetRepositories(); | ||
| _packageVulnerabilityService = new PackageVulnerabilityService(sourceRepositories, _uiLogger); |
There was a problem hiding this comment.
Sources can change. You probably want to listen to settings changes and reload the sources.
There was a problem hiding this comment.
Made note of this for a follow-up. Thanks!
| return null; | ||
| } | ||
|
|
||
| using (var sourceCacheContext = new SourceCacheContext()) |
There was a problem hiding this comment.
Share the SourceCacheContext among source repositories.
There was a problem hiding this comment.
The static extension methods operate on a single source repository. In order to share SourceCacheContext among various source repositories, we would need to supply the SourceCacheContext along with each of the method calls in the SourceRepositoryExtensions. The current precedent set by this static class and its methods is to create a new SourceCacheContext per each call, which is consistent with what we're doing here.
An alternative design would be to store a static reference to the SourceCacheContext but then we'd need methods on this static class that manages the lifecycle of the SourceCacheContext. Ideally, we would have a better way to manage a collection of source repositories and their operations and share the SourceCacheContext across multiple repos.
I'm making note of your suggestion as an improvement that should be made, but it's out of scope of this feature as this simply follows an existing pattern using an existing mechanism which can be improved upon, but was not introduced in this change.
There was a problem hiding this comment.
The current precedent set by this static class and its methods is to create a new SourceCacheContext per each call, which is consistent with what we're doing here.
I see. That's a poor pattern. See 9ea2def.
The caching context should be shared so that any caching decisions don't need to be changed in 10 different places.
I think we should change that, I've created an issue for that NuGet/Home#12859.
simply follows an existing pattern using an existing mechanism which can be improved upon, but was not introduced in this change.
I don't think we should be propagating bad patterns, even if it's a preexisting pattern. However, I think it's reasonable to say that all of those discussions should be handled on a case by case basis, as the cost/impact is different for each one individually.
My suggestion is to just pass down the source cache context, but I'm not too concerned about this single change, so feel free to make a call.
There was a problem hiding this comment.
@nkolev92 thanks for creating the follow-up issue!
| _logger = logger ?? throw new ArgumentNullException(nameof(logger)); ; | ||
| } | ||
|
|
||
| public async Task<IEnumerable<PackageVulnerabilityMetadataContextInfo>> GetVulnerabilityInfoAsync(PackageIdentity packageId, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Please use List to avoid constant allocations.
src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Services/PackageVulnerabilityService.cs
Show resolved
Hide resolved
…ulnerable to show warning indicator by Installed tab and by transitive package in package list
b097f4f to
486d803
Compare
Bug
Fixes: NuGet/Home#8756
Regression? Last working version: N/A
Description
Fetch vulnerabilities database for all enabled sources from VulnerabilityInfoResource and check if transitive packages are vulnerable to show warning indicator by Installed tab and by transitive package in package list.
Updated the install / uninstall / update action code paths that log telemetry to include a count of transitive vulnerable packages and maximum severity of each package.
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Automated tests added
Updated existing test for telemetry although the test does not fully cover the scenario so we should consider adding additional tests in the future. Code copied from AuditUtility does not have tests as we are going to refactor the code to be shareable and put it under test.
OR
Documentation