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

✨ Handle arbitrary metrics order and add testing #60

Merged
merged 6 commits into from
Sep 7, 2024

Conversation

superbuggy
Copy link
Collaborator

@superbuggy superbuggy commented Aug 26, 2024

Changes:

  • Handles any ordering of CVSS metrics (is this desirable?)
  • Defines test suite
  • Defines continuous integration workflow

@superbuggy superbuggy marked this pull request as draft August 26, 2024 15:57
@superbuggy superbuggy self-assigned this Aug 26, 2024
@superbuggy superbuggy force-pushed the feature/add-testing branch from 9b33056 to 95141c9 Compare August 27, 2024 01:03
@superbuggy superbuggy requested a review from skontar September 6, 2024 19:14
@superbuggy superbuggy marked this pull request as ready for review September 6, 2024 19:15
✅ Test class-based module

✅ Test longer vectors

🔥 Remove file

♻️ Use better test data

🔥 Remove old test data files

🔥 Remove old logs
cvss40.js Outdated Show resolved Hide resolved
cvss40.js Outdated Show resolved Hide resolved
cvss40.js Outdated Show resolved Hide resolved
cvss40.test.js Outdated Show resolved Hide resolved
Certain (valid?) CVSS4 vector strings were failing validation because they had a different vector ordering

💬 Fix language
Defines a Github Action to run the Jest test suite
Removes random selection of test cases.
@superbuggy superbuggy requested a review from skontar September 6, 2024 19:47
@superbuggy superbuggy changed the title Feature/add testing ✨ Handle arbitrary metrics order and add testing Sep 6, 2024
@superbuggy superbuggy merged commit db569ba into main Sep 7, 2024
1 check passed
@pandatix
Copy link
Contributor

pandatix commented Sep 7, 2024

Arbitrary metrics order is invalid according to spec...

Furthermore, the current approach within this SIG for testing is developing an assessment framework so developers can easily handle this part. This PR goes against this effort by bypassing it.
The CVSS v4 calculator serves as a reference for other librarires maintainers, but implementing 500k lines of code just for testing is not acceptable to me.

superbuggy added a commit that referenced this pull request Sep 7, 2024
…-testing"

This reverts commit db569ba, reversing
changes made to 75d6c98.
superbuggy added a commit that referenced this pull request Sep 7, 2024
@skontar
Copy link
Collaborator

skontar commented Sep 7, 2024

@pandatix OK, I understand that arbitrary metric order is not correct, that makes sense.

However, I do not understand how adding tests is a problem and "bypassing" any other effort. If this is a reference implementation, why having tests which pass and which can be re-used by others, is wrong? Please elaborate. Also, it is not 500k lines of code, but of test vectors.

I am completely fine removing it from this project, just please give a more detailed rationale. Thanks!

@pandatix
Copy link
Contributor

Hey, sorry for the late answer everyone.

Adding tests is not directly "bypassing" the SIG efforts, it is a good thing actually. From an external point of view (like an implementation maintainer/developer) the RedHat implementation is the de facto source of trust. NIST used it for their own calculator, many others accoring to our analysis.

This PR comes in parallel of this effort: one of the requirements we drew is to provide this test suite in an efficient manner (e.g., a file served by a CDN, a versionned file on GitHub, a GH release asset) and with a limited set of test cases (under 10k if possible) that would cover most cases without needing exhaustivity. Another one is to provide both valid and invalid test cases.
Indeed, we cannot expect an implementation to handle 5GB of data to say that we are confident enough, especially when the implementation is not performant (CI would take minutes up to hours to run at every commit, quickly becoming a burden for maintainers).

To avoid having parallel efforts, I think we should wait for the official FIRST.ORG SIG CVSS test data rather than creating one, propagating it to the community, then having to reach out maintainers to update their work one more time (update source of trust, consumption strategy, data model). We should hop on coordination rather than individual efforts bringing noise to the effort of CVSS v4 adoption.

@skontar
Copy link
Collaborator

skontar commented Sep 12, 2024

@pandatix I see. So if I understand correctly, the best course of action is to wait for official FIRST.ORG SIG CVSS test vectors, right?

@pandatix
Copy link
Contributor

Yes, I think so 😄

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