ci: pass untrusted pull-request fields through env in changeset-check - #122
Conversation
`${{ }}` expansion happens before the shell ever reads the script, so
interpolating github.event.pull_request.title and .head.ref into the
`Check for changeset` step's inline run: block made a crafted PR title or
branch name execute as shell source on the runner.
Pass both through the step's env: block instead, per GitHub's documented
mitigation. Every existing use already quotes "$PR_TITLE" / "$PR_BRANCH",
so the gate's skip/pass/fail decisions are unchanged.
Also quote $GITHUB_ENV in the pnpm store step so actionlint reports no
findings for this file (SC2086).
|
Release PreviewThis PR title will not trigger an automatic release on squash merge. Use a releaseable conventional PR title like Otherwise, add a manual changeset or apply the |
|
Correction to my note above: the remedy is already in flight — #124 Sequence to green this PR:
Everything in this PR's own scope is green: Equivalent generation-21 refreshes are open in the sibling repos too — sdk#1179 and |
There was a problem hiding this comment.
Pull request overview
This PR hardens the changeset-check GitHub Actions workflow against shell-injection by preventing attacker-controlled pull request fields from being expanded into an inline run: script, while keeping the changeset gate’s behavior unchanged.
Changes:
- Pass
github.event.pull_request.titleandgithub.event.pull_request.head.refinto the step viaenv:and remove the inline shell assignments that previously allowed injection. - Quote
"$GITHUB_ENV"in the pnpm store step to avoid unsafe word-splitting/globbing in the redirect target.
Closes #121
What was wrong
The
Check for changesetstep interpolated two attacker-controlled pull-requestfields directly into its inline
run:script:${{ }}expansion happens before the shell ever reads the script, so thesubstituted text becomes shell source. Anyone who can open a pull request could
run commands on the runner by choosing a title or branch name.
Reproduced locally by simulating GitHub's textual substitution with the title
fix: innocent"; touch pwned.txt; echo ":pwned.txtcreated — injected command ranenv:)The workflow's
permissions: { pull-requests: read, contents: read }and itspull_request(notpull_request_target) trigger keep the blast radius small —no write-scoped token, no repository secrets beyond the read token — but they do
not stop arbitrary execution inside the job, and
GH_TOKENis in scope for thestep.
The fix
Pass both fields through the step's
env:block, per GitHub's documentedmitigation, and drop the two assignments. Every existing use already quotes
"$PR_TITLE"/"$PR_BRANCH", so nothing else changed.actionlintreports only the first untrusted expression perrun:block, so itflagged the title but not the
head.refline one row below — both are fixed.Also quoted
>> "$GITHUB_ENV"in theGet pnpm store directorystep, clearing theone pre-existing
SC2086in this file.actionlintnow reports zero findings forchangeset-check.yml.Behaviour
The gate's skip/pass/fail decisions are unchanged. Verified by running the
PR_TITLE/PR_BRANCHdecision branches verbatim against a title/branch matrix:fix: quote untrusted PR fieldsclaude/…feat(pdf): add thingclaude/…refactor!: drop legacy apiclaude/…chore(release): v0.65.9changeset-release/mainchore(deps): bump foorenovate/foo-1.xchore: version packagesclaude/…docs: tweak readmeclaude/…One incidental improvement: a title containing a double quote used to be truncated
at the quote by the old assignment, so it could be classified on a mangled string.
It is now classified on the real title.
Sibling repo sweep
actionlintwas run across.github/workflows/in the repos that share thesecopied workflows. No
is potentially untrustedfindings:happyvertical/smrthappyvertical/sdkPR_TITLE/PR_BRANCHviaenv:inon-pull-request.ymlhappyvertical/happyvertical.comhappyvertical/iacpdfwas the only repo still carrying the pattern.Validation
actionlint .github/workflows/changeset-check.yml— no findingsactionlint(repo-wide) — nois potentially untrustedfindingspnpm lint/pnpm typecheck/pnpm build— passpnpm test— 17 files, 162 tests passscripts/validate-conventional-commits.sh— commit range and PR title cleanThis PR carries the
skip-changesetlabel: it touches only.github/workflows/,adds no changeset, and its
ci:commit is not releaseable, so the label'sconsistency checks all hold.
{ "schema": "hv-agent-run:v1", "runtime": "claude", "session": "4e86f515-9c37-41bb-9228-2542cccbac3f", "issue": "121", "policy_revision": "1.0.0", "validation": [ "actionlint .github/workflows/changeset-check.yml", "actionlint", "pnpm lint", "pnpm typecheck", "pnpm build", "pnpm test", "bash scripts/validate-conventional-commits.sh range origin/main HEAD" ] }