diff --git a/pkg/workflow/argument_injection_test.go b/pkg/workflow/argument_injection_test.go new file mode 100644 index 00000000000..35eb2728c6d --- /dev/null +++ b/pkg/workflow/argument_injection_test.go @@ -0,0 +1,207 @@ +//go:build !integration + +// Tests for argument injection prevention: package and image names starting with '-' +// must be rejected before being passed to exec.Command calls (npm, pip, uv, docker). + +package workflow + +import ( + "strings" + "testing" +) + +// TestRejectHyphenPrefixPackages tests the shared helper that guards against +// argument injection in all package validation paths. +func TestRejectHyphenPrefixPackages(t *testing.T) { + tests := []struct { + name string + packages []string + kind string + expectError bool + errContains string + }{ + { + name: "empty list is accepted", + packages: []string{}, + kind: "npx", + expectError: false, + }, + { + name: "valid package names are accepted", + packages: []string{"express", "@scope/pkg", "requests==2.28.0"}, + kind: "pip", + expectError: false, + }, + { + name: "single hyphen prefix is rejected", + packages: []string{"-exploit"}, + kind: "npx", + expectError: true, + errContains: "must not start with '-'", + }, + { + name: "double hyphen prefix is rejected", + packages: []string{"--privileged"}, + kind: "uv", + expectError: true, + errContains: "must not start with '-'", + }, + { + name: "mixed list with one invalid is rejected", + packages: []string{"valid-pkg", "-exploit"}, + kind: "pip", + expectError: true, + errContains: "-exploit", + }, + { + name: "package with version specifier starting with hyphen is rejected", + packages: []string{"-exploit==1.0"}, + kind: "uv", + expectError: true, + errContains: "-exploit==1.0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := rejectHyphenPrefixPackages(tt.packages, tt.kind) + if tt.expectError { + if err == nil { + t.Errorf("expected error for packages %v but got none", tt.packages) + return + } + if tt.errContains != "" && !strings.Contains(err.Error(), tt.errContains) { + t.Errorf("expected error to contain %q, got: %v", tt.errContains, err) + } + } else { + if err != nil { + t.Errorf("unexpected error for packages %v: %v", tt.packages, err) + } + } + }) + } +} + +// TestValidateDockerImage_RejectsHyphenPrefix tests that Docker image names starting +// with '-' are rejected without invoking docker. This is the primary injection path +// because image names are taken directly from frontmatter (not filtered by extractors). +func TestValidateDockerImage_RejectsHyphenPrefix(t *testing.T) { + tests := []struct { + name string + image string + errContains string + }{ + { + name: "single hyphen prefix", + image: "-exploit", + errContains: "must not start with '-'", + }, + { + name: "double hyphen prefix", + image: "--privileged", + errContains: "must not start with '-'", + }, + { + name: "looks like a docker flag", + image: "-rm", + errContains: "must not start with '-'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateDockerImage(tt.image, false) + if err == nil { + t.Errorf("expected error for image %q but got none", tt.image) + return + } + if !strings.Contains(err.Error(), tt.errContains) { + t.Errorf("expected error to contain %q, got: %v", tt.errContains, err) + } + }) + } +} + +// TestExtractNpxFromCommands_HyphenPrefixFiltered shows that the npx package +// extractor already filters out names starting with '-' (treating them as flags), +// so they never reach the validation guard. +func TestExtractNpxFromCommands_HyphenPrefixFiltered(t *testing.T) { + tests := []struct { + name string + commands string + wantLen int + }{ + { + name: "lone hyphen-prefix arg yields no packages", + commands: "npx -exploit", + wantLen: 0, + }, + { + name: "double-hyphen arg yields no packages", + commands: "npx --exploit", + wantLen: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + packages := extractNpxFromCommands(tt.commands) + if len(packages) != tt.wantLen { + t.Errorf("expected %d packages, got %d: %v", tt.wantLen, len(packages), packages) + } + }) + } +} + +// TestExtractPipFromCommands_HyphenPrefixFiltered shows that the pip package +// extractor already filters out names starting with '-', so they never reach +// the validation guard. +func TestExtractPipFromCommands_HyphenPrefixFiltered(t *testing.T) { + tests := []struct { + name string + commands string + wantLen int + }{ + { + name: "hyphen-prefix pip package yields no packages", + commands: "pip install -exploit", + wantLen: 0, + }, + { + name: "double-hyphen pip package yields no packages", + commands: "pip install --exploit", + wantLen: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + packages := extractPipFromCommands(tt.commands) + if len(packages) != tt.wantLen { + t.Errorf("expected %d packages, got %d: %v", tt.wantLen, len(packages), packages) + } + }) + } +} + +// TestCollectPackagesFromWorkflow_HyphenPrefixInArgs verifies that the MCP tool +// args extractor also filters out names starting with '-', providing an additional +// layer of defense for the structured args format. +func TestCollectPackagesFromWorkflow_HyphenPrefixInArgs(t *testing.T) { + workflowData := &WorkflowData{ + Tools: map[string]any{ + "test-tool": map[string]any{ + "command": "npx", + "args": []any{"-exploit", "safe-package"}, + }, + }, + } + + packages := collectPackagesFromWorkflow(workflowData, extractNpxFromCommands, "npx") + + for _, pkg := range packages { + if strings.HasPrefix(pkg, "-") { + t.Errorf("package starting with '-' should not be collected: %s", pkg) + } + } +} diff --git a/pkg/workflow/docker_validation.go b/pkg/workflow/docker_validation.go index f2cdfb6cfb8..b755875e669 100644 --- a/pkg/workflow/docker_validation.go +++ b/pkg/workflow/docker_validation.go @@ -89,6 +89,11 @@ func isDockerDaemonRunning() bool { func validateDockerImage(image string, verbose bool) error { dockerValidationLog.Printf("Validating Docker image: %s", image) + // Reject names starting with '-' to prevent argument injection + if strings.HasPrefix(image, "-") { + return fmt.Errorf("container image name '%s' is invalid: names must not start with '-'", image) + } + // Check if docker CLI is available on PATH _, err := exec.LookPath("docker") if err != nil { diff --git a/pkg/workflow/name_validation.go b/pkg/workflow/name_validation.go new file mode 100644 index 00000000000..b715bb67bf6 --- /dev/null +++ b/pkg/workflow/name_validation.go @@ -0,0 +1,44 @@ +//go:build !js && !wasm + +// This file provides package and image name validation utilities for agentic workflows. +// +// # Name Validation +// +// Package and image names passed to external tools (npm, pip, uv, docker) must not +// start with '-'. A leading '-' would be interpreted as a command-line flag by the +// downstream tool, causing unintended argument injection. +// +// Note: exec.Command uses argv directly (not sh -c), so this is argument injection, +// not shell injection. The risk is low — compilation runs on the developer's local +// machine with the developer's own privileges. + +package workflow + +import ( + "fmt" + "strings" +) + +// rejectHyphenPrefixPackages returns a ValidationError if any of the provided +// names starts with '-'. The kind parameter (e.g. "npx", "pip", "uv") is used +// in the error messages. +// +// Names starting with '-' would be interpreted as flags by the downstream CLI +// tool, constituting argument injection into the exec.Command call. +func rejectHyphenPrefixPackages(names []string, kind string) error { + var invalid []string + for _, name := range names { + if strings.HasPrefix(name, "-") { + invalid = append(invalid, fmt.Sprintf("%s package name '%s' is invalid: names must not start with '-'", kind, name)) + } + } + if len(invalid) == 0 { + return nil + } + return NewValidationError( + kind+".packages", + fmt.Sprintf("%d invalid package names", len(invalid)), + kind+" package names must not start with '-'", + "Fix invalid package names:\n\n"+strings.Join(invalid, "\n"), + ) +} diff --git a/pkg/workflow/npm_validation.go b/pkg/workflow/npm_validation.go index 20cd5030d7c..0e3c925a8f9 100644 --- a/pkg/workflow/npm_validation.go +++ b/pkg/workflow/npm_validation.go @@ -65,6 +65,13 @@ func (c *Compiler) validateNpxPackages(workflowData *WorkflowData) error { npmValidationLog.Printf("Validating %d npx packages", len(packages)) + // Reject any package names starting with '-' before invoking npm. + // These would be interpreted as flags by the npm CLI (argument injection). + if err := rejectHyphenPrefixPackages(packages, "npx"); err != nil { + npmValidationLog.Printf("npx package name validation failed: %v", err) + return err + } + // Check if npm is available _, err := exec.LookPath("npm") if err != nil { diff --git a/pkg/workflow/pip_validation.go b/pkg/workflow/pip_validation.go index f33f37e56a9..e364ac47f46 100644 --- a/pkg/workflow/pip_validation.go +++ b/pkg/workflow/pip_validation.go @@ -59,6 +59,12 @@ func (c *Compiler) validatePythonPackagesWithPip(packages []string, packageType pkgName = pkg[:eqIndex] } + // 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 + } + pipValidationLog.Printf("Validating %s package: %s", packageType, pkgName) // Use pip index to check if package exists on PyPI @@ -123,6 +129,13 @@ func (c *Compiler) validateUvPackages(workflowData *WorkflowData) error { pipValidationLog.Printf("Starting uv package validation for %d packages", len(packages)) + // Reject any package names starting with '-' before invoking uv or pip. + // These would be interpreted as flags by the CLI tools (argument injection). + if err := rejectHyphenPrefixPackages(packages, "uv"); err != nil { + pipValidationLog.Printf("uv package name validation failed: %v", err) + return err + } + // Check if uv is available _, err := exec.LookPath("uv") if err != nil {