Raising runtime errors makes it difficult to determine validity

[adamrdavid]

I have created a brittle method to determine the validity of a cvss_string. I would prefer to catch a specific error class raised by CvssSuite so that I do not have to rely on the error string as the string could likely change.

CvssVector.valid?

def valid?(cvss_string)
  return false if cvss_string.nil?
  CvssSuite.new(cvss_string).valid?
rescue ArgumentError, RuntimeError => e
  raise e if e == RuntimeError && e.message != 'Vector is not valid!'
  raise e if e == ArgumentError && e.message != 'bad value for range'
  false
end

Additionally, it would be (possibly) beneficial if CvssSuite would perform this validity checking without raising errors and simply return true/false.

I will create a pull request to add the error classing, however, this would unfortunately be a 'breaking change' and likely cause a major version bump to the gem since users will currently be expecting the RuntimeError and ArgumentError

[0llirocks]

Hi @adamrdavid, thank you for bringing up this issue and your pull request.

I agree that specific class errors are more convenient but since we are still throwing exceptions I think it's ok to bump to 1.1.0 what do you think?

Regarding the exception issue you have in your code: If you take a closer look you will notice that these exception are not thrown by the valid? method but on object initializing (self.new). The valid? method itself never throws an exception, it only returns true or false.

This is indeed an issue I have to fix. Please give me a few days (maybe weeks) since I am currently busy and/or traveling. But hopefully I can make this change this month!

You will hear from me soon. Best regards, Oliver

[adamrdavid]

Hi @oliverhamboerger thank you for your response and your time investigating this. Also thank you for this gem, we at /bugcrowd appreciate it! :D

Ya sorry the example I posted was obscuring some details, I have updated it. I see your point, it does make the most sense for the initialize to raise, and have the instance method #valid? only return true/false if the instance can be created.

I was thinking a nice addition would be a class method .valid? that takes a string and returns true/false by attempting to initialize an instance and calling #valid?. However, this class method may not be the responsibility of the gem and is out of scope of the suggested change.

Regarding the version bump, I agree with whatever you are comfortable with. The only reason I worried it might constitute a major version change, is that if I were to update the gem in my current application, my example method from above CvssVector.valid? would break. Perhaps if we make the error classes inherit from what they are now (RuntimeError and ArgumentError) it would be a more backward compatible change?

No problem, I understand if you are busy. Take your time 👍

Cheers, Adam

[0llirocks]

@adamrdavid I will release the changes within the next days. I think the new version will not break your current code. If you have an invalid cvss vector, the method will just return false without raising the exception anymore. I also improved the handling with params that are no strings. Do you know which cvss_vector gave you the second exception? (bad value for range)

[adamrdavid]

Hi @oliverhamboerger,

The input that results in bad value for range is CvssSuite.new('CVSS:3.0/') Thanks!

[0llirocks]

@adamrdavid I was able to reproduce this issue and created a new issue for that https://github.com/siemens/cvss-suite/issues/3