fix(ui): prevent extract-purls endpoint from returning invalid purls#1888
fix(ui): prevent extract-purls endpoint from returning invalid purls#1888ctron merged 2 commits intoguacsec:mainfrom
Conversation
Reviewer's GuideIntroduce PURL validation and integrate it into SPDX and CycloneDX extraction to filter out invalid PURLs, with accompanying unit tests. Class diagram for PURL validation integrationclassDiagram
class ExtractPackage
class Purl {
+try_from(str) Result<Purl, Error>
}
class service {
+extract_spdx_purls(sbom: SPDX) BTreeMap<String, ExtractPackage>
+extract_cyclonedx_purls(sbom: CycloneDX) BTreeMap<String, ExtractPackage>
+filter_purl(purl: String) Option<String>
}
service --> Purl : uses
service --> ExtractPackage : returns
Purl <.. filter_purl : validation
Flow diagram for PURL extraction and validationflowchart TD
A[Extract SPDX/CycloneDX SBOM] --> B[Extract PURLs]
B --> C[filter_purl]
C -- Valid PURL --> D[Add to result]
C -- Invalid PURL --> E[Discard]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ctron - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `modules/ui/src/service.rs:79` </location>
<code_context>
+/// Filter out invalid PURLs
+///
+/// If the PURL is valid, return the `Some(input)`, otherwise return `None`.
+fn filter_purl(purl: String) -> Option<String> {
+ Purl::try_from(purl.as_str()).ok().map(|_| purl)
+}
</code_context>
<issue_to_address>
filter_purl takes ownership of the String unnecessarily.
Change the parameter to accept a `&str` to avoid unnecessary allocations and improve flexibility.
Suggested implementation:
```rust
/// Filter out invalid PURLs
///
/// If the PURL is valid, return `Some(input)`, otherwise return `None`.
fn filter_purl(purl: &str) -> Option<&str> {
Purl::try_from(purl).ok().map(|_| purl)
}
```
```rust
let mut purls = vec![];
if let Some(ref purl_str) = component.purl {
if let Some(valid_purl) = filter_purl(purl_str) {
purls.push(valid_purl.to_string());
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
carlosthe19916
left a comment
carlosthe19916
left a comment
There was a problem hiding this comment.
I still see the package pkg:golang/archive/tar being returned by the SBOM
{
"packages": {
"archive/tar": {
"purls": [
"pkg:golang/archive/tar"
]
},
The package pkg:golang/archive/tar cannot be sent to POST /api/v2/vulnerability/analyze
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1888 +/- ##
==========================================
+ Coverage 67.97% 68.14% +0.17%
==========================================
Files 364 365 +1
Lines 22997 23123 +126
Branches 22997 23123 +126
==========================================
+ Hits 15632 15757 +125
+ Misses 6486 6485 -1
- Partials 879 881 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
And rightly so. Because it is a valid PURL. As mentioned above, the problem is more complex. Versions are optional. We'd need to add another change to return some kind of information in the case of a missing warning. Possible ideas:
|
ctron
force-pushed
the
feature/remove_invalid_1
branch
from
033b080 to
4f61844
Compare
ctron
force-pushed
the
feature/remove_invalid_1
branch
from
4f61844 to
b17bbb0
Compare
|
With the latest change this now does:
|
|
Worth mentioning: this breaks the current API. As the I think there are several options we have:
|
|
@ctron I don't mind adapting the RHDA backend to the new api changes as long as it is justified. So no problem from my side. |
ctron
force-pushed
the
feature/remove_invalid_1
branch
from
b17bbb0 to
654df5d
Compare
|
@carlosthe19916 could you take another look at this PR? |
There was a problem hiding this comment.
POST /api/v2/vulnerability/analyze
@ctron I confirm, now I can send multiple PURLs to POST /api/v2/vulnerability/analyze and if one PURL is missing the version then it contains warnings and avoids the request to fail. Just one minor thing:
- When I send PURLs, the response contains only the purls that either have warnings or vulnerabilities; those purls that do not have vulnerabilities and no warnings are omitted in the response. E.g.
POST /api/v2/vulnerability/analyze
{
"purls": [
"pkg:maven/com.fasterxml.jackson.module/jackson-module-jaxb-annotations@2.15.2",
"pkg:rpm/redhat/jenkins-2-plugins@4.11.1706516946-1.el8?arch=noarch",
"pkg:golang/archive/tar"
]
}
Generates the response:
{
"pkg:golang/archive/tar": {
"details": [],
"warnings": [
"Unable to process: missing version component"
]
},
"pkg:rpm/redhat/jenkins-2-plugins@4.11.1706516946-1.el8?arch=noarch": {
"details": [ // some vulnerabilities],
"warnings": []
}
}But notice that the PURL pkg:maven/com.fasterxml.jackson.module/jackson-module-jaxb-annotations@2.15.2 was not included in the response; my assumption is that it is due to the fact that there were no vulnerabilities found. To avoid assumptions like mine, I wonder if it is a good idea to always include all PURLs from the request into the response, even if no vulnerabilities are found (the array of vulnerabilities will be just an empty array)
POST /api/v2/ui/extract-sbom-purls
I see a new field warnings were added. I was not able to populate that field with the SBOMs I tried locally, which SBOM did you use to test that?
|
Regarding the missing entries, I noticed that too and was wondering if we want that (or not). However, that behavior hasn't changed. I'm open to adding this if we think it's helpful.
I'll add a unit test. That's currently missing but should be there. |
ctron
force-pushed
the
feature/remove_invalid_1
branch
from
654df5d to
391aca3
Compare
|
Added a test that should demonstrate it. |
carlosthe19916
left a comment
carlosthe19916
left a comment
There was a problem hiding this comment.
Regarding the missing entries, I noticed that too and was wondering if we want that (or not). However, that behavior hasn't changed. I'm open to adding this if we think it's helpful.
Let's leave it for a separate PR.
3 participants
Closes: #1887
Summary by Sourcery
Validate and filter PURLs in SPDX and CycloneDX extraction to prevent invalid PURLs from being returned
Bug Fixes:
Enhancements:
Tests: