Scan all classpath entries #195
Conversation
Summary of ChangesHello @DerGuteMoritz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request broadens the capability of the dependency-check system by allowing it to scan all entries provided in the classpath, rather than being limited to only JAR files. This enhancement is crucial for integrating with other analyzers, such as the Node Audit Analyzer, which may require processing different file types like Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
src/nvd/task/check.clj
Outdated
| ;; Source paths such as `src`, while part of the classpath, won't | ||
| ;; be meaningfully analyzed by dependency-check-core. Thus, skip | ||
| ;; directories in general as well as non-existing files. |
There was a problem hiding this comment.
The updated comment correctly reflects that directories are being skipped. However, this behavior is too restrictive and conflicts with the requirements of some analyzers.
For instance, the Node Audit Analyzer documentation (a key motivation for this PR) states that it operates on the project directory.
By stripping out all directories from the classpath, we prevent this analyzer from being used as documented. Relying on passing the package-lock.json file directly is brittle, as it depends on an undocumented implementation detail of the analyzer that could change in future dependency-check versions.
To align with the goal of "Scan all classpath entries" and ensure robustness, the directory filtering should be removed. The Engine.scan() method is designed to handle directories, and letting it do so would make this feature more powerful and reliable.
There was a problem hiding this comment.
Quoting from the linked documentation page:
OWASP dependency-check includes a Node Audit Analyzer that scans package-lock.json files.
[...]
Files Types Scanned: package-lock.json
|
Hi! Thanks for the contribution. I'd prefer to do the least impactful change, namely only analyzing .jar and package-lock.json files. Cheers - V |
Understandable! Note that this patch is motivated by wanting to use However, note that all non-Java analyzers are disabled by default anyway so I'd wager that the impact of the more general patch should be tolerable. WDYT? |
|
SGTM then, given that we're still filtering out directories, and it's rare to other have files as classpath entries. |
DerGuteMoritz
force-pushed
the
support-scanning-any-files
branch
from
4004d04 to
0cd9061
Compare
|
Thanks! Can you confirm that the build is green in your fork? |
Ah, this requires an NVD API token... I can try to run this locally with our organization's token. Or can you perhaps explicitly trigger the check explicitly in the scope of this repo? (I am not familiar with GitHub actions). How about I also extend the integration tests to check a |
|
NVD tokens can be obtained instantly so it should be fairly easy to run that from a fork
Your builds were triggered but fail probably due to secrets not being relayed for "external" contributors. Either way I don't have a lot of time to debug
Yes, that would be very much appreciated! |
This allows using e.g. the [Node Audit Analyzer](https://dependency-check.github.io/DependencyCheck/analyzers/node-audit-analyzer.html) (when enabled via the `nvd.analyzer.node-audit-enabled` config option) by passing `package-lock.json` as part of `classpath`. Note that the filtering in `-main` still takes care of removing directories and non-existing files. The comment there is updated to reflect the new behavior.
Otherwise, `~` would be interpreted literally and the existence check would always remove such entries.
Otherwise, entries like `~foo` would expand to something like `/home/userfoo`.
DerGuteMoritz
force-pushed
the
support-scanning-any-files
branch
from
edcdf6e to
c349827
Compare
|
Alright, here we go: bevuta#1 |
src/nvd/task/check.clj
Outdated
| (defn jar? [^String filename] | ||
| (.endsWith filename ".jar")) | ||
| (def classpath-separator-re | ||
| (re-pattern (str File/pathSeparatorChar))) |
There was a problem hiding this comment.
IIRC the classpath separator is the same in all OSes, which would be my bad!
There was a problem hiding this comment.
The documentation says:
The system-dependent path-separator character. This field is initialized to contain the first character of the value of the system property path.separator. This character is used to separate filenames in a sequence of files given as a path list. On UNIX systems, this character is ':'; on Microsoft Windows systems it is ';'.
TIL 😄 However, looking at it again now gave me pause because there is a potential for regex injection. Improved with 826ec57.
There was a problem hiding this comment.
Hm, for some reason, that commit doesn't show up in any of the PRs... 🤔 Looks like GH is having a hiccup.
Nevermind, it's there now!
In practice, only ":" and ";" are used but quoting is prudent to not give readers pause.
2 participants
This allows using e.g. the Node Audit Analyzer (when enabled via the
nvd.analyzer.node-audit-enabledconfig option) by passingpackage-lock.jsonas part ofclasspath. Note that the filtering in-mainstill takes care of removing directories and non-existing files. The comment there is updated to reflect the new behavior.