Add vulnerability messaging to package details page #8358

drewgillies · 2020-12-17T07:54:44Z

Completes: #7650

Here we change the view to include a vulnerabilities panel for an affected package version.

Non-owner view, not expanded:

Owner view, expanded:

bangs and tooltips in the version list (combined for deprecations and vulnerabilities). Again, formats and icons may be tweaked going forward.

Vulnerability only:

Vulnerability + deprecation:

skofman1 · 2020-12-17T19:46:28Z

src/NuGetGallery/Views/Packages/DisplayPackage.cshtml

+                                            useAlso = true;
+                                        }
+
+                                        if (Model.IsPackageVulnerabilitiesEnabled && packageVersion.Vulnerabilities != null && packageVersion.Vulnerabilities.Any())


packageVersion.Vulnerabilities != null && packageVersion.Vulnerabilities.Any() [](start = 85, length = 78)

This condition is repeated a couple of times. Can we move it to a shared location?

skofman1 · 2020-12-17T19:47:39Z

src/NuGetGallery/Views/Packages/DisplayPackage.cshtml

+                                        {
+                                            vulnerabilitiesTitle = useAlso ? "Also, " : "";
+                                            var severity = Enum.GetName(typeof(PackageVulnerabilitySeverity), packageVersion.MaxVulnerabilitySeverity).ToLowerInvariant();
+                                            vulnerabilitiesTitle += packageVersion.Version + " has at least one vulnerability with " + severity + " severity.";


packageVersion.Version + " has at least one vulnerability with " + severity + " severity."; [](start = 68, length = 91)

consider string.Format for readability.

Done--thanks. I wanted to do an interpolated string, but we're targeting a .NET version that doesn't support them in razor. string.Format is 👍 though.

skofman1 · 2020-12-17T19:49:48Z

src/NuGetGallery/Views/Packages/DisplayPackage.cshtml

@@ -753,15 +772,16 @@
                                            </td>
                                        }
                                    }
-                                    @if (Model.IsPackageDeprecationEnabled)
+
+                                    @if (Model.IsPackageDeprecationEnabled || Model.IsPackageVulnerabilitiesEnabled)
                                    {


There's a bunch of logic here and it's not testable because it's in the cshtml. Consider moving it to the ViewModel so you can add UTs

I've taken a simpler approach here and called the column "Package Warnings"--I don't think we need to customize a hidden column header based on feature flags.

If I understand correctly, we have a code block here to obtain "iconTitle", and set up a warning icon with the associated title based on some checks. Maybe it's better that we add another property named "warningIconTitle" (and with another additional boolean property named "hasWarningIcon") in the view model, and when we create the view model in the factory, we can run these checks and set up this property. Then in the view, we can easily check this property to determine whether to show the icon and title for a specific version.
In this way, we can move the logic of this code block to the factory, and some tests can be added to cover these checks and messages.

Good call--this is much more robust now.

zhhyu · 2020-12-29T06:31:48Z

src/NuGetGallery/Views/Packages/DisplayPackage.cshtml

                            {
-                                <th aria-hidden="true" abbr="Deprecation Information"></th>
+                                <th aria-hidden="true" abbr ="Package Warnings" />


Should we still keep </th> here?

I've made this consistent--thanks.

zhhyu · 2020-12-29T08:13:20Z

src/NuGetGallery/Scripts/gallery/page-display-package.js

+    var expanderAttributes = ['data-toggle', 'data-target', 'aria-expanded', 'aria-controls', 'tabindex'];
+    var vulnerabilitiesContainer = $('#show-vulnerabilities-content-container');
+    if ($('#vulnerabilities-content-container').children().length) {
+        // If the deprecation information container has content, configure it as an expander.


nit: "// If the vulnerabilities information container ..."

zhhyu · 2020-12-29T19:34:52Z

src/Bootstrap/less/theme/page-display-package.less

@@ -63,6 +63,50 @@
    }
  }

+  .vulnerabilities-container {


Maybe it's better that we share styles with the deprecation container? We can have a "warning-container" to define these shared styles, and it will be easier to maintain if we need more warning containers in the future.

@zhhyu Looks like this change isn't practical--the js script that toggles chevron state uses the outer style name to identify the panel on which to rotate it when expanded. Messing with an outer style name has cascading effects. It could be feasible to change inner style names but this wouldn't achieve much as the names would then be duplicated in each outer style.

zhhyu · 2020-12-29T20:00:15Z

src/NuGetGallery/Helpers/ViewModelExtensions/DisplayPackageViewModelFactory.cs

+            else
+            {
+                viewModel.Vulnerabilities = new List<PackageVulnerability>();
+                viewModel.MaxVulnerabilitySeverity = PackageVulnerabilitySeverity.Low;


Are there any considerations to have an empty list and a low "MaxVulnerabilitySeverity" if there are no vulnerabilities? I see that we check "Vulnerabilities.Any()" in the view. Maybe it's better that we just leave "Vulnerabilities" as null, and take a null check of "Vulnerabilities" when there are no vulnerabilities? Setting "MaxVulnerabilitySeverity" as low seems a little confusing.

There's no consideration except that it defaults to low. Changing it now would be disruptive to the db and I'm not sure much would be gained. I have made the null change though.

zhhyu · 2020-12-29T20:05:33Z

src/NuGetGallery/Scripts/gallery/page-display-package.js

@@ -5,22 +5,36 @@ $(function () {
    window.nuget.configureExpander("rename-content-container", "ChevronDown", null, "ChevronUp");
    configureExpanderWithEnterKeydown($('#show-rename-content-container'));

+    // Configure the vulnerability information container


nit: maybe it's better to put this comment below the next line as "expanderAttributes" is also used for the deprecation container.

zhhyu · 2020-12-29T20:11:16Z

src/NuGetGallery/Views/Packages/_DisplayPackageVulnerabilities.cshtml

+                        if (truncatedUrl.Length > 50)
+                        {
+                            truncatedUrl = truncatedUrl.Substring(0, 47) + "...";
+                        }


How do we pick up the number 50 or 47? This may lead to the accessibility issue @ryuyu. How will a page reflow affect the appearance? Feel free to ignore this if we have already used the trick.

I'm removing this truncation. It will simply wrap if it needs to, and the table caters for that nicely now.

drewgillies · 2021-01-07T05:45:17Z

Note that I've also made calls to the deprecation and vulnerabilities services conditional on the respective feature flags (for perf and cleanness), so UTs needed updating to not verify calls against them that we no longer expect to happen.

skofman1

zhhyu · 2021-01-08T07:11:42Z

src/NuGetGallery/Helpers/ViewModelExtensions/DisplayPackageViewModelFactory.cs

+            PackageDeprecation deprecation,
+            PackageVulnerabilitySeverity? maxSeverity)
+        {
+            // We want a tooltip title for the warning icon, which concatenates deprecation and vulnerability information cleanly


nit: empty line

zhhyu · 2021-01-08T07:13:08Z

src/NuGetGallery/Helpers/ViewModelExtensions/DisplayPackageViewModelFactory.cs

            }

+            viewModel.PackageWarningIconTitle =
+                GetWarningIconTitle(viewModel.Version, deprecation, maxSeverity);


nit: maybe it's more clear as: maxSeverity -> maxVulnerabilitySeverity

zhhyu · 2021-01-08T07:16:23Z

src/NuGetGallery/Views/Packages/DisplayPackage.cshtml

-                                        }
+                                    @if (string.IsNullOrEmpty(packageVersion.PackageWarningIconTitle))
+                                    {
+                                        <td class="package-icon-cell" aria-hidden="true" />


Should we still keep </td> here?

I'm not sure of the advantage of empty tags over self-closing ones. I've changed this to an empty tag for consistency, but it just seems like a waste of chars to me. Self-closing tags are neater IMHO.

zhhyu · 2021-01-08T08:05:05Z

tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs

+            [InlineData(false, true, "{0} has at least one vulnerability with {1} severity.")]
+            [InlineData(true, true, "{0} is deprecated because it's legacy and no longer maintained; {0} has at least one vulnerability with {1} severity.")]
+            public async Task ShowsCombinedDeprecationAndVulnerabilitiesIconTitle(
+                bool isDeprecationEnabled,


Great test!

drewgillies requested review from agr, cristinamanum, dannyjdev, joelverhagen, loic-sharma, lyndaidaii, ryuyu, shishirx34, skofman1, xavierdecoster and zhhyu as code owners December 17, 2020 07:54

drewgillies changed the title ~~Dg vulnerabilitymessaging~~ Add vulnerability messaging to package details page Dec 17, 2020

drewgillies force-pushed the dg-vulnerabilitymessaging branch 2 times, most recently from 7d3afb4 to 22e3b63 Compare December 17, 2020 08:02

skofman1 reviewed Dec 17, 2020

View reviewed changes

drewgillies force-pushed the dg-vulnerabilityservice-and-featureflags branch from ec2c439 to d0c7212 Compare December 17, 2020 20:39

Base automatically changed from dg-vulnerabilityservice-and-featureflags to dev December 17, 2020 20:51

drewgillies force-pushed the dg-vulnerabilitymessaging branch 9 times, most recently from d31453f to 5be36a5 Compare December 21, 2020 05:20

This was referenced Dec 22, 2020

Expand view models for vulnerabilities in manage packages page #8362

Closed

Show vulnerability alerts on manage packages page #8363

Closed

drewgillies mentioned this pull request Dec 22, 2020

[Epic] NuGet Package Vulnerability Auditing NuGet/Home#8087

Open

zhhyu reviewed Dec 29, 2020

View reviewed changes

drewgillies force-pushed the dg-vulnerabilitymessaging branch 3 times, most recently from c5776ce to 91f55fa Compare January 7, 2021 05:31

drewgillies force-pushed the dg-vulnerabilitymessaging branch from 91f55fa to 6975afc Compare January 7, 2021 05:52

drewgillies requested a review from zhhyu January 7, 2021 05:52

skofman1 approved these changes Jan 7, 2021

View reviewed changes

drewgillies added 2 commits January 8, 2021 09:53

Surface vulnerabilities in package detail page

a3c8701

Address feedback

65b1ace

drewgillies force-pushed the dg-vulnerabilitymessaging branch from 6975afc to 65b1ace Compare January 8, 2021 00:05

zhhyu reviewed Jan 8, 2021

View reviewed changes

Addressed feedback 2

1b76cc3

zhhyu reviewed Jan 8, 2021

View reviewed changes

zhhyu approved these changes Jan 8, 2021

View reviewed changes

drewgillies merged commit 5ce2f98 into dev Jan 8, 2021

drewgillies deleted the dg-vulnerabilitymessaging branch January 8, 2021 10:28

zhhyu mentioned this pull request Jan 28, 2021

[On-call] 01/25/2021 Deployment #8402

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vulnerability messaging to package details page #8358

Add vulnerability messaging to package details page #8358

drewgillies commented Dec 17, 2020 •

edited

Loading

skofman1 Dec 17, 2020

skofman1 Dec 17, 2020

drewgillies Dec 18, 2020

skofman1 Dec 17, 2020

drewgillies Dec 20, 2020

zhhyu Dec 29, 2020 •

edited

Loading

drewgillies Jan 7, 2021

zhhyu Dec 29, 2020 •

edited

Loading

drewgillies Jan 7, 2021

zhhyu Dec 29, 2020

zhhyu Dec 29, 2020

drewgillies Jan 7, 2021

zhhyu Dec 29, 2020

drewgillies Jan 7, 2021

zhhyu Dec 29, 2020

zhhyu Dec 29, 2020 •

edited

Loading

drewgillies Jan 7, 2021

drewgillies commented Jan 7, 2021

skofman1 left a comment

zhhyu Jan 8, 2021

zhhyu Jan 8, 2021

zhhyu Jan 8, 2021 •

edited

Loading

drewgillies Jan 8, 2021

zhhyu Jan 8, 2021

drewgillies Jan 8, 2021

Add vulnerability messaging to package details page #8358

Add vulnerability messaging to package details page #8358

Conversation

drewgillies commented Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu Dec 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu Dec 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu Dec 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewgillies commented Jan 7, 2021

skofman1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhhyu Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewgillies commented Dec 17, 2020 •

edited

Loading

zhhyu Dec 29, 2020 •

edited

Loading

zhhyu Dec 29, 2020 •

edited

Loading

zhhyu Dec 29, 2020 •

edited

Loading

zhhyu Jan 8, 2021 •

edited

Loading