Add OSV dependency vulnerability scanning#461
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (34)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (29)
📝 WalkthroughWalkthroughAdds OSV-backed SCA: new ChangesDependency Vulnerability Scanning Implementation
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as foxguard sca
participant Scanner as scan_dependency_vulnerabilities
participant Parser as LockfileParser
participant OSV as OSV Source (DB/Cache/Network)
participant Reporter as Report Emitter
User->>CLI: foxguard sca . --sca-db osv.json
CLI->>Scanner: scan_dependency_vulnerabilities(root, options)
Scanner->>Parser: discover & parse lockfiles
Parser->>Scanner: package refs (name, version, spans)
Scanner->>OSV: resolve vulnerabilities (local DB / cache / querybatch)
OSV->>Scanner: vulnerabilities for package keys
Scanner->>Reporter: findings with dep metadata
Reporter->>User: emit JSON/SARIF with dependency fields
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/config.rs (1)
253-283:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompute
sca_onlyafter merging effective SCA/no_builtins defaults.Line 253 computes
sca_onlybefore config defaults are applied, so a config-only SCA setup (scan.sca: true+scan.no_builtins: true) can still pull inscan.rulesat Lines 254-255. That breaks intended SCA-only behavior.💡 Proposed fix
pub fn apply_scan_defaults(scan: &mut ScanArgs, config: Option<&FoxguardConfig>) { let Some(config) = config else { return; }; - let sca_only = scan.sca && scan.no_builtins && !scan.pq_mode; - if scan.rules.is_none() && !sca_only { - scan.rules = config.scan.rules.clone(); - } if !scan.no_builtins && config.scan.no_builtins { scan.no_builtins = true; } + if !scan.sca && config.scan.sca { + scan.sca = true; + } + let sca_only = scan.sca && scan.no_builtins && !scan.pq_mode; + if scan.rules.is_none() && !sca_only { + scan.rules = config.scan.rules.clone(); + } if scan.severity.is_none() { scan.severity = config.scan.severity; } @@ - if !scan.sca && config.scan.sca { - scan.sca = true; - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.rs` around lines 253 - 283, The sca_only flag is computed too early using pre-merge values (sca_only = scan.sca && scan.no_builtins && !scan.pq_mode) which allows config-provided SCA + no_builtins to be treated as not-SCA-only and thus pull in config.scan.rules; to fix, compute or re-evaluate sca_only after applying the defaults (the blocks that set scan.sca, scan.no_builtins, scan.sca_offline, etc.) or change the rules-assignment condition to check the effective values (e.g., use the merged values of scan.sca and scan.no_builtins or test config.scan.* combined with current scan.*) so that when the effective state is SCA-only you do not assign config.scan.rules to scan.rules.src/app.rs (1)
351-358:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNo-files warning is misleading in SCA-only mode.
When SCA runs without lockfiles, this branch still emits source-extension guidance (Line 354), which is confusing for dependency scans.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app.rs` around lines 351 - 358, The "no files found" notice currently always includes source-file extension guidance even when running SCA-only (dependency/lockfile) scans; update the branch that checks files_scanned, coccinelle_candidate_files, codeql_candidate_rules and stats.files_discovered so that the notices.push(format!(...)) is only executed when we are not in SCA-only/lockfile-only mode—i.e., add a guard that detects dependency-only runs (for example by checking the existing lockfile-candidate/lockfile flag or a sca_only/dependency_scan_only indicator) and skip emitting the supported-extensions message in that case; modify the if-condition or wrap the notices.push call (referencing files_scanned, coccinelle_candidate_files, codeql_candidate_rules, stats.files_discovered, coccinelle_rules, and notices.push) to implement this behavior.
🧹 Nitpick comments (1)
Cargo.toml (1)
60-60: ⚡ Quick winMove or relabel
reqwestto avoid “optional deps” confusion.
reqwestis now always enabled, but it still sits under the “Optional deps for the GitHub App” comment block. Please move it outside that block (or adjust the comment) so the dependency intent stays clear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` at line 60, The reqwest dependency line currently listed as `reqwest = { version = "0.13.3", default-features = false, features = ["blocking", "json", "rustls"] }` is placed under the “Optional deps for the GitHub App” comment but is actually always enabled; move that reqwest entry out of that comment block (or update the comment to reflect it is a non-optional runtime dependency) so the intent is clear—locate the reqwest dependency in Cargo.toml and either relocate it above/below the optional-deps section or change the section heading to indicate it contains both optional and required deps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/deps.rs`:
- Around line 1099-1120: The compare_versions implementation misorders
pre-release and complex strings (e.g., "1.0.0-rc1" vs "1.0.0"); replace the
ad-hoc logic by first attempting to parse both strings with a semver-aware
parser (e.g., semver::Version::parse) and, if both parse successfully, compare
the resulting Version values (which correctly treat pre-release as lower
precedence); only if parsing fails for either side fall back to the existing
version_parts/lexical logic in compare_versions (and keep version_parts as the
non-semver fallback). Update compare_versions to try semver parsing first and
use the semver comparison result to drive version_in_range decisions.
---
Outside diff comments:
In `@src/app.rs`:
- Around line 351-358: The "no files found" notice currently always includes
source-file extension guidance even when running SCA-only (dependency/lockfile)
scans; update the branch that checks files_scanned, coccinelle_candidate_files,
codeql_candidate_rules and stats.files_discovered so that the
notices.push(format!(...)) is only executed when we are not in
SCA-only/lockfile-only mode—i.e., add a guard that detects dependency-only runs
(for example by checking the existing lockfile-candidate/lockfile flag or a
sca_only/dependency_scan_only indicator) and skip emitting the
supported-extensions message in that case; modify the if-condition or wrap the
notices.push call (referencing files_scanned, coccinelle_candidate_files,
codeql_candidate_rules, stats.files_discovered, coccinelle_rules, and
notices.push) to implement this behavior.
In `@src/config.rs`:
- Around line 253-283: The sca_only flag is computed too early using pre-merge
values (sca_only = scan.sca && scan.no_builtins && !scan.pq_mode) which allows
config-provided SCA + no_builtins to be treated as not-SCA-only and thus pull in
config.scan.rules; to fix, compute or re-evaluate sca_only after applying the
defaults (the blocks that set scan.sca, scan.no_builtins, scan.sca_offline,
etc.) or change the rules-assignment condition to check the effective values
(e.g., use the merged values of scan.sca and scan.no_builtins or test
config.scan.* combined with current scan.*) so that when the effective state is
SCA-only you do not assign config.scan.rules to scan.rules.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 60: The reqwest dependency line currently listed as `reqwest = { version
= "0.13.3", default-features = false, features = ["blocking", "json", "rustls"]
}` is placed under the “Optional deps for the GitHub App” comment but is
actually always enabled; move that reqwest entry out of that comment block (or
update the comment to reflect it is a non-optional runtime dependency) so the
intent is clear—locate the reqwest dependency in Cargo.toml and either relocate
it above/below the optional-deps section or change the section heading to
indicate it contains both optional and required deps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6784019d-9cc1-4c0d-b05b-71139bd569ce
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
Cargo.tomlREADME.mddocs/dependency-scanning.mdsrc/app.rssrc/baseline.rssrc/cli.rssrc/compliance.rssrc/config.rssrc/deps.rssrc/diff.rssrc/engine/coccinelle.rssrc/engine/codeql.rssrc/github_app/review.rssrc/lib.rssrc/main.rssrc/output/mod.rssrc/report/cbom.rssrc/report/github_pr.rssrc/report/json.rssrc/report/sarif.rssrc/rules/c.rssrc/rules/common.rssrc/rules/go.rssrc/rules/javascript.rssrc/rules/kotlin.rssrc/rules/manifest.rssrc/rules/mod.rssrc/rules/python.rssrc/rules/semgrep_compat.rssrc/rules/semgrep_taint.rssrc/secrets.rssrc/tui/tests.rstests/integration.rswww/src/data/rules.ts
| fn compare_versions(left: &str, right: &str) -> Ordering { | ||
| let left_parts = version_parts(left); | ||
| let right_parts = version_parts(right); | ||
| for (left, right) in left_parts.iter().zip(right_parts.iter()) { | ||
| let ordering = match (left.parse::<u64>(), right.parse::<u64>()) { | ||
| (Ok(left), Ok(right)) => left.cmp(&right), | ||
| _ => left.cmp(right), | ||
| }; | ||
| if ordering != Ordering::Equal { | ||
| return ordering; | ||
| } | ||
| } | ||
| left_parts.len().cmp(&right_parts.len()) | ||
| } | ||
|
|
||
| fn version_parts(value: &str) -> Vec<String> { | ||
| value | ||
| .split(|ch: char| !ch.is_ascii_alphanumeric()) | ||
| .filter(|part| !part.is_empty()) | ||
| .map(ToString::to_string) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
Version ordering is not advisory-accurate for pre-release/complex versions.
The comparator at Line 1099 can mis-order versions like 1.0.0-rc1 and 1.0.0, which directly affects version_in_range matching and can produce incorrect vulnerability findings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/deps.rs` around lines 1099 - 1120, The compare_versions implementation
misorders pre-release and complex strings (e.g., "1.0.0-rc1" vs "1.0.0");
replace the ad-hoc logic by first attempting to parse both strings with a
semver-aware parser (e.g., semver::Version::parse) and, if both parse
successfully, compare the resulting Version values (which correctly treat
pre-release as lower precedence); only if parsing fails for either side fall
back to the existing version_parts/lexical logic in compare_versions (and keep
version_parts as the non-semver fallback). Update compare_versions to try semver
parsing first and use the semver comparison result to drive version_in_range
decisions.
Darkroom4364
force-pushed
the
issue-448-osv-sca
branch
from
7eaffb7 to
5841160
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 60: The reqwest dependency is currently placed under the "Optional deps
for the GitHub App" block but is not declared optional and not tied to
features.github-app; either move the reqwest = { version = "0.13.3",
default-features = false, features = ["blocking", "json", "rustls"] } line into
the main [dependencies] section to show it is always enabled, or make it
optional by adding optional = true and then add "dep:reqwest" to the
features.github-app entry so features.github-app actually enables reqwest;
update Cargo.toml accordingly to keep placement and feature wiring consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cb871a78-d449-4140-a1e1-50b5ed754335
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
Cargo.tomlREADME.mddocs/dependency-scanning.mdsrc/app.rssrc/baseline.rssrc/cli.rssrc/compliance.rssrc/config.rssrc/deps.rssrc/diff.rssrc/engine/coccinelle.rssrc/engine/codeql.rssrc/github_app/review.rssrc/lib.rssrc/main.rssrc/output/mod.rssrc/report/cbom.rssrc/report/github_pr.rssrc/report/json.rssrc/report/sarif.rssrc/rules/c.rssrc/rules/common.rssrc/rules/go.rssrc/rules/javascript.rssrc/rules/kotlin.rssrc/rules/manifest.rssrc/rules/mod.rssrc/rules/python.rssrc/rules/semgrep_compat.rssrc/rules/semgrep_taint.rssrc/secrets.rssrc/tui/tests.rstests/integration.rswww/src/data/rules.ts
✅ Files skipped from review due to trivial changes (5)
- src/rules/python.rs
- src/compliance.rs
- src/github_app/review.rs
- docs/dependency-scanning.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (27)
- src/rules/semgrep_taint.rs
- src/output/mod.rs
- src/secrets.rs
- src/rules/mod.rs
- src/report/sarif.rs
- src/baseline.rs
- src/report/cbom.rs
- src/report/github_pr.rs
- src/engine/codeql.rs
- src/rules/kotlin.rs
- src/rules/manifest.rs
- src/engine/coccinelle.rs
- src/rules/javascript.rs
- src/rules/semgrep_compat.rs
- src/report/json.rs
- src/rules/common.rs
- src/lib.rs
- src/rules/c.rs
- src/rules/go.rs
- src/tui/tests.rs
- tests/integration.rs
- src/app.rs
- src/diff.rs
- src/main.rs
- src/config.rs
- src/deps.rs
- src/cli.rs
Darkroom4364
force-pushed
the
issue-448-osv-sca
branch
from
5841160 to
950ce9b
Compare
![foxguard-app[bot]](https://avatars.githubusercontent.com/in/3854821?s=60&v=4){kind=small}
| ) | ||
| } else if let Some(files) = targets.as_ref() { | ||
| scan_paths_with_root_with_notices( | ||
| Path::new(&scan.path), |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-path-traversal (CWE-22)
Path::new called with dynamic path — validate input to prevent path traversal
| cache_path: scan.sca_cache.as_ref().map(PathBuf::from), | ||
| }; | ||
| let sca_result = scan_dependency_vulnerabilities( | ||
| Path::new(&scan.path), |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-path-traversal (CWE-22)
Path::new called with dynamic path — validate input to prevent path traversal
|
|
||
| #[test] | ||
| fn local_advisory_matching_uses_ecosystem_name_and_version() { | ||
| let package = parse_package_lock( |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.unwrap() can panic at runtime — use proper error handling with ? or match
| dep_path: vec![], | ||
| }; | ||
|
|
||
| let caret = render_source_context("😀exec(cmd);\n", &finding, 0) |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
| dep_path: vec![], | ||
| }; | ||
|
|
||
| let caret = render_source_context("e\u{301}exec(cmd);\n", &finding, 0) |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
| dep_path: vec![], | ||
| }; | ||
|
|
||
| let caret = render_source_context("e\u{301}x\n", &finding, 0) |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
|
|
||
| #[test] | ||
| fn sca_reports_osv_vulnerabilities_for_supported_lockfiles() { | ||
| let dir = TempDir::new().expect("temp dir"); |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
| "HIGH" | ||
| ) | ||
| ]); | ||
| fs::write( |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
| ]); | ||
| fs::write( | ||
| &db_path, | ||
| serde_json::to_string_pretty(&advisories).expect("json"), |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
| ) | ||
| .expect("write osv fixture"); | ||
|
|
||
| let output = foxguard_cmd() |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
| "tests/fixtures/deps", | ||
| "--sca-offline", | ||
| "--sca-db", | ||
| db_path.to_str().expect("utf8 path"), |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
|
|
||
| #[test] | ||
| fn sca_sarif_includes_dependency_properties() { | ||
| let dir = TempDir::new().expect("temp dir"); |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
| "6.5.5", | ||
| "HIGH" | ||
| )]); | ||
| fs::write( |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
| )]); | ||
| fs::write( | ||
| &db_path, | ||
| serde_json::to_string_pretty(&advisories).expect("json"), |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
| ) | ||
| .expect("write osv fixture"); | ||
|
|
||
| let output = foxguard_cmd() |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
| "tests/fixtures/deps/package-lock.json", | ||
| "--sca-offline", | ||
| "--sca-db", | ||
| db_path.to_str().expect("utf8 path"), |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
|
|
||
| #[test] | ||
| fn sca_offline_without_db_does_not_disable_pq_manifest_rules() { | ||
| let output = foxguard_cmd() |
There was a problem hiding this comment.
foxguard · MEDIUM · rs/no-unwrap-in-lib (CWE-248)
.expect() can panic at runtime — use proper error handling with ? or match
1 participant
Summary
Verification
Closes #448
Summary by CodeRabbit
New Features
Documentation
Tests