fix(ci): harden GA publish guard — exact tag format, no shell interpolation - #767
Conversation
…lation Review findings on the verify-tag gate added in #764 (raised by @hedhoud and CodeRabbit/zizmor), all three confirmed against the merged workflow: 1. Tag format was too loose. The guard only rejected '-rc.', so v2.0.1-rc1, v2.0.1-beta, vfoo etc. passed and would publish GA images and move :latest. The rc1 case is the sharp one: build_rc.yml triggers on 'v*-rc.*' which requires the dot, so a one-character typo matched neither workflow's intent. Now validated against ^v[0-9]+\.[0-9]+\.[0-9]+$ and failed loud. 2. Template injection. ${{ github.ref_name }} expanded into the run body before the shell ran, and git permits ; $ ` " | & in ref names — arbitrary code execution in a job that holds packages:write and Docker Hub credentials. Tag name and SHA now passed via env: and referenced as shell variables. 3. Checkout persisted credentials, inconsistent with build_rc.yml which already sets persist-credentials: false on all three checkouts (95fd86f). The repo is public, so the origin/main fetch still works without them. Behavior: vX.Y.Z on main builds; vX.Y.Z-rc.N skips to build_rc.yml; malformed or prerelease tags and off-main tags now fail loudly instead of publishing.
📝 WalkthroughWalkthroughThe ChangesGA release gating
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@hedhoud this is the fix for both findings you raised on #765 — could you take a look when you have a moment? Short version of what changed in the
Worth flagging for prioritisation: the loose guard is what v2.0.1 shipped with and it is live on Once this lands I will re-sync #765 from |
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 @.github/workflows/build.yml:
- Around line 39-45: Update the tag gating around the workflow’s job-level if
condition so all tag refs, including non-v tags such as 1.2.3 and release-1,
reach the exact GA validation. Preserve the explicit valid-RC path handled by
build_rc.yml, while ensuring malformed or unsupported tags are rejected by the
existing TAG_NAME format check before publishing.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 150b1581-2eda-4521-a0ea-bb78b99ac873
📒 Files selected for processing (1)
.github/workflows/build.yml
hedhoud
left a comment
There was a problem hiding this comment.
Approved. This addresses the two blocking findings from #765: malformed v* prerelease/typo tags are rejected before any publish job can run, and the tag/SHA values are now passed through env instead of being interpolated into the shell script.
I do not consider the remaining CodeRabbit comment about non-v tags blocking here: this workflow is configured to trigger only on v* tag pushes, so tags like 1.2.3 or release-1 do not enter this publishing workflow and cannot publish GA images.
|
Re the remaining CodeRabbit thread ( The premise is that tags like on:
push:
tags: ["v*"]so a non- Adopting the suggestion would mean broadening The forgot-the- Resolving the thread on that basis. |
Addresses the review findings on the
verify-taggate added in #764. Raised by @hedhoud on #765 and independently by CodeRabbit/zizmor. All three verified against the merged workflow.Targets
mainrather than the back-merge branch (#765), because the affected guard is already on main — fixing it in the back-merge would leavemainexposed while onlydevelopgot the fix.1. GA tag format was too loose
The guard only rejected
-rc., so these all passed and would publish GA images and move:latest:v2.0.1v2.0.1-rc.1build_rc.ymlbuild_rc.ymlv2.0.1-rc1v2.0.1-beta,v2.0.1-alpha.1vfoo,v-testThe
-rc1case is the sharp one:build_rc.ymltriggers onv*-rc.*, which requires the dot — so a one-character typo matched neither workflow's intent and shipped a release candidate as GA.Now validated against
^v[0-9]+\.[0-9]+\.[0-9]+$.2. Shell injection via
github.ref_name${{ github.ref_name }}was expanded into therunbody before the shell executed.git check-ref-formatpermits;,$, backtick,",|,&in ref names, so a crafted tag could execute arbitrary code — in a job holdingpackages: writeand Docker Hub credentials.Tag name and SHA are now passed via
env:and referenced as"$TAG_NAME"/"$TAG_SHA".3. Checkout persisted credentials
build_rc.ymlalready setspersist-credentials: falseon all three checkouts (hardened in95fd86fc); the newverify-tagcheckout was inconsistent. Added. The repo is public, so theorigin/mainfetch still works without them.Validation
Regex checked against every case above (plus
v10.20.30,v2.0,v2.0.1.2,2.0.1, and an injection-shaped tag — only exactvMAJOR.MINOR.PATCHpasses). YAML re-parsed; confirmed no${{ }}remains in therunbody and all three build jobs are still gated byneeds: verify-tag.Once merged, #765 will be re-synced from
mainso the back-merge carries this too.Summary by CodeRabbit
vMAJOR.MINOR.PATCH).