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.

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

feat: include PkgPath information in image cve list and list export in zui #426

Closed
wants to merge 1 commit into from

Conversation

vrajashkr
Copy link
Contributor

What type of PR is this?
feature

Which issue does this PR fix:
Partially addresses project-zot/zot#2175

What does this PR do / Why do we need it:
This PR displays the Package Path information for the package list for a given CVE in the vulnerabilities list.
Since there is more data being displayed, this PR also brings in a change to display this information in the form of a card that is vertically arranged.

Testing done on this change:
Screenshots:
Screenshot 2024-02-22 at 00 33 33
Screenshot 2024-02-22 at 00 33 49
Screenshot 2024-02-22 at 00 34 40
Screenshot 2024-02-22 at 23 29 24
Screenshot 2024-02-22 at 00 34 15

CSV export:

id,severity,title,description,reference,packageName,packagePath,packageInstalledVersion,packageFixedVersion
CVE-2016-1000027,CRITICAL,spring: HttpInvokerServiceExporter readRemoteInvocation method untrusted java deserialization,"Pivotal Spring Framework through 5.3.16 suffers from a potential remote code execution (RCE) issue if used for Java deserialization of untrusted data. Depending on how the library is implemented within a product, this issue may or not occur, and authentication may be required. NOTE: the vendor's position is that untrusted data is not an intended use case. The product's behavior will not be changed because some users rely on deserialization of trusted data.",https://avd.aquasec.com/nvd/cve-2016-1000027,org.springframework:spring-web,usr/local/artifacts/spring-web-5.3.31.jar,5.3.31,6.0.0

XLSX export:
Screenshot 2024-02-22 at 23 38 56

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
Ideally, this should not break upgrades or downgrades as the older graphQL query should continue working just fine as well as the updated query.
No, updating a running cluster has not been tested.

Does this change require updates to the CNI daemonset config files to work?:
N/A

Does this PR introduce any user-facing change?:
Yes

The package list for a given CVE is now displayed in the form of vertically arranged cards.
Additionally, the package path for a given CVE is also displayed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@vrajashkr
Copy link
Contributor Author

Hi @andaaron , @rchincha , the PR is ready for review. Looking forward to your feedback and suggestions. Thank you!

Copy link

codecov bot commented Feb 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.87%. Comparing base (33524ce) to head (4e76918).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #426      +/-   ##
==========================================
+ Coverage   82.82%   82.87%   +0.04%     
==========================================
  Files          62       63       +1     
  Lines        1875     1880       +5     
  Branches      483      483              
==========================================
+ Hits         1553     1558       +5     
  Misses        311      311              
  Partials       11       11              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/__tests__/TagPage/VulnerabilitiesDetails.test.js Outdated Show resolved Hide resolved
src/__tests__/TagPage/VulnerabilitiesDetails.test.js Outdated Show resolved Hide resolved
src/components/Shared/VulnerabilityPackageCard.jsx Outdated Show resolved Hide resolved
src/components/Shared/VulnerabilityPackageCard.jsx Outdated Show resolved Hide resolved
@raulkele
Copy link
Collaborator

Thank you @vrajashkr for the PR!
In addition to the code comments.
Design-wise I'm not necessarily the biggest fan of the extra nested card. I feel like maybe a section for each Package with the Package name as a heading would be cleaner imo.
Also a point that we should consider regarding mobile view is if this additional information is something we consider useful in that scenario. We should just hide out package information that would not be useful for a mobile viewer
Up to @rchincha on that point.

@vrajashkr
Copy link
Contributor Author

@raulkele , thanks a lot for the feedback! I'll address the review comments.

I feel like maybe a section for each Package with the Package name as a heading would be cleaner imo

This sounds like a good idea! I'll give it a try to see how it looks.

@vrajashkr
Copy link
Contributor Author

I made an attempt at trying out the sections as suggested. Here's what it looks like:

Screenshot from 2024-02-24 01-05-34
Screenshot from 2024-02-24 01-06-07

@raulkele, what do you think about this design?

@andaaron
Copy link
Contributor

IMHO the earlier implementation in the description looks better (but I'm not a front-end developer so I don't know what to say about how the code is written behind the scenes).

@vrajashkr
Copy link
Contributor Author

Personally, I'm torn between the two. I can't seem to settle on one 😅.

One key item here (at least in my opinion) appears to be the need to handle a variation in the length of the package name and the package path.

In both approaches, what stands out to me is that there's a lot of horizontal space that's not being used. For instance, the python vulnerability has no paths and the library name and versions are short so we could ideally have 3 cards in the same line on wide screens and they can automatically scale to 2 on a line or 1 per line on smaller mobile screens. However, when there's a long path like in the spring web one, it needs to take up the full width on its own.

The counter is that while this may lead to better use of horizontal space, it becomes somewhat inconsistent as some are multi per line and others are single per line depending on the package path length, so I'm not sure if it's such a good idea either.

Any thoughts @raulkele @andaaron ?

@raulkele
Copy link
Collaborator

raulkele commented Feb 25, 2024

Design-wise I think I prefer the second implementation. I don't think it is an issue on larger screens that the row is empty if the path is not specified, we have examples where we do that in the ui.

I'll take a look at the code again as well if you update the PR or perhaps open up a second one with the other version.

One possible solution to the variance in the displayed data that you mentioned would be to make the row width dynamic. that way, depending on the amount of information, it would either be all three on one row or on separate rows as required.
The downside of this would be that then the display would be inconsistent. So again we have to make a choice if we prefer using all the screen real-estate and sometimes be inconsistent, or accept that sometimes the amount of data leads to some free space.

@vrajashkr
Copy link
Contributor Author

Thanks for the inputs @raulkele ! I've updated this PR to address the review comments so it's green. Any further feedback is welcome!

For the section approach, I'll raise a new PR.

@andaaron
Copy link
Contributor

Design-wise I think I prefer the second implementation. I don't think it is an issue on larger screens that the row is empty if the path is not specified, we have examples where we do that in the ui.

Maybe hide those rows entirely? Do not show both "Package Path" and the value?

@vrajashkr
Copy link
Contributor Author

Maybe hide those rows entirely? Do not show both "Package Path" and the value?

One thought about this that I had was related to consistency. For "Fixed Version", ZUI shows "Not Specified".
Displaying "Package Path" as "Not Specified" would help to keep it consistent so the user knows what to expect in this case.

@andaaron
Copy link
Contributor

We decided to use #428 instead.

@andaaron andaaron closed this Feb 29, 2024
@vrajashkr vrajashkr deleted the feat/pkg-path branch March 2, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants