Skip to content
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

fix(parser): multiple vendors for java #2802

Merged
merged 11 commits into from
Apr 18, 2023
Merged

Conversation

metabiswadeep
Copy link
Contributor

Fixes #2798

@metabiswadeep
Copy link
Contributor Author

@ffontaine Could you help me find out what is wrong with this fix?

@terriko
Copy link
Contributor

terriko commented Mar 9, 2023

Look closer to the top of the error in the logs, specifically at the package_names line.

self = <cve_bin_tool.cvedb.CVEDB object at 0x000001E01D0FDF90>
package_names = <Element '{http://maven.apache.org/POM/4.0.0}artifactId' at 0x000001E01DBD34C0>

    def get_vendor_product_pairs(self, package_names) -> list[dict[str, str]]:
        """
        Fetches vendor from the database for packages that doesn't have vendor info for Package List Parser Utility and Universal Python package checker.
        """
        cursor = self.db_open_and_get_cursor()
        vendor_package_pairs = []
        query = """
        SELECT DISTINCT vendor FROM cve_range
        WHERE product=?
        """
    
        # For python package checkers we don't need the progress bar running
        if type(package_names) != list:
>           cursor.execute(query, [package_names])
E           sqlite3.InterfaceError: Error binding parameter 0 - probably unsupported type.

cve_bin_tool\cvedb.py:474: InterfaceError
============================== warnings summary ===============================
test/test_csv2cve.py: 8 warnings
test/test_cvescanner.py: 10 warnings
test/test_exploits.py: 4 warnings
test/test_merge.py: 3 warnings
  C:\hostedtoolcache\windows\Python\3.10.10\x64\lib\site-packages\packaging\version.py:111: DeprecationWarning: Creating a LegacyVersion has been deprecated and will be removed in the next major release
    warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ===========================
FAILED test/test_language_scanner.py::TestLanguageScanner::test_java_package[D:\\a\\cve-bin-tool\\cve-bin-tool\\test\\language_data\\pom.xml-commons_io] - sqlite3.InterfaceError: Error binding parameter 0 - probably unsupported type.
FAILED test/test_language_scanner.py::TestLanguageScanner::test_language_package_none_found[D:\\a\\cve-bin-tool\\cve-bin-tool\\test\\language_data\\fail_pom.xml] - sqlite3.InterfaceError: Error binding parameter 0 - probably unsupported type.

you might want to toss a debug line in there or something, but it looks like the package names you're passing in aren't getting turned into searchable strings before you put them into sql.

@anthonyharrison
Copy link
Contributor

@terriko @metabiswadeep I have a fix which applies to all language parsers (I needed this for the SBOM generation work I am doing at the moment) although I haven't looked at the impact that it wll have on the tests.

@metabiswadeep
Copy link
Contributor Author

@anthonyharrison Ok, then should I close this PR?

@anthonyharrison
Copy link
Contributor

@metabiswadeep No carry on. I hadn't realised that you were already fixing the issue. I will pick up the fix once it gets merged.

@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2023

Codecov Report

Merging #2802 (2da1eee) into main (00a66ba) will increase coverage by 0.17%.
The diff coverage is 83.33%.

❗ Current head 2da1eee differs from pull request most recent head e728505. Consider uploading reports for the commit e728505 to get more accurate results

@@            Coverage Diff             @@
##             main    #2802      +/-   ##
==========================================
+ Coverage   82.41%   82.59%   +0.17%     
==========================================
  Files         671      671              
  Lines       10597    10602       +5     
  Branches     1426     1429       +3     
==========================================
+ Hits         8734     8757      +23     
+ Misses       1493     1479      -14     
+ Partials      370      366       -4     
Flag Coverage Δ
longtests 75.96% <83.33%> (+0.02%) ⬆️
win-longtests 80.33% <83.33%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cve_bin_tool/parsers/java.py 72.83% <83.33%> (+0.47%) ⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@metabiswadeep
Copy link
Contributor Author

@terriko Is this okay now?

@metabiswadeep
Copy link
Contributor Author

@terriko Are there any changes necessary for this?

@terriko
Copy link
Contributor

terriko commented Mar 30, 2023

Hey, sorry this one fell off my radar! I've got to run in a few minutes so i don't have time to do detailed code review, but I'll put a tag on it so I know I need to come back and do that hopefully later today.

@terriko terriko added the awaiting maintainer Need a maintainer to respond / help out label Mar 30, 2023
@metabiswadeep
Copy link
Contributor Author

@terriko Looks like all tests are passing. Is this okay?

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this needs a little refactoring to avoid duplicating the code. Rather than removing the find_vendor function and moving the code twice, can you just make sure the find_vendor function returns multiple matches and that they're used correctly in the other code?

@terriko terriko removed the awaiting maintainer Need a maintainer to respond / help out label Apr 12, 2023
@metabiswadeep
Copy link
Contributor Author

@terriko I have refactored the code.

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ready to go now. Thanks for refactoring!

@terriko terriko merged commit fa05ece into intel:main Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve handling of multiple vendors in package parsers
4 participants