[main] SRVKP-12265: Add CVE remediation skill to console-plugin#1138
Open
openshift-cherrypick-robot wants to merge 1 commit into
Open
Conversation
Signed-off-by: Ankur Sinha <anksinha@redhat.com>
Collaborator
|
@openshift-cherrypick-robot: Ignoring requests to cherry-pick non-bug issues: SRVKP-12265 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
requested review from
anwesha-palit-redhat and
arvindk-softwaredev
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: openshift-cherrypick-robot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Code Review by Qodo
1. Missing semver dependency
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+3
to
+7
| import { execFileSync } from 'child_process'; | ||
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
| import semver from 'semver'; | ||
|
|
There was a problem hiding this comment.
1. Missing semver dependency 🐞 Bug ≡ Correctness
scripts/fix-cves/analyze-deps.ts imports semver, but package.json doesn’t declare it, so the script relies on transitive hoisting and can fail with a module-not-found error when semver isn’t present at the top level.
Agent Prompt
## Issue description
`scripts/fix-cves/analyze-deps.ts` imports `semver` but the repository doesn’t declare it in `package.json`. This makes the script non-hermetic: it can break when dependency/hoisting changes across branches.
## Issue Context
This script is intended to be used as a remediation tool across multiple release branches; it should have stable, explicit dependencies.
## Fix Focus Areas
- package.json[1-192]
- scripts/fix-cves/analyze-deps.ts[3-7]
## Suggested fix
- Add `semver` to `devDependencies` (or `dependencies`) with an appropriate pinned range.
- Run `yarn install` to update `yarn.lock` accordingly and commit both changes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+54
to
+63
| function runCmd(cmd: string, args: string[]): string { | ||
| try { | ||
| return execFileSync(cmd, args, { | ||
| encoding: 'utf-8', | ||
| maxBuffer: 10 * 1024 * 1024, | ||
| }); | ||
| } catch (e: any) { | ||
| return e.stdout ?? ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Npm ls fallback misreports 🐞 Bug ≡ Correctness
If npm ls --json output can’t be parsed (including when execFileSync hits the 10MB maxBuffer), getAllInstalledVersions() returns an empty list and main falls back to only the hoisted node_modules/<pkg> version, which can ignore nested vulnerable copies while still allowing an already-remediated result.
Agent Prompt
## Issue description
The analysis relies on `npm ls --json` to enumerate *all* installed versions, but command failures/parsing failures are silently treated as “no versions found”, and the code falls back to the hoisted version only. This can produce incorrect remediation decisions.
## Issue Context
- `runCmd()` swallows command failures by returning `stdout` only.
- `getAllInstalledVersions()` swallows JSON parse errors.
- `main()` treats an empty list as a reason to trust `currentVersion`.
## Fix Focus Areas
- scripts/fix-cves/analyze-deps.ts[54-63]
- scripts/fix-cves/analyze-deps.ts[141-149]
- scripts/fix-cves/analyze-deps.ts[269-277]
## Suggested fix
- Make `runCmd` return `{ stdout, stderr, exitCode }` (or throw) so callers can distinguish success vs failure.
- In `getAllInstalledVersions`, if `npm ls` fails or JSON parsing fails, propagate an explicit error state.
- In `main`, if the full-tree enumeration failed, do **not** mark `already-remediated`; instead emit a strategy/reason indicating verification couldn’t be completed.
- Consider increasing `maxBuffer` substantially (or streaming output) to avoid truncation on large trees.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+193
to
+211
| function getDirectParents(pkg: string): string[] { | ||
| const pjPath = path.join(process.cwd(), 'package.json'); | ||
| if (!fs.existsSync(pjPath)) return []; | ||
| const pj = JSON.parse(fs.readFileSync(pjPath, 'utf-8')); | ||
| const allDirect = new Set([ | ||
| ...Object.keys(pj.dependencies ?? {}), | ||
| ...Object.keys(pj.devDependencies ?? {}), | ||
| ]); | ||
|
|
||
| const output = runCmd('yarn', ['why', pkg]); | ||
| const parents = new Set<string>(); | ||
| for (const line of output.split('\n')) { | ||
| for (const dep of allDirect) { | ||
| if (line.includes(dep)) parents.add(dep); | ||
| } | ||
| } | ||
| parents.delete(pkg); | ||
| return [...parents]; | ||
| } |
There was a problem hiding this comment.
3. Substring parent matching 🐞 Bug ≡ Correctness
getDirectParents() uses substring checks against raw yarn why output (line.includes(dep)), which can misidentify direct parents (e.g., react matching react-dom) and produce incorrect parentUpgradeSuggestions.
Agent Prompt
## Issue description
Direct parent detection is based on naive substring matches in unstructured `yarn why` output, which can return false positives and lead to incorrect recommended upgrades.
## Issue Context
This repo has direct deps with overlapping names (e.g. `react` and `react-dom`) making substring matching demonstrably ambiguous.
## Fix Focus Areas
- scripts/fix-cves/analyze-deps.ts[189-211]
- package.json[108-114]
## Suggested fix
- Switch to a structured source of truth:
- Prefer `yarn why --json <pkg>` (Yarn emits JSON lines) and parse exact package identifiers, or
- Use `npm ls <pkg> --all --json` to compute parent chains and then map to top-level deps precisely.
- If keeping text parsing, enforce exact package-name token matching (word boundaries / `@scope/name`-aware) rather than `includes()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+81
to
+90
| #### C2. Check base images | ||
|
|
||
| Query the Pyxis API to inspect the latest `ubi9/nodejs-24` and `ubi9/nginx-124` images. | ||
|
|
||
| **Step 1 — Get the latest image ID for each repo:** | ||
|
|
||
| ``` | ||
| https://catalog.redhat.com/api/containers/v1/images?filter=repositories.repository%3D%3Dubi9/nodejs-24%3Barchitecture%3D%3Damd64&sort_by=creation_date%5Bdesc%5D&page_size=1 | ||
| ``` | ||
|
|
There was a problem hiding this comment.
4. Wrong fedramp base image 🐞 Bug ≡ Correctness
SKILL.md instructs FedRAMP OS-CVE triage against ubi9/nodejs-24, but this repo’s build Dockerfile uses ubi9/nodejs-20, so the workflow can report incorrect “fixed/not fixed” status for the actual shipped image.
Agent Prompt
## Issue description
FedRAMP image triage guidance hardcodes the NodeJS base image repo/tag and does not match the repo’s actual container build arguments.
## Issue Context
The Dockerfile defines the builder image as `ubi9/nodejs-20`.
## Fix Focus Areas
- .cursor/skills/fix-cves/SKILL.md[81-90]
- Dockerfile[1-2]
## Suggested fix
- Update SKILL.md to query the correct base images used by this repo (currently `ubi9/nodejs-20` and `ubi9/nginx-124`).
- Optionally add a step instructing the user/bot to read the Dockerfile `ARG BUILDER`/`ARG RUNTIME` values first and then query Pyxis for those exact repos.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
qodo-code-review
Bot
added
documentation
Contributor
|
/lgtm |
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
5 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.
This is an automated cherry-pick of #1136
/assign anwesha-palit-redhat