security(ci): fail-closed release tag check + pin the toolchain action#321
Merged
Merged
Conversation
Finishes the two items #318 tracked that PR #319 deliberately deferred. Reopened first: #318 had auto-closed one second after #319 merged, purely from a `Closes #318.` keyword in that commit body, while both items were still present on main. Deliberately no closing keyword here — the issue is closed by hand once this is verified. 1. Fail-closed release-tag check ------------------------------- `release-auto.yml` decided whether to publish a release with: git ls-remote --exit-code --tags origin "refs/tags/$tag" >/dev/null 2>&1 That collapses three distinct outcomes — tag present, tag absent, lookup failed — into a two-way answer, reading ANY non-zero exit as "absent". A transient network or auth blip therefore pushed an already-released version down the should_release=true path. This one call decides the entire release, so guessing is the single thing it must not do. Replaced with `gh api repos/$GITHUB_REPOSITORY/git/matching-refs/tags/$tag`. `matching-refs` was chosen over the more obvious `git/ref/tags/$tag` specifically because it answers "absent" with HTTP 200 and an empty array rather than a 404 — a genuine miss can never look like an error, and no error-body parsing is needed to tell the two apart. `matching-refs` matches by PREFIX, so the exact ref is compared in jq. That is load-bearing rather than defensive, and was verified against the live API: `v2.2` prefix-matches two real tags (v2.2.0, v2.2.1) while exact- matching none, so a naive non-empty check would have reported a nonexistent tag as present. Fail-closed behaviour verified under `shell: bash` + `set -euo pipefail`, which is what the step actually runs: a `gh` HTTP error aborts the assignment (exit 1), and a non-array body makes jq error under pipefail (exit 5). Neither can resolve to a release decision. An unexpected match count aborts explicitly. (Verifying this in the local zsh gave a false "passes" result — zsh's `set -e` handling of `var="$(cmd)"` differs from bash's, so the check must be exercised with `bash -c`.) Because the check no longer touches Git, `release-auto.yml`'s checkout drops its persisted credentials as well. It was the single documented exception in #319's sweep, held back only by this ls-remote, so all 19 checkouts in the repo are now uniform and the exception comment is gone rather than stale. 2. Pin dtolnay/rust-toolchain ----------------------------- `.github/actions/rust-setup` used `dtolnay/rust-toolchain@master` — a BRANCH ref that advances on every upstream commit, so every run silently resolved to whatever HEAD happened to be, with no review window and no signal that anything changed. Every other action in this repo is referenced by a `@vN` tag, which is not expected to move. The exposure is disproportionate: this composite is consumed by 12 of the repo's 19 checkouts, including release.yml (contents: write, builds the shipped binaries) and web.yml (pages: write + id-token: write), and it is the action that installs the compiler — a position where a compromise is very hard to detect from build output. Pinned to e97e2d8cc328f1b50210efc529dca0028893a2d9, verified via the API as the current target of upstream's refs/tags/v1. The trailing `# v1` is the convention Dependabot's github-actions ecosystem reads; that ecosystem is already enabled in .github/dependabot.yml, so this does not trade a supply- chain risk for a stale-action one. Deliberately still NOT done: SHA-pinning the remaining 12 actions. Those are `@vN` tags rather than branch refs, so they carry materially less risk than `@master` did, and a blanket pin is a policy decision about pin maintenance rather than a drive-by fix. Addresses #318.
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR pins the shared Rust toolchain action, replaces release tag detection with a fail-closed GitHub API and ChangesCI hardening and release automation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant GitHubAPI
participant JQ
ReleaseWorkflow->>GitHubAPI: Fetch matching tag refs
GitHubAPI-->>ReleaseWorkflow: Return JSON refs
ReleaseWorkflow->>JQ: Count exact tag-ref matches
JQ-->>ReleaseWorkflow: Return match count
ReleaseWorkflow->>ReleaseWorkflow: Set should_release or fail
Possibly related PRs
🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR completes the remaining CI hardening items from #318 that were intentionally deferred from #319: it makes the release-tag existence check in release-auto.yml fail-closed (eliminating “lookup failed” being treated as “tag absent”), and it removes a supply-chain footgun by pinning the Rust toolchain installer action away from a moving branch ref.
Changes:
- Replaces
git ls-remotetag probing in the auto-release workflow with an explicitgh api .../git/matching-refs+ exact-refjqfilter that aborts on errors instead of guessing. - Drops the last persisted-Git-credentials checkout exception in
release-auto.ymlnow that the tag check is API-based. - Pins
dtolnay/rust-toolchainin the sharedrust-setupcomposite action to a specific commit SHA (tracked via# v1).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
CHANGELOG.md |
Documents the completed hardening items (fail-closed tag check + toolchain action pin). |
AGENTS.md |
Adds durable operational guidance capturing the “fail-closed tag check” and “no magic close keywords” lessons. |
.github/workflows/release-auto.yml |
Implements fail-closed tag existence checking via gh api and removes the last persisted-credentials exception. |
.github/actions/rust-setup/action.yml |
Pins dtolnay/rust-toolchain from a moving branch ref to a specific commit SHA. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 70-71: Update the continuation of the `persist-credentials: false`
changelog entry to state that all 19 checkouts are hardened, and remove the
outdated explanation describing `release-auto.yml` as a deliberate exception.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cc7eb60e-2eef-4e7b-ad16-0a06aeb8cf9a
📒 Files selected for processing (4)
.github/actions/rust-setup/action.yml.github/workflows/release-auto.ymlAGENTS.mdCHANGELOG.md
CodeRabbit flagged that the entry added here says release-auto.yml joined the sweep while the #319 entry below it still said "18 of 19" and called release-auto.yml a deliberate exception. Correct, and the underlying problem is larger than the numbers: both entries sit in the same [Unreleased] section, so they ship in one release and a reader gets "we hardened 18 with one exception" followed by "...and now the 19th". That is PR history leaking into release notes, which this project explicitly does not want there ("Keep deep engineering/lineage narrative out of the CHANGELOG"). Patching the count would have left two entries narrating one coherent change. Merged into a single entry describing the end state — all 19 checkouts hardened, with the fail-closed tag check and the toolchain pin as the two related items — and dropped the now-purely-historical intermediate narrative. The audit facts worth keeping are retained verbatim: rust-setup performs no checkout of its own, no job needed the credential, and web.yml's build is the highest-exposure site.
This was referenced Jul 21, 2026
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.
Finishes the two items #318 tracked that #319 deliberately deferred. Addresses #318 — no closing keyword on purpose; the issue gets closed by hand after this is verified, since a keyword is exactly what closed it prematurely last time.
1. Fail-closed release-tag check
release-auto.ymldecided whether to publish a release with:That collapses three outcomes — tag present, tag absent, lookup failed — into a two-way answer, reading any non-zero exit as "absent". A transient network or auth blip therefore pushed an already-released version down the
should_release=truepath. This one call decides the entire release, so guessing is the single thing it must not do.Now
gh api repos/$GITHUB_REPOSITORY/git/matching-refs/tags/$tag.Why
matching-refsand not the more obviousgit/ref/tags/$tag: it answers "absent" with HTTP 200 and an empty array rather than a 404, so a genuine miss can never look like an error and no error-body parsing is needed to tell them apart.Why the exact-ref
jqcompare is load-bearing, not defensive:matching-refsmatches by prefix. Verified against the live API —v2.2prefix-matches two real tags (v2.2.0,v2.2.1) while exact-matching none, so a naive non-empty check would report a nonexistent tag as present:v2.2.1(exists)v2.2(not a tag)v99.99.99Fail-closed verified under
shell: bash+set -euo pipefail, which is what the step actually runs: aghHTTP error aborts the assignment (exit 1), and a non-array body makesjqerror underpipefail(exit 5). Neither can resolve to a release decision; an unexpected match count aborts explicitly.Knock-on: because the check no longer touches Git,
release-auto.yml's checkout drops its persisted credentials too. It was the single documented exception in #319's sweep, held back only by thisls-remote— so all 19 checkouts are now uniform and the exception comment is gone rather than left stale.2. Pin
dtolnay/rust-toolchain.github/actions/rust-setupused@master— a branch ref advancing on every upstream commit, so each run silently resolved to whatever HEAD happened to be, with no review window. Every other action here uses a@vNtag, which is not expected to move.The exposure is disproportionate: this composite feeds 12 of the 19 checkouts, including
release.yml(contents: write, builds the shipped binaries) andweb.yml(pages: write+id-token: write) — and it is the action that installs the compiler, where a compromise is very hard to detect from build output.Pinned to
e97e2d8cc328f1b50210efc529dca0028893a2d9, verified via the API as the current target of upstream'srefs/tags/v1. The trailing# v1is what Dependabot'sgithub-actionsecosystem reads, and that ecosystem is already enabled in.github/dependabot.yml— so this doesn't trade a supply-chain risk for a stale-action one.Deliberately not done
SHA-pinning the remaining 12 actions. They are
@vNtags rather than branch refs, so they carry materially less risk than@masterdid, and a blanket pin is a policy decision about pin maintenance rather than a drive-by fix.Verification note
release-auto.ymlonly runs onworkflow_runafter CI succeeds onmain, so this PR's own CI cannot exercise it — the tag-check logic was instead verified directly against the live API and the fail-closed paths againstbash, as above. Therust-setuppin is exercised by essentially every job in this run.Summary by CodeRabbit
persist-credentials: falseon CI checkouts.