Skip to content

🛡️ Sentinel: Fix argument injection in git commands #7

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

Draft
T-ahamed2 wants to merge 1 commit into
base: main
Choose a base branch
from
+20 −16
Draft

🛡️ Sentinel: Fix argument injection in git commands #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## 2025-06-25 - [Git Argument Injection]
**Vulnerability:** Argument injection (flag injection) in `git` commands when passing user-controlled strings (like branch names, refs, or repository URLs) as positional arguments.
**Learning:** Even when using `exec.Command` (which avoids shell injection), some commands like `git` can interpret positional arguments starting with `-` as flags, potentially leading to unauthorized configuration changes or other security risks.
**Prevention:** Use the `--` separator to explicitly delineate options from positional arguments in `git` commands (e.g., `git checkout <ref> --`). Always place `--` before any argument that could potentially start with a hyphen.
6 changes: 3 additions & 3 deletions pkg/cli/git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ func getCurrentBranch() (string, error) {
func createAndSwitchBranch(branchName string, verbose bool) error {
console.LogVerbose(verbose, "Creating and switching to branch: "+branchName)

cmd := exec.Command("git", "checkout", "-b", branchName)
cmd := exec.Command("git", "checkout", "-b", branchName, "--")

@gemini-code-assist gemini-code-assist Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

Placing -- after branchName does not prevent flag injection if branchName starts with a hyphen, as it is positioned before the separator. To ensure absolute safety against any parser quirks or malicious branch names, validate that branchName does not start with a hyphen.

Suggested change
cmd := exec.Command("git", "checkout", "-b", branchName, "--")
if strings.HasPrefix(branchName, "-") {
return fmt.Errorf("invalid branch name: %s", branchName)
}
cmd := exec.Command("git", "checkout", "-b", branchName)

if err := cmd.Run(); err != nil {
return fmt.Errorf("failed to create and switch to branch %s: %w", branchName, err)
}
Expand All @@ -477,7 +477,7 @@ func createAndSwitchBranch(branchName string, verbose bool) error {
func switchBranch(branchName string, verbose bool) error {
console.LogVerbose(verbose, "Switching to branch: "+branchName)

cmd := exec.Command("git", "checkout", branchName)
cmd := exec.Command("git", "checkout", branchName, "--")

@gemini-code-assist gemini-code-assist Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

Placing -- after branchName does not prevent flag injection if branchName starts with a hyphen (e.g., -f or --help), because git parses arguments sequentially and will interpret branchName as an option before reaching the -- separator. Since checking out refs/heads/branchName directly can result in a detached HEAD state, the safest approach is to validate that branchName does not start with a hyphen before executing the command.

Suggested change
cmd := exec.Command("git", "checkout", branchName, "--")
if strings.HasPrefix(branchName, "-") {
return fmt.Errorf("invalid branch name: %s", branchName)
}
cmd := exec.Command("git", "checkout", branchName)

if err := cmd.Run(); err != nil {
return fmt.Errorf("failed to switch to branch %s: %w", branchName, err)
}
Expand All @@ -503,7 +503,7 @@ func commitChanges(message string, verbose bool) error {
func pushBranch(branchName string, verbose bool) error {
console.LogVerbose(verbose, "Pushing branch: "+branchName)

cmd := exec.Command("git", "push", "-u", "origin", branchName)
cmd := exec.Command("git", "push", "-u", "origin", "--", branchName)

@gemini-code-assist gemini-code-assist Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-critical critical

Using -- in git push is invalid and will cause the command to fail. Git interprets -- as a refspec to push, resulting in an error like error: src refspec -- does not match any. To safely prevent flag injection for the branch name, you should prefix it with refs/heads/ instead of using --.

Suggested change
cmd := exec.Command("git", "push", "-u", "origin", "--", branchName)
cmd := exec.Command("git", "push", "-u", "origin", "refs/heads/"+branchName)

if err := cmd.Run(); err != nil {
return fmt.Errorf("failed to push branch %s: %w", branchName, err)
}
Expand Down
26 changes: 13 additions & 13 deletions pkg/parser/remote_fetch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,17 @@

tmpDir, err := os.MkdirTemp("", "gh-aw-list-*")
if err != nil {
return "", fmt.Errorf("failed to create temp directory: %w", err)

Check failure on line 73 in pkg/parser/remote_fetch.go

View workflow job for this annotation

GitHub Actions / error-message-lint 

avoid generic 'failed to ...: %w' wrapping; add specific recovery guidance

Check failure on line 73 in pkg/parser/remote_fetch.go

View workflow job for this annotation

GitHub Actions / error-message-lint 

error message uses negative language without constructive guidance; include expected/requires/should/example details
}

cloneCmd := exec.Command("git", "clone", "--depth", "1", "--branch", ref, "--single-branch", "--filter=blob:none", "--no-checkout", repoURL, tmpDir)
cloneCmd := exec.Command("git", "clone", "--depth", "1", "--branch", ref, "--single-branch", "--filter=blob:none", "--no-checkout", "--", repoURL, tmpDir)
cloneOutput, err := cloneCmd.CombinedOutput()
if err != nil {
if cleanupErr := os.RemoveAll(tmpDir); cleanupErr != nil {
remoteLog.Printf("Failed to clean up temp directory %q: %v", tmpDir, cleanupErr)
}
remoteLog.Printf("Failed to clone repository: %s", string(cloneOutput))
return "", fmt.Errorf("failed to clone repository for %s/%s@%s: %w", owner, repo, ref, err)

Check failure on line 83 in pkg/parser/remote_fetch.go

View workflow job for this annotation

GitHub Actions / error-message-lint 

avoid generic 'failed to ...: %w' wrapping; add specific recovery guidance

Check failure on line 83 in pkg/parser/remote_fetch.go

View workflow job for this annotation

GitHub Actions / error-message-lint 

error message uses negative language without constructive guidance; include expected/requires/should/example details
}

existingDir, found := func() (string, bool) {
Expand Down Expand Up @@ -261,7 +261,7 @@
if stripped, ok := strings.CutPrefix(filepath.ToSlash(filePath), "/"); ok {
if !strings.HasPrefix(stripped, constants.GithubDir) && !strings.HasPrefix(stripped, ".agents/") {
remoteLog.Printf("Security: Path not within .github or .agents: %s", filePath)
return "", fmt.Errorf("security: path %s must be within .github or .agents folder", filePath)

Check failure on line 264 in pkg/parser/remote_fetch.go

View workflow job for this annotation

GitHub Actions / error-message-lint 

error message uses negative language without constructive guidance; include expected/requires/should/example details
}
}
fullPath := filepath.Join(resolveBase, filePath)
Expand All @@ -271,7 +271,7 @@
if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) {
allowedFolder := filepath.Base(normalizedSecurityBase)
remoteLog.Printf("Security: Path escapes allowed folder: %s (resolves to: %s)", filePath, relativePath)
return "", fmt.Errorf("security: path %s must be within %s folder (resolves to: %s)", filePath, allowedFolder, relativePath)

Check failure on line 274 in pkg/parser/remote_fetch.go

View workflow job for this annotation

GitHub Actions / error-message-lint 

error message uses negative language without constructive guidance; include expected/requires/should/example details
}

if _, err := os.Stat(fullPath); os.IsNotExist(err) {
Expand Down Expand Up @@ -359,7 +359,7 @@
remoteLog.Printf("Fetching file from GitHub: %s/%s/%s@%s", owner, repo, filePath, ref)
content, err := downloadFileFromGitHub(owner, repo, filePath, ref)
if err != nil {
return "", fmt.Errorf("failed to download include from %s: %w", spec, err)

Check failure on line 362 in pkg/parser/remote_fetch.go

View workflow job for this annotation

GitHub Actions / error-message-lint 

avoid generic 'failed to ...: %w' wrapping; add specific recovery guidance

Check failure on line 362 in pkg/parser/remote_fetch.go

View workflow job for this annotation

GitHub Actions / error-message-lint 

error message uses negative language without constructive guidance; include expected/requires/should/example details
}
remoteLog.Printf("Successfully downloaded file: size=%d bytes", len(content))

Expand Down Expand Up @@ -458,12 +458,12 @@

// Try to resolve the ref using git ls-remote
// Format: git ls-remote <repo> <ref>
cmd := exec.Command("git", "ls-remote", repoURL, ref)
cmd := exec.Command("git", "ls-remote", "--", repoURL, ref)
output, err := cmd.Output()
if err != nil {
// If exact ref doesn't work, try with refs/heads/ and refs/tags/ prefixes
for _, prefix := range []string{"refs/heads/", "refs/tags/"} {
cmd = exec.Command("git", "ls-remote", repoURL, prefix+ref)
cmd = exec.Command("git", "ls-remote", "--", repoURL, prefix+ref)
output, err = cmd.Output()
if err == nil && len(output) > 0 {
break
Expand Down Expand Up @@ -625,7 +625,7 @@
// git archive command: git archive --remote=<repo> <ref> <path>
// #nosec G204 -- repoURL, ref, and path are from workflow import configuration authored by the
// developer; exec.Command with separate args (not shell execution) prevents shell injection.
cmd := exec.Command("git", "archive", "--remote="+repoURL, ref, path)
cmd := exec.Command("git", "archive", "--remote="+repoURL, ref, "--", path)

@gemini-code-assist gemini-code-assist Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-medium medium

In git archive, placing -- after ref does not protect ref from being interpreted as a flag if it starts with a hyphen, because it is positioned before the separator. Since ref is user-controlled, you should validate that ref does not start with a hyphen to prevent potential flag injection.

	if strings.HasPrefix(ref, "-") {
		return nil, fmt.Errorf("invalid ref: %s", ref)
	}
	cmd := exec.Command("git", "archive", "--remote="+repoURL, ref, "--", path)

archiveOutput, err := cmd.Output()
if err != nil {
// If git archive fails, try with git clone + git show as a fallback
Expand Down Expand Up @@ -703,24 +703,24 @@
if isSHA {
// For SHA refs, we need to clone without --branch and then checkout the specific commit
// Clone with minimal depth and no branch specified
cloneCmd = exec.Command("git", "clone", "--depth", "1", "--no-single-branch", repoURL, tmpDir)
cloneCmd = exec.Command("git", "clone", "--depth", "1", "--no-single-branch", "--", repoURL, tmpDir)
if output, err := cloneCmd.CombinedOutput(); err != nil {
// Try without --no-single-branch if the first attempt fails
remoteLog.Printf("Clone with --no-single-branch failed, trying full clone: %s", string(output))
cloneCmd = exec.Command("git", "clone", repoURL, tmpDir)
cloneCmd = exec.Command("git", "clone", "--", repoURL, tmpDir)
if output, err := cloneCmd.CombinedOutput(); err != nil {
return nil, fmt.Errorf("failed to clone repository: %w\nOutput: %s", err, string(output))
}
}

// Now checkout the specific commit
checkoutCmd := exec.Command("git", "-C", tmpDir, "checkout", ref)
checkoutCmd := exec.Command("git", "-C", tmpDir, "checkout", ref, "--")

@gemini-code-assist gemini-code-assist Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-medium medium

Placing -- after ref in git checkout does not prevent flag injection if ref starts with a hyphen. Since ref is user-controlled, you should validate that ref does not start with a hyphen to ensure security.

Suggested change
checkoutCmd := exec.Command("git", "-C", tmpDir, "checkout", ref, "--")
if strings.HasPrefix(ref, "-") {
return nil, fmt.Errorf("invalid ref: %s", ref)
}
checkoutCmd := exec.Command("git", "-C", tmpDir, "checkout", ref)

if output, err := checkoutCmd.CombinedOutput(); err != nil {
return nil, fmt.Errorf("failed to checkout commit %s: %w\nOutput: %s", ref, err, string(output))
}
} else {
// For branch/tag refs, use --branch flag
cloneCmd = exec.Command("git", "clone", "--depth", "1", "--branch", ref, repoURL, tmpDir)
cloneCmd = exec.Command("git", "clone", "--depth", "1", "--branch", ref, "--", repoURL, tmpDir)
if output, err := cloneCmd.CombinedOutput(); err != nil {
return nil, fmt.Errorf("failed to clone repository: %w\nOutput: %s", err, string(output))
}
Expand Down Expand Up @@ -1144,7 +1144,7 @@
return nil, err
}

lsTreeCmd := exec.Command("git", "-C", tmpDir, "ls-tree", "-r", "--name-only", "HEAD", dirPath+"/")
lsTreeCmd := exec.Command("git", "-C", tmpDir, "ls-tree", "-r", "--name-only", "HEAD", "--", dirPath+"/")
lsTreeOutput, err := lsTreeCmd.CombinedOutput()
if err != nil {
remoteLog.Printf("Failed to list dir files: %s", string(lsTreeOutput))
Expand Down Expand Up @@ -1283,7 +1283,7 @@

// Normalise dirPath so it never has a trailing slash before we append one.
cleanDirPath := strings.TrimRight(dirPath, "/")
lsTreeCmd := exec.Command("git", "-C", tmpDir, "ls-tree", "-r", "--name-only", "HEAD", cleanDirPath+"/")
lsTreeCmd := exec.Command("git", "-C", tmpDir, "ls-tree", "-r", "--name-only", "HEAD", "--", cleanDirPath+"/")
lsTreeOutput, err := lsTreeCmd.CombinedOutput()
if err != nil {
remoteLog.Printf("Failed to list dir files recursively: %s", string(lsTreeOutput))
Expand Down Expand Up @@ -1404,7 +1404,7 @@
}

// Use ls-tree -d to list only direct subdirectory entries.
lsTreeDirsCmd := exec.Command("git", "-C", tmpDir, "ls-tree", "--name-only", "-d", "HEAD", dirPath+"/")
lsTreeDirsCmd := exec.Command("git", "-C", tmpDir, "ls-tree", "--name-only", "-d", "HEAD", "--", dirPath+"/")
lsTreeDirsOutput, err := lsTreeDirsCmd.CombinedOutput()
if err != nil {
remoteLog.Printf("Failed to list tree subdirs: %s", string(lsTreeDirsOutput))
Expand Down Expand Up @@ -1475,15 +1475,15 @@

// Do a minimal clone using filter=blob:none for faster cloning (metadata only, no blobs)
// Use --depth=1 for shallow clone and --no-checkout to skip checkout initially
cloneCmd := exec.Command("git", "clone", "--depth", "1", "--branch", ref, "--single-branch", "--filter=blob:none", "--no-checkout", repoURL, tmpDir)
cloneCmd := exec.Command("git", "clone", "--depth", "1", "--branch", ref, "--single-branch", "--filter=blob:none", "--no-checkout", "--", repoURL, tmpDir)
cloneOutput, err := cloneCmd.CombinedOutput()
if err != nil {
remoteLog.Printf("Failed to clone repository: %s", string(cloneOutput))
return nil, fmt.Errorf("failed to clone repository for %s/%s@%s: %w", owner, repo, ref, err)
}

// Use git ls-tree to list files in the specified workflows directory
lsTreeCmd := exec.Command("git", "-C", tmpDir, "ls-tree", "-r", "--name-only", "HEAD", workflowPath+"/")
lsTreeCmd := exec.Command("git", "-C", tmpDir, "ls-tree", "-r", "--name-only", "HEAD", "--", workflowPath+"/")
lsTreeOutput, err := lsTreeCmd.CombinedOutput()
if err != nil {
remoteLog.Printf("Failed to list files: %s", string(lsTreeOutput))
Expand Down
Loading