Pull vulnerabilities from GitHub Security Advisory Database #140
Conversation
There was a problem hiding this comment.
Since this is just a PoC, please move everything to com.sap.sgs.phosphor.fosstars.data.github.experimental package.
If you think we're ready to convert it to a real provider, please open a follow-up ticket.
Since it's a PoC, I didn't have a close look at the code. I put some other comments, feel free to address them. Or, you can just link them to the follow-up ticket, and we'll address them later when we work on the real provider. We'll also need to add tests but now it's not required.
| @@ -0,0 +1,180 @@ | |||
| package com.sap.sgs.phosphor.fosstars.data.github; | |||
There was a problem hiding this comment.
Please move everything to com.sap.sgs.phosphor.fosstars.data.github.experimental package.
| public class VulnerabilitiesFromGitHubAdvisories extends CachedSingleFeatureGitHubDataProvider { | ||
|
|
||
| /** | ||
| * A feature that hold info about vulnerabilities in the GitHub Advisory Database. |
|
|
||
| Vulnerabilities vulnerabilities = new Vulnerabilities(); | ||
|
|
||
| PackageManagers managers = (PackageManagers) packageManagement.fetchValueFor(project).get(); |
There was a problem hiding this comment.
packageManagement needs to be configured with callback and cache:
packageManagement.set(cache).set(callback);|
|
||
| PackageManagers managers = (PackageManagers) packageManagement.fetchValueFor(project).get(); | ||
| for (PackageManager packageManager : managers.list()) { | ||
| if (!PackageManager.MAVEN.equals(packageManager)) { |
There was a problem hiding this comment.
There may be multiple package managers for a project. The loop should not break here.
| private Vulnerability vulnerabilityFrom(Node node) { | ||
| Advisory advisory = node.getAdvisory(); | ||
|
|
||
| String id = advisory.getGhsaId(); |
There was a problem hiding this comment.
If an advisory has a CVE id, then it should be used. Otherwise, it will lead to duplicate vulnerabilities in the OssFeatures.VULNERABILITIES feature once we merge all available vulnerabilities.
| public List<Node> advisoriesFor(String ecosystem, String artifact) throws IOException { | ||
| List<Node> advisories = new ArrayList<>(); | ||
| for (Node node : download(ecosystem, artifact)) { | ||
| if (!hasCve(node.getAdvisory())) { |
There was a problem hiding this comment.
I am not sure that we should skip advisories with CVE identifiers. For such advisories, we can at least try to extract resolution status.
| "permalink", "publishedAt", "references", "severity", "summary", "updatedAt", "withdrawnAt"}) | ||
| public class Advisory { | ||
|
|
||
| @JsonProperty("identifiers") |
There was a problem hiding this comment.
Let's add an extra line between fields.
sourabhsparkala
force-pushed
the
advisory
branch
2 times, most recently
from
56fa205 to
420b2d6
Compare
|
Thank you for the review @artem-smotrakov. |
- Create data model, entry as GitHubAdvisories.java - Add associated data model classes - Add helper query templates first_run_template and next_page_run_template - Add a data provider VulnerabilitiesFromGitHubAdvisories.java This fixes SAP#90
sourabhsparkala
force-pushed
the
advisory
branch
from
420b2d6 to
0008ac8
Compare
This fixes #90