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: add average_severity to PurlStatus #1128

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

jcrossley3
Copy link
Contributor

Fixes: #1099

Fixes: 1099

Signed-off-by: Jim Crossley <[email protected]>
Copy link
Member

@carlosthe19916 carlosthe19916 left a comment

Choose a reason for hiding this comment

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

Thanks for the enhancement. It works as expected.

Only one thing I noticed is that the "average_severity" is outside the "vulnerability" object. while in other endpoints the "average_severity|severity" is part of the "vulnerability" object itself (next to the "identifier" field). But the current response is more than enough for me :)

{
    "advisories": [
            {                
                "status": [
                    {
                        "vulnerability": {
                            "identifier": "CVE-2023-28867"                            
                        },
                        "average_severity": "high",
                        "status": "not_affected"
                    }
                ]
            }
    ]
}

@jcrossley3
Copy link
Contributor Author

jcrossley3 commented Jan 9, 2025

I agree it's weird, but it was the least invasive solution, because that vulnerability object is re-used in other places that don't return severity. Because there is a cost to calculating severity, I didn't want to add it in all those other places.

One trivial alternative is to "flatten" the structure, removing the vulnerability name and promoting its fields one level up, like so:

{
    "advisories": [
            {                
                "status": [
                    {
                        "identifier": "CVE-2023-28867"            
                        ...                
                        "average_severity": "high",
                        "status": "not_affected"
                    }
                ]
            }
    ]
}

Would that be better?

@carlosthe19916
Copy link
Member

Let's leave it as it is (I approved this PR already as it is) :)

Both options works fine technically (the current code and what you suggested). My comment was more on aesthetics

@jcrossley3
Copy link
Contributor Author

Ok, but I'm now thinking that it might be ok to add severity/score to that object everywhere it's used. It seems that, after this change, all but one place return the severity/score with the object, so I think we can safely just add those fields to it.

I'll do that in a separate PR, though.

@jcrossley3 jcrossley3 added this pull request to the merge queue Jan 9, 2025
Merged via the queue into trustification:main with commit 4a7ae84 Jan 9, 2025
5 checks passed
@jcrossley3 jcrossley3 deleted the 1099 branch January 9, 2025 14:30
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.

/api/v1/purl/{id} does not contain advisories.status.vulnerability.severity field
2 participants