Skip to content

Guard git command arguments against flag injection (Sighthound findings) #52401

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
Copilot wants to merge 3 commits into
base: main
Choose a base branch
from
+41 −0
Open

Guard git command arguments against flag injection (Sighthound findings) #52401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a225320
Initial plan
Copilot Aug 13, 2026
db5c06d
Guard git command args against flag injection (Sighthound findings)
Copilot Aug 13, 2026
8562188
Address git revision review feedback
Copilot Aug 13, 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
7 changes: 7 additions & 0 deletions pkg/cli/experiments_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,10 @@ func loadLocalMetricEvalResults(workflowID string) map[string]MetricEvalResults
}
ref = branchName
}
if !isSafeGitRevisionArg(ref) {

@github-actions github-actions Bot Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] loadLocalMetricEvalResults returns nil on an unsafe ref, but no test covers this path. A regression could re-open the injection vector silently.

💡 Suggested test skeleton 
func TestLoadLocalMetricEvalResultsRejectsUnsafeRef(t *testing.T) {
    // A leading-dash ref must not reach exec.Command
    result := loadLocalMetricEvalResults("--evil")
    assert.Nil(t, result, "expected nil for unsafe ref")
}

Combine with a spy/mock on exec.Command to assert the command is never invoked.

@copilot please address this.

Copilot AI Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No caller-path test added: workflowID is always prefixed by WorkflowStateBranchName and then origin/, so loadLocalMetricEvalResults("--evil") cannot produce a dash-leading Git argument or exercise this guard. The security invariant itself remains directly covered by TestIsSafeGitRevisionArg.

experimentsLog.Printf("Rejecting unsafe git ref: %q", ref)
return nil
}
cmd := exec.Command("git", "show", ref+":"+constants.EvalsResultFilename)
out, err := cmd.Output()
if err != nil {
Expand Down Expand Up @@ -907,6 +911,9 @@ func extractExperimentName(ref string) string {

// gitRefExists reports whether a git ref exists locally.
func gitRefExists(ref string) bool {
if !isSafeGitRevisionArg(ref) {
return false
}
cmd := exec.Command("git", "rev-parse", "--verify", ref)
return cmd.Run() == nil
}
Expand Down
12 changes: 12 additions & 0 deletions pkg/cli/git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ import (

var gitLog = logger.New("cli:git")

// isSafeGitRevisionArg reports whether ref cannot be misinterpreted as a git
// CLI flag by rejecting empty strings and values starting with "-". It does
// not validate that ref is a well-formed git revision.
func isSafeGitRevisionArg(ref string) bool {

@github-actions github-actions Bot Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] The function name and comment say "safe git revision arg" but the implementation only blocks leading-dash flag injection — it does not validate rev syntax. This over-broad name may invite callers to rely on a guarantee it doesn't provide.

💡 Suggestion

Narrow the doc comment to the actual invariant:

// isSafeGitRevisionArg reports whether ref cannot be misinterpreted as a
// git CLI flag (non-empty, no leading dash). It does NOT validate that
// ref is a well-formed git revision.

This prevents future callers from treating it as a full validation gate.

@copilot please address this.

Copilot AI Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the helper comment in 8562188 to state the exact invariant: it rejects empty and dash-prefixed values to prevent flag interpretation, but does not validate Git revision syntax.

return ref != "" && !strings.HasPrefix(ref, "-")
}

func isGitRepo() bool {
_, err := gitutil.FindGitRoot()
return err == nil
Expand Down Expand Up @@ -689,6 +696,11 @@ func checkWorkflowFileStatus(workflowPath string) (*WorkflowFileStatus, error) {
upstream := strings.TrimSpace(string(output))
gitLog.Printf("Upstream branch: %s", upstream)

if !isSafeGitRevisionArg(upstream) {
gitLog.Printf("Rejecting unsafe upstream ref: %q", upstream)
return status, fmt.Errorf("unexpected upstream ref %q", upstream)
}

@github-actions github-actions Bot Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] On an unsafe upstream ref the function silently returns the current status with no error. Callers that rely on the returned status to decide whether to alert the user will silently skip the diff check — potentially hiding real workflow drift.

💡 Suggestion

Return a wrapped error so the caller can decide how to surface it:

if !isSafeGitRevisionArg(upstream) {
    return status, fmt.Errorf("unexpected upstream ref %q", upstream)
}

The caller can then log a warning and fall through gracefully, rather than silently losing the check.

@copilot please address this.

Copilot AI Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated checkWorkflowFileStatus in 8562188 to return an explicit unexpected upstream ref error instead of silently returning the partial status.


// Check if there are commits in the current branch that affect this file and aren't in upstream
cmd = exec.Command("git", "-C", gitRoot, "log", upstream+"..HEAD", "--oneline", "--", relPath)
output, err = cmd.Output()
Expand Down
22 changes: 22 additions & 0 deletions pkg/cli/git_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,28 @@ import (
// - TestStageWorkflowChanges (tests staging behavior during workflow compilation)
// - TestStageGitAttributesIfChanged (tests conditional staging during compilation)

func TestIsSafeGitRevisionArg(t *testing.T) {
tests := []struct {
name string
ref string
want bool
}{
{"empty", "", false},
{"leading dash", "-oops", false},
{"leading double dash", "--upload-pack=evil", false},
{"plain branch", "main", true},
{"remote branch", "origin/main", true},
{"contains dash not leading", "feature-branch", true},
{"short sha", "abc1234", true},
{"fully qualified ref", "refs/heads/main", true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.want, isSafeGitRevisionArg(tt.ref))
})
}
}

func TestGetCurrentBranch(t *testing.T) {
tmpDir := testutil.TempDir(t, "test-*")

Expand Down
Loading