From d413d950d1eb38ad34eca1e90c82d2ccf4f190f3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 15 Aug 2026 14:16:44 +0000 Subject: [PATCH 1/4] Initial plan From 225d342c7140c8b46d83b9f699bfc2e0b002a28e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 15 Aug 2026 14:27:58 +0000 Subject: [PATCH 2/4] Harden git show args and npm lock generation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/experiments_command.go | 40 ++++++++++++++++++++-- pkg/cli/experiments_command_test.go | 53 +++++++++++++++++++++++++++++ pkg/workflow/dependabot.go | 9 +++-- pkg/workflow/dependabot_test.go | 50 +++++++++++++++++++++++++++ 4 files changed, 147 insertions(+), 5 deletions(-) diff --git a/pkg/cli/experiments_command.go b/pkg/cli/experiments_command.go index 10f45d90724..22fd0e841e9 100644 --- a/pkg/cli/experiments_command.go +++ b/pkg/cli/experiments_command.go @@ -339,7 +339,12 @@ func loadLocalMetricEvalResults(workflowID string) map[string]MetricEvalResults 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 +741,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 +755,32 @@ 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 !isSafeGitRevisionArg(ref) { + return "", errors.New("unsafe git ref") + } + if !isSafeGitTreePath(fileName) { + return "", errors.New("unsafe git tree path") + } + return ref + ":" + fileName, nil +} + +func isSafeGitTreePath(fileName string) bool { + if fileName == "" || strings.HasPrefix(fileName, "-") { + return false + } + if filepath.IsAbs(fileName) || strings.Contains(fileName, ":") || strings.ContainsRune(fileName, '\x00') { + return false + } + clean := filepath.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 { diff --git a/pkg/cli/experiments_command_test.go b/pkg/cli/experiments_command_test.go index e145bdbb0ca..4470849536a 100644 --- a/pkg/cli/experiments_command_test.go +++ b/pkg/cli/experiments_command_test.go @@ -23,6 +23,59 @@ 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 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 TestExtractExperimentName(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/dependabot.go b/pkg/workflow/dependabot.go index d470534c6ae..27300809e85 100644 --- a/pkg/workflow/dependabot.go +++ b/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() diff --git a/pkg/workflow/dependabot_test.go b/pkg/workflow/dependabot_test.go index 4ebfa3b9aed..8af428f0e87 100644 --- a/pkg/workflow/dependabot_test.go +++ b/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) { From 82fff2b852413abb9e3c6d6ef7ad154286c0139d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 15 Aug 2026 14:31:49 +0000 Subject: [PATCH 3/4] Document git tree path invariants for safe git show args Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/experiments_command.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/cli/experiments_command.go b/pkg/cli/experiments_command.go index 22fd0e841e9..8cb0f9a68e1 100644 --- a/pkg/cli/experiments_command.go +++ b/pkg/cli/experiments_command.go @@ -10,6 +10,7 @@ import ( "net/url" "os" "os/exec" + "path" "path/filepath" "slices" "strconv" @@ -767,14 +768,17 @@ func buildSafeGitShowObjectArg(ref, fileName string) (string, error) { return ref + ":" + fileName, nil } +// 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 filepath.IsAbs(fileName) || strings.Contains(fileName, ":") || strings.ContainsRune(fileName, '\x00') { + if path.IsAbs(fileName) || strings.Contains(fileName, "\\") || strings.Contains(fileName, ":") || strings.ContainsRune(fileName, '\x00') { return false } - clean := filepath.Clean(fileName) + clean := path.Clean(fileName) if clean == "." || clean == ".." || strings.HasPrefix(clean, "../") { return false } From b77a1c6cbce6e467f2cfb507fd7c5ccfce67f038 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:17:39 +0000 Subject: [PATCH 4/4] Harden experiments git ref validation for local state reads Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/experiments_command.go | 69 +++++++++++++++++++++++++++-- pkg/cli/experiments_command_test.go | 43 ++++++++++++++++++ 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/pkg/cli/experiments_command.go b/pkg/cli/experiments_command.go index 8cb0f9a68e1..500626cf3b6 100644 --- a/pkg/cli/experiments_command.go +++ b/pkg/cli/experiments_command.go @@ -32,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. @@ -336,7 +337,7 @@ 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 } @@ -759,7 +760,7 @@ func readLocalExperimentState(ref string) *ExperimentState { // 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 !isSafeGitRevisionArg(ref) { + if !isSafeExperimentStateRef(ref) { return "", errors.New("unsafe git ref") } if !isSafeGitTreePath(fileName) { @@ -768,6 +769,66 @@ func buildSafeGitShowObjectArg(ref, fileName string) (string, error) { 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. @@ -950,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) diff --git a/pkg/cli/experiments_command_test.go b/pkg/cli/experiments_command_test.go index 4470849536a..033042c1484 100644 --- a/pkg/cli/experiments_command_test.go +++ b/pkg/cli/experiments_command_test.go @@ -43,6 +43,24 @@ func TestBuildSafeGitShowObjectArg(t *testing.T) { 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", @@ -76,6 +94,31 @@ func TestBuildSafeGitShowObjectArg(t *testing.T) { } } +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