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

[Cloud Security] do not filter out vulnerabilities without the score field #163949

Conversation

maxcold
Copy link
Contributor

@maxcold maxcold commented Aug 15, 2023

Summary

as a part of an effort to remove the vulnerability documents filter in https://github.com/elastic/security-team/issues/7146 this PR removes the filter for missing vulnerabiltiy.score.* fields.

Here is how the CNVM features look like when documents without these fields are present

Screenshot 2023-08-15 at 17 54 28 Screenshot 2023-08-15 at 17 53 54 Screenshot 2023-08-15 at 17 54 56 Screenshot 2023-08-15 at 17 54 49

Checklist

Delete any items that are not applicable to this PR.

@maxcold maxcold added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Cloud Security Cloud Security team related 8.11 candidate labels Aug 15, 2023
@maxcold maxcold force-pushed the 7342-cloud-security-do-not-filter-out-vulnerabilities-without-score branch from 1c7ae73 to 5f4e753 Compare August 17, 2023 08:41
@maxcold maxcold marked this pull request as ready for review August 17, 2023 08:49
@maxcold maxcold requested a review from a team as a code owner August 17, 2023 08:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

Copy link
Contributor

@JordanSh JordanSh left a comment

Choose a reason for hiding this comment

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

👑

<CVSScoreBadge version={cvss.version} score={cvss.score} />
</EuiLink>
),
render: (cvss: { score: number; version: string }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
render: (cvss: { score: number; version: string }) => {
render: (cvss: Vulnerability["score"]) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JordanSh thanks for the suggestion! the Vulnerability["score"] is not the type we can use here, but I changed the code to use the correct type (as we are passing formatted data to the table)

</EuiLink>
),
render: (cvss: { score: number; version: string }) => {
if (!cvss.score || !cvss.version) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm approving but was this agreed by product? if not, worth consulting. they might prefer some unknown/undefined tag or something. same for the vuln table itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for highlighting this! I'm bringing it up in this comment https://github.com/elastic/security-team/issues/7146#issuecomment-1682475106 and think we need to specifically work on the UX of missing fields. Without this change, the behavior is different between dashboards and data grid (in the dashboard the empty green badge is being displayed as CVSScoreBadge doesn't handle this case itself, while in the data grid, we already have guards for a missing score in grid cells), so the question is if it's ok to move forward with the simplest solution. @nick-alayil do you see any problem with displaying empty cells consistently when the score is missing on the vulnerability document

Choose a reason for hiding this comment

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

@nick-alayil do you see any problem with displaying empty cells consistently when the score is missing on the vulnerability document

This is totally acceptable at the moment since our primary goal is to ensure that all identified CVEs are presented to the end user, accompanied by the resource ID. We can focus on enhancing the UX for the default values in the future.

Comment on lines 236 to 237
render: (cvss: { score: number; version: string }) => {
if (!cvss.score || !cvss.version) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

same comments

}

return (
<EuiLink onClick={() => onCellClick({ 'vulnerability.score.base': cvss.score! })}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<EuiLink onClick={() => onCellClick({ 'vulnerability.score.base': cvss.score! })}>
<EuiLink onClick={() => onCellClick({ 'vulnerability.score.base': cvss.score })}>

i might be missing something but this shouldn't be needed if you already verified it exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for some reason, TS is not picking up the early return in case cvss.score is missing and I couldn't figure out why quickly. Do you have an idea why it might be?

Copy link
Contributor

@JordanSh JordanSh Aug 21, 2023

Choose a reason for hiding this comment

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

I tried locally and i loled. looks like a typescript bug. consulted with the master @orouz and those two solutions work:

render: (cvss: PatchableVulnerabilityStat['cvss']) => {
            if (!cvss.score || !cvss.version) {
              return null;
            }
            const score = cvss.score;
            // const query = { 'vulnerability.score.base': cvss.score };

            return (
              <EuiLink onClick={() => onCellClick({ 'vulnerability.score.base': score})}>
              // <EuiLink onClick={() => onCellClick(query)}>
                <CVSScoreBadge version={cvss.version} score={cvss.score} />
              </EuiLink>
            );
          },

you can either declare a new constant for score, or the entire query object on a new constant. either way the inferring seems bugged inside the onClick function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for finding a way to avoid the use of cvss.score!! definitely seems like a TS bug

@maxcold
Copy link
Contributor Author

maxcold commented Aug 21, 2023

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #53 / aiops change point detection shows multiple results when split field is selected

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cloudSecurityPosture 264.0KB 264.1KB +74.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@maxcold maxcold merged commit 443283c into main Aug 23, 2023
@maxcold maxcold deleted the 7342-cloud-security-do-not-filter-out-vulnerabilities-without-score branch August 23, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants