Skip to content

🛡️ Sentinel: [CRITICAL] Fix command injection risks #569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MasumRab wants to merge 6 commits into
base: main
Choose a base branch
from
Open

🛡️ Sentinel: [CRITICAL] Fix command injection risks #569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3934ec8
🛡️ Sentinel: [CRITICAL] Fix command injection risks
google-labs-jules[bot] Mar 28, 2026
8d55e66
🛡️ Sentinel: [CRITICAL] Fix syntax errors and broken files
google-labs-jules[bot] Mar 28, 2026
ea78d1f
🛡️ Sentinel: [CRITICAL] Fix command injection security hotspot warnings
google-labs-jules[bot] Mar 28, 2026
ec155e5
🛡️ Sentinel: [CRITICAL] Fix lint errors causing CI checks to fail
google-labs-jules[bot] Mar 28, 2026
13a70ac
🛡️ Sentinel: [CRITICAL] Fix lint errors causing CI checks to fail
google-labs-jules[bot] Mar 28, 2026
065edc7
🛡️ Sentinel: [CRITICAL] Fix lint errors causing CI checks to fail
google-labs-jules[bot] Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions scripts/branch_rename_migration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
def run_command(command):
"""Run a shell command and return the output."""
try:
result = subprocess.run(command, shell=True, capture_output=True, text=True, check=True)
result = subprocess.run(command, capture_output=True, text=True, check=True)
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Normalize and log commands consistently now that shell=True is removed

With shell=True removed and most callers now passing a list, command will often be a sequence, so the error message will log a Python list repr, which is noisy and harder to reuse. Consider normalizing command to List[str] and logging ' '.join(command), and tightening the type of command to discourage string inputs that rely on shell parsing.

Suggested implementation:

from collections.abc import Sequence


def run_command(command: Sequence[str]) -> str | None:
    """Run a shell command and return the output.

    Args:
        command: The command to run as a sequence of arguments, e.g. ["git", "status"].
    """
    if isinstance(command, str):
        raise TypeError(
            "run_command expects 'command' as a sequence of strings, "
            "not a single shell-parsed string. Use ['git', 'status'] "
            "instead of 'git status'."
        )

    try:
        result = subprocess.run(command, capture_output=True, text=True, check=True)
        return result.stdout.strip()
    except subprocess.CalledProcessError as e:
        normalized = " ".join(command)
        # stderr may contain additional diagnostics from the subprocess
        stderr_msg = e.stderr.strip() if e.stderr else ""
        if stderr_msg:
            print(f"Error running command: {normalized}\n{stderr_msg}")
        else:
            print(f"Error running command: {normalized}")
        return None
def get_local_branches() -> list[str]:
    """Get list of local branches."""
    output = run_command(["git", "branch"])
    if output is None:
        return []

If this file (or project) targets Python versions earlier than 3.10, you should replace the str | None and list[str] annotations with Optional[str] and List[str] and import them from typing. Also ensure there isn’t already an import named Sequence from a different module to avoid conflicts with the newly added from collections.abc import Sequence.

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

Source: opengrep

return result.stdout.strip()
except subprocess.CalledProcessError as e:
print(f"Error running command: {command}")
Expand All @@ -26,7 +26,7 @@ def run_command(command):

def get_local_branches():
"""Get list of local branches."""
output = run_command("git branch")
output = run_command(["git", "branch"])
if output is None:
return []

Expand All @@ -39,7 +39,7 @@ def get_local_branches():

def get_remote_branches():
"""Get list of remote branches."""
output = run_command("git branch -r")
output = run_command(["git", "branch", "-r"])
if output is None:
return []

Expand Down Expand Up @@ -140,19 +140,19 @@ def suggest_new_name(branch_name):
def rename_local_branch(old_name, new_name):
"""Rename a local branch."""
print(f"Renaming local branch '{old_name}' to '{new_name}'")
result = run_command(f"git branch -m {old_name} {new_name}")
result = run_command(["git", "branch", "-m", old_name, new_name])
return result is not None

def delete_remote_branch(branch_name):
"""Delete a remote branch."""
print(f"Deleting remote branch '{branch_name}'")
result = run_command(f"git push origin --delete {branch_name.replace('origin/', '')}")
result = run_command(["git", "push", "origin", "--delete", branch_name.replace('origin/', '')])
return result is not None

def push_new_branch(branch_name):
"""Push a new branch to remote."""
print(f"Pushing new branch '{branch_name}'")
result = run_command(f"git push -u origin {branch_name}")
result = run_command(["git", "push", "-u", "origin", branch_name])
return result is not None

def main():
Expand Down Expand Up @@ -214,7 +214,7 @@ def main():
# Delete local branches marked for deletion
for branch in branches_to_delete:
print(f"Deleting local branch '{branch}'")
result = run_command(f"git branch -d {branch}")
result = run_command(["git", "branch", "-d", branch])
if result is not None:
print(f"✓ Deleted local branch '{branch}'")
else:
Expand Down
3 changes: 2 additions & 1 deletion scripts/sync_common_docs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ def check_worktree_status(self):
self.print_status("Checking worktree status")

print("\nWorktrees found:")
os.system("git worktree list")
import subprocess
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While this correctly fixes the security vulnerability, for better code style and adherence to PEP 8 guidelines, imports should be placed at the top of the file. Please move import subprocess to the top-level imports section. This improves code readability and maintainability by making all dependencies explicit in one place.

References
  1. According to PEP 8, imports should be at the top of the file, just after any module comments and docstrings. This improves readability and makes it easier to see what modules a file depends on. (link)

subprocess.run(["git", "worktree", "list"])

print("\nWorktree documentation status:")

Expand Down
Loading
Loading