-
Notifications
You must be signed in to change notification settings - Fork 21
feat(vulnerability-remediation): support upgrading transitive dependencies without adding as top-level #118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -177,19 +177,85 @@ def create_remediation_prompt(vuln: Vulnerability, repo_name: str) -> str: | |
|
|
||
| 1. **Analyze** the vulnerability and understand what needs to be fixed | ||
|
|
||
| 2. **Find ALL dependency files across the repository** | ||
| **CRITICAL**: Search the repository for ALL dependency files that contain the vulnerable package. Do NOT assume dependencies only exist in the root directory. | ||
| 2. **Determine if the package is a DIRECT or TRANSITIVE dependency** | ||
| **CRITICAL**: Before modifying any files, determine whether `{vuln.package_name}` is a direct dependency (explicitly listed in manifest files) or a transitive dependency (pulled in by another package). | ||
|
|
||
| ```bash | ||
| # Find ALL dependency files containing the package (excludes dependency/build directories) | ||
| find . \( -name node_modules -o -name .venv -o -name venv -o -name vendor -o -name .git -o -name __pycache__ -o -name dist -o -name build \) -prune -o -name "pyproject.toml" -exec grep -l "{vuln.package_name}" {} + 2>/dev/null | ||
| find . \( -name node_modules -o -name .venv -o -name venv -o -name vendor -o -name .git \) -prune -o -name "requirements*.txt" -exec grep -l "{vuln.package_name}" {} + 2>/dev/null | ||
| find . \( -name node_modules -o -name .venv -o -name venv -o -name vendor -o -name .git \) -prune -o -name "package.json" -exec grep -l "{vuln.package_name}" {} + 2>/dev/null | ||
| # Check if the package is listed as a DIRECT dependency in manifest files | ||
| echo "=== Checking for direct dependency declarations ===" | ||
| find . \\( -name node_modules -o -name .venv -o -name venv -o -name vendor -o -name .git -o -name __pycache__ -o -name dist -o -name build \\) -prune -o -name "pyproject.toml" -print | xargs grep -l "{vuln.package_name}" 2>/dev/null | ||
| find . \\( -name node_modules -o -name .venv -o -name venv -o -name vendor -o -name .git \\) -prune -o -name "requirements*.txt" -print | xargs grep -l "{vuln.package_name}" 2>/dev/null | ||
| find . \\( -name node_modules -o -name .venv -o -name venv -o -name vendor -o -name .git \\) -prune -o -name "package.json" -print | xargs grep -l "{vuln.package_name}" 2>/dev/null | ||
| ``` | ||
|
|
||
| Update {vuln.package_name} from {vuln.installed_version} to {vuln.fixed_version} in **EVERY** file found. | ||
| **If {vuln.package_name} is found in manifest files**: It's a DIRECT dependency - proceed to update those files (Step 3). | ||
|
|
||
| 3. **CRITICAL: Sync/regenerate ALL lockfiles in the repository** | ||
| **If {vuln.package_name} is NOT found in any manifest files**: It's a TRANSITIVE dependency - try to upgrade it via lockfile tools FIRST (Step 2a) before falling back to adding as a direct dependency. | ||
|
|
||
| ### 2a. Upgrading TRANSITIVE Dependencies (if applicable) | ||
| If the package is a transitive dependency, try upgrading it without adding to manifest files. This is the preferred approach as it keeps dependencies minimal. | ||
|
|
||
| **General Principle**: Most modern package managers support upgrading specific transitive dependencies in the lockfile without adding them as top-level dependencies: | ||
|
|
||
| For EACH lockfile found, attempt the transitive upgrade: | ||
|
|
||
| - **uv (uv.lock)**: | ||
| ```bash | ||
| # Upgrade transitive dependency without modifying pyproject.toml | ||
| uv lock --upgrade-package {vuln.package_name} | ||
|
|
||
| # If the upgrade fails due to version constraints from another package, | ||
|
Comment on lines
+202
to
+207
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion - Hand-wavy Constraint Handling: You tell the agent to upgrade constraining packages if the upgrade fails due to version constraints: uv lock --upgrade-package {vuln.package_name} --upgrade-package <parent_package_that_constrains_it>But how does the agent identify Suggestion: Either provide concrete steps to identify the constraining package (e.g., "parse the uv lock error message for 'required by '") or remove this advanced case and stick with the fallback to direct dependency. |
||
| # you may need to upgrade that parent package too: | ||
| uv lock --upgrade-package {vuln.package_name} --upgrade-package <parent_package_that_constrains_it> | ||
| ``` | ||
|
|
||
| - **Poetry (poetry.lock)**: | ||
| ```bash | ||
| # Try updating just the specific package in the lockfile | ||
| poetry update {vuln.package_name} | ||
| ``` | ||
|
|
||
| - **npm (package-lock.json)**: | ||
| ```bash | ||
| # npm can update nested dependencies via overrides in package.json or: | ||
| npm update {vuln.package_name} | ||
| ``` | ||
|
|
||
| - **yarn (yarn.lock)**: | ||
| ```bash | ||
| # yarn supports resolutions in package.json to override transitive deps | ||
| yarn upgrade {vuln.package_name} | ||
| ``` | ||
|
|
||
| - **pnpm (pnpm-lock.yaml)**: | ||
| ```bash | ||
| # pnpm supports overrides in package.json | ||
| pnpm update {vuln.package_name} | ||
| ``` | ||
|
|
||
| - **Cargo (Cargo.lock)**: | ||
| ```bash | ||
| # Cargo can update transitive deps | ||
| cargo update -p {vuln.package_name} | ||
| ``` | ||
|
|
||
| - **Go (go.sum)**: | ||
| ```bash | ||
| # Go modules can update transitive deps | ||
| go get {vuln.package_name}@v{vuln.fixed_version} | ||
| go mod tidy | ||
|
Comment on lines
+195
to
+246
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Important - Prompt Complexity Explosion: You've hardcoded 7 package managers' worth of commands directly into the prompt. This is a maintenance nightmare. Every time a package manager updates its CLI, you'll need to modify this prompt. Every new package manager means another block of text. This prompt is becoming a reference manual. Better approach: Consider extracting package manager commands to a separate reference file or lookup table that the agent can consult. The prompt should describe what to do ("attempt transitive upgrade"), not how for every possible tool. |
||
| ``` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Important - Vague Verification: "Verify the upgrade succeeded by checking the lockfile for the new version" - but no concrete command is provided. How should the agent verify? Suggestion: Provide specific verification commands like: # For uv.lock
grep '{ name = "{vuln.package_name}", version = "{vuln.fixed_version}"' uv.lock
# For package-lock.json
jq -r '.packages[] | select(.name == "{vuln.package_name}") | .version' package-lock.json |
||
|
|
||
| **After attempting transitive upgrade**: Verify the upgrade succeeded by checking the lockfile for the new version. If successful, skip Step 3 and proceed to Step 4 (lockfile regeneration). | ||
|
|
||
| **If transitive upgrade fails** (e.g., version constraints prevent it): Fall back to adding as a direct dependency in Step 3. | ||
|
|
||
| 3. **Update DIRECT dependencies in manifest files** | ||
| **Only if the package is a direct dependency OR transitive upgrade failed:** | ||
|
|
||
| Update {vuln.package_name} from {vuln.installed_version} to {vuln.fixed_version} in **EVERY** manifest file where it appears. | ||
|
|
||
| 4. **CRITICAL: Sync/regenerate ALL lockfiles in the repository** | ||
| After updating the version in ALL manifest files, you MUST regenerate ALL corresponding lockfiles. Do NOT manually edit lockfiles. | ||
|
|
||
| **CRITICAL**: First, find ALL lockfiles (excluding dependency/build directories): | ||
|
|
@@ -219,7 +285,9 @@ def create_remediation_prompt(vuln: Vulnerability, repo_name: str) -> str: | |
| 3. If a version is found, install it: `pipx install uv==$UV_VERSION --force` | ||
| 4. Verify installation: `uv --version | grep "$UV_VERSION"` (proceed only if successful) | ||
| 5. If version extraction fails or returns empty, proceed with the currently installed version and note this in your output | ||
| 6. Run: `uv lock --upgrade-package {vuln.package_name}` or `uv sync` | ||
| 6. For transitive deps: `uv lock --upgrade-package {vuln.package_name}` | ||
| If constraints prevent upgrade: `uv lock --upgrade-package {vuln.package_name} --upgrade-package <constraining_package>` | ||
| For direct deps after manifest update: `uv sync` | ||
|
|
||
| - **npm (package.json + package-lock.json)**: `cd` to directory, run `npm install` or `npm update {vuln.package_name}` | ||
| - **yarn (package.json + yarn.lock)**: `cd` to directory, run `yarn install` or `yarn upgrade {vuln.package_name}` | ||
|
|
@@ -230,11 +298,11 @@ def create_remediation_prompt(vuln: Vulnerability, repo_name: str) -> str: | |
| - **Maven (pom.xml)**: Update the version directly in pom.xml | ||
|
Comment on lines
177
to
298
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion - No Testing: This is a significant behavior change to the remediation workflow (adding transitive vs direct dependency detection, new fallback logic) with no corresponding tests. How do you know:
Recommendation: Add integration tests that verify the prompt generates correct remediation steps for both direct and transitive dependency scenarios across major package managers. |
||
| - **Gradle**: Update the version in build.gradle/build.gradle.kts | ||
|
|
||
| 4. **Verify** the change doesn't break the build (run any available build/test commands) | ||
| 5. **Create a branch** named `fix/{vuln.vuln_id.lower()}` | ||
| 6. **Commit** your changes with a clear message explaining the security fix | ||
| 7. **Push** the branch to origin | ||
| 8. **Create a Pull Request** using the GitHub CLI: | ||
| 5. **Verify** the change doesn't break the build (run any available build/test commands) | ||
| 6. **Create a branch** named `fix/{vuln.vuln_id.lower()}` | ||
| 7. **Commit** your changes with a clear message explaining the security fix | ||
| 8. **Push** the branch to origin | ||
| 9. **Create a Pull Request** using the GitHub CLI: | ||
| ```bash | ||
| gh pr create --title "fix: {vuln.vuln_id} - Update {vuln.package_name} to {vuln.fixed_version}" \\ | ||
| --body "## Security Fix | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 Acceptable - Minor Robustness Regression: You changed from
-exec grep -l "{vuln.package_name}" {} +to-print | xargs grep -l.The
-execversion is more robust (handles filenames with spaces/special characters better without needingxargs), but thexargsversion works fine for typical project structures.This is a pragmatic trade-off—the
xargspattern is more common and readable for most developers. Just be aware it can fail on edge cases with weird filenames.