-
Notifications
You must be signed in to change notification settings - Fork 30
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 support for CVSSv4 #45
Conversation
This looks really good. Thank you! |
@mprpic can you please check CI? It seems to fail for some reason. |
@jobiewinserapck I presume you used the js implementation to generate test vectors? |
Yes, I generated the vectors using the js implementation. I could Maybe get some screenshots tomorrow of how I did so. If thats helpful. The only test failures I encountered after writing this was due to python3's rounding not being "away from zero", causing scores such as 7.25 to round towards 7.2 rather than 7.3 as expected (hence the round_away_from_zero func) |
No need for screenshots, just curious. Rounding was always "fun" previously. I am glad you resolved it easily. |
@jobiewinserapck can you rebase with master? I think that should re-run the CI with latest setup. |
Sure thing I'll do that ASAP tomorrow morning |
-Added class CVSS4 -Based the CVSS4 class on the js implementation found here: https://github.com/RedHatProductSecurity/cvss-v4-calculator -Added new tests for CVSS4 -Added new constants and exceptions for CVSS4 -Added json schema for CVSS4
e64b9de
to
c99cbbd
Compare
@mprpic can you please review? |
I'ved added those improvements, thanks for that @skontar! |
just commenting for support, gogogo |
-Fixed various typos: CVSS3 -> CVSS4 -Added malformed CVSS4MalformedError check for wrong cvss prefix -Fixed prefix in clean_vector: "CVSS:4/" -> "CVSS:4.0/"
68030d6
to
0f9079a
Compare
Just commenting to say I've reformatted cvss/cvss4.py, hopefully the pipeline will pass on next run |
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 also please add import statements into the cvss/__init__.py
file?
from .cvss4 import CVSS4
from .exceptions import CVSS4Error
-Added additional CVSS4 imports to cvss/__init__.py -Added additional metric checks that raise CVSS4MalformedError -Cleaned up some left over comments -Added additional tests for malformed CVSS4 strings
I've pushed up a commit addressing the issues raised by @mprpic. I also added some additional tests for malformed cvss strings |
@mprpic please re-review. |
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!
@jobiewinserapck there were recent changes to rounding in official Javascript implementation. Would you be so kind and double-check everything is still shiny and both implementations match? I think that the Python one will be more correct as it uses |
I don't think $ python << EOF
from decimal import ROUND_HALF_UP
from decimal import Decimal as D
def round_away_from_zero(x, dp=1):
return float(D(x).quantize(D("0." + "0" * dp), rounding=ROUND_HALF_UP))
for i in range(6):
print(round_away_from_zero(i + 0.05))
EOF
0.1
1.1
2.0
3.0
4.0
5.0 It's inconsistent. I would have said we should just do The specification document is not clear on which rounding convention should be used:
|
Correct |
@AdrianVollmer if the input is
|
Okay, but this function is only used once here and is passed a float: Line 558 in c5dd7e2
|
I think this function will suit this particular use case: def round_away_from_zero(value):
# Only works correctly for positive values
return int(value * 10 + 0.5) / 10.0 Since we only need to round values between 0.0 and 10.0, it should be fine. $ python << EOF
for i in range(10):
print(int((i+0.05)*10 + 0.5)/10)
EOF
0.1
1.1
2.1
3.1
4.1
5.1
6.1
7.1
8.1
9.1 |
-Added class CVSS4
-Based the CVSS4 class on the js implementation found here: https://github.com/RedHatProductSecurity/cvss-v4-calculator -Added new tests for CVSS4
-Added new constants and exceptions for CVSS4
-Added json schema for CVSS4