Harden dynamic exec arguments in experiments/dependabot paths

pkg/cli/experiments_command.go

@@ -10,6 +10,7 @@ import (
 	"net/url"
 	"os"
 	"os/exec"
+	"path"
 	"path/filepath"
 	"slices"
 	"strconv"
@@ -31,6 +32,7 @@ var experimentsLog = logger.New("cli:experiments_command")
 
 // experimentsBranchPrefix is the git branch prefix used to identify experiment state branches.
 const experimentsBranchPrefix = "experiments/"
+const evalsBranchPrefix = constants.EvalsBranchPrefix + "/"
 
 // ExperimentState represents experiment state stored in experiments/* branches.
 // This matches the legacy JSON snapshot format and the JSONL run-ledger format written by pick_experiment.cjs.
@@ -335,11 +337,16 @@ func loadLocalMetricEvalResults(workflowID string) map[string]MetricEvalResults
 		}
 		ref = branchName
 	}
-	if !isSafeGitRevisionArg(ref) {
+	if !isSafeExperimentStateRef(ref) {
 		experimentsLog.Printf("Rejecting unsafe git ref: %q", ref)
 		return nil
 	}
-	cmd := exec.Command("git", "show", ref+":"+constants.EvalsResultFilename)
+	objectArg, err := buildSafeGitShowObjectArg(ref, constants.EvalsResultFilename)
+	if err != nil {
+		experimentsLog.Printf("Rejecting unsafe git show argument (ref=%q file=%q): %v", ref, constants.EvalsResultFilename, err)
+		return nil
+	}
+	cmd := exec.Command("git", "show", objectArg)
 	out, err := cmd.Output()
 	if err != nil {
 		return nil
@@ -736,7 +743,12 @@ func experimentStateFilenames() []string {
 // Returns an empty state when the file is absent or cannot be parsed.
 func readLocalExperimentState(ref string) *ExperimentState {
 	for _, fileName := range experimentStateFilenames() {
-		cmd := exec.Command("git", "show", ref+":"+fileName)
+		objectArg, err := buildSafeGitShowObjectArg(ref, fileName)
+		if err != nil {
+			experimentsLog.Printf("Skipping unsafe git show argument (ref=%q file=%q): %v", ref, fileName, err)
+			continue
+		}
+		cmd := exec.Command("git", "show", objectArg)
 		out, err := cmd.Output()
 		if err == nil {
 			return parseExperimentState(out)
@@ -745,6 +757,95 @@ func readLocalExperimentState(ref string) *ExperimentState {
 	return emptyExperimentState()
 }
 
+// buildSafeGitShowObjectArg validates git show's "ref:path" object argument parts
+// before joining them, preventing flag and path-traversal style injections.
+func buildSafeGitShowObjectArg(ref, fileName string) (string, error) {
+	if !isSafeExperimentStateRef(ref) {
+		return "", errors.New("unsafe git ref")
+	}
+	if !isSafeGitTreePath(fileName) {
+		return "", errors.New("unsafe git tree path")
+	}
+	return ref + ":" + fileName, nil
+}
+
+func isSafeExperimentStateRef(ref string) bool {
+	if !isSafeGitRevisionArg(ref) {
+		return false
+	}
+
+	// Allow direct object IDs (including abbreviated prefixes) for future callers
+	// while rejecting revision operators.
+	if isHexObjectIDPrefix(ref) {
+		return true
+	}
+
+	trimmed := strings.TrimPrefix(ref, "origin/")
+	if !strings.HasPrefix(trimmed, experimentsBranchPrefix) && !strings.HasPrefix(trimmed, evalsBranchPrefix) {
+		return false
+	}
+
+	return isSafeGitRefName(trimmed)
+}
+
+func isHexObjectIDPrefix(ref string) bool {
+	// Require >=7 chars to avoid accepting short hex-like experiment names as SHAs.
+	// 64 keeps compatibility with SHA-256 object IDs.
+	if len(ref) < 7 || len(ref) > 64 {
+		return false
+	}
+	for _, r := range ref {
+		if (r < '0' || r > '9') && (r < 'a' || r > 'f') && (r < 'A' || r > 'F') {
+			return false
+		}
+	}
+	return true
+}
+
+// isSafeGitRefName validates a refname with check-ref-format-equivalent rules.
+func isSafeGitRefName(ref string) bool {
+	hasInvalidShape := ref == "" ||
+		strings.HasPrefix(ref, "/") ||
+		strings.HasSuffix(ref, "/") ||
+		strings.HasSuffix(ref, ".")
+	hasInvalidSequences := strings.Contains(ref, "//") ||
+		strings.Contains(ref, "..") ||
+		strings.Contains(ref, "@{") ||
+		strings.Contains(ref, "\\")
+	if hasInvalidShape || hasInvalidSequences {
+		return false
+	}
+
+	for part := range strings.SplitSeq(ref, "/") {
+		if part == "" || strings.HasPrefix(part, ".") || strings.HasSuffix(part, ".lock") {
+			return false
+		}
+		for _, r := range part {
+			if r <= ' ' || r == '~' || r == '^' || r == ':' || r == '?' || r == '*' || r == '[' || r == '\x7f' {
+				return false
+			}
+		}
+	}
+	return true
+}
+
+// isSafeGitTreePath validates a git tree entry path used in "ref:path" syntax.
+// Git tree paths always use forward slashes across platforms, so this intentionally
+// uses the slash-based path package (not filepath) for normalization checks.
+func isSafeGitTreePath(fileName string) bool {
+	if fileName == "" || strings.HasPrefix(fileName, "-") {
+		return false
+	}
+	if path.IsAbs(fileName) || strings.Contains(fileName, "\\") || strings.Contains(fileName, ":") || strings.ContainsRune(fileName, '\x00') {
+		return false
+	}
+	clean := path.Clean(fileName)
+	if clean == "." || clean == ".." || strings.HasPrefix(clean, "../") {
+		return false
+	}
+	return clean == fileName
+}
+
 // readRemoteExperimentState fetches experiment state from an experiments/* branch via the GitHub API.
 // Returns an empty state on any error (branch missing, file absent, parse failure).
 func readRemoteExperimentState(repoOverride, branchName string) *ExperimentState {
@@ -910,9 +1011,9 @@ func extractExperimentName(ref string) string {
 	return strings.TrimPrefix(ref, experimentsBranchPrefix)
 }
 
-// gitRefExists reports whether a git ref exists locally.
+// gitRefExists reports whether an experiments/evals state ref exists locally.
 func gitRefExists(ref string) bool {
-	if !isSafeGitRevisionArg(ref) {
+	if !isSafeExperimentStateRef(ref) {
 		return false
 	}
 	cmd := exec.Command("git", "rev-parse", "--verify", ref)

pkg/cli/experiments_command_test.go

@@ -23,6 +23,102 @@ func TestFetchRemoteExperimentDetailsClassifiesTitleCaseNotFound(t *testing.T) {
 	require.EqualError(t, err, `experiment "missing" not found in octo/repo`)
 }
 
+func TestBuildSafeGitShowObjectArg(t *testing.T) {
+	tests := []struct {
+		name      string
+		ref       string
+		fileName  string
+		want      string
+		shouldErr bool
+	}{
+		{
+			name:     "valid ref and file",
+			ref:      "origin/experiments/my-feature",
+			fileName: "state.jsonl",
+			want:     "origin/experiments/my-feature:state.jsonl",
+		},
+		{
+			name:      "rejects flag-like ref",
+			ref:       "--help",
+			fileName:  "state.jsonl",
+			shouldErr: true,
+		},
+		{
+			name:      "rejects revision expression suffix",
+			ref:       "origin/experiments/my-feature~1",
+			fileName:  "state.jsonl",
+			shouldErr: true,
+		},
+		{
+			name:      "rejects revision expression braces",
+			ref:       "origin/experiments/my-feature^{tree}",
+			fileName:  "state.jsonl",
+			shouldErr: true,
+		},
+		{
+			name:      "rejects colon in ref",
+			ref:       "origin/experiments/my-feature:other",
+			fileName:  "state.jsonl",
+			shouldErr: true,
+		},
+		{
+			name:      "rejects path traversal",
+			ref:       "origin/experiments/my-feature",
+			fileName:  "../state.json",
+			shouldErr: true,
+		},
+		{
+			name:      "rejects colon in file name",
+			ref:       "origin/experiments/my-feature",
+			fileName:  "state.json:HEAD",
+			shouldErr: true,
+		},
+		{
+			name:      "rejects flag-like file name",
+			ref:       "origin/experiments/my-feature",
+			fileName:  "-n",
+			shouldErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := buildSafeGitShowObjectArg(tt.ref, tt.fileName)
+			if tt.shouldErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+func TestIsSafeExperimentStateRef(t *testing.T) {
+	tests := []struct {
+		name string
+		ref  string
+		want bool
+	}{
+		{name: "experiment branch", ref: "origin/experiments/my-feature", want: true},
+		{name: "local experiment branch", ref: "experiments/my-feature", want: true},
+		{name: "evals branch", ref: "evals/myworkflow", want: true},
+		{name: "short sha", ref: "a1b2c3d", want: true},
+		{name: "reject too-short sha", ref: "a1b2c3", want: false},
+		{name: "full sha", ref: "0123456789abcdef0123456789abcdef01234567", want: true},
+		{name: "reject revision operator", ref: "origin/experiments/my-feature~1", want: false},
+		{name: "reject brace expression", ref: "origin/experiments/my-feature^{tree}", want: false},
+		{name: "reject wrong prefix", ref: "origin/main", want: false},
+		{name: "reject invalid sequence", ref: "origin/experiments/my..feature", want: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, isSafeExperimentStateRef(tt.ref))
+		})
+	}
+}
+
 func TestExtractExperimentName(t *testing.T) {
 	tests := []struct {
 		name     string

pkg/workflow/dependabot.go

@@ -336,11 +336,14 @@ func (c *Compiler) generatePackageLock(workflowDir string) error {
 		fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Running npm install --package-lock-only..."))
 	}
 
-	// Run npm install --package-lock-only
+	// Run npm install --package-lock-only without lifecycle scripts.
+	// The generated package.json can be influenced by workflow content, so explicitly
+	// disable script execution to avoid running untrusted hooks while generating lockfiles.
 	// #nosec G204 -- npmPath is resolved by exec.LookPath and validated as an absolute path above;
-	// the fixed arguments "install" and "--package-lock-only" contain no user-controlled data.
-	cmd := exec.Command(npmPath, "install", "--package-lock-only")
+	// the fixed arguments contain no user-controlled data.
+	cmd := exec.Command(npmPath, "install", "--package-lock-only", "--ignore-scripts")
 	cmd.Dir = workflowDir
+	cmd.Env = append(os.Environ(), "NPM_CONFIG_IGNORE_SCRIPTS=true")
 
 	// Capture output for error reporting
 	output, err := cmd.CombinedOutput()

pkg/workflow/dependabot_test.go

@@ -653,6 +653,56 @@ func TestGenerateDependabotManifests_StrictMode(t *testing.T) {
 	}
 }
 
+func TestGeneratePackageLock_DisablesNpmScripts(t *testing.T) {
+	compiler := NewCompiler()
+	workflowDir := testutil.TempDir(t, "workflow-*")
+	fakeBinDir := testutil.TempDir(t, "fake-bin-*")
+
+	argsFile := filepath.Join(workflowDir, "npm-args.txt")
+	envFile := filepath.Join(workflowDir, "npm-ignore-scripts-env.txt")
+
+	fakeNpm := filepath.Join(fakeBinDir, "npm")
+	script := `#!/bin/sh
+printf "%s\n" "$@" > "$GH_AW_TEST_ARGS_FILE"
+printf "%s" "$NPM_CONFIG_IGNORE_SCRIPTS" > "$GH_AW_TEST_ENV_FILE"
+touch package-lock.json
+`
+	if err := os.WriteFile(fakeNpm, []byte(script), 0o755); err != nil {
+		t.Fatalf("failed to write fake npm binary: %v", err)
+	}
+
+	t.Setenv("PATH", fakeBinDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+	t.Setenv("GH_AW_TEST_ARGS_FILE", argsFile)
+	t.Setenv("GH_AW_TEST_ENV_FILE", envFile)
+
+	if err := compiler.generatePackageLock(workflowDir); err != nil {
+		t.Fatalf("generatePackageLock() error = %v", err)
+	}
+
+	argsData, err := os.ReadFile(argsFile)
+	if err != nil {
+		t.Fatalf("failed to read recorded npm args: %v", err)
+	}
+	args := string(argsData)
+	if !strings.Contains(args, "install\n") {
+		t.Fatalf("expected npm args to contain install, got: %q", args)
+	}
+	if !strings.Contains(args, "--package-lock-only\n") {
+		t.Fatalf("expected npm args to contain --package-lock-only, got: %q", args)
+	}
+	if !strings.Contains(args, "--ignore-scripts\n") {
+		t.Fatalf("expected npm args to contain --ignore-scripts, got: %q", args)
+	}
+
+	envData, err := os.ReadFile(envFile)
+	if err != nil {
+		t.Fatalf("failed to read recorded NPM_CONFIG_IGNORE_SCRIPTS: %v", err)
+	}
+	if string(envData) != "true" {
+		t.Fatalf("expected NPM_CONFIG_IGNORE_SCRIPTS=true, got: %q", string(envData))
+	}
+}
+
 // Tests for Python (pip) support
 
 func TestParsePipPackage(t *testing.T) {