Add govulncheck CVE detection to PR checks#158
Conversation
📝 WalkthroughWalkthroughAdds a PR-gated GitHub Actions workflow ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/govulncheck.yaml:
- Around line 15-18: The govulncheck job currently lacks an explicit job-level
permissions block; add a minimal permissions map directly under the govulncheck
job (identified by the job name govulncheck) that grants only the
least-privilege scopes required for vulnerability scanning — e.g., set contents:
read (to fetch repo code), packages: read (if packages access needed), and
security-events: write (if the job reports findings), and avoid granting broad
write or admin scopes; place this permissions stanza beneath the govulncheck job
declaration.
- Around line 28-33: Replace the unstable go install invocation that uses
`@latest` with a pinned release version for the govulncheck tool in the CI
workflow: change the install command "go install
golang.org/x/vuln/cmd/govulncheck@latest" to use a specific released tag (e.g.,
`@v0.4.0` or the current vetted release) so the "Install govulncheck" step
consistently installs a known-good binary and avoids upstream drift; keep the
rest of the step (the "go version" echo and "govulncheck -version" call) intact
to verify the pinned tool.
- Around line 20-24: The workflow uses mutable tags actions/checkout@v4 and
actions/setup-go@v5; replace those with their corresponding immutable commit
SHAs (e.g., actions/checkout@<commit-sha> and actions/setup-go@<commit-sha>) so
the CI references exact commits. Locate the two uses: "uses:
actions/checkout@v4" and "uses: actions/setup-go@v5", look up the desired stable
commit SHAs from the official repos, update the uses lines to the full sha
values, and run the workflow to verify nothing else needs adjustment.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e234878d-895b-4fa7-9513-99f97a78af16
📒 Files selected for processing (1)
.github/workflows/govulncheck.yaml
| govulncheck: | ||
| name: govulncheck | ||
| runs-on: ubuntu-latest | ||
|
|
There was a problem hiding this comment.
Declare explicit job-level permissions with least privilege (CWE-732).
No permissions block is set for jobs.govulncheck; token scope falls back to repository defaults. Lock it down at the job level.
Suggested patch
jobs:
govulncheck:
name: govulncheck
runs-on: ubuntu-latest
+ permissions:
+ contents: readAs per coding guidelines, Set least-privilege permissions per job, not workflow level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/govulncheck.yaml around lines 15 - 18, The govulncheck job
currently lacks an explicit job-level permissions block; add a minimal
permissions map directly under the govulncheck job (identified by the job name
govulncheck) that grants only the least-privilege scopes required for
vulnerability scanning — e.g., set contents: read (to fetch repo code),
packages: read (if packages access needed), and security-events: write (if the
job reports findings), and avoid granting broad write or admin scopes; place
this permissions stanza beneath the govulncheck job declaration.
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Go | ||
| uses: actions/setup-go@v5 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the govulncheck.yaml file
find . -name "govulncheck.yaml" -o -name "govulncheck.yml" | head -20Repository: opendatahub-io/trainer
Length of output: 103
🏁 Script executed:
# List GitHub workflow files
ls -la .github/workflows/ 2>/dev/null || echo "No workflows directory found"Repository: opendatahub-io/trainer
Length of output: 1071
🏁 Script executed:
# Search for the specific file
git ls-files | grep -i govulncheckRepository: opendatahub-io/trainer
Length of output: 101
🏁 Script executed:
cat -n .github/workflows/govulncheck.yamlRepository: opendatahub-io/trainer
Length of output: 3487
🏁 Script executed:
# Verify if there are any other GitHub actions in the file using tags
rg 'uses: .*@[v0-9]' .github/workflows/govulncheck.yamlRepository: opendatahub-io/trainer
Length of output: 134
🏁 Script executed:
# Check if there's a permissions block anywhere in the file
rg 'permissions:' .github/workflows/govulncheck.yamlRepository: opendatahub-io/trainer
Length of output: 48
Pin third-party actions to immutable commit SHAs to prevent supply chain attacks.
actions/checkout@v4 and actions/setup-go@v5 use mutable version tags instead of commit SHAs. Replace with full commit hashes.
Fix
- name: Checkout code
- uses: actions/checkout@v4
+ uses: actions/checkout@e2f20987662482ea2eaa29644ccb66a9b6048355
- name: Setup Go
- uses: actions/setup-go@v5
+ uses: actions/setup-go@0a12ed9d6470cabf33ff6a8a57a28fa131d498419📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| - name: Setup Go | |
| uses: actions/setup-go@v5 | |
| - name: Checkout code | |
| uses: actions/checkout@e2f20987662482ea2eaa29644ccb66a9b6048355 | |
| - name: Setup Go | |
| uses: actions/setup-go@0a12ed9d6470cabf33ff6a8a57a28fa131d498419 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/govulncheck.yaml around lines 20 - 24, The workflow uses
mutable tags actions/checkout@v4 and actions/setup-go@v5; replace those with
their corresponding immutable commit SHAs (e.g., actions/checkout@<commit-sha>
and actions/setup-go@<commit-sha>) so the CI references exact commits. Locate
the two uses: "uses: actions/checkout@v4" and "uses: actions/setup-go@v5", look
up the desired stable commit SHAs from the official repos, update the uses lines
to the full sha values, and run the workflow to verify nothing else needs
adjustment.
| - name: Install govulncheck | ||
| run: | | ||
| go install golang.org/x/vuln/cmd/govulncheck@latest | ||
| echo "Go version: $(go version)" | ||
| govulncheck -version | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "./.github/workflows/govulncheck.yaml" -type fRepository: opendatahub-io/trainer
Length of output: 103
🏁 Script executed:
cat -n .github/workflows/govulncheck.yamlRepository: opendatahub-io/trainer
Length of output: 3487
🏁 Script executed:
# Search for other `@latest` instances in workflows
rg "@latest" .github/workflows/Repository: opendatahub-io/trainer
Length of output: 163
Pin govulncheck to a released version (CWE-494, supply chain security).
@latest makes builds non-reproducible and exposes the CI to upstream drift in the security tool itself. Pin to a tested release version.
Suggested patch
- name: Install govulncheck
run: |
- go install golang.org/x/vuln/cmd/govulncheck@latest
+ GOVULNCHECK_VERSION=vX.Y.Z
+ go install golang.org/x/vuln/cmd/govulncheck@${GOVULNCHECK_VERSION}
echo "Go version: $(go version)"
govulncheck -version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/govulncheck.yaml around lines 28 - 33, Replace the
unstable go install invocation that uses `@latest` with a pinned release version
for the govulncheck tool in the CI workflow: change the install command "go
install golang.org/x/vuln/cmd/govulncheck@latest" to use a specific released tag
(e.g., `@v0.4.0` or the current vetted release) so the "Install govulncheck" step
consistently installs a known-good binary and avoids upstream drift; keep the
rest of the step (the "go version" echo and "govulncheck -version" call) intact
to verify the pinned tool.
|
/lgtm |
e4bd188 to
23daf10
Compare
|
/hold @ChughShilpa @sutaakar can this go upstream? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/govulncheck.yaml (1)
5-11: Path-filteredpull_requestonly — scanner won’t catch CVEs disclosed against unchanged deps.A new CVE published against a version already pinned in
go.sumwill not surface until somebody happens to touch one of the listed paths. For a security-gate workflow you want a daily/weekly schedule (and optionalworkflow_dispatch) targetingmain, plus aconcurrencygroup to dedupe per-PR runs.on: pull_request: paths: - 'cmd/trainer-controller-manager/**' - 'pkg/**' - 'go.mod' - 'go.sum' + schedule: + - cron: '0 6 * * *' + workflow_dispatch: + +concurrency: + group: govulncheck-${{ github.ref }} + cancel-in-progress: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/govulncheck.yaml around lines 5 - 11, Current workflow only triggers govulncheck on pull_request filtered by on.pull_request.paths, which misses CVEs against unchanged dependencies; update the workflow (govulncheck.yaml) to add a scheduled trigger (e.g. cron for daily or weekly) targeting the main branch and also add on.workflow_dispatch for manual runs, and introduce a concurrency: group to dedupe runs (use a stable group name like "govulncheck-${{ github.ref }}") so scheduled and PR runs don't overlap; keep the existing pull_request trigger but ensure the schedule covers dependency-only CVE detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/govulncheck.yaml:
- Around line 33-35: Update the "Run govulncheck" workflow step to enable strict
shell failure handling by adding "set -euo pipefail" at the start of the run
script, so any non-zero exit from govulncheck fails the job; run govulncheck to
write to govulncheck-output.json as before, then assert the output file is
non-empty and valid JSON (e.g., check file size and/or run a JSON parse check
like using jq to read/validate govulncheck-output.json) before any jq-based
parsing so invalid or empty output cannot be interpreted as zero
vulnerabilities.
- Around line 50-52: Update the templated fields to provide a "// \"N/A\""
fallback for missing values so the table doesn't show nulls: replace occurrences
of $f.trace[0].module, $f.trace[0].version and $f.fixed_version with their
fallback forms (e.g. $f.trace[0].module // "N/A", $f.trace[0].version // "N/A",
$f.fixed_version // "N/A") mirroring the existing summary fallback; keep the
rest of the structure unchanged so stdlib findings and OSV-only records display
"N/A" instead of null.
- Around line 66-70: The table row construction inserts raw OSV summaries via
the interpolation \(.summary[:80]) in the jq map, which breaks markdown when
summaries contain pipes, backticks, or newlines; update the jq expression inside
the map (the map(... ) | join("\n")) to sanitize the summary before
interpolation, e.g. replace/escape "|" (gsub("\\|"; "\\|") or substitute with
"│"), convert newlines to spaces (gsub("\n"; " ")), and
replace/backtick-normalize "`" (gsub("`"; "'")) so the generated string feeding
>> "$GITHUB_STEP_SUMMARY" is safe; locate and change the \(.summary[:80]) usage
in the string concatenation inside the jq -s "$JQ_PARSE" pipeline to use the
sanitized expression (e.g., \((.summary[:80] | gsub("\\|"; "\\|") | gsub("\n"; "
") | gsub("`"; "'")))).
---
Nitpick comments:
In @.github/workflows/govulncheck.yaml:
- Around line 5-11: Current workflow only triggers govulncheck on pull_request
filtered by on.pull_request.paths, which misses CVEs against unchanged
dependencies; update the workflow (govulncheck.yaml) to add a scheduled trigger
(e.g. cron for daily or weekly) targeting the main branch and also add
on.workflow_dispatch for manual runs, and introduce a concurrency: group to
dedupe runs (use a stable group name like "govulncheck-${{ github.ref }}") so
scheduled and PR runs don't overlap; keep the existing pull_request trigger but
ensure the schedule covers dependency-only CVE detection.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 47e58d21-e8cd-4070-9312-43bfd6ff9ead
📒 Files selected for processing (1)
.github/workflows/govulncheck.yaml
| - name: Run govulncheck | ||
| run: | | ||
| govulncheck -format json ./... > govulncheck-output.json |
There was a problem hiding this comment.
Add set -euo pipefail — silent failure mode hides scanner errors.
Without it, a non-zero from govulncheck (network blip, build error in ./..., OOM) leaves a possibly-empty govulncheck-output.json; the next jq block then computes VULN_COUNT=0 and posts a green “No vulnerabilities found” summary. That is the worst possible failure mode for a security gate.
- name: Run govulncheck
run: |
+ set -euo pipefail
govulncheck -format json ./... > govulncheck-output.jsonAlso worth asserting the file is non-empty / valid JSON before parsing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run govulncheck | |
| run: | | |
| govulncheck -format json ./... > govulncheck-output.json | |
| - name: Run govulncheck | |
| run: | | |
| set -euo pipefail | |
| govulncheck -format json ./... > govulncheck-output.json |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/govulncheck.yaml around lines 33 - 35, Update the "Run
govulncheck" workflow step to enable strict shell failure handling by adding
"set -euo pipefail" at the start of the run script, so any non-zero exit from
govulncheck fails the job; run govulncheck to write to govulncheck-output.json
as before, then assert the output file is non-empty and valid JSON (e.g., check
file size and/or run a JSON parse check like using jq to read/validate
govulncheck-output.json) before any jq-based parsing so invalid or empty output
cannot be interpreted as zero vulnerabilities.
| module: $f.trace[0].module, | ||
| version: $f.trace[0].version, | ||
| fixed: $f.fixed_version |
There was a problem hiding this comment.
Add // "N/A" fallbacks for module/version/fixed.
$f.trace[0].module, .version, and $f.fixed_version can be absent for stdlib findings or pre-trace OSV-only records, which renders null in the table. Mirror the summary fallback for consistency.
- module: $f.trace[0].module,
- version: $f.trace[0].version,
- fixed: $f.fixed_version
+ module: ($f.trace[0].module // "N/A"),
+ version: ($f.trace[0].version // "N/A"),
+ fixed: ($f.fixed_version // "N/A")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/govulncheck.yaml around lines 50 - 52, Update the
templated fields to provide a "// \"N/A\"" fallback for missing values so the
table doesn't show nulls: replace occurrences of $f.trace[0].module,
$f.trace[0].version and $f.fixed_version with their fallback forms (e.g.
$f.trace[0].module // "N/A", $f.trace[0].version // "N/A", $f.fixed_version //
"N/A") mirroring the existing summary fallback; keep the rest of the structure
unchanged so stdlib findings and OSV-only records display "N/A" instead of null.
| jq -s "$JQ_PARSE"' | | ||
| "## **Summary report of govulncheck:**\n\n| CVE | Vuln ID | Description | Module | Current | Fixed |\n|-----|---------|-------------|--------|---------|-------|\n" + | ||
| (map("| \(.cve) | [\(.osv_id)](https://pkg.go.dev/vuln/\(.osv_id)) | \(.summary[:80]) | `\(.module)` | \(.version) | \(.fixed) |") | join("\n")) + | ||
| "\n\n**Total: \(length) vulnerabilities found**" | ||
| ' -r govulncheck-output.json >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
Markdown table will fracture on pipe/newline characters in OSV summary (CWE-116).
\(.summary[:80]) is dropped raw into a |-delimited table cell. OSV/CVE descriptions routinely contain | (e.g., "A | B"), backticks, or embedded newlines — any one of those splits the row, mangles the Summary page, and can mask real findings. Sanitize before interpolation.
Suggested fix
- (map("| \(.cve) | [\(.osv_id)](https://pkg.go.dev/vuln/\(.osv_id)) | \(.summary[:80]) | `\(.module)` | \(.version) | \(.fixed) |") | join("\n")) +
+ (map("| \(.cve) | [\(.osv_id)](https://pkg.go.dev/vuln/\(.osv_id)) | \(.summary[:160] | gsub("[\r\n]+";" ") | gsub("\\|";"\\|")) | `\(.module)` | \(.version // "N/A") | \(.fixed // "N/A") |") | join("\n")) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/govulncheck.yaml around lines 66 - 70, The table row
construction inserts raw OSV summaries via the interpolation \(.summary[:80]) in
the jq map, which breaks markdown when summaries contain pipes, backticks, or
newlines; update the jq expression inside the map (the map(... ) | join("\n"))
to sanitize the summary before interpolation, e.g. replace/escape "|"
(gsub("\\|"; "\\|") or substitute with "│"), convert newlines to spaces
(gsub("\n"; " ")), and replace/backtick-normalize "`" (gsub("`"; "'")) so the
generated string feeding >> "$GITHUB_STEP_SUMMARY" is safe; locate and change
the \(.summary[:80]) usage in the string concatenation inside the jq -s
"$JQ_PARSE" pipeline to use the sanitized expression (e.g., \((.summary[:80] |
gsub("\\|"; "\\|") | gsub("\n"; " ") | gsub("`"; "'")))).
I haven't raised the issue in upstream yet |
|
I think it can |
Well this can go upstream as there is no mistream/ODH specific in this workflow Can we directly open a PR upstream? @robert-bell |
|
@ChughShilpa: The following test has Succeeded: OCI Artifact Browser URLInspecting Test Artifacts ManuallyTo inspect your test artifacts manually, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:odh-pr-test-distributed-workloads-mw8d6 |
|
Changing it to Draft PR as /hold label doesn't work in this repo |
5 participants
What this PR does / why we need it:
with CVE details in both workflow logs and the check run summary page
cmd/trainer-controller-manager/,pkg/,api/,go.mod,go.sum-format jsonoutput parsed with jq to produce:Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes # RHOAIENG-45704
How this PR is tested ?
Validated the worflow on my fork - https://github.com/ChughShilpa/trainer/actions/runs/24896109686
Checklist:
Summary by CodeRabbit