Java libraries for working with available vulnerability data sources (GitHub Security Advisories, NVD, EPSS, CISA Known Exploited Vulnerabilities, etc.)
Apache License 2.0
107
stars
30
forks
source link
Discussion: NVD API client should not retry/continue on JsonProcessingExceptions? #167
Currently whenever there is a JsonProcessingException (specific cause JsonMappingException the library current gets stuck in a loop retrying to retrieve and parse data that is never going to work.
This leads to confusion like mentioned in https://github.com/jeremylong/DependencyCheck/issues/6747 and https://github.com/jeremylong/DependencyCheck/issues/6746 since the errors are hidden and it looks like NVD API problems. This is exacerbated on Gradle, because enabling debug logging to see the problem is non-trivial and in bigger projects will cause Gradle to freak out as it produces too many logs (e.g when using dependencyCheckAggregate). Additionally, I believe it creates a storm of unnecessary extra load on the NVD API?
This was changed in the below, but no real reason shared as to why.
Can we re-evaluate why this was changed, and consider reverting? Generally I'd have thought a JsonProcessingException is a fatal, unretryable error, and the library should fast fail with a clear error.
Since there are multiple reasons for JsonProcessingException perhaps the concern was retrying on temporary stream read issues. If that is the case, an alternative is to fail fast on JsonMappingException but to retry on others, e.g StreamReadException. Not convinced that is the concern though, as the code seems to parse from a string in memory, fully extracted from the body text, not stream it into the parser:
Currently whenever there is a
JsonProcessingException(specific causeJsonMappingExceptionthe library current gets stuck in a loop retrying to retrieve and parse data that is never going to work.This leads to confusion like mentioned in https://github.com/jeremylong/DependencyCheck/issues/6747 and https://github.com/jeremylong/DependencyCheck/issues/6746 since the errors are hidden and it looks like NVD API problems. This is exacerbated on Gradle, because enabling debug logging to see the problem is non-trivial and in bigger projects will cause Gradle to freak out as it produces too many logs (e.g when using
dependencyCheckAggregate). Additionally, I believe it creates a storm of unnecessary extra load on the NVD API?This was changed in the below, but no real reason shared as to why.
https://github.com/jeremylong/Open-Vulnerability-Project/pull/77/commits/825977e9d8bdb3f2c5937b2476713d5bea4b5497#diff-13b186ee3d6fa6c024eff401d1250ba1e32d00a02464e586aba44bd84015ff66L311-R315
Can we re-evaluate why this was changed, and consider reverting? Generally I'd have thought a
JsonProcessingExceptionis a fatal, unretryable error, and the library should fast fail with a clear error.Since there are multiple reasons for
JsonProcessingExceptionperhaps the concern was retrying on temporary stream read issues. If that is the case, an alternative is to fail fast onJsonMappingExceptionbut to retry on others, e.gStreamReadException. Not convinced that is the concern though, as the code seems to parse from a string in memory, fully extracted from the body text, not stream it into the parser:https://github.com/jeremylong/Open-Vulnerability-Project/blob/94b7996bf2af05a55c55e55f69ebf910d1f20355/open-vulnerability-clients/src/main/java/io/github/jeremylong/openvulnerability/client/nvd/NvdCveClient.java#L330-L344