feat: perl language parser #2614
Conversation
There was a problem hiding this comment.
Looking good. However I think the version checking needs to be more robust to allow for a range of versions rather than a fixed version. Currently if no version is specified the module is ignored - is this desirable?
I think we need to get a range of example cpanfiles to understand the likely use of this file (most of the examples I find on GitHub don't specify a version). We also need to make sure that when we find a module match, it relates to the Perl module and not some other language module with the same name.
Some of the examples in cpanfile documentation might be useful.
There was a problem hiding this comment.
As well as Anthony's questions from his review, I'd like to see the perl language parser added to the docs here. If we're not sure what to do about the version issues right now, we should at least document the design choices that were made and why and MANUAL.md would be the place to do that.
|
@anthonyharrison I have looked at a bunch of cpanfile on github. Yes, as you told there were a bunch of them where the version was not mentioned. One thing that could be done is could just like python parser where it installs requirements.txt on the go and detects their version but I am unsure if it would work for perl. Another way is to ignore them and mention it doesn't specify a version. And for the range of version, we cannot include it as it would be difficult to predict version between the range. And even if get the range it would still give a lot of excessive information and false positive as well. |
|
@Rexbeast2 I don't think it is a good idea to do a trial install of the module. It will require the Perl ecosystem and that is unlikely to be available on many systems. So I think what should be done is to use the examples to work out the various ways which versions can be specified - we need to handle cases where no version is specified as this is a common approach for many developers. So options to consider include No version which means ANY version Start simply and gradually introduce more complexity. There are a number of libraries which may help in parsing version strings. And of course we need test cases and the documentation will need updating. |
|
@anthonyharrison could you guide me on how to get version when no version is provided. |
|
@Rexbeast2 If there is no version specified, we are looking for any version of the module. The key module is the get_cves in cve_scanner. I think a version string of "" will work but you probably need to write a quick test to see if that does what is needed. Of course if the product is not found in the database, it is assumed that there are no vulnerabilities for any version. |
There was a problem hiding this comment.
Just a ping on this one: looks like isort is failing (you can fix it by running isort locally and it'll reorder the file), and we're still waiting on documentation at the very least, but ideally also the improved version stuff discussed in the comments above.
|
Oh! And also: if version detection is eluding you right now, it would also be viable to just document what this does and does not do, leave it disabled in the main cli.py code, and open it up to someone else to finish at a later date. |
|
@terriko I have finished up with the documentation. I am trying to incorporate multiple formats of canfile. There are many formats available for cpanfile. So right now I am trying to make a parser that uses regex to find and scan through most of the types of cpanfile. I would send a PR wrapping up with documentation and will open an issue for improving parser and will next PR of the improved perl parser when it finishes. |
|
@Rexbeast2 If we just start with a cpanfile with fixed versions, then we should document that as a limitation. However, iff= we find a cpanfile with version settings that the parser can't currently handle, we should log the fact that we are unable to process it. We can then refine the parser later to add more capability. |
|
Okay, I agree with your suggestion. I will send a pull request that includes this limitation in the documentation. As for the cpanfile parser, I will continue working on improving it to handle more generalized version settings. If we come across a cpanfile with version settings that the parser currently cannot handle, we will log this information and refine the parser in the future. Once the improvements are made, I will send another pull request. |
There was a problem hiding this comment.
I've flagged a small change in the docs in hopes of attracting someone with genius ideas on how to improve our support.
I think this is otherwise pretty close to ready to merge. but I do want to see the tests pass correctly, I think something was up with windows when this was last run so you might need to update your branch to make that happen.
README.md
Outdated
|
|
||
| The scanner examines the `cpanfile` file within a perl application to identify components. The package names and versions are used to search the database for vulnerabilities. | ||
|
|
||
| The `cpanfile` must contain the specific version of the dependency it have. If the dependency have the version range or no version specified it will be overlook. Here's an example of what a [`cpanfile`](https://github.com/intel/cve-bin-tool/blob/main/test/language_data/cpanfile) might look like. |
There was a problem hiding this comment.
| The `cpanfile` must contain the specific version of the dependency it have. If the dependency have the version range or no version specified it will be overlook. Here's an example of what a [`cpanfile`](https://github.com/intel/cve-bin-tool/blob/main/test/language_data/cpanfile) might look like. | |
| The `cpanfile` must specify the version data for the vulnerability scanner to work. At this time packages without versions will be ignored. (Patches welcome to improve this behaviour in future!) | |
| Here's an example of what a [`cpanfile`](https://github.com/intel/cve-bin-tool/blob/main/test/language_data/cpanfile) might look like. |
Codecov Report
@@ Coverage Diff @@
## main #2614 +/- ##
==========================================
+ Coverage 77.02% 83.08% +6.05%
==========================================
Files 642 666 +24
Lines 9999 10527 +528
Branches 1332 1425 +93
==========================================
+ Hits 7702 8746 +1044
+ Misses 1996 1424 -572
- Partials 301 357 +56
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 71 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I missed that this one was reviewed by Anthony several weeks ago, sorry! I've fixed the merge conflicts and I'm going to let the tests re-run now. I'm just heading out so I probably won't be back to review results until tomorrow. |
fixes #1978