fix: git argument injection via unvalidated ref/path in remote import fallbacks (VULN-001)

docs/adr/47957-validate-git-subprocess-inputs-against-argument-injection.md

@@ -0,0 +1,49 @@
+# ADR-47957: Validate Git Subprocess Inputs Against Argument Injection (CWE-88)
+
+**Date**: 2026-07-25
+**Status**: Accepted
+**Deciders**: GitHub Agentic Workflows maintainers
+
+---
+
+### 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 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** — 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 `<tree-ish>` from `<path>`. `git checkout --detach <sha>` 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 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 -- <sha>` changes semantics entirely. Centralised validation makes the invariant visible, testable, and reusable.
+
+#### 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 (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 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, 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), 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
+- `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.
+
+---
+
+*Finalized from the draft generated by the adr-writer agent to match the merged implementation in this PR.*

pkg/gitutil/gitutil.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	stdpath "path"
 	"path/filepath"
 	"regexp"
 	"strings"
@@ -67,6 +68,46 @@ 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.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)
+	}
+	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)
+	}
+	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
+}
+
 // ExtractBaseRepo extracts the base repository (owner/repo) from a repository path
 // that may include subfolders.
 // For "actions/checkout" -> "actions/checkout"

pkg/gitutil/gitutil_test.go

@@ -440,3 +440,143 @@ 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:        "NUL byte is rejected",
+			ref:         "main\x00evil",
+			expectError: true,
+			errContains: "NUL",
+		},
+		{
+			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 '-'",
+		},
+		{
+			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 {
+		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)
+			}
+		})
+	}
+}

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
 }

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"
 )
 
@@ -33,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
@@ -54,11 +55,16 @@ 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 {
+		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)
@@ -88,5 +94,5 @@ func parseRemoteOrigin(spec string) *remoteImportOrigin {
 		Repo:     slashParts[1],
 		Ref:      ref,
 		BasePath: basePath,
-	}
+	}, nil
 }