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

Updates for CVSS v3.1 #23

Merged
merged 8 commits into from
Aug 29, 2019
Merged

Updates for CVSS v3.1 #23

merged 8 commits into from
Aug 29, 2019

Conversation

skontar
Copy link
Collaborator

@skontar skontar commented Aug 26, 2019

No description provided.

@@ -7,12 +7,15 @@ matrix:
include:
- name: "Python 2.6"
python: 2.6
dist: trusty # required for legacy Python
Copy link

Choose a reason for hiding this comment

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

Wow, that really is reaching back into the past! :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If Modified Scope Unchanged 6.42 x [ISCModified]
If Modified Scope Changed 7.52 x [ISCModified-0.029] - 3.25 x [ISCModified-0.02]^15
If Modified Scope Changed 7.52 x [ISCModified-0.029] - 3.25 x [ISCModified-0.02]^15
Copy link

Choose a reason for hiding this comment

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

It is a little embarrassing but when I saw this comment last week I did not immediately understand that it was the formula being used. Also it may be better as a code comment than as part of the docstring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. However, it is everywhere in the code and for some reason I thought it is a good idea back when I wrote the first version.

@bac
Copy link

bac commented Aug 26, 2019

@skontar These changes look great and the tests are very thorough.

To see the changes in the test data I needed to first sort vectors_simple3 and vectors_simple31. You may want to sort those files and check them in.

Finally it was difficult to run the tests locally. If that is never the intention you may want to add that to the README.

Thank you for supporting 3.1 so quickly!

else: # Modified scope has always value, if not defined then matches Scope
self.modified_isc = (D('7.52') * (self.modified_isc_base - D('0.029')) -
D('3.25') * (self.modified_isc_base * D('0.9731') - D('0.02'))
** D('13'))
Copy link

Choose a reason for hiding this comment

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

Since the only differences in the formulas are the constants being used, you could keep one formula and parameterize the differences in a look-up table? What you have is very clear. Maybe something to consider if there is a 3.2...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. I will leave it for now but as you said, something to keep on mind if 3.2 ever comes.

@skontar
Copy link
Collaborator Author

skontar commented Aug 26, 2019

@bac I do not understand your comment about sorting the vectors_simple3 and vectors_simple31, can you explain? When I was comparing them I just replaced CVSS:3.1 with CVSS:3.0 in the later file and then used meld for compare.

Also, can you explain the difficulties with running tests? I am usually just running run_tests.sh on my Linux system where I have both Python 2 and Python 3 installed.

@bac
Copy link

bac commented Aug 26, 2019

  • When I first opened the test data files I wanted to see the differences between scores computed for 3.0 and 3.1. The files checked-in had data in different order, i.e. the first line were not for the same vector. So I sorted them to more easily compare. Not a big deal.

  • I didn't even see run_tests.sh. It works great. Perhaps a line in the README referencing it would be nice. I tried all of the usual suspects like just running pytest or python setup.py test and those didn't work.

@skontar
Copy link
Collaborator Author

skontar commented Aug 26, 2019

  • I do not see it. For whatever reason I see them in the same order. Can you please double-check? Can you please let me know exact files and line numbers which you do not see matching?

  • OK, I will add it to the README.

@bac
Copy link

bac commented Aug 26, 2019

I retract my statement. I'm not sure what I thought I saw initially. Sorry for the noise.

skontar and others added 2 commits August 29, 2019 13:07
More readable code for hint exceptions.

Co-Authored-By: Danil Ineev <saigono@users.noreply.github.com>
@skontar skontar merged commit d49423c into master Aug 29, 2019
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.

3 participants