From f6c3f6da7168101f2c1898fd8cea5960ceda9f39 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 08:06:18 +0000 Subject: [PATCH 1/5] Initial plan From 0bf9b4ddf69f3431a241dcc9dd5269729ab3c19d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 08:24:36 +0000 Subject: [PATCH 2/5] fix: prevent git argument injection via ref/path validation and -- separators (VULN-001) - Add ValidateGitRef and ValidateGitPath helpers to pkg/gitutil that reject refs/paths starting with '-' (argument injection CWE-88) and refs containing '..' (git traversal expressions) - Validate ref at parse time in parseWorkflowSpecParts and parseRemoteOrigin so malicious workflowspec imports are rejected before any git subprocess runs - Add '--' end-of-options separator to all git subprocess calls that take user-derived positional arguments: - git archive --remote= -- (remote_download_file.go) - git checkout -- (remote_download_file.go) - git ls-remote -- (remote_resolve_sha.go) - Add early ref/path validation guards in downloadFileViaGit and downloadFileViaGitClone fallback paths - Add unit tests for ValidateGitRef and ValidateGitPath covering valid cases, empty inputs, leading-dash injection, and dotdot traversal Fixes #47940 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/gitutil/gitutil.go | 30 +++++++ pkg/gitutil/gitutil_test.go | 122 +++++++++++++++++++++++++++++ pkg/parser/import_remote.go | 7 ++ pkg/parser/remote_download_file.go | 26 ++++-- pkg/parser/remote_resolve_sha.go | 13 ++- pkg/parser/remote_workflow_spec.go | 19 ++++- 6 files changed, 207 insertions(+), 10 deletions(-) diff --git a/pkg/gitutil/gitutil.go b/pkg/gitutil/gitutil.go index b391d976d1d..8e9d15ca707 100644 --- a/pkg/gitutil/gitutil.go +++ b/pkg/gitutil/gitutil.go @@ -67,6 +67,36 @@ func IsValidFullSHA(s string) bool { return fullSHARegex.MatchString(s) } +// ValidateGitRef returns an error if ref would be unsafe to pass as a positional +// argument to a git subprocess. A ref starting with '-' would be parsed as an +// option flag rather than a value (argument injection, CWE-88). Refs containing +// '..' can trigger git object traversal expressions. +func ValidateGitRef(ref string) error { + if ref == "" { + return errors.New("git ref must not be empty") + } + if strings.HasPrefix(ref, "-") { + return fmt.Errorf("invalid git ref %q: refs must not start with '-' to prevent argument injection", ref) + } + if strings.Contains(ref, "..") { + return fmt.Errorf("invalid git ref %q: refs must not contain '..'", ref) + } + return nil +} + +// ValidateGitPath returns an error if path would be unsafe to pass as a positional +// argument to a git subprocess. A path starting with '-' would be parsed as an +// option flag rather than a value (argument injection, CWE-88). +func ValidateGitPath(path string) error { + if path == "" { + return errors.New("git path must not be empty") + } + if strings.HasPrefix(path, "-") { + return fmt.Errorf("invalid git path %q: paths must not start with '-' to prevent argument injection", path) + } + return nil +} + // ExtractBaseRepo extracts the base repository (owner/repo) from a repository path // that may include subfolders. // For "actions/checkout" -> "actions/checkout" diff --git a/pkg/gitutil/gitutil_test.go b/pkg/gitutil/gitutil_test.go index 5bd2e81b28a..e71656c428e 100644 --- a/pkg/gitutil/gitutil_test.go +++ b/pkg/gitutil/gitutil_test.go @@ -440,3 +440,125 @@ func TestReadFileFromHEAD(t *testing.T) { require.ErrorContains(t, err, "gitRoot must not be empty", "error should mention empty gitRoot") }) } + +func TestValidateGitRef(t *testing.T) { + tests := []struct { + name string + ref string + expectError bool + errContains string + }{ + { + name: "valid branch name", + ref: "main", + expectError: false, + }, + { + name: "valid tag name", + ref: "v1.2.3", + expectError: false, + }, + { + name: "valid SHA", + ref: "abcdef0123456789abcdef0123456789abcdef01", + expectError: false, + }, + { + name: "valid branch with slash", + ref: "feature/my-feature", + expectError: false, + }, + { + name: "empty ref is rejected", + ref: "", + expectError: true, + errContains: "must not be empty", + }, + { + name: "leading dash is rejected (argument injection)", + ref: "-evil", + expectError: true, + errContains: "must not start with '-'", + }, + { + name: "double dash is rejected (argument injection)", + ref: "--upload-pack=malicious", + expectError: true, + errContains: "must not start with '-'", + }, + { + name: "dotdot is rejected (git traversal)", + ref: "main..evil", + expectError: true, + errContains: "must not contain '..'", + }, + { + name: "dotdot prefix is rejected", + ref: "..evil", + expectError: true, + errContains: "must not contain '..'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateGitRef(tt.ref) + if tt.expectError { + require.Error(t, err, "expected error for ref %q", tt.ref) + assert.Contains(t, err.Error(), tt.errContains) + } else { + require.NoError(t, err, "unexpected error for ref %q", tt.ref) + } + }) + } +} + +func TestValidateGitPath(t *testing.T) { + tests := []struct { + name string + path string + expectError bool + errContains string + }{ + { + name: "valid file path", + path: ".github/workflows/workflow.md", + expectError: false, + }, + { + name: "valid simple filename", + path: "file.md", + expectError: false, + }, + { + name: "empty path is rejected", + path: "", + expectError: true, + errContains: "must not be empty", + }, + { + name: "leading dash is rejected (argument injection)", + path: "-evil", + expectError: true, + errContains: "must not start with '-'", + }, + { + name: "leading double dash is rejected", + path: "--output=/etc/passwd", + expectError: true, + errContains: "must not start with '-'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateGitPath(tt.path) + if tt.expectError { + require.Error(t, err, "expected error for path %q", tt.path) + assert.Contains(t, err.Error(), tt.errContains) + } else { + require.NoError(t, err, "unexpected error for path %q", tt.path) + } + }) + } +} diff --git a/pkg/parser/import_remote.go b/pkg/parser/import_remote.go index e6f9f890a74..b1eaae8ebd4 100644 --- a/pkg/parser/import_remote.go +++ b/pkg/parser/import_remote.go @@ -7,6 +7,7 @@ import ( "path" "strings" + "github.com/github/gh-aw/pkg/gitutil" "github.com/github/gh-aw/pkg/logger" ) @@ -54,6 +55,12 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin { ref = parts[1] } + // Reject refs that would be unsafe to pass to git subprocesses. + if err := gitutil.ValidateGitRef(ref); err != nil { + importRemoteLog.Printf("Rejecting spec %q: invalid ref: %v", spec, err) + return nil + } + // Parse path: owner/repo/path/to/file.md slashParts := strings.Split(pathPart, "/") if len(slashParts) < 3 { diff --git a/pkg/parser/remote_download_file.go b/pkg/parser/remote_download_file.go index 585f17bf2d7..3aaf6a955be 100644 --- a/pkg/parser/remote_download_file.go +++ b/pkg/parser/remote_download_file.go @@ -293,6 +293,13 @@ func resolveAndValidateRemoteSymlinkBase(parentDir, target, dirPath string) (str func downloadFileViaGit(ctx context.Context, owner, repo, path, ref, host string) ([]byte, error) { remoteLog.Printf("Attempting git fallback for %s/%s/%s@%s", owner, repo, path, ref) + if err := gitutil.ValidateGitRef(ref); err != nil { + return nil, fmt.Errorf("refusing git fallback: %w", err) + } + if err := gitutil.ValidateGitPath(path); err != nil { + return nil, fmt.Errorf("refusing git fallback: %w", err) + } + // First, try via raw.githubusercontent.com — no auth required for public repos and // no dependency on git being installed. // Only attempt raw URL for github.com repos (not GHE) since raw.githubusercontent.com @@ -315,10 +322,14 @@ func downloadFileViaGit(ctx context.Context, owner, repo, path, ref, host string } repoURL := fmt.Sprintf("%s/%s/%s.git", githubHost, owner, repo) - // git archive command: git archive --remote= + // git archive command: git archive --remote= -- + // The '--' end-of-options separator ensures ref and path are never parsed as + // git flags even if they begin with '-' (argument injection, CWE-88). + // ValidateGitRef/ValidateGitPath above guard against leading '-' and '..' at + // this layer; '--' is kept as defence-in-depth per the git(1) specification. // #nosec G204 -- repoURL, ref, and path are from workflow import configuration authored by the // developer; exec.CommandContext with separate args (not shell execution) prevents shell injection. - cmd := exec.CommandContext(ctx, "git", "archive", "--remote="+repoURL, ref, path) + cmd := exec.CommandContext(ctx, "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 @@ -372,6 +383,10 @@ func downloadFileViaRawURL(ctx context.Context, owner, repo, filePath, ref strin func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host string) ([]byte, error) { remoteLog.Printf("Attempting git clone fallback for %s/%s/%s@%s", owner, repo, path, ref) + if err := gitutil.ValidateGitRef(ref); err != nil { + return nil, fmt.Errorf("refusing git clone fallback: %w", err) + } + // Create a temporary directory for the shallow clone tmpDir, err := os.MkdirTemp("", "gh-aw-git-clone-*") if err != nil { @@ -404,13 +419,14 @@ func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host s } } - // Now checkout the specific commit - checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", ref) + // Now checkout the specific commit; '--' prevents ref from being parsed as a flag. + checkoutCmd := exec.CommandContext(ctx, "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 + // For branch/tag refs, use --branch flag; the value is passed via a separate + // argument slot and cannot be confused with a flag because it follows --branch. cloneCmd = exec.CommandContext(ctx, "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)) diff --git a/pkg/parser/remote_resolve_sha.go b/pkg/parser/remote_resolve_sha.go index 608ff1d0fe9..5385af48485 100644 --- a/pkg/parser/remote_resolve_sha.go +++ b/pkg/parser/remote_resolve_sha.go @@ -28,6 +28,10 @@ var resolveRefToSHAViaGitFunc = resolveRefToSHAViaGit func resolveRefToSHAViaGit(ctx context.Context, owner, repo, ref, host string) (string, error) { remoteLog.Printf("Attempting git ls-remote fallback for ref resolution: %s/%s@%s", owner, repo, ref) + if err := gitutil.ValidateGitRef(ref); err != nil { + return "", fmt.Errorf("refusing git ls-remote fallback: %w", err) + } + var githubHost string if host != "" { githubHost = "https://" + host @@ -37,13 +41,16 @@ func resolveRefToSHAViaGit(ctx context.Context, owner, repo, ref, host string) ( repoURL := fmt.Sprintf("%s/%s/%s.git", githubHost, owner, repo) // Try to resolve the ref using git ls-remote - // Format: git ls-remote - cmd := exec.CommandContext(ctx, "git", "ls-remote", repoURL, ref) + // Format: git ls-remote -- + // The '--' end-of-options separator ensures ref is never parsed as a flag even + // if it begins with '-' (argument injection, CWE-88). ValidateGitRef above also + // rejects such values; '--' is kept as defence-in-depth. + cmd := exec.CommandContext(ctx, "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.CommandContext(ctx, "git", "ls-remote", repoURL, prefix+ref) + cmd = exec.CommandContext(ctx, "git", "ls-remote", repoURL, "--", prefix+ref) output, err = cmd.Output() if err == nil && len(output) > 0 { break diff --git a/pkg/parser/remote_workflow_spec.go b/pkg/parser/remote_workflow_spec.go index eda6738c9ba..e658ca3900b 100644 --- a/pkg/parser/remote_workflow_spec.go +++ b/pkg/parser/remote_workflow_spec.go @@ -8,6 +8,8 @@ import ( "fmt" "os" "strings" + + "github.com/github/gh-aw/pkg/gitutil" ) // IsWorkflowSpec checks if a path looks like a workflowspec (owner/repo/path[@ref]). @@ -120,6 +122,11 @@ func parseWorkflowSpecParts(spec string) (string, string, string, string, string } else { remoteLog.Print("No ref specified, defaulting to 'main'") } + + if err := gitutil.ValidateGitRef(ref); err != nil { + return "", "", "", "", "", fmt.Errorf("invalid workflowspec ref: %w", err) + } + slashParts := strings.Split(pathPart, "/") if len(slashParts) < 3 { remoteLog.Printf("Invalid workflowspec format: %s", spec) @@ -128,10 +135,18 @@ func parseWorkflowSpecParts(spec string) (string, string, string, string, string // Optional host-prefixed format: host/owner/repo/path[@ref] if len(slashParts) >= 4 && strings.Contains(slashParts[0], ".") { - return slashParts[0], slashParts[1], slashParts[2], strings.Join(slashParts[3:], "/"), ref, nil + filePath := strings.Join(slashParts[3:], "/") + if err := gitutil.ValidateGitPath(filePath); err != nil { + return "", "", "", "", "", fmt.Errorf("invalid workflowspec path: %w", err) + } + return slashParts[0], slashParts[1], slashParts[2], filePath, ref, nil } - return "", slashParts[0], slashParts[1], strings.Join(slashParts[2:], "/"), ref, nil + filePath := strings.Join(slashParts[2:], "/") + if err := gitutil.ValidateGitPath(filePath); err != nil { + return "", "", "", "", "", fmt.Errorf("invalid workflowspec path: %w", err) + } + return "", slashParts[0], slashParts[1], filePath, ref, nil } func resolveWorkflowSpecSHAForCache(owner, repo, ref, host string, cache *ImportCache) string { From b646dd5555e73173a44b65dce39440a8b86c6630 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 25 Jul 2026 08:35:16 +0000 Subject: [PATCH 3/5] docs(adr): add draft ADR-47957 for git argument injection validation strategy --- ...ocess-inputs-against-argument-injection.md | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 docs/adr/47957-validate-git-subprocess-inputs-against-argument-injection.md diff --git a/docs/adr/47957-validate-git-subprocess-inputs-against-argument-injection.md b/docs/adr/47957-validate-git-subprocess-inputs-against-argument-injection.md new file mode 100644 index 00000000000..9609011df52 --- /dev/null +++ b/docs/adr/47957-validate-git-subprocess-inputs-against-argument-injection.md @@ -0,0 +1,47 @@ +# ADR-47957: Validate Git Subprocess Inputs Against Argument Injection (CWE-88) + +**Date**: 2026-07-25 +**Status**: Draft +**Deciders**: Unknown (security fix by copilot-swe-agent, see VULN-001) + +--- + +### Context + +The remote workflow import feature resolves `owner/repo/path@ref` workflowspec strings and fetches the referenced files by invoking git subprocesses (`git archive`, `git ls-remote`, `git clone`, `git checkout`). The `ref` and `path` components of these specs are supplied by developers in workflow configuration and are user-controlled at the workflowspec level. Prior to this change, both values were passed directly as positional arguments to those subprocesses with no sanitisation and no `--` end-of-options separator. A `ref` value of `--upload-pack=malicious` or a `path` value of `--output=/etc/cron.d/pwned` would be parsed by git as option flags rather than values (CWE-88, argument injection). The primary attack surface was the auth-error fallback path, which is reached without requiring a valid token on token-less or unauthorised executions. + +### Decision + +We will apply a two-layer defence against git argument injection for all user-supplied `ref` and `path` values passed to git subprocesses: + +1. **Centralised input validation** — two new shared guards, `gitutil.ValidateGitRef` and `gitutil.ValidateGitPath`, are called at the earliest possible points (workflowspec parse time and at each subprocess call site) and return an error for empty values, values starting with `-`, and refs containing `..`. +2. **`--` end-of-options separators** — all git subprocess invocations that accept a ref or path as a positional argument are updated to insert `--` before those arguments, ensuring that even if validation were bypassed, git itself would not interpret them as flags (defence-in-depth per the git(1) specification). + +### Alternatives Considered + +#### Alternative 1: `--` separator only, no validation guards + +Add `--` end-of-options separators to every git call without introducing explicit validation functions. This is the minimal fix: it stops git from interpreting leading-`-` values as flags at the subprocess level. It was not chosen because it provides no early-fail signal — an attacker-controlled ref would still reach the subprocess and produce a confusing git error rather than a clear security rejection. It also gives no protection against `..`-based git object traversal expressions, which are not mitigated by `--`. Centralised validation makes the security invariant visible, testable, and reusable for future subprocess additions. + +#### Alternative 2: Allowlist-based validation only (no `--` separator) + +Reject any ref or path that does not match an explicit allowlist pattern (e.g., alphanumeric characters, slashes, dots, hyphens, underscores). This would be stricter and would block a wider class of unexpected inputs. It was not chosen because an allowlist tight enough to be safe is also tight enough to break legitimate edge-case refs (e.g., refs with `@`, unicode characters, or non-standard tag formats used in real repositories). The denylist approach (`ValidateGitRef`/`ValidateGitPath`) blocks the specific injection vectors (leading `-`, `..`) without rejecting valid refs. Using `--` as defence-in-depth alongside the denylist gives comparable protection without the breakage risk of an allowlist. + +### Consequences + +#### Positive +- Closes the CWE-88 argument injection attack vector across all git fallback paths (`git archive`, `git ls-remote`, `git clone`, `git checkout`) for both ref and path inputs. +- `ValidateGitRef` and `ValidateGitPath` are centralised in `pkg/gitutil` and reusable for any future git subprocess additions, ensuring the security invariant is easy to apply consistently. +- Unit tests covering valid inputs, empty values, leading-`-` injection, and `..` traversal cases provide a regression safety net. + +#### Negative +- Legitimate refs or paths that start with `-` (an unusual but theoretically valid git ref format) or contain `..` (e.g., range expressions used in some tooling) will now be rejected. In practice these are not expected in normal workflow import usage, but the restriction is a breaking change for any consumer relying on such values. +- Validation is applied redundantly at multiple layers (parse time and again at each subprocess call site), which adds some code repetition and means a failed import may report the rejection at the subprocess call rather than at parse time if the parse-time guard is bypassed by future code paths. + +#### Neutral +- The `--` separator changes git command signatures (e.g., `git ls-remote -- ` instead of `git ls-remote `). This is semantically equivalent for all supported git versions and should have no observable behaviour change for valid inputs. +- The `#nosec G204` annotation on the `git archive` call was retained; its justification (exec.CommandContext with separate args, not shell execution) remains accurate, and the new validation further strengthens the rationale. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From a06d4c9126a7f21e76a38add0e8d3470cc6be730 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 09:02:24 +0000 Subject: [PATCH 4/5] fix: restore git ref fallback semantics Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/gitutil/gitutil.go | 11 +++++++++++ pkg/gitutil/gitutil_test.go | 18 ++++++++++++++++++ pkg/parser/remote_download_file.go | 8 ++++++-- pkg/parser/remote_resolve_sha.go | 12 +++++------- 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/pkg/gitutil/gitutil.go b/pkg/gitutil/gitutil.go index 8e9d15ca707..c675bd4fae7 100644 --- a/pkg/gitutil/gitutil.go +++ b/pkg/gitutil/gitutil.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "os/exec" + stdpath "path" "path/filepath" "regexp" "strings" @@ -78,6 +79,9 @@ func ValidateGitRef(ref string) error { if strings.HasPrefix(ref, "-") { return fmt.Errorf("invalid git ref %q: refs must not start with '-' to prevent argument injection", ref) } + if strings.ContainsRune(ref, '\x00') { + return fmt.Errorf("invalid git ref %q: refs must not contain NUL bytes", ref) + } if strings.Contains(ref, "..") { return fmt.Errorf("invalid git ref %q: refs must not contain '..'", ref) } @@ -94,6 +98,13 @@ func ValidateGitPath(path string) error { if strings.HasPrefix(path, "-") { return fmt.Errorf("invalid git path %q: paths must not start with '-' to prevent argument injection", path) } + if stdpath.IsAbs(path) { + return fmt.Errorf("invalid git path %q: paths must not be absolute", path) + } + cleaned := stdpath.Clean(path) + if cleaned == ".." || strings.HasPrefix(cleaned, "../") { + return fmt.Errorf("invalid git path %q: paths must not contain '..' path traversal", path) + } return nil } diff --git a/pkg/gitutil/gitutil_test.go b/pkg/gitutil/gitutil_test.go index e71656c428e..7bb2f6db75f 100644 --- a/pkg/gitutil/gitutil_test.go +++ b/pkg/gitutil/gitutil_test.go @@ -492,6 +492,12 @@ func TestValidateGitRef(t *testing.T) { expectError: true, errContains: "must not contain '..'", }, + { + name: "NUL byte is rejected", + ref: "main\x00evil", + expectError: true, + errContains: "NUL", + }, { name: "dotdot prefix is rejected", ref: "..evil", @@ -548,6 +554,18 @@ func TestValidateGitPath(t *testing.T) { expectError: true, errContains: "must not start with '-'", }, + { + name: "path traversal is rejected", + path: "../etc/passwd", + expectError: true, + errContains: "must not contain '..'", + }, + { + name: "absolute path is rejected", + path: "/etc/passwd", + expectError: true, + errContains: "must not be absolute", + }, } for _, tt := range tests { diff --git a/pkg/parser/remote_download_file.go b/pkg/parser/remote_download_file.go index 3aaf6a955be..381f2dd4b80 100644 --- a/pkg/parser/remote_download_file.go +++ b/pkg/parser/remote_download_file.go @@ -386,6 +386,9 @@ func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host s if err := gitutil.ValidateGitRef(ref); err != nil { return nil, fmt.Errorf("refusing git clone fallback: %w", err) } + if err := gitutil.ValidateGitPath(path); err != nil { + return nil, fmt.Errorf("refusing git clone fallback: %w", err) + } // Create a temporary directory for the shallow clone tmpDir, err := os.MkdirTemp("", "gh-aw-git-clone-*") @@ -419,8 +422,9 @@ func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host s } } - // Now checkout the specific commit; '--' prevents ref from being parsed as a flag. - checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--", ref) + // Now checkout the specific commit in detached HEAD mode. ValidateGitRef above + // guarantees ref does not start with '-', so it remains a revision argument. + checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--detach", ref) if output, err := checkoutCmd.CombinedOutput(); err != nil { return nil, fmt.Errorf("failed to checkout commit %s: %w\nOutput: %s", ref, err, string(output)) } diff --git a/pkg/parser/remote_resolve_sha.go b/pkg/parser/remote_resolve_sha.go index 5385af48485..b7da9cff34c 100644 --- a/pkg/parser/remote_resolve_sha.go +++ b/pkg/parser/remote_resolve_sha.go @@ -40,17 +40,15 @@ func resolveRefToSHAViaGit(ctx context.Context, owner, repo, ref, host string) ( } repoURL := fmt.Sprintf("%s/%s/%s.git", githubHost, owner, repo) - // Try to resolve the ref using git ls-remote - // Format: git ls-remote -- - // The '--' end-of-options separator ensures ref is never parsed as a flag even - // if it begins with '-' (argument injection, CWE-88). ValidateGitRef above also - // rejects such values; '--' is kept as defence-in-depth. - cmd := exec.CommandContext(ctx, "git", "ls-remote", repoURL, "--", ref) + // Try to resolve the ref using git ls-remote. + // ValidateGitRef above guarantees ref does not begin with '-' before it is passed + // as a separate argument, so no extra separator is needed here. + cmd := exec.CommandContext(ctx, "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.CommandContext(ctx, "git", "ls-remote", repoURL, "--", prefix+ref) + cmd = exec.CommandContext(ctx, "git", "ls-remote", repoURL, prefix+ref) output, err = cmd.Output() if err == nil && len(output) > 0 { break From 9deff4df3e671f54b9f8164e5ea551099ee8dd98 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 25 Jul 2026 09:26:24 +0000 Subject: [PATCH 5/5] fix: surface invalid remote import refs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- ...ocess-inputs-against-argument-injection.md | 30 ++++++------ pkg/parser/import_bfs.go | 26 ++++++++--- pkg/parser/import_remote.go | 11 ++--- pkg/parser/import_remote_nested_test.go | 46 ++++++++++++++----- 4 files changed, 74 insertions(+), 39 deletions(-) diff --git a/docs/adr/47957-validate-git-subprocess-inputs-against-argument-injection.md b/docs/adr/47957-validate-git-subprocess-inputs-against-argument-injection.md index 9609011df52..3f0618039b4 100644 --- a/docs/adr/47957-validate-git-subprocess-inputs-against-argument-injection.md +++ b/docs/adr/47957-validate-git-subprocess-inputs-against-argument-injection.md @@ -1,8 +1,8 @@ # ADR-47957: Validate Git Subprocess Inputs Against Argument Injection (CWE-88) **Date**: 2026-07-25 -**Status**: Draft -**Deciders**: Unknown (security fix by copilot-swe-agent, see VULN-001) +**Status**: Accepted +**Deciders**: GitHub Agentic Workflows maintainers --- @@ -12,36 +12,38 @@ The remote workflow import feature resolves `owner/repo/path@ref` workflowspec s ### Decision -We will apply a two-layer defence against git argument injection for all user-supplied `ref` and `path` values passed to git subprocesses: +We will apply targeted validation at parse time and subprocess boundaries, and use git-specific argument forms that preserve command semantics while preventing option injection: -1. **Centralised input validation** — two new shared guards, `gitutil.ValidateGitRef` and `gitutil.ValidateGitPath`, are called at the earliest possible points (workflowspec parse time and at each subprocess call site) and return an error for empty values, values starting with `-`, and refs containing `..`. -2. **`--` end-of-options separators** — all git subprocess invocations that accept a ref or path as a positional argument are updated to insert `--` before those arguments, ensuring that even if validation were bypassed, git itself would not interpret them as flags (defence-in-depth per the git(1) specification). +1. **Centralised input validation** — shared guards `gitutil.ValidateGitRef` and `gitutil.ValidateGitPath` are called at the earliest possible points (workflowspec parse time and at each subprocess call site). `ValidateGitRef` rejects empty refs, leading `-`, NUL bytes, and `..` traversal expressions. `ValidateGitPath` rejects empty paths, leading `-`, absolute paths, and `..` path traversal after normalization. +2. **Use only git argument separators that preserve semantics** — `git archive` keeps `--` before the pathspec, because that command explicitly separates `` from ``. `git checkout --detach ` is used for full-SHA clone fallbacks so the validated SHA remains a revision argument rather than becoming a pathspec. `git ls-remote` does not receive an extra `--`, because its interface does not define a remote/refs separator there. +3. **Propagate invalid remote-origin refs as errors** — remote-origin parsing now returns an error for unsafe workflowspec refs instead of silently dropping the origin, so callers can distinguish security rejections from non-workflowspec inputs. ### Alternatives Considered #### Alternative 1: `--` separator only, no validation guards -Add `--` end-of-options separators to every git call without introducing explicit validation functions. This is the minimal fix: it stops git from interpreting leading-`-` values as flags at the subprocess level. It was not chosen because it provides no early-fail signal — an attacker-controlled ref would still reach the subprocess and produce a confusing git error rather than a clear security rejection. It also gives no protection against `..`-based git object traversal expressions, which are not mitigated by `--`. Centralised validation makes the security invariant visible, testable, and reusable for future subprocess additions. +Add `--` end-of-options separators to every git call without introducing explicit validation functions. This is the minimal fix for commands that define such a separator, but it was not chosen because it provides no early-fail signal and does not apply uniformly across git commands. `git ls-remote` has no supported separator in this position, and `git checkout -- ` changes semantics entirely. Centralised validation makes the invariant visible, testable, and reusable. -#### Alternative 2: Allowlist-based validation only (no `--` separator) +#### Alternative 2: Allowlist-based validation only (no command-specific argument hardening) -Reject any ref or path that does not match an explicit allowlist pattern (e.g., alphanumeric characters, slashes, dots, hyphens, underscores). This would be stricter and would block a wider class of unexpected inputs. It was not chosen because an allowlist tight enough to be safe is also tight enough to break legitimate edge-case refs (e.g., refs with `@`, unicode characters, or non-standard tag formats used in real repositories). The denylist approach (`ValidateGitRef`/`ValidateGitPath`) blocks the specific injection vectors (leading `-`, `..`) without rejecting valid refs. Using `--` as defence-in-depth alongside the denylist gives comparable protection without the breakage risk of an allowlist. +Reject any ref or path that does not match an explicit allowlist pattern (e.g., alphanumeric characters, slashes, dots, hyphens, underscores). This would be stricter and would block a wider class of unexpected inputs. It was not chosen because an allowlist tight enough to be safe is also tight enough to break legitimate edge-case refs (for example refs with `@`, unicode characters, or non-standard tag formats used in real repositories). The chosen validation blocks the concrete dangerous forms while preserving valid git syntax, and the command-specific git argument changes provide the remaining defence-in-depth where supported. ### Consequences #### Positive -- Closes the CWE-88 argument injection attack vector across all git fallback paths (`git archive`, `git ls-remote`, `git clone`, `git checkout`) for both ref and path inputs. +- Closes the CWE-88 argument injection attack vector across all git fallback paths (`git archive`, `git ls-remote`, `git clone`, `git checkout`) for both ref and path inputs without changing the semantics of SHA checkout or ref resolution. - `ValidateGitRef` and `ValidateGitPath` are centralised in `pkg/gitutil` and reusable for any future git subprocess additions, ensuring the security invariant is easy to apply consistently. -- Unit tests covering valid inputs, empty values, leading-`-` injection, and `..` traversal cases provide a regression safety net. +- Unit tests covering valid inputs, empty values, leading-`-` injection, NUL bytes, absolute paths, and traversal cases provide a regression safety net. +- Invalid remote-origin refs now surface as explicit errors to callers instead of being visible only through debug logging. #### Negative -- Legitimate refs or paths that start with `-` (an unusual but theoretically valid git ref format) or contain `..` (e.g., range expressions used in some tooling) will now be rejected. In practice these are not expected in normal workflow import usage, but the restriction is a breaking change for any consumer relying on such values. -- Validation is applied redundantly at multiple layers (parse time and again at each subprocess call site), which adds some code repetition and means a failed import may report the rejection at the subprocess call rather than at parse time if the parse-time guard is bypassed by future code paths. +- Legitimate refs or paths that start with `-` (an unusual but theoretically valid git ref format), contain NUL bytes, or resolve to absolute/traversing paths will now be rejected. In practice these are not expected in normal workflow import usage, but the restriction is a breaking change for any consumer relying on such values. +- Validation is applied redundantly at multiple layers (parse time and again at each subprocess call site), which adds some code repetition in exchange for defence-in-depth. #### Neutral -- The `--` separator changes git command signatures (e.g., `git ls-remote -- ` instead of `git ls-remote `). This is semantically equivalent for all supported git versions and should have no observable behaviour change for valid inputs. +- `git archive` retains a `--` pathspec separator, while other commands rely on validation plus git-native argument forms instead of a one-size-fits-all separator rule. - The `#nosec G204` annotation on the `git archive` call was retained; its justification (exec.CommandContext with separate args, not shell execution) remains accurate, and the new validation further strengthens the rationale. --- -*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* +*Finalized from the draft generated by the adr-writer agent to match the merged implementation in this PR.* diff --git a/pkg/parser/import_bfs.go b/pkg/parser/import_bfs.go index bb3bc899804..209a2ef4f52 100644 --- a/pkg/parser/import_bfs.go +++ b/pkg/parser/import_bfs.go @@ -133,7 +133,10 @@ func seedSingleImportSpec(importSpec ImportSpec, baseDir string, cache *ImportCa if err != nil { return err } - origin := detectRemoteImportOrigin(filePath) + origin, err := detectRemoteImportOrigin(filePath) + if err != nil { + return err + } return enqueueImportPath(state, importPath, fullPath, sectionName, baseDir, importSpec.Inputs, origin) } @@ -184,15 +187,18 @@ func validateNoLockYMLImport(fullPath, importPath, workflowFilePath, yamlContent return fmt.Errorf("cannot import .lock.yml files: '%s'. Lock files are compiled outputs from gh-aw. Import the source .md file instead", importPath) } -func detectRemoteImportOrigin(filePath string) *remoteImportOrigin { +func detectRemoteImportOrigin(filePath string) (*remoteImportOrigin, error) { if !IsWorkflowSpec(filePath) { - return nil + return nil, nil + } + origin, err := parseRemoteOrigin(filePath) + if err != nil { + return nil, fmt.Errorf("invalid workflowspec ref in %q: %w", filePath, err) } - origin := parseRemoteOrigin(filePath) if origin != nil { importLog.Printf("Tracking remote origin for workflowspec: %s/%s@%s", origin.Owner, origin.Repo, origin.Ref) } - return origin + return origin, nil } func enqueueImportPath(state *importBFSState, importPath, fullPath, sectionName, baseDir string, inputs map[string]any, origin *remoteImportOrigin) error { @@ -412,7 +418,10 @@ func resolveNestedImportPathAndOrigin(item importQueueItem, nestedFilePath strin return resolveRemoteNestedPath(item, nestedFilePath) } if IsWorkflowSpec(nestedFilePath) { - nestedRemoteOrigin := parseRemoteOrigin(nestedFilePath) + nestedRemoteOrigin, err := parseRemoteOrigin(nestedFilePath) + if err != nil { + return "", nil, fmt.Errorf("invalid workflowspec ref in %q: %w", nestedFilePath, err) + } if nestedRemoteOrigin != nil { importLog.Printf("Nested workflowspec import detected: %s (origin: %s/%s@%s)", nestedFilePath, nestedRemoteOrigin.Owner, nestedRemoteOrigin.Repo, nestedRemoteOrigin.Ref) } @@ -433,7 +442,10 @@ func resolveRemoteNestedPath(item importQueueItem, nestedFilePath string) (strin basePath = path.Clean(basePath) resolvedPath := fmt.Sprintf("%s/%s/%s/%s@%s", item.remoteOrigin.Owner, item.remoteOrigin.Repo, basePath, cleanPath, item.remoteOrigin.Ref) - nestedRemoteOrigin := parseRemoteOrigin(resolvedPath) + nestedRemoteOrigin, err := parseRemoteOrigin(resolvedPath) + if err != nil { + return "", nil, fmt.Errorf("invalid workflowspec ref in %q: %w", resolvedPath, err) + } importLog.Printf("Resolving nested import as remote workflowspec: %s -> %s (basePath=%s)", nestedFilePath, resolvedPath, basePath) return resolvedPath, nestedRemoteOrigin, nil } diff --git a/pkg/parser/import_remote.go b/pkg/parser/import_remote.go index b1eaae8ebd4..07679e9835c 100644 --- a/pkg/parser/import_remote.go +++ b/pkg/parser/import_remote.go @@ -34,12 +34,12 @@ type importQueueItem struct { } // parseRemoteOrigin extracts the remote origin (owner, repo, ref, basePath) from a workflowspec path. -// Returns nil if the path is not a valid workflowspec. +// Returns nil, nil if the path is not a valid workflowspec. // Format: owner/repo/path[@ref] where ref defaults to "main" if not specified. // BasePath is derived from the parent workflowspec path and used for resolving nested relative imports. // For example, "elastic/ai-github-actions/gh-agent-workflows/gh-aw-workflows/file.md@main" // produces BasePath="gh-agent-workflows" so nested imports resolve relative to that directory. -func parseRemoteOrigin(spec string) *remoteImportOrigin { +func parseRemoteOrigin(spec string) (*remoteImportOrigin, error) { importRemoteLog.Printf("Parsing remote import origin from spec: %q", spec) // Remove section reference if present cleanSpec := spec @@ -57,15 +57,14 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin { // Reject refs that would be unsafe to pass to git subprocesses. if err := gitutil.ValidateGitRef(ref); err != nil { - importRemoteLog.Printf("Rejecting spec %q: invalid ref: %v", spec, err) - return nil + return nil, err } // Parse path: owner/repo/path/to/file.md slashParts := strings.Split(pathPart, "/") if len(slashParts) < 3 { importRemoteLog.Printf("Spec %q has fewer than 3 path components; not a valid workflowspec", spec) - return nil + return nil, nil } // Derive BasePath: everything between owner/repo and the last component (filename) @@ -95,5 +94,5 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin { Repo: slashParts[1], Ref: ref, BasePath: basePath, - } + }, nil } diff --git a/pkg/parser/import_remote_nested_test.go b/pkg/parser/import_remote_nested_test.go index 8c3ef5a8f0b..0e530761574 100644 --- a/pkg/parser/import_remote_nested_test.go +++ b/pkg/parser/import_remote_nested_test.go @@ -16,11 +16,19 @@ import ( "github.com/stretchr/testify/require" ) +func mustParseRemoteOrigin(t *testing.T, spec string) *remoteImportOrigin { + t.Helper() + origin, err := parseRemoteOrigin(spec) + require.NoError(t, err) + return origin +} + func TestParseRemoteOrigin(t *testing.T) { tests := []struct { - name string - spec string - expected *remoteImportOrigin + name string + spec string + expected *remoteImportOrigin + errContains string }{ { name: "basic workflowspec with ref", @@ -122,11 +130,23 @@ func TestParseRemoteOrigin(t *testing.T) { spec: "file.md", expected: nil, }, + { + name: "invalid ref returns error", + spec: "owner/repo/file.md@--upload-pack=malicious", + errContains: "must not start with '-'", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := parseRemoteOrigin(tt.spec) + result, err := parseRemoteOrigin(tt.spec) + if tt.errContains != "" { + require.Error(t, err) + require.ErrorContains(t, err, tt.errContains) + assert.Nil(t, result) + return + } + require.NoError(t, err) if tt.expected == nil { assert.Nilf(t, result, "Expected nil for spec: %s", tt.spec) } else { @@ -279,7 +299,7 @@ func TestRemoteOriginPropagation(t *testing.T) { spec := "elastic/ai-github-actions/gh-agent-workflows/mention-in-pr/rwxp.md@main" assert.True(t, IsWorkflowSpec(spec), "Should be recognized as workflowspec") - origin := parseRemoteOrigin(spec) + origin := mustParseRemoteOrigin(t, spec) require.NotNil(t, origin, "Should parse remote origin") assert.Equal(t, "elastic", origin.Owner, "Owner should be elastic") assert.Equal(t, "ai-github-actions", origin.Repo, "Repo should be ai-github-actions") @@ -291,7 +311,8 @@ func TestRemoteOriginPropagation(t *testing.T) { localPath := "shared/tools.md" assert.False(t, IsWorkflowSpec(localPath), "Should not be recognized as workflowspec") - origin := parseRemoteOrigin(localPath) + origin, err := parseRemoteOrigin(localPath) + require.NoError(t, err) assert.Nil(t, origin, "Local paths should not produce remote origin") }) @@ -385,7 +406,7 @@ func TestRemoteOriginPropagation(t *testing.T) { nestedSpec := "other-org/other-repo/path/file.md@v2.0" assert.True(t, IsWorkflowSpec(nestedSpec), "Should be recognized as workflowspec") - origin := parseRemoteOrigin(nestedSpec) + origin := mustParseRemoteOrigin(t, nestedSpec) require.NotNil(t, origin, "Should parse remote origin for nested workflowspec") assert.Equal(t, "other-org", origin.Owner, "Should use nested spec's owner") assert.Equal(t, "other-repo", origin.Repo, "Should use nested spec's repo") @@ -439,7 +460,7 @@ func TestRemoteOriginPropagation(t *testing.T) { // → file1.md imports: file2.md (should resolve to shared/file2.md) // Step 1: Top-level workflow import produces this remoteOrigin - topLevelOrigin := parseRemoteOrigin("githubnext/agentics/workflows/workflow.md@main") + topLevelOrigin := mustParseRemoteOrigin(t, "githubnext/agentics/workflows/workflow.md@main") require.NotNil(t, topLevelOrigin, "Should parse top-level workflow") assert.Equal(t, "workflows", topLevelOrigin.BasePath, "Top-level BasePath should be 'workflows'") @@ -460,7 +481,7 @@ func TestRemoteOriginPropagation(t *testing.T) { // Step 3: Parse the remoteOrigin from file1's resolved spec // This is the KEY fix - file1's origin should have BasePath="workflows/shared" - file1Origin := parseRemoteOrigin(file1ResolvedSpec) + file1Origin := mustParseRemoteOrigin(t, file1ResolvedSpec) require.NotNil(t, file1Origin, "Should parse file1's remote origin from resolved spec") assert.Equal(t, "githubnext", file1Origin.Owner, "File1 Owner") assert.Equal(t, "agentics", file1Origin.Repo, "File1 Repo") @@ -672,7 +693,7 @@ func TestParseRemoteOriginWithCleanedPaths(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := parseRemoteOrigin(tt.spec) + result := mustParseRemoteOrigin(t, tt.spec) require.NotNil(t, result, "Should parse remote origin for spec: %s", tt.spec) assert.Equal(t, tt.expected.Owner, result.Owner, "Owner mismatch") assert.Equal(t, tt.expected.Repo, result.Repo, "Repo mismatch") @@ -702,7 +723,8 @@ func TestParseRemoteOriginWithURLFormats(t *testing.T) { // - Parts: ["https:", "", "github.com", "owner", "repo", "path", "file.md"] // - Owner would be "https:" (first part after splitting by /) // This test documents the current behavior for future reference - origin := parseRemoteOrigin(urlPath) + origin, err := parseRemoteOrigin(urlPath) + require.NoError(t, err) if origin != nil { t.Logf("URL %s parsed as: owner=%s, repo=%s, basePath=%s", urlPath, origin.Owner, origin.Repo, origin.BasePath) @@ -715,7 +737,7 @@ func TestParseRemoteOriginWithURLFormats(t *testing.T) { // The domain is handled by GH_HOST environment variable, not in the workflowspec spec := "enterprise-org/enterprise-repo/workflows/test.md@main" - result := parseRemoteOrigin(spec) + result := mustParseRemoteOrigin(t, spec) require.NotNil(t, result, "Should parse enterprise workflowspec") assert.Equal(t, "enterprise-org", result.Owner) assert.Equal(t, "enterprise-repo", result.Repo)