Repository navigation
ci(release): carry release.yml's tag as data instead of script text - #168
Merged
Merged
Conversation
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.
Root cause
release.yml's "Resolve release tag" step read two values that the repository does not control andwrote them into its
run:body as script text:The runner expands an expression inside a
run:body as text before bash parses it, so the valuebecomes shell source. The double quotes stop an apostrophe from closing the literal (the defect that
stalled the release-please auto-merge step in #166), but they do not make the value data: a
"in itends the literal, and the rest is parsed as shell code. Both values are attacker-shaped rather than
merely user-shaped —
inputs.release_tagis caller-supplied onworkflow_dispatch, and a git refmay legally contain
",$and backticks — and this step runs in the job that declarescontents: writeand hosts the Modrinth and Hangar publishing steps.Reproduced with the checked-in wording and the value
0.15.14"; touch "$CANARY"; : "(the runner'stextual expansion is the only thing done by hand):
The historical shape is worse than a crash: the step stays green, runs the injected command, and
reports a truncated tag.
steps.release-tag.outputs.tagthen selects the release that is uploaded,verified, and propagated to Hangar and Modrinth. With
~-style values (${VAR}, backslashes) thesame code path silently reports a corrupted tag and no command runs at all.
What changed
release.yml— the step's tag now reaches the script as data:Resolution order is unchanged (dispatch input → the release's own tag name → the ref this run was
dispatched on;
set -umakes a missing env entry fail the step instead of silently publishing underthe run's ref name). Step name and
id: release-tagare unchanged, so the steps that readsteps.release-tag.outputs.tagand the three tests that pin this workflow's step names areuntouched. The
run:body no longer contains a single${{ }}.The invariant is now repo-wide (the durable part). The scan that fix(release): don't let a merged commit's apostrophe stall the release-please auto-merge step #166 added to
ReleasePleasePayloadTestmoved into one shared implementation,WorkflowRunBlockPolicy, andReleaseWorkflowShellBoundaryTestasserts it over every.github/workflows/*.y[a]ml— withthe expected file set asserted first, so a sweep that silently stops seeing a file cannot pass as
a green no-op.
core/build.gradle.ktsdeclares the whole workflow directory as atasks.testinput, so editing any workflow re-runs the suite instead of being served from Gradle's cache.
Adding a workflow file is covered automatically.
ReleaseWorkflowShellBoundaryTest(new) pins the step and executes it:runcontract, including precedence and the ref-name fallback;run:block with a stubbed runner context (values exportedfrom files, never spliced — the first draft of the harness had the same defect and the payload
ran inside the harness itself) for four paths: dispatch with an input, dispatch without one,
a release event, and a release tag with build metadata. It asserts the fixed step writes the
same
tag=bytes the historical step wrote;",$( ), backtick,${ }, backslash) on both paths: thefixed step preserves the value byte for byte and runs nothing, while the historical step is
asserted to still execute it / corrupt it with a green exit code;
caught by the policy by name, so the RED the fix started from stays visible;
shell-interpolated-dispatch-input,shell-interpolated-release-tag-name,tag-env-removed,precedence-swapped,ref-name-fallback-removed,unquoted-env-read,output-write-removed,env-read-echoed-inert,tag-env-from-a-step-output-instead-of-the-input..agents/skills/release/SKILL.md— the contract now says any workflow, and documents thisstep's env/run/resolution contract and the test that pins it.
RED → GREEN
RED on the unfixed tree (
./gradlew :core:test --tests '*ReleaseWorkflowShellBoundaryTest*', 6/6failed):
GREEN after the fix: 6/6 pass, 9/9 mutations rejected, and the full
./gradlew buildisBUILD SUCCESSFUL with 430 tests across
core/spigot/bungee/velocity.Scope decision (card item 4)
The
inputs.release_taginjection is worth a source fix, and the card's premise that it lands ina read-only job is not correct:
release.ymlhas a singlebuildjob that declarescontents: write(it uploads the release assets) and hosts thePublish to Hangar/Publish to Modrinthsteps, so injected text runs with a write-scopedGITHUB_TOKEN. Thedispatch-permission precondition still bounds who can supply the value, which is why this is
hardening rather than an incident — but the cost is four lines plus a test, and the same edit makes
the invariant repo-wide, which is the part that keeps the class from coming back.
release-repair.yml(andpullrequest.yml) are not touched, and do not need to be: every one oftheir
run:blocks already readsRELEASE_TAG/ other values from job- or step-levelenv:, whichis why the repo-wide sweep passes over them. They are now covered by the sweep, so a future edit that
reintroduces the pattern fails the suite. Re-running the sweep over all workflows reports exactly
these two historical lines and nothing else.
No production mutation: source change on a trust-boundary workflow, plus tests.
Release impact
Title prefix is
ci:, notfix:— the change touches a workflow and test sources only, so theshipped plugin bytes are unchanged and no release needs to be cut for it. The new harness does
execute the step's own
run:block in CI (pullrequest.ymlruns./gradlew build), and the firstreal release after this merge exercises the same script.