Refactor: Transform to a class-based design for better organization

[n3rada]

Hello @skontar, @pandatix and other maintainers 👋,

After reading through the entire CVSS40 specification, I became interested in how to calculate the score programmatically. I discovered that your repository is the original one used by FIRST. I confess I didn't know where to make this Pull Request. I hope I'm making the right choice by doing it here and hoping that First will follow this repo.

I needed to implement this calculator in another language (C#), and in doing so, I made the code my own. Upon careful review, I realized that adopting a class-based approach would better meet the needs of maintainability and future improvements.

Calculating the CVSS 4.0 score for a vector representing a stored-XSS is as simple as:

let xss = new CVSS40("CVSS:4.0/AV:N/AC:L/AT:N/PR:N/UI:N/VC:H/VI:H/VA:H/SC:N/SI:N/SA:N")

Then, you can access all needed attributes such as the score or severity.

Thank you for considering this improvement. I believe it will make the codebase easier to maintain and extend.

[n3rada]

Of course, I took care to make app.js compatible so that the site could work as before!

[skontar]

Hello. I am sorry that I missed this PR. My colleague is working on something similar I believe so I will ask him to look into it and hopefully use as much as possible. It is my bad that I did not notice it 🤦‍♂️ . We are also working on fixing rounding issues.

[n3rada]

You'll see, it is easy to give it a try, It is working as expected! Currently using it for a project.

[skontar]

Hey, @superbuggy , please have a look. I believe this would help our test efforts.

[n3rada]

Normally I've commented enough on the code and it seems like this is more of a refactoring Pull Request than anything new, it's mainly for maintainability and ease of use to build other projects on top of this class. I am keen to have your feedback @superbuggy 😊

[superbuggy]

In general, this is a definite improvement that improves the ease of us in the short term. To be transparent about my personal preferences, I don't care for large classes and find they are hard to maintain over time. Others do not share this view, and I believe this implementation makes it easier/possible to test the business logic with a test suite.

I had a refactor in progress to modularize the existing files, but this implementation spares me the remainder of that work to refactor to integrate a test suite. Thank you for your work on this effort!

[n3rada]

Thank you for taking the time to review my PR, @superbuggy.

I wanted to clarify and gather your thoughts: Do you think it would be better to drop the idea of a class-based approach and close the PR, or is there still a chance it could be accepted despite the ongoing work on your side?

I agree with moving the user interface elements to another file. I've made the necessary changes. Having a json file is easier to maintain anyway.

I personally prefer the class-based approach because it makes it easier to integrate with an API implementation, as suggested in issue #56.

[superbuggy]

Thank you for taking the time to review my PR, @superbuggy.

I wanted to clarify and gather your thoughts: Do you think it would be better to drop the idea of a class-based approach and close the PR, or is there still a chance it could be accepted despite the ongoing work on your side?

I agree with moving the user interface elements to another file. I've made the necessary changes. Having a json file is easier to maintain anyway.

I personally prefer the class-based approach because it makes it easier to integrate with an API implementation, as suggested in issue #56.

Totally fine with this being a class for the time being! I have a personal preference against bigger classes, but it's just a personal one--many like them! Honestly, this one is not particularly enormous. Despite the difference of my initial approach, this implementation saves me work. Thank you again!

[n3rada]

It's true that large classes shouldn't be the solution. I've taken the time to rethink the module with the Single Responsibility Principle (SRP) in mind. I'm trying to maintain a good balance by separating concerns and keeping related functionality together. The Vector class takes care of metrics management and vector calculations, while the CVSS40 class focuses on score and severity calculation. For the name, perhaps you prefer CVSSVector, I don't know.

I've also added a nomenclature getter that could be used in the future to tell the user the nomenclature of the raw vector.

If you want to test, do not hesitate to clone my fork and start a simple HTTP server with:

python3 -m http.server 80

Then, you will be able to use the console in order to play with the CVSS object:

[superbuggy]

Have a fix for the floating point math errors

cc @skontar

[n3rada]

Well, we're going to get to the end of this PR!

[superbuggy]

So fun fact, the test I had been provided with was poisoned and the report about the web calculator being broken was mistaken. This is the correct implementation:

Math.round(value * 10) / 10;

The 'fix' that I wrote didn't make sense to me either, but I assumed my original understanding of how to handle floats in JS was flawed. Your intuitions about floats in JS were probably correct--my apologies!

[superbuggy]

We can probably merge your changes after this change

[n3rada]

You gave me a headache about that rounding! 😄 Here we are tho'

[n3rada]

It's all good on my side. As a final touch, I have separated the severity breakdown from the UI to make it belongs to Vector class. Which is more logical and respect the SRP.

Redy to merge now @pandatix, @superbuggy?

[skontar]

First of all, this looks like amazing work was done here. Thanks everyone involved and especially @n3rada !
If @superbuggy and @pandatix are fine with it, then either me or @superbuggy can merge.

[n3rada]

For me, it's perfect. Thank you! I'll let you do the merging when you have time 😊.