Harden cve-fix with pre-fix scanning, binary verification, PR dedup and input guardrails#41
Merged
Merged
Conversation
…and input guardrails Add /scan phase between /start and /patch that runs ecosystem-specific vulnerability scanners to confirm a CVE is present before attempting fixes. Supports Go GOTOOLCHAIN matching to prevent false negatives. Enhance /validate with post-fix binary verification — for Go projects, builds the binary and runs govulncheck -mode binary to catch cases where replace directives or transitive overrides silently reintroduce the vulnerable version. Add existing-PR deduplication check to /pr — three-layer search (exact CVE, bot updates, same-package from other CVEs) prevents duplicate PRs. Add untrusted input guardrails to guidelines and /start to prevent injection from Jira ticket content. All deterministic operations are backed by Python scripts (stdlib-only) with inline fallback instructions in the skill files.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR introduces a ChangesCVE-Fix Scan Phase Integration
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 5
🧹 Nitpick comments (3)
cve-fix/scripts/verify.py (2)
56-102: 💤 Low valueTemp directory leaks if any step between
mkdtempandrmtreeraises.
binary_diris created on Line 56 but only cleaned up on Line 102 in linear flow — asubprocessexception or interrupt between those points leaves the temp dir behind. Wrap the build/scan block intry/finally(and move theimport shutilto module scope).♻️ Suggested fix
+import shutil ... binary_dir = tempfile.mkdtemp(prefix="cve-verify-") - scan_method = "binary" - scan_output = "" - scan_exit = 0 - - # Try building binaries - build_exit, build_out = run( - ["go", "build", "-o", f"{binary_dir}/", "./..."], - env=env, - ) - ... - # Clean up - import shutil - shutil.rmtree(binary_dir, ignore_errors=True) + try: + ... + finally: + shutil.rmtree(binary_dir, ignore_errors=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 `@cve-fix/scripts/verify.py` around lines 56 - 102, The temp dir created with tempfile.mkdtemp (binary_dir) can leak if an exception/interrupt occurs before shutil.rmtree is reached; wrap the build/scan logic that uses binary_dir (the run(...) call that does go build, the binaries iteration with govulncheck, and the fallback govulncheck calls) in a try/finally that always calls shutil.rmtree(binary_dir, ignore_errors=True) in the finally block, and move the import shutil to module scope so cleanup is available regardless of failures; ensure variables like scan_exit/scan_output are initialized before the try so their values remain defined after cleanup.
111-126: ⚖️ Poor tradeoff
npm install --package-lock-onlymutates the working tree during verification.The verify step is intended to be informational, but this rewrites
package-lock.jsonin the user's repo. If the user then runsgit diff/git status, the lockfile churn is conflated with the actual fix commit. Consider either (a) running the regeneration in a temp copy of the repo, (b) stashing/reverting the lockfile change after the audit, or (c) at minimum documenting that/validatemay modify the lockfile so users can git-stash beforehand.🤖 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 `@cve-fix/scripts/verify.py` around lines 111 - 126, The verify_node function currently runs "npm install --package-lock-only" in-place which mutates package-lock.json; change verify_node to avoid changing the working tree by either (preferred) performing the regeneration and audit inside a temporary copy of the repo (create a tempdir, copy the repo files (excluding .git) into it and call run(["npm","install","--package-lock-only"], cwd=tmpdir) and run(["npm","audit","--json"], cwd=tmpdir)), or (simpler) run the install in-place but immediately revert the lockfile change with git (e.g., call run(["git","checkout","--","package-lock.json"]) after the regen) so the working tree is unmodified; update the run(...) invocations in verify_node accordingly (use the cwd argument for tempdir or add the revert run call) and ensure scan_method and exit codes are preserved.cve-fix/scripts/check_existing_prs.py (1)
66-91: 💤 Low value
search_by_packagesubstring match (lines 88, 70) can trigger false positives for common package tokens.
package in diff_outand--search packagematch any occurrence of the token in PR titles and diffs, not just dependency manifest lines. Short or common names likego,url,http, or path components likehelmwill match unrelated PRs. Consider restricting the diff match to manifest-file sections (e.g., require the package to appear on ago.mod,package.json, orrequirements.txtline) or at minimum within a hunk header context.🤖 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 `@cve-fix/scripts/check_existing_prs.py` around lines 66 - 91, search_by_package currently uses a raw substring check ("package in diff_out") and the gh "--search" token which yields false positives for short/common tokens; update search_by_package to parse the PR diff returned by gh("pr","diff",...) and only consider matches that occur in dependency/manifest contexts or hunk headers (e.g., file path lines starting with "diff --git" / "+++ b/" or hunk headers "@@") or within lines that look like manifest entries (e.g., lines from go.mod, package.json, requirements.txt) and match the package as a whole word (use regex word boundaries) instead of a plain substring; keep the rest of the logic (calling gh, iterating candidates) but replace the simple package substring test with a function that scans diff_out for the package in those manifest/hunk contexts before returning the PR.
🤖 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 `@cve-fix/scripts/check_existing_prs.py`:
- Around line 110-154: The code currently only wraps search_by_cve in a
try/except, so gh failures in search_by_bot and search_by_package can crash or
be misinterpreted as "no match"; wrap each call to search_by_cve, search_by_bot,
and search_by_package in a try/except Exception as exc, and on any exception
emit the standardized JSON {"found": false, "match_type": "none", "error":
str(exc)} and return exit code 2; also ensure that the functions either raise on
gh non-zero failures or that you detect and treat a special error return
consistently (i.e., convert any gh-error sentinel into raising an Exception so
the centralized except block handles it).
In `@cve-fix/scripts/scan.py`:
- Around line 166-183: The function check_manifests currently returns
content.splitlines()[0] which is the file's first line, not the line that
actually contains the package; change it to search the file lines for the first
line where package.lower() is in line.lower() and return that matching line (or
"" if none), preserving the existing manifests dict, OSError handling, and
behavior of returning a single context string from check_manifests.
- Around line 206-231: The determine_verdict function currently assumes
govulncheck exit semantics when computing scanner_success from
scan_result["scan_exit_code"], which causes npm and pip-audit (which use exit
code 1 to indicate found vulns) to be misclassified as scan_failed; update
determine_verdict to read scan_result["scan_tool"] and normalize exit codes per
tool (e.g., treat exit 1 for "npm" and "pip-audit" as "vulns found"/successful
scan similar to govulncheck's 3), then compute scanner_success from the
normalized semantics and use that normalized value in the subsequent
manifest_match/base_images/scan_failed logic so Node/Python scans that report
vulnerabilities are not treated as failures; keep references to
determine_verdict, scan_result["scan_exit_code"], scan_result["scan_tool"],
scanner_success, and existing conditional branches when updating the logic.
In `@cve-fix/scripts/verify.py`:
- Around line 152-162: The determine_verdict function misclassifies scanners
that exit with code 1 (vulns found) as failures; update determine_verdict to
mirror scan.py's normalization: keep the initial check that non-zero exit with
empty output => "scan_failed" (i.e., if exit_code != 0 and not output.strip()
return "scan_failed"), but treat exit codes like 1 (and 3) as non-fatal when
output exists by changing the next condition to check exit_code not in (0,1,3)
(or otherwise normalize per scanner) before returning "scan_failed"; use the
existing symbols determine_verdict, scan_result["scan_exit_code"],
scan_result["scan_output"], and cve_id to implement this minimal adjustment so
scanners that ran successfully but found other vulns don't cause a false
"scan_failed".
In `@cve-fix/skills/validate.md`:
- Line 59: Replace the absolute path "/tmp/verify/" used in the Go build and
govulncheck examples with a relative artifact path (e.g.,
".artifacts/cve-fix/{context}/verify/") so the markdown follows the
relative-path rule and CI validate-structure.sh passes; update both the build
command (`go build -o /tmp/verify/ ./...`) and the scan command (`govulncheck
-mode binary /tmp/verify/{binary}` and fallback `govulncheck -show verbose
./...`) to use the new relative directory while keeping the same filenames and
placeholders.
---
Nitpick comments:
In `@cve-fix/scripts/check_existing_prs.py`:
- Around line 66-91: search_by_package currently uses a raw substring check
("package in diff_out") and the gh "--search" token which yields false positives
for short/common tokens; update search_by_package to parse the PR diff returned
by gh("pr","diff",...) and only consider matches that occur in
dependency/manifest contexts or hunk headers (e.g., file path lines starting
with "diff --git" / "+++ b/" or hunk headers "@@") or within lines that look
like manifest entries (e.g., lines from go.mod, package.json, requirements.txt)
and match the package as a whole word (use regex word boundaries) instead of a
plain substring; keep the rest of the logic (calling gh, iterating candidates)
but replace the simple package substring test with a function that scans
diff_out for the package in those manifest/hunk contexts before returning the
PR.
In `@cve-fix/scripts/verify.py`:
- Around line 56-102: The temp dir created with tempfile.mkdtemp (binary_dir)
can leak if an exception/interrupt occurs before shutil.rmtree is reached; wrap
the build/scan logic that uses binary_dir (the run(...) call that does go build,
the binaries iteration with govulncheck, and the fallback govulncheck calls) in
a try/finally that always calls shutil.rmtree(binary_dir, ignore_errors=True) in
the finally block, and move the import shutil to module scope so cleanup is
available regardless of failures; ensure variables like scan_exit/scan_output
are initialized before the try so their values remain defined after cleanup.
- Around line 111-126: The verify_node function currently runs "npm install
--package-lock-only" in-place which mutates package-lock.json; change
verify_node to avoid changing the working tree by either (preferred) performing
the regeneration and audit inside a temporary copy of the repo (create a
tempdir, copy the repo files (excluding .git) into it and call
run(["npm","install","--package-lock-only"], cwd=tmpdir) and
run(["npm","audit","--json"], cwd=tmpdir)), or (simpler) run the install
in-place but immediately revert the lockfile change with git (e.g., call
run(["git","checkout","--","package-lock.json"]) after the regen) so the working
tree is unmodified; update the run(...) invocations in verify_node accordingly
(use the cwd argument for tempdir or add the revert run call) and ensure
scan_method and exit codes are preserved.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9b6e723b-7cec-496f-9e20-0fc14eb5352b
📒 Files selected for processing (13)
AGENTS.mdcve-fix/README.mdcve-fix/SKILL.mdcve-fix/commands/scan.mdcve-fix/guidelines.mdcve-fix/scripts/check_existing_prs.pycve-fix/scripts/scan.pycve-fix/scripts/verify.pycve-fix/skills/controller.mdcve-fix/skills/pr.mdcve-fix/skills/scan.mdcve-fix/skills/start.mdcve-fix/skills/validate.md
Wrap all three search layers in a unified try/except so that subprocess.TimeoutExpired or OSError from gh CLI in Layers 2 and 3 produces the documented JSON error and exit code 2 instead of an unhandled traceback misinterpreted as "no match found".
Return the actual line containing the package name rather than the first line of the manifest file, so the scan result includes meaningful context about the dependency.
npm audit and pip-audit exit with code 1 when vulnerabilities are found (a successful scan), but the code assumed govulncheck semantics where only 0 and 3 are success. Extract _is_successful_scan() that normalizes exit codes per tool so Node.js and Python scans that find vulnerabilities are not misclassified as scan_failed.
Same issue as scan.py: npm audit and pip-audit exit with code 1 when vulnerabilities are found, which was misclassified as scan_failed. Add _is_successful_scan() that normalizes exit codes per scan method so post-fix verification correctly reports "fixed" when the target CVE is gone but other vulnerabilities remain.
When /scan determines a CVE is absent, informational, or in a base image, produce a structured VEX justification (following the 5 CSAF types) that /close can paste directly into the Jira comment. Three types are auto-detected: component not present, vulnerable code not present, vulnerable code not in execute path (Go only). Two require human judgment and are flagged as needs_human_review. The /close phase now supports two paths: fix-based closure (with PRs) and VEX-based closure (scan → close shortcut, no patch needed). The controller routing is updated to recommend /close directly when the scan verdict is absent or informational.
adalton
reviewed
May 11, 2026
Contributor
adalton
left a comment
adalton
left a comment
There was a problem hiding this comment.
Good feature set — pre-fix scanning, binary verification, PR dedup, and VEX justification are all the right additions. The skill markdown is well-structured and the inline fallback instructions are a nice portability pattern.
A few things to address before merging. Inline comments below are organized by severity: must-fix, should-fix, and suggestions.
Process note: The commit messages don't include Co-Authored-By trailers. If AI tooling was used to generate the Python scripts, our project convention expects that attribution.
Contributor
|
Should fix — code duplication between
A small Suggestion — If the patched lockfile was already committed as part of the fix, Suggestion — CONTRIBUTING.md exit code convention CONTRIBUTING.md documents |
When the scanner runs successfully and confirms the CVE is absent, but the project has a Dockerfile and the package is not in any manifest, the verdict was incorrectly set to in_base_image instead of absent. Guard the in_base_image check on scanner failure so that a successful clean scan always reaches the absent verdict.
When CVE ID format is invalid or the target directory does not exist, scan.py and verify.py now write a scan_failed JSON result and return exit code 1 instead of silently returning 0 with no output. This ensures the skill markdown always has structured JSON to parse.
…sion When the scanner exits with an unknown error code (e.g., compilation failure exit 2 from govulncheck), the situation is unknown — not "present by version". Now returns scan_failed for tool-level crashes instead of incorrectly claiming the CVE is likely present.
scan_go has a fallback that retries without GOTOOLCHAIN when the Go version download fails, but verify_go was missing it. Both the build step and the source-scan fallback now detect GOTOOLCHAIN failures and retry without the env var, matching scan_go's behavior.
The temp directory created by verify_go could leak if an exception or interrupt occurred between mkdtemp and rmtree. Wrap the build/scan block in try/finally to guarantee cleanup and move the shutil import to the top of the file.
…os.chdir Centralizes run(), is_successful_scan(), CVE_PATTERN, SCAN_TIMEOUT, write_json(), and timestamp() into _common.py. Both scan.py and verify.py now import from this module instead of duplicating code. Key changes: - Unified is_successful_scan() uses scan_tool consistently (verify.py previously used scan_method — different key name for the same concept) - Replaces os.chdir/restore pattern with subprocess cwd= parameter, eliminating fragile global state mutation - SCAN_TIMEOUT parsing now handles ValueError with fallback to 300 - verify.py npm lockfile side effect documented in docstring - Output truncation in scan.py now extracts CVE-relevant lines with context instead of blindly taking the first 30 lines - check_base_images uses Dockerfile* glob for rhel8/rhel9 naming
The package substring match against the entire PR diff produced false positives for short or common package names that appeared in code comments, docs, or context lines. Now only matches against +/- diff lines (excluding +++ and --- file headers).
Add a note in validate.md that npm install --package-lock-only modifies the working tree's package-lock.json during verification. Update CONTRIBUTING.md to acknowledge that search/query scripts (like check_existing_prs.py) may define their own exit code semantics in their docstrings, distinct from the report-script convention.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@cve-fix/scripts/check_existing_prs.py`:
- Around line 40-41: Several code paths that execute the GitHub CLI (`gh`)
currently swallow non-zero exits by returning None (seen at the return None
sites around the `gh` calls), which masks API/auth/rate-limit failures; change
those return-None branches to propagate a structured error: raise a specific
exception (e.g., GhQueryError) or return an error object containing the exit
code and stderr from the `gh` invocation in the functions that run the `gh`
command, and update `main()` to catch that exception/error and exit with code 2;
specifically replace the `return None` after `if code != 0:` in the functions
that invoke `gh` (the occurrences referenced around lines 40-41, 59-60, 90-91,
102-104) with raising/returning the structured error that includes the exit code
and message so `main()` can detect failures and return 2.
In `@cve-fix/scripts/scan.py`:
- Around line 355-358: The current join of work_dir = repo_dir / build_location
can escape the repository via paths like ../../; before using work_dir, resolve
it (e.g., resolved = (repo_dir / build_location).resolve()) and verify it is
contained within repo_dir.resolve() (e.g., check resolved == repo_dir.resolve()
or resolved.is_relative_to(repo_dir.resolve()) / startswith comparison), and if
the check fails call _write_error with the same signature (_write_error(...,
cve_id, package)) and return 1; keep the existing is_dir() check after this
containment validation.
In `@cve-fix/scripts/verify.py`:
- Around line 227-230: The code currently constructs work_dir = repo_dir /
build_location which allows path traversal via '..'; fix by resolving both
repo_dir and the candidate work_dir (use Path.resolve(strict=False)) and verify
the resolved work_dir is inside the resolved repo_dir (e.g., via
Path.is_relative_to or by comparing prefixes of their str() values / parents)
before proceeding; if the check fails call _write_error with a clear message and
return 1 (preserve the existing error-handling flow around work_dir, repo_dir,
build_location and _write_error).
- Around line 129-137: The current logic sets scan_tool to a non-canonical value
("npm_audit_stale_lockfile") when lockfile regen fails and that breaks
downstream success-code normalization which expects "npm_audit"; change the code
so scan_tool always remains "npm_audit" (regardless of regen_exit) and add a
separate field (e.g., "lockfile_regen_failed" boolean or
"lockfile_regen_status") to the returned dict to indicate regen failure; update
both places where regen_exit is checked (the block around regen_exit and the
similar block at lines ~174-176) to stop mutating scan_tool and instead set the
new status field while leaving scan_exit/scan_output behavior unchanged.
In `@cve-fix/skills/validate.md`:
- Around line 49-53: The markdown examples use mixed path
styles—`../scripts/verify.py`, `scripts/verify.py`, and `${TMPDIR}/verify/`—so
normalize them to relative paths: change all occurrences of `scripts/verify.py`
to `../scripts/verify.py` and replace `${TMPDIR}/verify/` (or any absolute/tmp
path) with the relative artifact path used elsewhere (e.g.,
`.artifacts/cve-fix/{context}` or `.artifacts/...`) so every command example
consistently uses relative paths and remains symlink-safe.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: d25a92dd-3df9-4906-a7e7-ac9e31412c37
📒 Files selected for processing (11)
CONTRIBUTING.mdcve-fix/README.mdcve-fix/guidelines.mdcve-fix/scripts/_common.pycve-fix/scripts/check_existing_prs.pycve-fix/scripts/scan.pycve-fix/scripts/verify.pycve-fix/skills/close.mdcve-fix/skills/controller.mdcve-fix/skills/scan.mdcve-fix/skills/validate.md
✅ Files skipped from review due to trivial changes (3)
- cve-fix/skills/controller.md
- cve-fix/skills/scan.md
- cve-fix/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- cve-fix/guidelines.md
Non-zero gh exit codes (auth errors, rate limits) were silently treated as "no match found" because the search functions returned None. Introduce GhQueryError and _gh_or_raise() so API failures propagate to main() and produce the documented exit code 2.
A build_location like ../../etc could scan outside the target repository. Add validate_work_dir() to _common.py that resolves the path and verifies containment within repo_dir. scan.py now rejects traversal attempts with an error-state JSON result.
Same containment check as scan.py: resolve the path and verify it stays within repo_dir before scanning, rejecting traversal attempts.
The non-canonical value npm_audit_stale_lockfile was not recognized by is_successful_scan(), causing valid npm audit runs (exit 1 with findings) to be misclassified as scan_failed. Keep scan_tool as npm_audit and report lockfile regeneration status in a separate lockfile_regen_failed field.
Fix mixed path references: use ../scripts/verify.py consistently
(relative to skills/) and replace ${TMPDIR}/verify/ with the
relative artifact path .artifacts/cve-fix/{context}/verify/ for
symlink-safe behavior.
Collaborator
Author
|
I ignored the ask to add "Co-Authored-By". |
adalton
approved these changes
May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-fix CVE scanning:
Add /scan phase between /start and /patch that runs ecosystem-specific vulnerability scanners to confirm a CVE is present before attempting fixes. Supports Go GOTOOLCHAIN matching to prevent false negatives.
Post-fix binary verification:
Enhance /validate with post-fix binary verification — for Go projects, builds the binary and runs govulncheck -mode binary to catch cases where replace directives or transitive overrides silently reintroduce the vulnerable version.
Existing PR deduplication:
Add existing-PR deduplication check to /pr — three-layer search (exact CVE, bot updates, same-package from other CVEs) prevents duplicate PRs.
Untrusted input guardrails:
Add untrusted input guardrails to guidelines and /start to prevent injection from Jira ticket content.
All deterministic operations are backed by Python scripts (stdlib-only) with inline fallback instructions in the skill files.