Skip to content

feat(cve-scan): make the slack notify event allowlist an input - #224

Merged
vcauesantos merged 6 commits into
mainfrom
devops-1292/cve-scan-notify-events
Aug 17, 2026
Merged

feat(cve-scan): make the slack notify event allowlist an input#224
vcauesantos merged 6 commits into
mainfrom
devops-1292/cve-scan-notify-events

Conversation

@vcauesantos

@vcauesantos vcauesantos commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

cve-scan has only ever posted to Slack on schedule and release, hardcoded in the manifest's if:. That default is the right one and it stays: a channel that pings every time an engineer pokes an action is a channel people stop reading. What it did not account for is that a workflow_call callee sees the caller's event name. vcluster-pro's release.yaml is dispatch-only by design (DEVOPS-1050), so its pre-release pipeline shows up as workflow_dispatch on the release path as much as the manual one, and could never satisfy the allowlist no matter what it passed. Rather than go unnotified, that caller hand-rolled its own Slack step, which covers two outcomes where this action covers four. Config errors and cancelled scans go silent there.

So the allowlist becomes an input. notify-events defaults to schedule,release, which leaves every existing caller exactly where it was, and widening it becomes a caller's decision rather than an edit to this file. It is still an allowlist, so a new trigger has to opt in deliberately instead of starting to post by surprise.

The matching sits in run.sh, next to where notify is already resolved, for the same reason that comparison moved out of YAML in the first place: a repo variable typed as TRUE used to drop every future notification with nothing anywhere able to annotate why. A comma-separated event list carries that same failure mode with more ways to fumble it, so the list tolerates case and whitespace, matches entries whole so release cannot match prerelease, warns and falls back to the default when it arrives empty, and leaves a ::notice:: naming the event and the list whenever it blocks one. The two switches stay independent, the way they were as separate gates: notify and notify-events both have to admit a run before anything posts, so an unrecognised value on one cannot carry a run past the other.

Since the message was being touched anyway, it also regains the findings link that only the caller-side version had. When the scanner produced a SARIF, the Slack message now links to the Security tab filtered to the ref that was scanned. It is derived from github.ref rather than taken as an input, so it cannot drift from the ref upload-sarif defaults to; a caller passing the URL itself could let the two disagree. The filter is load-bearing rather than cosmetic: without it the tab shows the default branch, so an RC scanned from a tag reads as empty. It sits outside the code fence because Slack will not linkify inside one.

One risk carries over, and it is narrow. A cancel landing before run.sh resolves leaves the gate unapplied, so a cancelled run could post where the old YAML check would have stopped it. That is the same posture an unrecognised notify value already gets, unresolved gates resolving toward notifying, and the cancelled arm exists precisely so a killed scan does not pass unnoticed. It is documented in action.yml next to that arm rather than left to be discovered.

Twelve bats cases cover the gate, none of them reachable from a manifest if:, which is the concrete argument for where the logic lives. make test-cve-scan is 109 pass and 0 fail, and shellcheck, actionlint, zizmor and make check-docs are all clean. The action's own input table is regenerated by auto-doc; the hand-maintained input list in the repo README is updated too, since check-docs only compares the generated block and would not have caught it. No composite-smoke scenario was added: that job cannot observe this input without a real scan, and it already documents the same limitation for severity-threshold and block-on-findings. Nothing reaches callers until cve-scan/v1 is advanced after merge.

References DEVOPS-1292

@vcauesantos
vcauesantos marked this pull request as ready for review August 15, 2026 00:45
@vcauesantos
vcauesantos requested a review from a team as a code owner August 15, 2026 00:45

@Piotr1215 Piotr1215 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One CI test issue to fix before moving the tag.

Comment thread .github/actions/cve-scan/test/run.bats
@vcauesantos
vcauesantos merged commit 1d9b147 into main Aug 17, 2026
7 checks passed
@vcauesantos
vcauesantos deleted the devops-1292/cve-scan-notify-events branch August 17, 2026 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants