Run govulncheck on a schedule - #95
Open
jeremy wants to merge 9 commits into
Open
Conversation
This repo had no vulnerability scanning, so four (six for kamal-proxy) Go stdlib advisories sat against it unnoticed until a sibling repo that does scan went red and prompted a fleet check. Nothing reported them here because nothing was looking. Scheduled rather than PR-gating on purpose. A toolchain or dependency advisory is published against code that has not changed, so gating pull requests on it turns every new CVE into a red build on unrelated work -- which is exactly what happened to basecamp/cli today. The clock is the right trigger; the diff is not. Failures open an issue rather than only reddening the Actions tab, since a scheduled job nobody watches is the same silence this is meant to end. One issue at a time.
There was a problem hiding this comment.
Pull request overview
Adds scheduled vulnerability scanning with accepted-finding filtering and issue-based reporting.
Changes:
- Runs
govulncheckdaily and on demand. - Filters explicitly accepted advisories.
- Opens a deduplicated issue for new findings.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11108b31e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
govulncheck exits 3 for findings and other nonzero codes for failing to run. Conflating them meant a transient module-proxy error would file an issue saying vulnerabilities were found -- and that wrong issue would then suppress the next real one. Demuxed: 3 is a finding, anything else is a scanner error, reported as such. Bailing out whenever any govulncheck issue was open meant an issue filed for advisory A silenced advisory B entirely. Now every run writes itself into the tracking issue: comment if one is open, create if not. Nothing gets swallowed. Reporting was also skipped for failures before the scan -- a broken checkout or a failed govulncheck install reported nothing at all. It now runs on any job failure, and tolerates a missing output file instead of aborting on an unguarded cat. Added a concurrency group: read-then-write on the issue is not atomic, so a manual dispatch overlapping the cron could file two. ./... does not cross module boundaries, so modules are listed explicitly rather than assumed to be one.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7af5c74ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e the ref govulncheck's source mode only analyses the files the current build configuration selects, so a single native run covered one of the configurations we actually ship. It now scans each GOOS/GOARCH in TARGETS with CGO_ENABLED=0, matching the Makefile's release matrix. An accepted ID was suppressed unconditionally, which kept the job green at exactly the moment a fix became available -- advisory metadata changes without any repository change, so nothing would have prompted anyone. Accepted IDs are now re-checked against the advisory's fixed versions and reported as actionable once a fix lands. The check is scoped to the module path we actually import: these advisories list a renamed module (moby/moby/v2) carrying a fix while the path we depend on (docker/docker) has none, so an unscoped check would fire wrongly. workflow_dispatch can target any ref, so a branch-only finding was being appended to an issue titled for the default branch. The report now names the ref and SHA it scanned and keeps non-default refs in their own issue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c230eb1f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
script/release publishes linux/amd64 and linux/arm64 via buildx, so scanning only amd64 missed half of what ships. My earlier check looked at the Makefile and the workflows and not at script/, which is where the release actually happens. Added linux/arm64. The issue lookup interpolated the title -- which carries a ref name, and git permits quotes in those -- straight into the jq program. A ref like foo"bar turned it into a syntax error, so the lookup failed and no tracking issue was filed at all. Passed as data via $ENV.TITLE instead; verified the old form errors and the new one matches. The fixed-version check collected every fixed event for the module, so an advisory that was fixed and later reintroduced would report the historical fix as an available upgrade and fail forever with advice that does not apply. It now walks each range's events in order and only reports a fix for the interval our own version actually falls in.
…worked The early exit on scan_error ran before the filter, so if one target or module failed to analyse while another found a real reachable vulnerability, the run reported only a scanner error -- explicitly telling the team it was not a vulnerability report -- and never wrote new.txt at all. A persistent target-specific failure could hide confirmed findings indefinitely. The filter now runs over whatever JSON was produced regardless, and the two conditions are reported independently: vulnerable, error, or vulnerable-and-error, which says both need attention and that the coverage gap may be hiding more. Verified with one good module and one unresolvable module on an old toolchain: outcome=vulnerable-and-error, all four stdlib findings still listed, scanner error still reported.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51d61a199b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three separate bugs in the same small function, so it is rewritten to SemVer rules once rather than patched per symptom. Stdlib findings carry a go prefix (go1.26.5) while the advisory's own ranges use a bare 1.26.5. Stripping only v made the former parse as [0,26,5], which matches a historical fixed interval and would fail the run forever with an inapplicable instruction. Prerelease identifiers were compared as one string, so beta.2 sorted above beta.10 and a real available fix went unreported. They are now split on dots and compared per SemVer 11.4, with numeric identifiers ranking below alphanumeric ones and any prerelease ranking below its release. Build metadata is dropped before comparison rather than left to influence precedence. Also: an accepted ID that no longer appears in the scan is dead config that would silently swallow the advisory if a later version reintroduced it. It is now listed in the job summary as removable, without failing an otherwise clean run. And the tracking-issue lookup fetched a bounded page, so with enough per-ref issues open the one for this ref could fall outside it and a duplicate would be created. Comparator verified against 11 cases including both reported bugs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc7280e6ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Reporting only ran on failure, so the tracking state was stale in the other direction: once a vulnerability was fixed, or a transient setup failure cleared, the next run passed silently and the issue it had filed stayed open forever with nothing to say the scan was green again. The step now runs unless the job was cancelled. A clean run comments the result on the open issue for that ref and closes it; if none is open it does nothing. Failing runs behave as before. Verified all five paths against a mocked gh: clean with an issue closes it, clean without one is a no-op, vulnerable and error paths still comment on an existing issue or create one.
The concurrency group was workflow-wide, but GitHub keeps only one run pending per group, so a third dispatch silently cancelled the second. Keyed by ref now, which is the actual unit of contention: each ref has its own tracking issue, so different refs never touch the same one and only same-ref runs need to queue. github.ref_name reduces a branch and a tag with the same short name to the same value, so their findings would have shared an issue. The full ref decides the key and tags are labelled as such. An issue body or comment is capped at 65536 characters. Twelve scans of verbose output can exceed that, and gh would fail the report entirely -- turning a large finding into no notification, the exact failure this workflow exists to prevent. Long output keeps its head and tail and says what was dropped. Verified with a mocked gh: branch main and tag main get separate issue titles, and a 90000-character report is truncated to well under the limit.
The last two review rounds were all about the same thing: a branch and a tag colliding in the tracking key, a dispatch on a feature branch appending to the canonical issue, per-ref concurrency, queued dispatches being dropped. Every one of those existed to support scanning a non-default ref -- a capability nobody asked for. Scheduled runs only ever run on the default branch, and the default branch is what we ship. So checkout now pins to it and workflow_dispatch means run it now, not run it here. That removes the ref-type case statement, the per-ref titles, the per-ref concurrency key and the full-ref plumbing, and makes the workflow-level concurrency group correct rather than a compromise: every run scans the same tree, so a superseded queued run was redundant. One repo, one tracking issue, one fixed title. The commit actually scanned is recorded and reported, so pinning the ref does not cost traceability. Verified all seven report paths against a mocked gh -- clean/vulnerable/ error/vulnerable-and-error/pre-scan-failure, with and without an open issue -- plus truncation of a 90000-character report, and an end-to-end scan of 2 targets x 3 modules.
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
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 repo had no vulnerability scanning at all. That's how four Go standard library advisories sat against it unnoticed — I only found them because
basecamp/cliruns govulncheck, went red, and prompted a check of the other Go repos. Nothing reported them here because nothing was looking.Merge #94 first (the go1.26.7 bump). This job would fail on today's
main— correctly, sincemainis still on the vulnerable toolchain.The accepted-findings list
This repo permanently carries two advisories that have no fix, so a plain
govulncheck ./...would be red from day one — noise, not signal. The job therefore fails only on findings not in an explicitACCEPTEDlist:Both are in
github.com/docker/docker, which reportsFixed in: N/A— the fix exists only ingithub.com/moby/moby/v2 >= 2.0.0-beta.8, a module-path migration onto a beta. The reasoning is written into the workflow next to the list, so the next reader doesn't have to reconstruct it.One correction to something I said on the bump PR: govulncheck reports these as reachable across most of the docker client surface we use (
ContainerCreate,ImagePull,Events, …), not merely through error handling as I first described it. That doesn't change the conclusion — both bugs are in the daemon's plugin authorization and we're a client of the daemon, not a host of it — but the trace list is much broader than I characterized, and the workflow comment now says so accurately.The filter matches govulncheck's own "Your code is affected by N vulnerabilities" line: it counts findings with a symbol-level trace, from
-format json, minus the accepted IDs.I verified it in both directions against this repo rather than trusting the shape:
That third row is the one that matters: the accepted list silences exactly the two IDs named and nothing else — a new stdlib advisory still fails the job.
Why scheduled and not a PR gate
A toolchain or dependency advisory is published against code that has not changed. Gating pull requests on it means the next CVE turns unrelated work red — which is precisely what happened to
basecamp/clitoday: its govulncheck job passed at 09:24 and, on the identical commit, failed an hour later. The clock is the right trigger; the diff isn't.Runs daily, plus
workflow_dispatchfor on-demand. Times are staggered across the three repos.Why it opens an issue
A scheduled job that only goes red in the Actions tab is the same silence this exists to end — GitHub's failure notifications for cron workflows go to whoever last touched the schedule, which isn't a team signal. So a failing run files one issue, labelled
govulncheck, with the run URL and the full output. It files one at a time: if agovulncheckissue is already open, the finding is already visible and it won't pile on.Permissions are
contents: read+issues: write, scoped to the job, under a top-levelpermissions: {}. Schedules only run on the default branch, so no untrusted PR code reaches that token.Verification
actionlint1.7.12 andzizmor1.29.0 (default persona, online) both report clean on the new file.