-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add severity column to vulnerability list #2262
Add severity column to vulnerability list #2262
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTMish (without seeing a preview), with some thoughts on handling multiple CVSS versions.
There's also the UX question of records that do not have any CVSS scores and how they would sort...
{% for score in (vulnerability.severity_scores) %} | ||
<li>{{ score }}</li> | ||
{% endfor %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be preferable to only output the highest available CVSS score, I.e. where CVSSv3 and CVSSv4 scores are provided, favour the v4 score.
5c89f1c
to
1f28fe8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This adds more width to our page :/ Should we evaluate removing some other columns first? |
e.g. fix available, and versions probably isn't that useful. |
Hey @ZhangChen199102 , thanks for implementing this feature! After discussing with the team, we have some thoughts on this change:
|
1f28fe8
to
9661b2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! Can you also try keeping the color, but for the darker colors have the text color switch from black to white to make it easier to read? I'm not sure if we'll go with that, but I think it would be nice to compare. (Or choose different tag colors to maintain the contrast. ) |
With the current colors, switching the text color to white instead of black actually fails the contrast ratio for accessibility (checked using this tool). I’ve attached a sample output for reference, showing how the text looks with white colors on these backgrounds. Additionally, I’ve attached a photo demonstrating the appearance with black text on these colors. As another alternative, I’ve used red-themed colors for high and critical severities to maintain the contrast. Sample output with white text on colors( fails accessibility checks ) Black and white text on colors ( passes accessibility checks ) |
I personally prefer the middle option, though can you try switching the orange (High) text color to white instead of black? |
+1, I also think the middle one looks better. The third one has a pink colour, which is the same as our "no fix available" tag. |
@another-rex @hogo6002, unfortunately, using white on orange fails the accessibility contrast checks. the result is in demonstrated on pic1 in my previous comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Styling LGTM! Just some minor comments.
gcp/appengine/Pipfile
Outdated
@@ -22,6 +22,7 @@ requests = "==2.32.2" | |||
grpcio-status = "==1.62.2" | |||
gunicorn = "==22.0.0" | |||
whitenoise = "6.5.0" | |||
cvss = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pin this version
@@ -537,6 +544,26 @@ md-icon-button.mdc-data-table__sort-icon-button { | |||
&.fix-unavailable { | |||
background: $osv-red-300; | |||
} | |||
|
|||
&.severity-low { | |||
background: #53aa33; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set these values as (scss) variables, I think we might want to use them for each vulnerability page as well.
gcp/appengine/frontend_handlers.py
Outdated
if detailed: | ||
add_links(response) | ||
add_source_info(bug, response) | ||
return response | ||
|
||
|
||
def add_cvss_score(bug): | ||
"""Add severity score where possible.""" | ||
severity_score = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can this just be 3 variables?
This change refactors the existing functionality of calculating the severity base score and rating, to enable reusing it in vulnerability page.
41de3b9
to
307c07d
Compare
gcp/appengine/Pipfile.lock
Outdated
"markers": "platform_python_implementation != 'PyPy'", | ||
"version": "==0.22.0" | ||
"markers": "python_version >= '3.7'", | ||
"version": "==6.5.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version of whitenoise
specified in the Pipfile is 6.5.0
, so it makes sense that pipenv lock
downgrades the version from 6.7.0
to 6.5.0
, as introduced by this commit.
However I'm unsure why it removed zstandard
. I suspect it might be because I'm running it from a mac machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm guessing whitenoise added zstd support somewhere between 6.7.0 and 6.5.0. Since it looks like we are already using whitenoise 6.7.0, can you update our pipfile for whitenoise to ==6.7.0 (The only reason renovatebot didn't do it is because there's currently a dependency conflict in the Google cloud dependencies. )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re zstandard
, looking at pipenv graph
it's a dependancy for flask-compress
version 1.15
as the version in Pipfile is specified as 1.13
it's been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@another-rex Should I upgradeflask-compress
from 1.13
to 1.15
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that'll be good
def calculate_severity_details( | ||
severity: dict) -> tuple[float | None, str | None]: | ||
"""Calculate score and rating of severity""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cherry-picked this commit from my other branch which adds link to cvss calculator.
FYI - Once this PR was merged, I'll resolve the potential conflicts and open a seperate PR for the mentioned issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice! Thanks Zahra!
Issue: #2255