diff --git a/pkg/workflow/argument_injection_test.go b/pkg/workflow/argument_injection_test.go index 0f79b59e1a1..19e9814e217 100644 --- a/pkg/workflow/argument_injection_test.go +++ b/pkg/workflow/argument_injection_test.go @@ -412,6 +412,33 @@ func TestValidatePipPackageName(t *testing.T) { t.Errorf("unexpected error for package %q: %v", tt.pkg, err) } } + + }) + } +} + +func TestValidatePipCommandPackageArg(t *testing.T) { + tests := []struct { + name string + pkg string + expectError bool + }{ + {name: "valid package", pkg: "requests"}, + {name: "rejects hyphen prefix", pkg: "--index-url", expectError: true}, + {name: "rejects control characters", pkg: "pkg\nname", expectError: true}, + {name: "rejects whitespace", pkg: "pkg name", expectError: true}, + {name: "rejects shell separators", pkg: "pkg;whoami", expectError: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validatePipCommandPackageArg(tt.pkg) + if tt.expectError && err == nil { + t.Fatalf("expected error for package %q", tt.pkg) + } + if !tt.expectError && err != nil { + t.Fatalf("unexpected error for package %q: %v", tt.pkg, err) + } }) } } diff --git a/pkg/workflow/dependabot_manifests.go b/pkg/workflow/dependabot_manifests.go index a5809860150..f51b419913f 100644 --- a/pkg/workflow/dependabot_manifests.go +++ b/pkg/workflow/dependabot_manifests.go @@ -245,6 +245,25 @@ func (c *Compiler) generatePackageJSON(path string, deps []NpmDependency, forceO func (c *Compiler) generatePackageLock(workflowDir string) error { dependabotLog.Printf("Generating package-lock.json in %s", workflowDir) + if strings.TrimSpace(workflowDir) == "" { + return fmt.Errorf("invalid workflow directory %q: must not be empty or whitespace", workflowDir) + } + absWorkflowDir, err := filepath.Abs(workflowDir) + if err != nil { + return fmt.Errorf("failed to resolve workflow directory %q: %w", workflowDir, err) + } + absWorkflowDir, err = fileutil.ValidateAbsolutePath(absWorkflowDir) + if err != nil { + return fmt.Errorf("invalid workflow directory %q: %w", workflowDir, err) + } + info, err := os.Stat(absWorkflowDir) + if err != nil { + return fmt.Errorf("failed to stat workflow directory %q: %w", absWorkflowDir, err) + } + if !info.IsDir() { + return fmt.Errorf("workflow directory %q is not a directory", absWorkflowDir) + } + // Check if npm is available npmPath, err := fileutil.ResolveExecutablePath("npm") if err != nil { @@ -261,7 +280,7 @@ func (c *Compiler) generatePackageLock(workflowDir string) error { // #nosec G204 -- npmPath is resolved by exec.LookPath and validated as an absolute path above; // the fixed arguments contain no user-controlled data. cmd := exec.Command(npmPath, "install", "--package-lock-only", "--ignore-scripts") - cmd.Dir = workflowDir + cmd.Dir = absWorkflowDir cmd.Env = append(os.Environ(), "NPM_CONFIG_IGNORE_SCRIPTS=true") // Capture output for error reporting @@ -270,7 +289,7 @@ func (c *Compiler) generatePackageLock(workflowDir string) error { return fmt.Errorf("npm install --package-lock-only failed: %w\nOutput: %s", err, string(output)) } - lockfilePath := filepath.Join(workflowDir, "package-lock.json") + lockfilePath := filepath.Join(absWorkflowDir, "package-lock.json") if _, err := os.Stat(lockfilePath); err != nil { return errors.New("package-lock.json was not created") } diff --git a/pkg/workflow/dependabot_test.go b/pkg/workflow/dependabot_test.go index 8a501c90e5c..37de997f5ea 100644 --- a/pkg/workflow/dependabot_test.go +++ b/pkg/workflow/dependabot_test.go @@ -703,6 +703,71 @@ touch package-lock.json } } +func TestGeneratePackageLock_UsesNormalizedWorkflowDir(t *testing.T) { + compiler := NewCompiler() + parentDir := testutil.TempDir(t, "parent-*") + workflowDir := filepath.Join(parentDir, "workflow") + fakeBinDir := testutil.TempDir(t, "fake-bin-*") + + if err := os.Mkdir(workflowDir, 0o755); err != nil { + t.Fatalf("failed to create workflow directory: %v", err) + } + + pwdFile := filepath.Join(parentDir, "npm-pwd.txt") + fakeNpm := filepath.Join(fakeBinDir, "npm") + script := `#!/bin/sh +pwd > "$GH_AW_TEST_PWD_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_PWD_FILE", pwdFile) + t.Chdir(parentDir) + + if err := compiler.generatePackageLock("workflow"); err != nil { + t.Fatalf("generatePackageLock() error = %v", err) + } + + pwdData, err := os.ReadFile(pwdFile) + if err != nil { + t.Fatalf("failed to read recorded npm working directory: %v", err) + } + if strings.TrimSpace(string(pwdData)) != workflowDir { + t.Fatalf("expected npm to run in %q, got %q", workflowDir, strings.TrimSpace(string(pwdData))) + } + if _, err := os.Stat(filepath.Join(workflowDir, "package-lock.json")); err != nil { + t.Fatalf("expected package-lock.json in normalized workflow directory: %v", err) + } +} + +func TestGeneratePackageLock_RejectsInvalidWorkflowDir(t *testing.T) { + compiler := NewCompiler() + + tests := []struct { + name string + workflowDir string + }{ + {name: "empty", workflowDir: ""}, + {name: "whitespace", workflowDir: " "}, + {name: "control character", workflowDir: "bad\nworkflow-dir"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := compiler.generatePackageLock(tt.workflowDir) + if err == nil { + t.Fatal("expected error for invalid workflow directory") + } + if !strings.Contains(err.Error(), "invalid workflow directory") { + t.Fatalf("expected invalid workflow directory error, got: %v", err) + } + }) + } +} + // Tests for Python (pip) support func TestParsePipPackage(t *testing.T) { diff --git a/pkg/workflow/pip_validation.go b/pkg/workflow/pip_validation.go index 0ce72bd6c16..280095ba3b5 100644 --- a/pkg/workflow/pip_validation.go +++ b/pkg/workflow/pip_validation.go @@ -42,6 +42,7 @@ import ( "os" "os/exec" "strings" + "unicode" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/fileutil" @@ -50,6 +51,16 @@ import ( var pipValidationLog = logger.New("workflow:pip_validation") +func validatePipCommandPackageArg(pkgName string) error { + if strings.HasPrefix(pkgName, "-") { + return errors.New("names must not start with '-'") + } + if strings.IndexFunc(pkgName, unicode.IsControl) >= 0 { + return errors.New("names must not contain control characters") + } + return validatePipPackageName(pkgName) +} + // validatePythonPackagesWithPip is a generic helper that validates Python packages using pip index. // It accepts a package list, package type name for error messaging, and a validated pip executable path. func (c *Compiler) validatePythonPackagesWithPip(packages []string, packageType string, pipPath string) { @@ -60,15 +71,9 @@ func (c *Compiler) validatePythonPackagesWithPip(packages []string, packageType // or uvx-style "@version", e.g. "ruff@0.1.0"). pkgName := stripUvPackageVersion(pkg) - // Reject names starting with '-' to prevent argument injection - if strings.HasPrefix(pkgName, "-") { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("%s package name '%s' is invalid: names must not start with '-'", packageType, pkg))) - continue - } - // Validate the package name against PyPI naming rules (PEP 508). // pip does not universally honour '--', so we validate upfront. - if err := validatePipPackageName(pkgName); err != nil { + if err := validatePipCommandPackageArg(pkgName); err != nil { fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("%s package name '%s' is invalid: %v", packageType, pkg, err))) continue } @@ -165,7 +170,7 @@ func (c *Compiler) validateUvPackages(workflowData *WorkflowData) error { var invalidNameErrors []string for _, pkg := range packages { pkgName := stripUvPackageVersion(pkg) - if err := validatePipPackageName(pkgName); err != nil { + if err := validatePipCommandPackageArg(pkgName); err != nil { pipValidationLog.Printf("Invalid uv package name %s: %v", pkgName, err) invalidNameErrors = append(invalidNameErrors, fmt.Sprintf("uv package '%s' is invalid: %v", pkg, err)) }