Harden command argument validation for pip/uv checks and npm lockfile execution context

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)
+			}
 		})
 	}
 }

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")
 	}

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) {

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))
 		}