-
Notifications
You must be signed in to change notification settings - Fork 532
Add basic json output #418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I'm pretty sure I'm going to merge #410 soon, so maybe we could make this fit with the framework that's started there? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging #410 has made this patch no longer mergeable. I think the idea is sound, but you'll need to put the json output code into the new OutputEngine.py before this can be merged.
0b4b40f
to
f140ee7
Compare
I have updated my branch to ensure it is compatible with the latest improvements in #410 I also took the opportunity to format the JSON in my preferred style, let me know what you think:
|
@mariuszskon Could you please include a test for this? For example, run the checker in each output mode, and ensure the output can be loaded successfully by either |
@pdxjohnny That's a great idea. |
Hi @pdxjohnny, I have added the tests. |
def formatted_modules(self): | ||
""" Returns self.modules converted into form | ||
formatted_output[modulesname][version] = { | ||
"cve": cve_number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't decide if this should be cve_number
(rather than cve
) for consistency, since we use cve_number just about anywhere we refer to a single number (as opposed to a list of cves). I think rather than hold up a review for this I'm going to just merge, but we might have to revisit when we see what the dffml team decides they actually want out of the json output and whether the difference matters to them. @pdxjohnny if you have an opinion now, maybe we could open an issue to discuss what "ideal" json output will look like?
class TestOutputEngine(unittest.TestCase): | ||
""" Test the OutputEngine class functions """ | ||
|
||
MOCK_MODULES = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hurrah for mock tests!
Thanks for your guidance @terriko and @pdxjohnny! Glad I could help out. |
Currently, CVE Binary Tool only outputs results in CSV format. It might be good to allow a wider range of uses by implementing alternative output formats such as JSON.
This pull requests implements very naive JSON output:
Immediately obvious to me is that the JSON is not structured intuitively. I would suggest further changes like adding not having the key be the CVE and value be the severity, but rather have each version contain a list of objects structured like this:
{"cve": "CVE-2012-0876", "severity": "MEDIUM"}
This might be better implemented by modifying the code relating to
scanner.all_cves
rather than working around the problem in the JSON formatter. This would mean adding other similar formats, like XML, would be very nice and easy.Another question, is whether this should be considered an "output mode" in the command line arguments. Based on the manual, it seems that "output mode" only deals with the verbosity of the output, but then again, this is the first non-CSV format 😃
Anyway, thanks in advance for reviewing this.