fix(ci): reject unsafe release dispatch tags - #249
Conversation
📝 WalkthroughWalkthroughAdds a shared Bash script for validating and resolving release tags, updates Docker and npm release jobs to use it, and adds PR checks for valid and invalid inputs plus workflow wiring. ChangesRelease tag validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 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 |
github-actions
Bot
added
the
workflow-change
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
- [P3] Add the claimed regression coverage, or correct the PR description
.github/workflows/release.yml:86
The PR says that the new validation cases are covered, but this change adds no executable check for any of the threeResolve release tagblocks; Actionlint only validates workflow syntax and does not execute the shell. A later edit can therefore restore the newline$GITHUB_OUTPUTinjection while CI remains green. Add a focused probe that exercises a valid tag and a newline-containing tag for each resolver (or remove the coverage claim).
|
Addressed the requested executable coverage in The three release jobs now call one shared The regression script is now a blocking Validation:
|
![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 `@scripts/test-resolve-release-tag.sh`:
- Around line 16-28: Add rejected test cases in
scripts/test-resolve-release-tag.sh for release tags containing whitespace,
shell metacharacters, and values that do not match the v[0-9]* format. Follow
the existing newline and empty-tag assertions: invoke $resolver with each
invalid value, require failure, and verify the corresponding GITHUB_OUTPUT file
remains empty.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ae66396-c00b-4045-a864-498dc8c16b79
📒 Files selected for processing (4)
.github/workflows/pr-checks.yml.github/workflows/release.ymlscripts/resolve-release-tag.shscripts/test-resolve-release-tag.sh
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
@beardthelion LGTM but needs your evaluation
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The resolver itself holds up. I reproduced the original injection against the pre-PR logic (v1\nversion=9.9.9\ntag=ghcr.io/attacker/evil passes the old glob, and last-wins resolution yields tag=ghcr.io/attacker/evil), confirmed the new script rejects that exact payload with a zero-byte $GITHUB_OUTPUT, and mutated every protection in it: the character allowlist, the shape check, and removing a call site each turn the suite red. actionlint 1.7.7 runs clean on both workflows.
What needs another round is the drift guard, not the fix.
Findings
- [P2] Assert the invariant, not a line count
scripts/test-resolve-release-tag.sh:40
grep -c 'scripts/resolve-release-tag.sh' ... -ne 3 counts matching lines rather than resolver invocations, so it fails in both directions. I executed three reintroductions of the original bug that all leave the suite green: reverting one call site to inline while a comment still names the script, adding a fourth job with the old inline write, and keeping all three calls but wrapping one in || { echo "tag=$TAG" >> "$GITHUB_OUTPUT"; } so the rejection is swallowed. Meanwhile a benign comment naming the script flips it red with all three call sites intact.
Replacement, run against the current head plus all three bypasses plus the comment case, 5/5 as intended:
workflows="$repo_root/.github/workflows"
steps="$(grep -c 'name: Resolve release tag' "$workflows/release.yml" || true)"
calls="$(grep -cE '^[[:space:]]*scripts/resolve-release-tag\.sh ' "$workflows/release.yml" || true)"
if [[ "$steps" -lt 1 || "$steps" -ne "$calls" ]]; then
printf '%s\n' "every 'Resolve release tag' step must call the tested resolver; steps=$steps calls=$calls" >&2
exit 1
fi
if grep -rqE '(echo|printf)[^|]*(tag|version)=.*>>[[:space:]]*"?\$GITHUB_OUTPUT' "$workflows"; then
printf '%s\n' "a workflow writes a tag/version output without the tested resolver:" >&2
grep -rnE '(echo|printf)[^|]*(tag|version)=.*>>[[:space:]]*"?\$GITHUB_OUTPUT' "$workflows" >&2
exit 1
fiThe || true earns its place separately: with all three call sites gone the count is 0, grep -c exits 1, and set -e kills the assignment before the intended "found 0" message ever prints. The second grep is clean on the current tree, where the only other $GITHUB_OUTPUT writers are image= and enabled=.
- [P2] Cover the two resolver relaxations that survive the suite
scripts/test-resolve-release-tag.sh:31
Changing v[0-9]*) to v*) keeps the suite green and admits v-1, vlatest, and bare v. Widening the allowlist to [!A-Za-z0-9._/-] also stays green and admits v1.2.3/../../x. Extending the existing invalid loop to "v-1" "vlatest" "v" "v1.2.3/../../x" "V1.2.3" closes both: I ran all five against the current resolver (rejected, zero-byte output) and against each mutant (accepted, so every added case is load-bearing).
- [P3] Fix three claims in the description
The body omits the three added actions/checkout steps and docker-manifest's new contents: read. It says the test asserts all three release jobs call the tested resolver, which the first finding disproves. It also calls the new job blocking: the job does run on pull_request and merge_group with no path filter and passes here in 5s, but the main ruleset carries no required-status-checks rule at all. Adding it there is on me rather than this PR, so the wording is the only thing to change.
- [P3] Note two narrow behavior changes
scripts/resolve-release-tag.sh:7
The allowlist rejects +, so v1.2.3+build.5 now fails where the old glob accepted it. release-please emits plain vX.Y.Z, so this only reaches a hand-dispatched backfill, and it fails loudly rather than silently. Separately, the reject-path ::error:: annotations moved from stdout to stderr. The step still exits non-zero either way, but worth confirming the annotation still renders, since I did not test that against a runner.
Scope note on framing: the tag validation here is input hygiene rather than a privilege boundary. Only a write-access collaborator can supply docker_backfill_tag or npm_backfill_tag, and a dispatch runs the workflow from the selected ref, so that same actor controls both the invoking run: block and, since the new checkouts carry no ref:, the validator script itself. That is not an argument against the change, just a reason to keep the description modest. The gap tracked in #234 is what actually constrains that surface, and it is deliberately out of scope here.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/test-resolve-release-tag.sh`:
- Around line 62-78: Update the workflow validation in the test script to
inspect each individual “Resolve release tag” step rather than comparing
aggregate counts. Extract each matching step’s run block, require that block to
invoke scripts/resolve-release-tag.sh, and reject tag= or version= writes to
GITHUB_OUTPUT even when the echo/printf command spans multiple lines; retain the
existing failure diagnostics and nonzero exit behavior.
- Around line 16-21: Update the resolver exercised by the test to prevent
Docker-incompatible versions containing “+” build metadata from being published:
either reject such tags through the existing allowlist validation or normalize
the metadata before producing the release version used for Docker tagging.
Adjust the test’s expected behavior so v1.2.3+build.5 is not accepted as a valid
release version, while preserving valid semver tag handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 580b5996-6788-468f-bc4a-be29195dee6d
📒 Files selected for processing (2)
scripts/resolve-release-tag.shscripts/test-resolve-release-tag.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/resolve-release-tag.sh
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The resolver is right and I could not get anything past it: the original injection payload, shell metacharacters, whitespace, path traversal and case variants all reject with a zero-byte $GITHUB_OUTPUT, and the five invalid cases from last round are in and load-bearing. Two things need another pass, and the first one is a hole in the guard I handed you.
Findings
-
[P2] Bind the drift guard to each resolver step, not to file-wide counts
scripts/test-resolve-release-tag.sh:62
The guard I proposed compares a count ofname: Resolve release taglines against a count of resolver-call lines, and both greps are line-scoped. I rewrote thedocker-manifeststep to writetag/versionthrough a{ echo ...; echo ...; } >> "$GITHUB_OUTPUT"brace group and moved a decoy resolver call into a new step so the counts stayed 3 and 3. The suite exits 0 and prints "release tag resolver tests passed" while the original injection is fully back: that step resolvestag=ghcr.io/attacker/evil,version=9.9.9. The brace form is the idiomrelease.ymlalready uses elsewhere, so this is the shape drift takes by default, not a contrived edit. What worked when I ran it: extract eachResolve release tagstep's ownrunblock, flatten it to one line, require the resolver call in that block, and reject(tag|version)=followed by a$GITHUB_OUTPUTor$GITHUB_ENVredirect anywhere in it. Exit 0 on the current tree with steps=3, exit 1 on the bypass. Keep it per-step and key-scoped rather than a blanket ban, becauseecho "image=..."atrelease.yml:97legitimately lives inside the first resolver step; my first attempt banned every redirect and false-positived on exactly that line. CodeRabbit raised the same class at line 78 and that thread is still unresolved. -
[P2] Reject
+again and assert the build-metadata tag fails
scripts/resolve-release-tag.sh:8
+entered the allowlist inc00eaa2;64e228fdid not have it. That came from my own P3 last round, which flagged the+rejection as a behavior change worth noting, and I should have been clearer that noting it was the whole ask. Accepting it is worse:versionflows intodocker buildx imagetools create -t "$IMAGE:$VERSION"atrelease.yml:222, and1.2.3+build.5is not a legal image tag, nor is the1.2.3+buildthatMAJOR_MINOR="${VERSION%.*}"derives at:216. The suite at line 16 currently pins that broken value as correct. I dropped+from the allowlist and turned the build-metadata block into a rejection assertion: suite green,v0.7.0still resolves, and putting+back turns it red, so the new case carries its weight. release-please emits plainvX.Y.Z, so nothing real regresses. -
[P3] CI had never run on this head
.github/workflows/pr-checks.yml:51
PR Checkswas sitting ataction_required, so therelease tag validationjob had not executed once in the environment it exists for and the green rollup was triage jobs only. I approved the pending run, so it is going now. The job's own wiring reads correctly:pull_requestandmerge_group, no path filter,contents: read,persist-credentials: false. -
[P3] Correct two claims in the description
The body says the change "preserves safe+buildmetadata", which the finding above asks you to reverse, and that the drift guard requires every namedResolve release tagstep to call the tested helper, which the bypass above disproves.
Superseded by my review on c00eaa2; dismissing so the state reflects the current head.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/test-resolve-release-tag.sh (1)
138-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBypass fixture is coupled to release.yml's exact job order and step naming.
The
resolver_calls == 2check assumes the 2ndscripts/resolve-release-tag.shoccurrence is always the docker-manifest job, and the decoy insertion is anchored to a step literally named "Download digests". If jobs are reordered or that step is renamed, this fixture silently stops testing what it intends to (thoughcheck_resolver_stepswould likely still fail correctly via the replaced-call detection alone). Consider anchoring on thedocker-manifest:job header instead of an ordinal count for resilience against future release.yml restructuring.🤖 Prompt for 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. In `@scripts/test-resolve-release-tag.sh` around lines 138 - 164, Make the bypass fixture target the docker-manifest job explicitly instead of relying on resolver_calls == 2 and the literal “Download digests” step name. Update the awk logic around the release workflow transformation to track the docker-manifest: job and inject the malicious resolver output and decoy call within that job, preserving the existing check_resolver_steps assertion.
🤖 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.
Nitpick comments:
In `@scripts/test-resolve-release-tag.sh`:
- Around line 138-164: Make the bypass fixture target the docker-manifest job
explicitly instead of relying on resolver_calls == 2 and the literal “Download
digests” step name. Update the awk logic around the release workflow
transformation to track the docker-manifest: job and inject the malicious
resolver output and decoy call within that job, preserving the existing
check_resolver_steps assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02222c60-cae8-4263-ac30-a43f18051351
📒 Files selected for processing (2)
scripts/resolve-release-tag.shscripts/test-resolve-release-tag.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/resolve-release-tag.sh
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
The #239 fix on the current tree looks correct. I still found gaps in the regression guard and CI evidence before I would call this fully ready.
What looks good
scripts/resolve-release-tag.shrejects the original multilineworkflow_dispatchpayload (v1.2.3\nname=owned): non-zero exit, zero-byte$GITHUB_OUTPUT.- All three release jobs on head call the real script, not a comment or decoy.
- beardthelion's earlier brace-group and
+buildfollow-ups appear addressed on73a6a0a.
Findings
-
[P2] Require a real resolver invocation, not a commented reference
scripts/test-resolve-release-tag.sh:62
This does not reopen #239 on the current diff, but it does leave the drift guard weaker than the PR description implies.resolver_call_retreats# scripts/resolve-release-tag.sh …as a call, so a step can comment out the script, keep the old inline path (orecho "$VAR" >> "$GITHUB_OUTPUT"with a multilineVAR), andscripts/test-resolve-release-tag.shstill exits 0. Please match only uncommented invocations. -
[P2] Extend the per-step output guard beyond
echo/printf
scripts/test-resolve-release-tag.sh:63
Same scope note: not a live bug in today'srelease.yml, but a real guard hole if someone regresses later. The brace-group bypass is fixed, yet a step that calls the resolver and then appendstag/versionviatee -a "$GITHUB_OUTPUT"or acat >> "$GITHUB_OUTPUT"heredoc still passescheck_resolver_steps. Please reject any same-steptag=/version=write to$GITHUB_OUTPUTor$GITHUB_ENV, and add mutation cases forteeand heredoc appends. -
[P2] Bind the drift guard to all three release jobs
scripts/test-resolve-release-tag.sh:88
Also a future-regression gap, not a current-tree failure. The guard only inspects steps literally namedResolve release tagand only fails when zero such steps exist. Renaming thedockerjob's resolver step and reverting that job to inlineecho "tag=$TAG" >> "$GITHUB_OUTPUT"leaves two guarded steps and the suite still passes. Please assert all three call sites stay guarded, and add a rename/revert regression. -
[P3]
release tag validationhas not reported on the current head
.github/workflows/pr-checks.yml:51
GitHub's status rollup for head73a6a0ac6da814a09a1efea293c457c5d9a1c62fonly showsQuality-signal triageandCodeRabbit; none of thePR Checksjobs completed on this SHA. This may be fork workflow-approval noise, but please get a greenrelease tag validationrun on the latest head before merge so the new suite is actually exercised in CI.
Merge posture
If the bar is only "fix #239 in today's workflow," the resolver half is there. If the bar is "make the new regression suite as load-bearing as described," the three guard items above still need another pass.
3 participants
Summary
Validate manual release tags before writing them to
$GITHUB_OUTPUT, preventing malformed multiline input from creating additional step outputs.Motivation & context
The existing
v[0-9]*shell pattern is a prefix check, not a complete validation rule, andecho "tag=$TAG"writes embedded newlines verbatim. A malformedworkflow_dispatchtag can therefore add unintended output keys consumed by later release steps.This is input hygiene rather than a privilege boundary: manual dispatch inputs are limited to write-access collaborators, who also control the selected workflow ref. The separate workflow-trust boundary is tracked in #234 and remains out of scope.
Closes #239
Kind of change
What changed
actions/checkoutsteps so each job can run the shared helper;docker-manifestnow explicitly requestscontents: read.+build, and bad-shape inputs before any output is written.v[0-9]*release-tag shape check.printffor validatedtagandversionoutputs while preserving::error::annotations on stdout.+buildrejection.Resolve release tagstep in each ofdocker,docker-manifest, andnpm-publish; commented references do not count as resolver invocations.tag/versionwrites to$GITHUB_OUTPUTor$GITHUB_ENVregardless of output mechanism, with mutation regressions for decoy calls, commented calls, multiline brace groups,tee, heredocs, a renamed resolver step, and a renamed/reverted resolver step.How a reviewer can verify
scripts/test-resolve-release-tag.sh go run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7 \ -ignore 'label "macos-15-intel" is unknown' \ .github/workflows/release.yml .github/workflows/pr-checks.ymlThe regression script verifies exact outputs for
v1.2.3. It verifies that empty, multiline, whitespace-containing, shell-metacharacter, path-containing, build-metadata, uppercase-prefix, and bad-shape inputs fail without writing output. It also binds the resolver invariant to all three release jobs and proves the guard rejects decoy calls, commented calls, multiline brace-group writes,teeand heredoc appends, a renamed resolver step, and a renamed/reverted resolver step.Before you request review
cargo test --workspacepasses locally (Rust sources are unchanged)cargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare clean (Rust sources are unchanged)feat(...),fix(...),docs(...)).env.exampleupdated if behavior or config changed (N/A)Protocol & signing impact
N/A — this does not change identity, signatures, UCANs, ref certificates, or P2P wire formats.
Notes for reviewers
The
release tag validationjob runs onpull_requestandmerge_group, but the current repository ruleset does not require status checks, so this PR does not claim that the job is branch-protection blocking.The ignored Actionlint diagnostic is an existing repository baseline: Actionlint 1.7.7 does not recognize GitHub's
macos-15-intelrunner label. With that unrelated diagnostic ignored, both workflows are clean.Summary by CodeRabbit
Reliability
Tests