feat: dependency-check-plugin-revision#1529
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
kriswest
left a comment
There was a problem hiding this comment.
It seems odd to have a plug-in to a complete extra clone of the repository and compute its own diff in order to run scanning tools on it, when when the built-in scanners already do both of these things... Can this not be written to use the data from the existing clone and diff operations?
|
@kriswest Yes, I thought it was strange in the original PR too. I believe it was structured this way because plugins are inserted at position 1 in the processor chain after parsePush so when the plugin executes, neither the bare clone nor the diff step exist yet. To reuse that data, I believe the plugin insertion point would need to move to later in the chain, after pullRemote and getDiff have run. I can take a look at changing that. |
jescalada
left a comment
There was a problem hiding this comment.
LGTM - although as Kris mentioned, this seems more appropriate (and useful for the wider community) as a default processor rather than a plugin since we're pulling/scanning the diff. Is there a reason to keep it around as a plugin?
|
|
||
| // Apply the pushed pack data to the local bare clone. | ||
| // req.body is the raw pack buffer, set by proxyFilter before the chain runs. | ||
| spawnSync('git', ['receive-pack', action.repoName], { |
There was a problem hiding this comment.
We might want to check the result of spawnSync or wrap it in a try-catch. If unpacking silently fails then the subsequent diff would be empty and the plugin/processor would allow the push regardless of the actual contents.
|
|
||
| **Configuration** | ||
|
|
||
| Set the `DEPENDENCY_VULN_THRESHOLD` environment variable to control which severity levels trigger a block. |
There was a problem hiding this comment.
The code below mentions that we should set the dependencyVulnThreshold on the proxy.config.json, which I think is the cleaner solution. Should we perhaps replace the environment variable with the config entry (plugin array) for this plugin?
Description
Revises and addresses review feedback on #799, adding dependency vulnerability scanner plugin.
The plugin is intended to intercept pushes and scans changed dependency files against the OWASP Vuln database. If a vulnerability is found at or above a severity threshold, the push will be held for review.
Made changes according to reviewer comments on original PR.
General changes:
DEPENDENCY_VULN_THRESHOLDenvironment variable (default:HIGH) rather than a top-level config schema entrygit diffargument order (Previously showed removals instead of additions)git showusingHEADinstead of the pushed commit SHAsrc/lib/foo.json)deleteTempReponow deletes the entire.tempRepo/directory instead of only the current push's subdirectorydependency-checkto prevent silent false negatives on failurespawnSyncfor the clone operation with asyncspawnto avoid blockingthe Node.js event loop
pullRemoteprocessorexportsentry and README documentation for the new pluginRelated Issue
Revises #799
Checklist
General
Documentation
Configuration
config.schema.json) was modified:npm run generate-config-types)npm run gen-schema-doc)Tests
npm test)npm run lintandnpm run format:check)npm run check-types)