Skip to content

Commit 85d757b

Browse files
Copilotpelikhan
andauthored
fix: argument injection in git archive fallback (CWE-88) (#49500)
* Initial plan * Plan: fix argument injection in downloadWorkflowContentViaGit Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * fix: argument injection in downloadWorkflowContentViaGit (CWE-88) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Co-authored-by: Peli de Halleux <pelikhan@users.noreply.github.com>
1 parent 4a25e8e commit 85d757b

2 files changed

Lines changed: 78 additions & 2 deletions

File tree

pkg/cli/download_workflow.go

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,25 @@ func downloadWorkflowContentViaGit(ctx context.Context, repo, path, ref string,
2828

2929
downloadLog.Printf("Attempting git fallback for downloading workflow content: %s/%s@%s", repo, path, ref)
3030

31+
if err := gitutil.ValidateGitRef(ref); err != nil {
32+
return nil, fmt.Errorf("refusing git fallback: %w", err)
33+
}
34+
if err := gitutil.ValidateGitPath(path); err != nil {
35+
return nil, fmt.Errorf("refusing git fallback: %w", err)
36+
}
37+
3138
// Use git archive to get the file content without cloning
3239
githubHost := getGitHubHostForRepo(repo)
3340
repoURL := fmt.Sprintf("%s/%s.git", githubHost, repo)
3441

35-
// git archive command: git archive --remote=<repo> <ref> <path>
42+
// git archive command: git archive --remote=<repo> <ref> -- <path>
43+
// The '--' end-of-options separator ensures path is never parsed as a git flag
44+
// even if it begins with '-' (argument injection, CWE-88).
45+
// ValidateGitRef/ValidateGitPath above guard against leading '-' and '..' at
46+
// this layer; '--' is kept as defence-in-depth per the git(1) specification.
3647
// #nosec G204 -- repoURL, ref, and path are from workflow import configuration authored by the
3748
// developer; exec.CommandContext with separate args (not shell execution) prevents shell injection.
38-
cmd := exec.CommandContext(ctx, "git", "archive", "--remote="+repoURL, ref, path)
49+
cmd := exec.CommandContext(ctx, "git", "archive", "--remote="+repoURL, ref, "--", path)
3950
archiveOutput, err := cmd.Output()
4051
if err != nil {
4152
downloadLog.Printf("git archive failed, falling back to git clone: repo=%s, ref=%s, err=%v", repo, ref, err)

pkg/cli/download_workflow_test.go

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package cli
44

55
import (
6+
"context"
67
"encoding/base64"
78
"strings"
89
"testing"
@@ -75,3 +76,67 @@ func TestDecodeBase64FileContent(t *testing.T) {
7576
})
7677
}
7778
}
79+
80+
// TestDownloadWorkflowContentViaGitValidation ensures that downloadWorkflowContentViaGit
81+
// rejects dangerous ref/path values before spawning any git subprocess (CWE-88).
82+
func TestDownloadWorkflowContentViaGitValidation(t *testing.T) {
83+
tests := []struct {
84+
name string
85+
repo string
86+
path string
87+
ref string
88+
errContains string
89+
}{
90+
{
91+
name: "path starting with dash is rejected",
92+
repo: "owner/repo",
93+
path: "--output=/tmp/evil",
94+
ref: "abc123",
95+
errContains: "refusing git fallback",
96+
},
97+
{
98+
name: "ref starting with dash is rejected",
99+
repo: "owner/repo",
100+
path: "workflow.md",
101+
ref: "--upload-pack=evil",
102+
errContains: "refusing git fallback",
103+
},
104+
{
105+
name: "path with traversal is rejected",
106+
repo: "owner/repo",
107+
path: "../../../etc/passwd",
108+
ref: "abc123",
109+
errContains: "refusing git fallback",
110+
},
111+
{
112+
name: "ref with dotdot is rejected",
113+
repo: "owner/repo",
114+
path: "workflow.md",
115+
ref: "main..evil",
116+
errContains: "refusing git fallback",
117+
},
118+
{
119+
name: "empty ref is rejected",
120+
repo: "owner/repo",
121+
path: "workflow.md",
122+
ref: "",
123+
errContains: "refusing git fallback",
124+
},
125+
{
126+
name: "empty path is rejected",
127+
repo: "owner/repo",
128+
path: "",
129+
ref: "abc123",
130+
errContains: "refusing git fallback",
131+
},
132+
}
133+
134+
ctx := context.Background()
135+
for _, tt := range tests {
136+
t.Run(tt.name, func(t *testing.T) {
137+
_, err := downloadWorkflowContentViaGit(ctx, tt.repo, tt.path, tt.ref, false)
138+
require.Error(t, err, "expected validation error for invalid input")
139+
assert.Contains(t, err.Error(), tt.errContains)
140+
})
141+
}
142+
}

0 commit comments

Comments
 (0)