fix(VulnerableCode): Handle invalid URLs in references #6807
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #6807 +/- ##
============================================
+ Coverage 64.63% 64.68% +0.05%
- Complexity 1955 1956 +1
============================================
Files 322 322
Lines 16164 16192 +28
Branches 2295 2308 +13
============================================
+ Hits 10447 10474 +27
+ Misses 4725 4722 -3
- Partials 992 996 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Please also report this to https://github.com/nexB/vulnerablecode/issues if not already done so. |
| * advisor fails, handle those URLs by returning them with a special scheme and URL-encoded. | ||
| */ | ||
| private fun VulnerableCodeService.VulnerabilityReference.safeUri(): URI = | ||
| url.toUri().getOrDefault(URI("unparsed:${URLEncoder.encode(url, StandardCharsets.UTF_8)}")) |
There was a problem hiding this comment.
I wonder whether having such an "unparsed" URI is of any value, or whether we rather should create a warning issue / log for such things. I currently have a tendency for the latter. What do you think @oheger-bosch?
There was a problem hiding this comment.
The problem I see here is that the vulnerability reference could also be associated with scores and severities. Consider the case that there is a reference indicating a high severity, but with an invalid URL. Wouldn't it be better than if the reference is still visible in the reports where it is expected? But since the URL is not nullable, it must contain something. The hope is that a human reader can at least get an impression from where this reference comes from.
There was a problem hiding this comment.
Wouldn't it be better than if the reference is still visible in the reports where it is expected?
To be honest, I don't necessarily think so. First of all, personally I do not care much about the references at all; all I want to know is if there is a vulnerability, and I do regard all sources that ORT's advisors use as trustful anyway.
The hope is that a human reader can at least get an impression from where this reference comes from.
With the escaping happening, a human reading is going to have a hard time making sense of the URI anyway. So I'd still prefer the issue-approach.
There was a problem hiding this comment.
Another idea could be to fixup the URI before parsing so it becomes valid. Is there a pattern behind what makes these URIs invalid?
There was a problem hiding this comment.
The solution with an issue is also fine with me, I can try this out.
I am not sure whether we can identify a general pattern. In theory, the URI conversion could always fail.
There was a problem hiding this comment.
Changed the implementation to create an issue.
Created this issue: aboutcode-org/vulnerablecode#1173 |
oheger-bosch
force-pushed
the
oheger-bosch/advisor/vc_references
branch
from
2d04b52 to
66641d2
Compare
There are some packages for which VulnerableCode returns vulnerabilities with URL references that cannot be converted to URIs, since they contain invalid characters. In such cases, no meaningful advisor result was generated. Prevent this by handling exceptions when converting to URIs. Signed-off-by: Oliver Heger <oliver.heger@bosch.io>
oheger-bosch
force-pushed
the
oheger-bosch/advisor/vc_references
branch
from
66641d2 to
b971234
Compare
There are some packages for which VulnerableCode returns vulnerabilities with URL references that cannot be converted to URIs, since they contain invalid characters. In such cases, no meaningful advisor result was generated. Prevent this by handling exceptions when converting to URIs.