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

Add CVSS v3.1 support #8

Closed
wants to merge 2 commits into from
Closed

Add CVSS v3.1 support #8

wants to merge 2 commits into from

Conversation

luc-x41
Copy link

@luc-x41 luc-x41 commented Feb 28, 2020

This commit adds support for CVSS v3.1 with tests.

The test data was taken, like the existing ones, from https://github.com/skontar/cvss (as well as locally running millions of random tests generated using the JavaScript code on first.org).

The difference between 3.0 and 3.1 is one line of code, so this basically added an if-statement in cvss3/calculations.py to avoid duplication and a way for the code to know which line it should use, plus a lot of code to handle and test everything around the different versions.

Fixes #7

Tests were taken, like the existing ones, from skontar/cvss.

The only difference between 3.0 and 3.1 is one line of code, so this
basically added an if-statement and a way to signal which line it should
use plus a lot of code to handle and test the different versions.
@dgaus
Copy link
Contributor

dgaus commented Mar 2, 2020

Hi, thanks for the PR. In general it looks good but the use of the integer 31 as a version constant feels odd. Could you make this into a either Decimal or a "3.1" string? @orf I don't know if you are too busy to review this, do you want to take a look?

@orf
Copy link
Contributor

orf commented Mar 2, 2020

I agree - can we use 3.1 rather than 31? I've had a quick look and I don't think we assume the version is an integer anywhere.

Overall this looks excellent, however I don't have permission to merge this anymore, let alone publish it :(

Edit: I can publish it, but I can't merge to this repository anymore. Perhaps we should fork it to somewhere we both control?

@dgaus
Copy link
Contributor

dgaus commented Mar 2, 2020

I don't have write permissions either, I think forking it is a good idea as I doubt anyone at Context will be working on this

@orf
Copy link
Contributor

orf commented Mar 2, 2020

Ok, I can fork this to my account and merge later today/tomorrow, but if you want to fork it to your account and do the same then be my guest 👍

@dgaus
Copy link
Contributor

dgaus commented Mar 2, 2020

Go ahead, it's your baby after all 😀

@luc-x41
Copy link
Author

luc-x41 commented Mar 3, 2020

Hey guys, thanks for the replies :)

You mean the VERSION constant right? I guess I was using "31" in so many places that I forgot the decimal could be used here.

The class name or directory name can't be 3.1 by the way: while python supports importing from subfolders with a . and we could abuse that, the subdirectory may not start with a number.

Let me know in which repo to do the new pull request :)

@dgaus
Copy link
Contributor

dgaus commented Mar 4, 2020

Yes, I don't mind the modules / classes being named cvss31, but VERSION = 31 feels weird

@luc-x41
Copy link
Author

luc-x41 commented Mar 5, 2020

Pushed the version change :)

@orf Could you create the fork?

@orf
Copy link
Contributor

orf commented Mar 5, 2020

Done! https://github.com/orf/cvsslib

If you could make the PR there that would make things simpler, otherwise I will merge it manually tonight and do a release 👍

@orf
Copy link
Contributor

orf commented Mar 7, 2020

This is now merged to https://github.com/orf/cvsslib. I'll do a release soon, can you close this MR please? 👍

@luc-x41 luc-x41 closed this Mar 10, 2020
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.

add cvss 3.1 support
3 participants