Skip to content

fix(ci): prevent command injection via head_ref in benchmark-trigger - #223

Merged
assafvayner merged 2 commits into
mainfrom
assaf/fix-benchmark-trigger-cmd-injection
Sep 22, 2026
Merged

assafvayner merged 2 commits into
mainfrom
assaf/fix-benchmark-trigger-cmd-injection

Conversation

@assafvayner

@assafvayner assafvayner commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Fixes a command injection in benchmark-trigger.yml: steps.pr.outputs.head_ref (the PR branch name, fully attacker-controlled by whoever opens the PR) was interpolated directly into a run: shell block via ${{ }}. That expansion happens before the shell parses the line, and a branch name like $(cmd) is a valid git ref, so any member/owner/collaborator commenting /benchmark on a malicious PR would trigger arbitrary command execution on the trigger job's ubuntu-latest runner with its GITHUB_TOKEN (actions:write, checks:write).

  • Moved head_ref into an env: var and reference it as "$HEAD_REF" in the shell.
  • Did the same for head_sha, pr_number, and check_run_id in the same two run: blocks for consistency, since they're the same interpolation pattern even though their values aren't attacker-controlled today.

No behavior change — same commands, same values, just no ${{ }} inside run:.

paulinebm pushed a commit that referenced this pull request Sep 22, 2026
Co-authored-by: hf-security-analysis[bot] <265538906+hf-security-analysis[bot]@users.noreply.github.com>
steps.pr.outputs.head_ref (the PR branch name, attacker-controlled) was
interpolated directly into a run: shell block via ${{ }}, which expands
before the shell parses the line. A branch name like
$(cmd) is a valid git ref and would execute arbitrary commands on the
ubuntu-latest trigger runner with the job's GITHUB_TOKEN (actions:write,
checks:write).

Move all steps.*.outputs values used in these run blocks into env: and
reference them as shell variables instead, closing the same injection
pattern for head_sha, pr_number, and check_run_id for consistency.
Co-authored-by: hf-security-analysis[bot] <265538906+hf-security-analysis[bot]@users.noreply.github.com>
@assafvayner
assafvayner merged commit f6b6526 into main Sep 22, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants