Avoid script injection when setting Percy branch from refs#729
Open
arpitjain099 wants to merge 1 commit into
Open
Avoid script injection when setting Percy branch from refs#729arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
Both visual-regression workflows build PERCY_BRANCH by interpolating github.head_ref and github.ref_name directly into a run block. The runner expands these expressions into the script before bash sees it, so a pull request opened from a branch with a crafted name could inject commands into the step. Move the branch values into a step env block and reference them as shell variables. The Percy branch resolution is identical; only the substitution point changes from the script text to the environment. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Collaborator
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
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
2 participants
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 Percy-branch steps in
visreg-mobile.ymlandvisreg-web.ymlsetPERCY_BRANCHby writing${{ github.head_ref }}(and${{ github.ref_name }}) directly into arun:script:echo "PERCY_BRANCH=${{ github.head_ref }}" >> "$GITHUB_ENV"GitHub Actions substitutes
${{ }}expressions into the script text before the shell runs, so the branch name a contributor chooses becomes part of the command. A pull request branch named with shell metacharacters could run arbitrary commands in the job. The mobile workflow also folds${{ inputs.branch }}into the same script, which has the same issue.The fix is the one recommended in GitHub's script-injection hardening guidance: bind the untrusted values to step
env:entries and read them as ordinary shell variables. Static analyzers such as CodeQL (actions/expression-injection) and zizmor report the original form for the same reason.I kept the branch-selection logic identical; only the reference style changed. Given the repo already pins actions by SHA and runs harden-runner, this felt in line with the existing posture.