feat(cve-scan): add reusable action to scan container images for cves

[vcauesantos]

Summary

• New composite action cve-scan in loft-sh/github-actions: scans a container image for CVEs via a swappable scanner adapter (Snyk ships first), never hardcoding the vendor in its public contract
• Three failure modes handled distinctly: a scanner error never fails the job (Notion "Building a Release Pipeline" §9.2); findings only fail it when block-on-findings: true (advisory by default); a config or setup error always fails it, so a permanently broken scanner can't report "inconclusive" forever while scanning nothing
• Ignore-file support for CVEs with no fix yet, with a mandatory owner and expiry date (no permanent silent suppression, and a named person to ask when one lapses)
• Reports via a markdown report, SARIF (the caller uploads it, kept out of this action's own permission footprint), a Job Summary entry on every outcome, and Slack

References DEVOPS-1292 — this is Phase 2 (the action itself) of that ticket's phased plan. Phase 3 (wiring both triggers — a scheduled head sweep and a prerelease-filtered release scan — into vcluster, vcluster-pro, and loft-enterprise) is a follow-up, not included here.

Test plan

• make test-cve-scan — 111 bats tests across run.sh, src/scanners/snyk.sh, src/process-findings.sh, and a new integration.bats that wires run.sh to the real process-findings.sh
• A composite-smoke job drives the action through uses: with enabled: false and asserts its outputs, so action.yml's input→env wiring is exercised in CI
• shellcheck, actionlint and zizmor clean; make lint and make check-docs clean
• Key fixes verified by reproducing the defect first, then re-running: the swallowed expired-ignore warning, the unchecked jq partition, the enabled kill switch, scanner-name path traversal, and GITHUB_OUTPUT forgery via a newline in image-ref
• Coverage verified by mutation: ranking critical below low, inverting the SARIF level map, dropping SARIF locations, dropping the owner requirement, gating the config-error exit on block-on-findings, and dropping the has-vulnerabilities half of the gate are each now caught

Review response

Second commit addresses the panel review. All 24 inline comments were verified against the code before acting; none were spurious. Notable deltas from what was suggested:

• The set +e nit and the unchecked jq partition were the same bug. All three scripts now use set -euo pipefail (the repo norm — 34 of 39 action scripts) with genuine set +e/set -e brackets, which kills the class rather than the instance. govulncheck/run.sh has the inverse latent trap: it starts without errexit and a bracket's set -e switches it on mid-script.
• The suggested test for the swallowed warning could not pass. It has the mock echo to stdout, which run.sh swallows regardless of the fix — the fix changes the real child, not the mock. Closed instead with integration.bats, verified to fail before the >&2 fix and pass after, while the old unit test reports ok in both cases.
• enabled now fails safe rather than just normalising case. Only an explicit falsey value skips; flase scans and warns. A kill switch on a security control shouldn't be trippable by a typo in a web-UI field.
• Adapter exit codes gained a setup-error value (2). A missing CLI or credential is a provisioning mistake, not an inconclusive scan, and previously had no way to say so — every setup gap folded into the silent scanner-error path. The adapter also provisions its own tool, keeping action.yml scanner-agnostic.
• Two findings were undersold and are fixed as real: the SARIF results carried no locations[], and GitHub requires at least one to display a result — so the documented upload example would have shown nothing; and a newline in image-ref demonstrably forged a second has-vulnerabilities=false after the genuine one.
• Four things the review missed: summary was documented as empty on a clean scan (it never is); the missing SARIF locations; an empty scanner-token going silently green, which is the live state today since the secret isn't provisioned yet; and — found while verifying the arch-less tag question — release tags carry a v prefix that registry tags don't, so image-ref: ...:${{ github.event.release.tag_name }} resolves to a tag that doesn't exist. Both READMEs now derive the tag in a step.
• Deferred deliberately: the severity-vocabulary enum stays out of the adapter contract until a second adapter lands, per the reviewer, with a test documenting today's behaviour. severity-threshold validation stays in process-findings.sh rather than being duplicated in run.sh; only the zero-drift ignore-file existence check moved up front.

ghcr.io/loft-sh/vcluster-pro:head was confirmed against ghcr.io as a live multi-arch manifest list (linux/amd64, linux/arm64). The versioned target list per repo is recorded on DEVOPS-1292 and remains Phase 3 work.

[loft-bot]

Panel review: a nine-lane pass over the new cve-scan action. This is a non-blocking COMMENT review — nothing here gates the merge button.

Solid, well-documented action with a genuinely swappable adapter seam and a real bats suite. The findings below cluster around one theme: because a scanner error is designed never to fail the job, every prerequisite gap and internal error degrades into a permanently green, permanently non-scanning job. Several of these I reproduced by running the scripts rather than reading them; where that's the case the comment says so.

Blocking concerns

Highest-impact first. Each is a real defect, not a style preference.

1. src/process-findings.sh:99 — the expired-ignore ::warning:: never reaches the job log. Verified end to end: run the pair with an expired ignore entry and the job log is completely empty while high-count=1 shows the suppressed CVE has returned. run.sh captures the child's whole stdout as a key=value channel and re-emits only grepped keys. The one signal telling a human a CVE came back is destroyed. One-word fix (>&2).
2. src/process-findings.sh:121 — the jq partition's exit status is unchecked, so the gate silently no-ops. Verified: with a findings JSON the filter can't iterate, the script still exits 0, every count is written blank, scanner-error=false, and block-on-findings=true does not fire. A data error is indistinguishable from a clean scan.
3. action.yml:87-94 — the snyk CLI is never installed or version-pinned. yq gets a conditional install step; the scanner the action exists to run gets none, and isn't on ubuntu-latest. Exit 127 folds into the silent scanner-error path, so the action reports "scanner error" forever and never scans.
4. src/scanners/snyk.sh:48 — no image pull or registry auth. The design ticket for this work explicitly records that Snyk's own GHCR credential has broken repeatedly and that a CI scan should pull the image itself; the manual runbook does exactly that. As written, private-image scanning depends on the very credential this was meant to route around, and fails silently.
5. run.sh:69 — the enabled kill switch silently skips on any non-exact value. Verified: TRUE, True, yes, 1, and " true" all skip the scan; only lowercase true runs it. The examples wire this to a repo variable — a free-text web-UI field. Result is a green check and no annotation. (An empty value is safe — ${ENABLED:-true} re-defaults it.)
6. test/run.bats:57-67 — the mock only ever emits clean key=value lines, so nothing exercises the parent's handling of other child stdout. This is why #1 ships green.
7. test/process_findings.bats:149-165 — the severity-rank table is only ever exercised at high; ranking critical below low passes all 23 tests.
8. test/run.bats:141-153 — the gate matrix never varies block-on-findings where it matters; two mutations that break release gating pass the suite.

What was checked

• Correctness: boundary/empty inputs, ignore-file expiry and id/cve matching, the gate decision, temp-file and trap lifecycle. Expiry logic, zero-padding validation, and id/cve matching are correct; the unchecked jq exit above is the real defect.
• Security: injection, secrets in logs, authz. Clean on the important ones — untrusted scanner data reaches jq only via --arg/--argjson, never interpolated into program text, and the CLI invocation uses a proper argv array. Three defense-in-depth notes inline. Permission footprint is genuinely minimal and the SARIF-upload split is the right call.
• Test quality: exercised by mutation testing — production code was deliberately broken and the suite re-run to prove what it catches. Four gate-coverage gaps are blocking above.
• Operability: the disabled / scanner-error / config-error / completed taxonomy versus what an operator can actually observe. This is where the action is weakest; see blocking #1, #3, #4, #5 plus inline notes on step-summary and timeouts.
• Architecture: the adapter seam is real and the canonical shape is documented in one place. One leak remains (severity vocabulary) — inline.
• Reuse: compared against govulncheck and ~30 siblings. The inline output/table/truncation helpers are per-action convention here, not missed reuse; no shared helper exists to call. Two genuine items inline.
• Infra/CI: third-party actions correctly SHA-pinned; renovate annotations verified against the actual renovate.json regex; the notify step's compound if:/ternary verified correct across all reachable output states; empty webhook-url already handled safely by ci-test-notify.
• Typos/prose: doc-comment count mismatch and the set +e intent inline.
• PR metadata: below.
• Skipped: dead-code-guard (nothing deleted, moved, or renamed — 9 added files plus two append-only edits). The four e2e lanes and gap-analysis (no e2e*/ suite in this repo and no Go unit test, so neither signal is present).

Not re-reported: anything actionlint, zizmor, shellcheck, or check-docs already covers — all five checks are green. The ci-test-notify SHA pin is correct per this repo's convention and is deliberately not flagged.

What the change does well

The scanner seam is honest rather than nominal: the canonical findings shape is documented as an explicit adapter contract, and the Snyk-specific two-array/dedup trap is handled inside the adapter where it belongs instead of leaking into the gating logic.

PR-level notes

• nit — The body says "41 bats tests"; the PR now has 53 (23 + 15 + 15). 41 was the count at the first commit; the second added 12 more. The claim undersells the suite rather than overstating it. Everything else checks out: all five checks green, and References DEVOPS-1292 is the right form since this is Phase 2 and does not close the ticket.

Quality notes (non-blocking)

Low-severity test-assertion gaps, recorded for tracking rather than as inline comments:

• test/run.bats:167 — only critical-count is asserted from the real grep/cut output parsing; hardcoding high/medium/low-count to 0 still passes the suite.
• test/process_findings.bats:291-297 — the Slack summary test checks only truncation, never the embedded per-severity counts; swapping which count fills the critical= field still passes.

[loft-bot]

Panel review: second pass over cve-scan, at 0d0419b. Nine lanes ran. This is a non-blocking COMMENT review — nothing here gates the merge button.

First, the round-one response holds up. I checked all 24 resolved threads against the code rather than the replies, and every one is genuinely fixed; several are fixed better than what was suggested. The two places you pushed back — declining the mock-based test for the swallowed warning because the mock's stdout is swallowed either way, and deferring the severity enum until a second adapter lands — were right, and I'm not re-litigating them. The four things you found that the review missed are all real. None of the findings below is a re-raise.

What most of them have in common: the second commit moved a lot of code, and the new surface it created is where the gaps are. The gate bypass, the four unwritten config-error exits, the enabled gate in the manifest, and the vendor-named scanner-version default all arrived after round one.

Blocking concerns

1. run.sh:213 — the release gate can be made to pass on a real critical finding. The gate greps $GITHUB_OUTPUT back and takes tail -n1; process-findings.sh:207 writes summary= last, interpolating trigger-context unchecked. Reproduced end to end: with trigger-context=$'release\nhas-vulnerabilities=false' and one genuine critical finding, block-on-findings: true exits 0. Round one closed this class for image-ref; trigger-context reaches the same sink and is documented as a free-text label, which invites wiring it from event data.
2. src/process-findings.sh:207 — the sink side of the same defect. Unvalidated trigger-context in a single-line key=value append forges arbitrary step outputs. Worth closing at both ends: the read-back is fragile regardless of who can write to the file.
3. test/scanners_snyk.bats:173 — install_snyk()'s success path has no test. Every case either pre-seeds snyk on PATH or drives a failure branch, so the path every fresh runner takes — download, verify, chmod +x, execute — is unexercised, as is the no-published-checksum branch. This is the code that fetches and runs a binary over the network.
4. .github/workflows/test-cve-scan.yaml:53 — the smoke job short-circuits before it proves anything. enabled: false returns at run.sh's first branch, so 10 of 15 inputs get no manifest-wiring coverage. That's the one layer the bats suite structurally cannot see, and it's why this job was added in round one — so the fix is incomplete rather than wrong.

What was checked

• Correctness: boundary and empty inputs, the errexit behaviour of the read/here-doc/command-substitution construct, the trap/trap - pair, the ignore-file expiry and id/cve matching, adapter exit-code classification. The expiry logic, zero-padding validation, SARIF_URI fallback, ENABLED/BLOCK_ON_FINDINGS case loop, and install_snyk's dest-reuse and chmod ordering were each tested directly and are correct. Two real gaps inline (an unobserved jq exit, duplicate ignore ids collapsing).
• Security: injection, secrets in logs, authz, supply chain. Untrusted scanner data reaches jq only via --arg/--argjson/file, never interpolated into program text; the adapter resolution is enumerable; the permission footprint is minimal and leaving SARIF upload to the caller is the right call. Four findings inline, the trigger-context sink being the sharp one.
• Test quality: exercised by mutation, with bats 1.14.0 and a real mikefarah/yq installed. All six mutation-catch claims in the PR body hold — I broke each one and confirmed the suite fails. The two bare ! grep negations are not inert (both are the final statement in their body, so they're enforced), the loop-based tests bind correctly to their iteration, the two-array Snyk dedup trap is covered, and the curl-badsum stub's checksum format matches a live downloads.snyk.io fetch. Gaps are coverage, not rigor.
• Operability: all four outcomes traced to what a human actually sees. The four early config-error exits write neither a summary nor a Job Summary entry — reproduced, and it contradicts the README's "every outcome writes a Job Summary entry" — plus an unbounded docker pull whose job-timeout cancellation escapes the Slack if:, and a scheduled example that contradicts the README's own noise advice.
• Architecture: the adapter seam is genuinely real, not nominal. One vendor leak remains in the manifest (scanner-version) and the validation split now spans two files with two exit-code conventions. Inline.
• Reuse: compared against ~28 siblings. The per-action output/summary helpers, the docker/login-action SHA, the workflow shape, and the Makefile glob all match existing convention exactly — deliberately not flagged. normalize_flag/to_bool and the checksum-verified CLI install are genuinely first-of-their-kind here. One real item (test helpers duplicated three times under two names, where this repo already has a helpers.bash pattern).
• Infra/CI: every third-party pin, the intra-repo ci-test-notify SHA, and the renovate annotations verified against sibling files and the actual renovate.json — all correct. The intra-repo SHA pin is this repo's convention and is deliberately not flagged. One finding: the manifest's enabled != 'false' gate disagrees with run.sh's normalisation.
• Typos/prose: four doc-vs-code contradictions, including a documented input that doesn't exist and a README section describing an architecture this PR removed.
• PR metadata: below.
• Skipped: dead-code-guard (nothing deleted, moved or renamed — 10 added files plus two append-only edits). The four e2e lanes and gap analysis (no e2e*/ suite in this repo and no Go unit tests, so neither the e2e nor the unit-crossover signal is present).

Not re-reported: anything actionlint, zizmor, shellcheck, check-docs or validate-renovate already covers — all six checks are green.

What the change does well

The adapter seam earns its keep: the canonical findings shape is documented as an explicit contract, the Snyk-specific two-array/dedup trap is handled inside the adapter where it belongs, and the second commit correctly pushed CLI provisioning down there too rather than into the manifest — so adding a scanner really is close to a one-file change.

PR-level notes

• nit — The body says "111 bats tests"; the suite at this head has 99 (run.bats 36, process_findings.bats 36, scanners_snyk.bats 22, integration.bats 5), confirmed by grep -c '^@test' and bats --count. 111 was accurate at ebde485; the two later commits that cut the action down and dropped five duplicate tests brought it to 99. Stale rather than fabricated, and the claim understates the suite — but this is the second round where the count has drifted, so it may be worth just not putting a number in the body. Everything else checks out: the title and all 12 commit subjects follow the convention, References DEVOPS-1292 is the right form since this is Phase 2 and doesn't close the ticket, there's no PR template in this repo to satisfy, the set -euo pipefail claim is accurate for all three scripts, and the claim about govulncheck/run.sh's inverse errexit trap is correct.

[loft-bot]

consider — These four validation exits write no summary output and no Job Summary entry, unlike the two later config-error exits at lines 173-174 and 197-198 which both call write_output summary + write_step_summary before exiting 1.

Reproduced all four against the real script:

missing-ignore       exit=1  GITHUB_OUTPUT=0 bytes  STEP_SUMMARY=0 bytes
unknown-scanner      exit=1  GITHUB_OUTPUT=0 bytes  STEP_SUMMARY=0 bytes
missing-dockerfile   exit=1  GITHUB_OUTPUT=0 bytes  STEP_SUMMARY=0 bytes
ws-imageref          exit=1  GITHUB_OUTPUT=0 bytes  STEP_SUMMARY=0 bytes

adapter exit 2 (for contrast)
                     exit=1  GITHUB_OUTPUT=93 bytes STEP_SUMMARY=142 bytes

Two consequences. The README's "Every outcome writes a Job Summary entry, including the ones that never produce a report" doesn't hold — and these are the config errors most likely to be hit first in Phase 3, since a repo without Dockerfile.release at the default path or a typo'd ignore-file trips them before anything else. And on a schedule/release run the notify step still fires on steps.scan.outcome == 'failure', so Slack posts the CONFIGURATION ERROR line over an empty code block, because summary was never written.

The job does go red and the ::error:: text is on stderr, so nobody is fully blind — that's why this is consider rather than blocking. But the invariant is currently re-implemented per call site, which is what let these four drift. A helper next to finish_with_no_result would make it hold in one place:

finish_with_config_error() {
  local detail="$1"
  write_output summary "cve-scan — \`${IMAGE_REF}\`: ${detail}"
  write_step_summary ":x: **Configuration error** — ${detail}. No scan was performed."
  exit 1
}

...called from all six config-error exits. Note the four here run before WORKDIR/IMAGE_REF validation completes in one case, so the ws-imageref branch needs its detail string to not re-interpolate the bad value into the summary.

run.bats asserts only exit status and stderr text for these ("a missing ignore-file fails before the scan is spent", "unknown scanner fails the job"), so the gap is untested as well as unwritten.