fix(ci): reject unsafe release dispatch tags - #249
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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: 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 |
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
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 overstates the new job's role: it runs on pull_request and merge_group with no path filter and passes here in 5s, and the description should not claim more than that. 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.
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 do not see a live reopening of the injection bug. What remains is drift-guard coverage and CI evidence.
Findings
-
[P3] Remaining drift-guard holes if you want the suite to match the PR description
scripts/test-resolve-release-tag.sh:121
This does not reopen #239 on today'srelease.yml. All three jobs still call the real resolver, and I could not get the original multiline payload past it. The follow-up work on1fe33c7also addressed the commented-call, tee, heredoc-with-tag=, brace-group, rename, and rename+revert regressions I asked for on73a6a0a. If the bar is only "fix #239 in today's workflow," you can treat the rest as optional hardening.
If the bar is "make the regression suite as load-bearing as the description claims," there are still guard holes.has_output_sinkmisses>> "${GITHUB_OUTPUT}", andhas_tag_assignmentonly looks for literal(tag|version)=, so several same-step write shapes passcheck_resolver_stepson mutated workflows while reintroducing arbitrarytag/versionoutputs after the resolver:|| printf '%s=%s\n' tag "$TAG",|| trueplus a follow-up write, GitHub'stag<<EOFdelimiter syntax,process.env.GITHUB_OUTPUT/environ["GITHUB_OUTPUT"]writes, and dynamic-key forms likeKEY="tag"; echo "${KEY}=evil". I reproduced each locally against copiedrelease.ymlfixtures; the suite stayed green. Consider tightening the guard and adding mutation cases only if you want that broader claim to be true. -
[P3]
release tag validationstill has not reported on the current head
.github/workflows/pr-checks.yml:51
Head1fe33c7only showsQuality-signal triageandCodeRabbitin GitHub's status rollup. ThePR Checksrun for that SHA (30362003516) finished asaction_requiredwith no completed jobs, so the new job has not executed in the environment it exists for. Localscripts/test-resolve-release-tag.shpasses on this checkout, but the latest guard-hardening commits (509f79f,1fe33c7) have not been exercised by CI on the merge candidate SHA. Please get a greenrelease tag validationrun on the latest head before merge.
There was a problem hiding this comment.
The #239 fix is done and I am satisfied with it. release tag validation is green on 1fe33c7, which closes the CI-evidence gap from the last two rounds. Gutting the character allowlist turns the suite red on the original payload, widening v[0-9]* to v* turns it red on v-1, and the resolver rejects everything I threw at it.
The drift guard is where I am calling a stop. I am settling scope here rather than running a sixth enumeration round, so treat the first finding as a design decision rather than an open question.
Findings
-
[P2] Drop the guard's universal claim; pin the three
id: relsteps instead
scripts/test-resolve-release-tag.sh:59
check_resolver_stepsclaims no step can writetag/versionoutputs by any mechanism, and I got six shapes past it against copiedrelease.ymlfixtures:>> "${GITHUB_OUTPUT}"(the sink regex needs a bare$sigil),printf '%s=%s\n' tag "$EVIL"and thetag<<EOFdelimiter form (both need a literaltag=), a step withshell: pythonwritingos.environ["GITHUB_OUTPUT"], a new release job outsideexpected_jobs, and rebinding a consumer from${{ steps.rel.outputs.version }}to${{ inputs.docker_backfill_tag }}while leaving the resolver step untouched. It also fails in the other direction: aCompute cache keystep doingecho "version=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT"indocker-manifestis rejected as "writes tag/version outputs directly", and rewriting the resolver step to the equivalent one-linerun: scripts/resolve-release-tag.sh "$TAG"is rejected as "does not invoke the tested resolver". Five rounds have each closed a spelling without shrinking the space, which is what a blacklist over shell embedded in YAML does. Anchor on structure instead: everytag/versionconsumer inrelease.ymlreadssteps.rel.outputs.*, there are exactly threeid: relsteps, so pin those step blocks by content and require one per job. Twenty-five lines rejected all seven of your existing fixtures, passed the current tree, and did not false-red on either benign edit. On the six bypasses it catches three: the braced sink, theprintfsplit key and the delimiter form. Correcting my own numbers here, since I first wrote four and had only run three of them. It missesshell: python, the unlisted job and the consumer rebind, and those three share the property that makes them worth stating: each leaves all three pinned blocks byte-identical, so the write just moves to a new step, a new job or the reader. A content pin sees edits to what it pins and nothing else. The other limit is that it fires on any edit inside a pinned step including a comment, which is the deliberate-update property rather than a bug. Say what it checks rather than restating the universal claim. Deletingcheck_resolver_stepsand its fixtures outright and keeping the resolver unit tests is also fine by me; what I do not want is a sixth round of adding spellings. -
[P3] Two fixtures do not exercise the clause they name. Recording this so it gets carried into whichever landing you pick, not asking for a separate fix on the current guard.
scripts/test-resolve-release-tag.sh:204
release-bypassdeletes the resolver call line, sohas_resolver_callhits 0 and it is rejected by the missing-call clause, not the output-injection clause. I rebuilt it without the "Decoy resolver call" step and got the identical rejection, so the decoy half proves nothing and the fixture duplicatesrelease-commented-call.release-renamed-revertedat :386 is caught by the same generic direct-output clause asrelease-direct-output,teeandheredoc, not the per-job count clause its name implies. Both slip through becauseassert_guard_rejectsdiscards stderr and checks only the exit status; taking an expected-message argument would have caught it. The other five fire the clause they claim. -
[P3] Correct the guard claim in the description
The body says the guard "Rejects same-steptag/versionwrites to$GITHUB_OUTPUTor$GITHUB_ENVregardless of output mechanism". Four of the six bypasses above are same-step writes, so that sentence needs to go whichever way the finding above lands.$GITHUB_ENVis in fact caught, but no fixture covers it.
On the CI point from the last two rounds: I approved the pending PR Checks run on 1fe33c7, release tag validation completed green, and the job wiring is right (pull_request and merge_group, pinned checkout, persist-credentials: false, contents: read).
Nothing in release.yml or the new checkout steps concerns me: all three are SHA-pinned with persist-credentials: false, docker-manifest gaining contents: read is the minimum for the checkout it now needs, and the shape check guarantees version starts with a digit, which rules out an argv-flag tag and a latest collision.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/test-resolve-release-tag.sh (2)
87-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBlank lines inside a step are silently dropped from the snapshot.
The
in_step && !/^[[:space:]]*$/guard skips empty lines, so the extracted text can never contain a blank line while the heredoc snapshot can't either. Harmless today, but it means whitespace-separated additions inside a step won't be represented exactly as written. Worth a short comment so the intent is obvious to the next reader.🤖 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 87 - 92, Add a brief explanatory comment next to the `in_step && !/^[[:space:]]*$/` condition documenting that blank lines are intentionally excluded from the extracted step snapshot, so future changes preserve this behavior.
99-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winByte-exact snapshot pins unrelated step content.
The expected block includes the ghcr comment prose and the
image=echo, so purely cosmetic edits (rewording a comment, reorderingenv:keys) fail CI with a diff that looks like a security regression. Consider narrowing the assertion to what the security property actually requires — eachid: relstep'sruninvokesscripts/resolve-release-tag.shand writes notag=/version=line to$GITHUB_OUTPUT— and leaving the rest unpinned. This keeps the per-step binding from the earlier review without the maintenance drag.🤖 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 99 - 138, Update the snapshot assertion in the expected resolver steps comparison to validate only the security-relevant behavior for each job’s id: rel step: invoking scripts/resolve-release-tag.sh and not writing tag= or version= values to GITHUB_OUTPUT. Remove byte-exact pinning of unrelated comments, environment ordering, and the docker image echo while preserving the per-step binding checks for docker, docker-manifest, and npm-publish.
🤖 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 87-92: Add a brief explanatory comment next to the `in_step &&
!/^[[:space:]]*$/` condition documenting that blank lines are intentionally
excluded from the extracted step snapshot, so future changes preserve this
behavior.
- Around line 99-138: Update the snapshot assertion in the expected resolver
steps comparison to validate only the security-relevant behavior for each job’s
id: rel step: invoking scripts/resolve-release-tag.sh and not writing tag= or
version= values to GITHUB_OUTPUT. Remove byte-exact pinning of unrelated
comments, environment ordering, and the docker image echo while preserving the
per-step binding checks for docker, docker-manifest, and npm-publish.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b03c9572-a57d-436a-b4e3-9471fad68eb6
📒 Files selected for processing (1)
scripts/test-resolve-release-tag.sh
|
Addressed the maintainer's structural-design decision in
Validation:
|
There was a problem hiding this comment.
The resolver is settled and I am not reopening it. On b0c185f the original payload still rejects with a zero-byte $GITHUB_OUTPUT, gutting the character allowlist reds the suite, widening v[0-9]* to v* reds it, and putting + back reds it. The rewrite is the right call: deleting the blacklist and its seven fixtures for a content pin cost -342/+71 and closed every shape that beat the old guard. Brace group, printf split key, >> "${GITHUB_OUTPUT}", tee -a, the tag<<EOF delimiter form, a re-indented step, id: moved to the first line, a duplicate id: rel, and even a comment-only edit inside a pinned block all fail it now. The false-red I flagged last round is gone.
One gap survives, and it is in the design I settled, not in your implementation of it.
Findings
-
[P2] Pin the checkout step that delivers the resolver, or the pinned block's code is not pinned
scripts/test-resolve-release-tag.sh:59
The pinned block's whole behavior is one line,scripts/resolve-release-tag.sh "${DISPATCH_TAG:-$RELEASE_TAG}", and that file arrives from theCheck out workflow scriptsstep immediately above, which the extractor never emits. I ran three substitutions against copied fixtures and the suite exits 0 on all three: addingref: ${{ inputs.docker_backfill_tag }}to the checkoutwith:, repointing it torepository: attacker/evil, and deleting the step outright. The first two swap the validator for caller-chosen content in a job holdingpackages: write; the third breaks the release at runtime with exit 127 while CI stays green. This is not one of the three limits I accepted last round, which move the write elsewhere; this replaces the code being pinned. Extending the extractor to also emit steps namedCheck out workflow scriptscloses it: three awk lines plus the regenerated expected block, 22 lines total. I ran it, and it reds all three substitutions plus an unpinnedactions/checkout@v4, stays green on a benignCompute cache keystep and on an unrelated rename inrelease-binaries, and still reds the charset, shape,+and brace-group mutations. Deleting the pin entirely and keeping only the resolver unit tests remains fine by me. What I do not want is another round of enumerating spellings, and this is not one. -
[P3]
release tag validationstill has not run on this head
.github/workflows/pr-checks.yml:51
PR Checksonb0c185fisaction_required, so the rollup's green is triage only. The run I approved last round was on1fe33c7, which carried the old guard, so the rewritten suite has never executed in CI. That one is mine to approve rather than yours to fix, and I am approving it alongside this review.
Three things I am recording, none of them asks, all pre-existing and none introduced here. Please do not change them in this PR. The shape check accepts v1atest and v1.2.3.4 while its own error text says vX.Y.Z, so the description's "bad-shape inputs" claims more than the code does, and the wording is the only thing worth touching. docker-manifest re-points :latest and MAJOR_MINOR unconditionally, so backfilling an older tag walks :latest backwards, and an rc tag resolves 1.2.3-rc.1 into a :1.2.3-rc moving tag; I ran both. And ref: ${{ steps.rel.outputs.tag }} takes an unqualified ref, which actions/checkout resolves as a branch before a tag, so a branch named v9.9.9 shadows the tag of that name. All three need write access, the same actor who can already dispatch, so none of them crosses a boundary this PR claims to hold. I will file them separately.
Nothing else in release.yml concerns me. All three checkouts are SHA-pinned with persist-credentials: false and no ref:, docker-manifest gaining contents: read is the minimum for the checkout it now needs, top-level permissions: {} holds, and every $GITHUB_OUTPUT writer in the file is either a validated resolver output or a workflow literal.
|
Addressed beardthelion's checkout-pin finding in 89e17e0. The structural snapshot now pins each |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I rechecked head 89e17e05 after the drift question. The #239 fix still holds on the current tree: the shared resolver rejects the original multiline payload with a zero-byte $GITHUB_OUTPUT, and beardthelion's checkout-pin ask from b0c185f looks addressed — the structural snapshot now pins each Check out workflow scripts block with its id: rel step and reds checkout ref/repository mutations and checkout deletion. I do not see a live reopening of the injection bug.
What remains is one real process gap, not a code defect.
Findings
- [P3]
release tag validationstill has not reported on the current head
.github/workflows/pr-checks.yml:51
This is a CI-evidence gap, not a resolver or pin regression. On89e17e05, GitHub's rollup shows onlyQuality-signal triageandCodeRabbit;PR Checksfor that SHA completed asaction_requiredwith no finished jobs, so the new regression suite has not executed in the environment it exists for. Localscripts/test-resolve-release-tag.shpasses on this checkout. Please get a greenrelease tag validationrun on the latest head before merge. Fork PR workflow approval is the usual fix here — beardthelion approved/reran this on earlier heads, but not yet on89e17e0.
Recorded, not asks for this PR
I looked again at the other threads that can read like findings. None of them reopen #239 on today's diff, and beardthelion asked not to expand scope on several of them here.
- Description drift, not a new bug: loose
v[0-9]*still accepts shapes likev1atestandv1.2.3.4while the error text saysvX.Y.Z. That is pre-existing prefix behavior tightened by charset/newline guards, not a regression from this PR. Wording-only follow-up if you want the body to match the code. - Accepted pin limits, not live bypasses: rebinding consumers away from
steps.rel.outputs.*, adding a rogue job outside the three pinned jobs, or writingtag/versionin an unpinned step can still pass the snapshot while the pinned blocks stay byte-identical. That is the narrow structural guarantee you documented; deleting the pin and keeping resolver unit tests remains a valid maintainer choice. - Pin hole, correct on head: dropping
docker-manifest's newcontents: readwould break the script checkout at runtime while the snapshot stays green. Worth knowing if you extend the pin later; the permission is correct in the current diff. - Intentional tightening:
+buildrejection is deliberate, tested, and scoped to dispatch/backfill hygiene. - Out of scope / pre-existing: unvalidated
release-pleaseoutputs onrelease-binaries/web-sync, branch-before-tag checkout resolution, and backfill:latestbehavior were already outside this PR's claimed boundary or pre-date it.
If the bar is "fix #239 in today's workflow," the resolver and checkout pin are there. If the bar is "green CI on the merge candidate SHA," the item above is the remaining work.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Approving. The #239 fix holds on 89e17e0 and the checkout-pin ask from b0c185f landed exactly as scoped, +22 lines and nothing else.
On this head the original payload rejects with a zero-byte $GITHUB_OUTPUT. Sixteen mutations run: gutting the character allowlist, widening v[0-9]* to v*, re-admitting +, admitting /, and replacing the resolver with the pre-PR unvalidated echo all red the suite. So do all four checkout substitutions (attacker-chosen ref:, repository: attacker/evil, deleting the step, unpinning to @v4), reverting the docker job to inline echo "tag=...", collapsing the resolver step to a one-line run:, and a comment-only edit inside a pinned block. It stays green on an unrelated job rename and on a benign version= write in an unpinned step, so the false red I flagged two rounds ago has not returned. Actionlint 1.7.7 is clean on both workflows.
release tag validation is green on 89e17e0. I approved the pending run; that was mine to clear, not yours, and it closes the CI-evidence gap jatmn raised on the last three heads.
Findings
- [P3] Say that the pin covers the checkout blocks too
scripts/test-resolve-release-tag.sh:60
The description calls it pinning "the completeid: relstep blocks in exactly three jobs", and the verification section says it compares "the complete three resolver step blocks". It pins six blocks now, eachCheck out workflow scriptsstep alongside itsid: relstep, which was the whole point of the last round. Wording only, and it should not pick up a universal claim on the way past. This is the only thing I am asking for and it should not hold the merge.
Two limits worth knowing, neither an ask. A step inserted between the pinned checkout and the pinned id: rel step can rewrite scripts/resolve-release-tag.sh before it runs, with all six blocks byte-identical and the suite at exit 0; I ran it. I am not asking you to close it: the four checkout spellings I did ask for are things a maintainer does by accident, and a sed -i on the validator is not drift, so a content pin is the wrong instrument for it. A job-level defaults.run.shell wrapper is the same class. Second, : "${GITHUB_OUTPUT:?...}" is the one resolver branch the suite does not exercise; deleting that line leaves the suite green. It fails closed, so I am recording it rather than asking.
docker and npm-publish now check out github.ref and then the release tag over the same workspace. That is correct because actions/checkout cleans and force-checks-out, but the default is load-bearing now in a way it was not before; I read that rather than ran it. The three pre-existing items from last round are unchanged and still out of scope here.
@kevincodex1 this one is ready.
|
Two description edits whenever you make the P3 change, so it is one pass rather than two:
|
|
Verified on current head |
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.Check out workflow scriptsandid: relstep blocks in exactly three jobs:docker,docker-manifest, andnpm-publish. Any change to those reviewed blocks must be reflected deliberately in the test fixture.The structural check is intentionally narrow: it verifies those three exact checkout-and-resolver block pairs and does not claim to detect arbitrary output writes elsewhere in the workflow.
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; verifies that empty, multiline, whitespace-containing, shell-metacharacter, path-containing, build-metadata, uppercase-prefix, and bad-shape inputs fail without writing output; and compares the complete three checkout-and-resolver block pairs against their reviewed expected content.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.