-
Notifications
You must be signed in to change notification settings - Fork 567
feat: added deduplication database table #4206
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
Hm, that's a good question. Quick brainstorm on pros and cons: pro:
con:
I don't think there's an obvious winner here, but if you think it's worth loading it in to cvedb, the cons seem pretty minimal. |
BTW, since I didn't say in my nitpicky code review: this is looking good. I think we can punt on whether we need to add product into the dedupe table for now, but let's remove that test completely and probably not add the unknowns unless there was a reason for doing it that I misunderstood. |
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.
Summarizing from re-review: need to remove the UNKNOWNs and fix the zstandard test (either take zstandard out or replace it with something like pydantic that doesn't have the same version problem). But I see you got the FAIL-PKG-INFO test file removed, thanks!
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.
Rethought comments below. I still want a change but it's a different one.
cve_bin_tool/parsers/__init__.py
Outdated
for item in vendorlist: | ||
if item.product_info.vendor in invalidVendorList: | ||
vendorlist_filtered.append( | ||
ScanInfo( | ||
ProductInfo( | ||
"UNKNOWN", | ||
item.product_info.product, | ||
item.product_info.version, | ||
"/usr/local/bin/product", | ||
item.product_info.purl, | ||
), | ||
item.file_path, | ||
) | ||
) | ||
else: | ||
vendorlist_filtered.append(item) |
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.
Okay, after re-thinking, I think what we need is something more like this:
for item in vendorlist: | |
if item.product_info.vendor in invalidVendorList: | |
vendorlist_filtered.append( | |
ScanInfo( | |
ProductInfo( | |
"UNKNOWN", | |
item.product_info.product, | |
item.product_info.version, | |
"/usr/local/bin/product", | |
item.product_info.purl, | |
), | |
item.file_path, | |
) | |
) | |
else: | |
vendorlist_filtered.append(item) | |
for item in vendorlist: | |
if item.product_info.vendor not in invalidVendorList: | |
vendorlist_filtered.append(item) | |
# if we never found a valid vendor, add an unknown entry for this product | |
if len(vendorlist_filtered) == 0: | |
vendorlist_filtered.append( ... ) # FIXME: fill this in as above, but with a real filename |
As in, you're right that we want an unknown entry if there are no valid vendors, but we only need one per product.
When you fill it in, please make sure you're using self.filename instead of "/user/local/bin/product" please! If you're doing it because it makes the tests work, we should err on the side of changing the tests.
added the table inside our existing cve database Signed-off-by: Meet Soni <[email protected]>
the test did not make any sense as the package should be found Signed-off-by: Meet Soni <[email protected]>
Signed-off-by: Meet Soni <[email protected]>
Signed-off-by: Meet Soni <[email protected]>
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.
Ready to merge! Thanks for iterating with me on this!
I've disabled the test_language_package_none_found test for
FAIL-PKG-INFO
because it didn't really make any sense. And python-parser forPKG-INFO
ormetadata
was constructed solely to bypass this test which too didn't make any sense.Also, adding the table in our existing database (
cve.db
) makes me wonder if I should add the purl2cpe table into it too.Let me know what you think @terriko @anthonyharrison