ci: prevent shell injection in status-check workflow - #39
Open
olaservo wants to merge 3 commits into
Open
Conversation
The "Get PR number" step interpolated github.event.workflow_run.head_branch directly into a run: script. A PR author controls their branch name, so a name like rce$(...) executes arbitrary commands on the runner, which has a GITHUB_TOKEN with statuses:write and checks:write. Pass the branch name, event name, and PR number through env: and reference them as quoted shell variables so untrusted input is never templated into the script body. Fixes code scanning alert #1 (actions/code-injection/critical). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Secures the status-check workflow against branch-name shell injection.
Changes:
- Passes GitHub event values through environment variables.
- Uses quoted shell expansions for attacker-controlled branch names.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jeongukjae
reviewed
Aug 19, 2026
The step's number output was never read by any other step. The Check required statuses step derives its ref from head.sha / head_sha in the event payload, not from a PR number. Removing the step also removes the shell-injection sink outright (gh pr list --head with an attacker-controlled branch name) instead of only quoting around it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/status-check.yml:28
- The implementation removes the entire PR-number step rather than moving its expressions into
env:as the PR's Fix section states. Since the removed output is unused, deletion still addresses the injection, but the PR description should be updated to document the actual remediation and avoid a misleading security audit trail.
- name: Get changed files
jeongukjae
approved these changes
Aug 20, 2026
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.
Summary
Fixes the open critical code scanning alert (
actions/code-injection/critical) in.github/workflows/status-check.yml.The "Get PR number" step interpolated
${{ github.event.workflow_run.head_branch }}directly into arun:shell script. A PR author controls their own branch name, so a branch namedrce$(...)executes arbitrary commands on the runner — which holds aGITHUB_TOKENwithstatuses: writeandchecks: write. This is a live target: PR #32's branch name is a working injection payload against this exact line.Fix
Move the branch name (and the other two templated values in the step) into
env:and reference them as quoted shell variables. GitHub Actions template expansion no longer touches the script body, so attacker-controlled input can't break out of the string.No behavior change for legitimate runs.
Not addressed here
The other open alert — critical vitest advisory (Dependabot #4) — is already covered by open PR #14, so it's intentionally left out of this PR.
🤖 Generated with Claude Code