🔒 fix command injection in prepare-release.py#3717
🔒 fix command injection in prepare-release.py#3717jkowall wants to merge 3 commits intojaegertracing:mainfrom
Conversation
Refactor `run_command` in `scripts/prepare-release.py` to use `shell=False` and pass arguments as a list to `subprocess.run`. This prevents command injection vulnerabilities where shell characters in variables like commit messages or branch names could be interpreted by the shell. All call sites of `run_command` have been updated accordingly. Co-authored-by: jkowall <1859948+jkowall@users.noreply.github.com>
Signed-off-by: Jonah Kowall <jkowall@kowall.net>
…-prepare-release-15644608474505552789 # Conflicts: # scripts/prepare-release.py
Acknowledged that this PR is being closed in favor of jaegertracing#3717. Co-authored-by: jkowall <1859948+jkowall@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3717 +/- ##
=======================================
Coverage 89.21% 89.21%
=======================================
Files 330 330
Lines 9889 9889
Branches 2568 2568
=======================================
Hits 8822 8822
Misses 925 925
Partials 142 142 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens scripts/prepare-release.py against command injection by removing shell-based command execution and tightening how commands are constructed/executed.
Changes:
- Replaced
subprocess.run(..., shell=True)string commands withshell=Falseargument lists throughout. - Added
run_command()validation to require a sequence-of-args input to prevent regressions. - Narrowed exception handling around subprocess calls and introduced a
sensitiveflag to suppress stdout/stderr echoing on failures.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return run_command(["gh", "auth", "token"], sensitive=True) | ||
| except (subprocess.CalledProcessError, FileNotFoundError): | ||
| print("Error: Could not retrieve GitHub token using 'gh auth token'. Please login via 'gh auth login'.") |
There was a problem hiding this comment.
get_gh_token() still executes gh auth token and captures the token into Python memory (and returns it) even though the script no longer uses the value. This increases the blast radius for accidental secret exposure (e.g., future logging/tracebacks) and is unnecessary if the goal is only to validate auth. Consider replacing this with a non-secret-bearing check like gh auth status (or a minimal gh api user call), or adjust run_command()/get_gh_token() so sensitive commands don’t return/capture the secret unless explicitly needed.
| return run_command(["gh", "auth", "token"], sensitive=True) | |
| except (subprocess.CalledProcessError, FileNotFoundError): | |
| print("Error: Could not retrieve GitHub token using 'gh auth token'. Please login via 'gh auth login'.") | |
| run_command( | |
| ["gh", "auth", "status"], | |
| capture_stdout=False, | |
| capture_stderr=False, | |
| sensitive=True | |
| ) | |
| return "" | |
| except (subprocess.CalledProcessError, FileNotFoundError): | |
| print("Error: GitHub CLI is not authenticated. Please login via 'gh auth login'.") |
| # check_git_status() # Optional: strict check, but might be annoying in dev. Uncomment if needed. | ||
| validate_version(version) | ||
| token = get_gh_token() | ||
| get_gh_token() |
There was a problem hiding this comment.
The return value of get_gh_token() is ignored here, but the call still retrieves (and internally captures) a GitHub token. If this is intended only as an auth preflight, prefer a tokenless command (e.g., gh auth status) to avoid handling secrets unnecessarily; otherwise, use the returned token for something concrete so it’s clear why it’s being fetched.
| get_gh_token() | |
| run_command(["gh", "auth", "status"], capture_stdout=False) |
Summary
Fixes a command injection vulnerability in
scripts/prepare-release.py.Reference: earlier fork PR jkowall/jaeger-ui-F#24 is now closed in favor of this upstream draft.
What Changed
subprocess.run()usage fromshell=Truestring commands toshell=Falseargument listsrun_command()to prevent future regressionsexcept:handlers with targeted subprocess exceptionsgh auth tokencallmainby keeping the newrun_fmt()helper while preserving the security hardeningWhy
The previous implementation built shell commands from interpolated strings, which allowed malicious input such as a crafted version string or commit message to trigger arbitrary command execution.
Validation
python3 -m py_compile scripts/prepare-release.pynpm run lintcould not complete in this environment after mergingmainbecause the branch now expectsvpnpm testcould not complete in this environment becausevitestis not installednpm run buildcould not complete in this environment because mergedmaincurrently lacks required Zustand/Vitest dependencies here