From 27c9aff80490db9190117cd5af37c3e5327bb514 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 26 Mar 2026 05:05:44 +0000 Subject: [PATCH 1/3] fix: reject package/image names starting with '-' to prevent argument injection Adds a prefix check for '-' before passing package names and Docker image names to exec.Command in npm_validation.go, pip_validation.go, and docker_validation.go. A name beginning with '-' would be interpreted as a flag by npm, pip, uv, or docker, causing unintended argument injection. Since exec.Command uses argv directly (not sh -c), this is not shell injection but argument injection. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/6ad34640-b523-4027-bdfc-4ce8e067840e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/docker_validation.go | 5 +++++ pkg/workflow/npm_validation.go | 6 ++++++ pkg/workflow/pip_validation.go | 12 ++++++++++++ 3 files changed, 23 insertions(+) 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/npm_validation.go b/pkg/workflow/npm_validation.go index 20cd5030d7c..94a1a135c0f 100644 --- a/pkg/workflow/npm_validation.go +++ b/pkg/workflow/npm_validation.go @@ -76,6 +76,12 @@ func (c *Compiler) validateNpxPackages(workflowData *WorkflowData) error { for _, pkg := range packages { npmValidationLog.Printf("Validating npm package: %s", pkg) + // Reject names starting with '-' to prevent argument injection + if strings.HasPrefix(pkg, "-") { + errors = append(errors, fmt.Sprintf("npx package name '%s' is invalid: names must not start with '-'", pkg)) + continue + } + // Use npm view to check if package exists cmd := exec.Command("npm", "view", pkg, "name") output, err := cmd.CombinedOutput() diff --git a/pkg/workflow/pip_validation.go b/pkg/workflow/pip_validation.go index f33f37e56a9..228dafdda34 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 '-' - skipping validation", packageType, pkg))) + continue + } + pipValidationLog.Printf("Validating %s package: %s", packageType, pkgName) // Use pip index to check if package exists on PyPI @@ -161,6 +167,12 @@ func (c *Compiler) validateUvPackages(workflowData *WorkflowData) error { pkgName = pkg[:eqIndex] } + // Reject names starting with '-' to prevent argument injection + if strings.HasPrefix(pkgName, "-") { + errors = append(errors, fmt.Sprintf("uv package name '%s' is invalid: names must not start with '-'", pkg)) + continue + } + // Use uv pip show to check if package exists on PyPI cmd := exec.Command("uv", "pip", "show", pkgName, "--no-cache") _, err := cmd.CombinedOutput() From 9a7aca93b18e04cb157f4e401f5370f1f788b37c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 26 Mar 2026 05:06:58 +0000 Subject: [PATCH 2/3] fix: consistent error message format for pip argument injection guard Agent-Logs-Url: https://github.com/github/gh-aw/sessions/6ad34640-b523-4027-bdfc-4ce8e067840e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/pip_validation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/workflow/pip_validation.go b/pkg/workflow/pip_validation.go index 228dafdda34..e8c42c07d82 100644 --- a/pkg/workflow/pip_validation.go +++ b/pkg/workflow/pip_validation.go @@ -61,7 +61,7 @@ func (c *Compiler) validatePythonPackagesWithPip(packages []string, packageType // 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 '-' - skipping validation", packageType, pkg))) + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("%s package name '%s' is invalid: names must not start with '-'", packageType, pkg))) continue } From 9895bbc8811aa4a126c6bb6a0b7865ea6ad92f96 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 26 Mar 2026 05:26:59 +0000 Subject: [PATCH 3/3] test: add argument injection prevention tests - Add name_validation.go with shared rejectHyphenPrefixPackages helper - Refactor npm and uv validators to use the shared helper - Move the name guard before LookPath in both validators (fail-fast, testable) - Add argument_injection_test.go with 12 unit tests covering: - rejectHyphenPrefixPackages (6 cases) - validateDockerImage with hyphen-prefix image names (3 cases) - extractor filtering of hyphen-prefix names for npx/pip (2+2 cases) - collectPackagesFromWorkflow args filtering (1 case) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c33112e5-6785-4175-8878-cec9c4ced1e0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/argument_injection_test.go | 207 ++++++++++++++++++++++++ pkg/workflow/name_validation.go | 44 +++++ pkg/workflow/npm_validation.go | 13 +- pkg/workflow/pip_validation.go | 13 +- 4 files changed, 265 insertions(+), 12 deletions(-) create mode 100644 pkg/workflow/argument_injection_test.go create mode 100644 pkg/workflow/name_validation.go 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/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 94a1a135c0f..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 { @@ -76,12 +83,6 @@ func (c *Compiler) validateNpxPackages(workflowData *WorkflowData) error { for _, pkg := range packages { npmValidationLog.Printf("Validating npm package: %s", pkg) - // Reject names starting with '-' to prevent argument injection - if strings.HasPrefix(pkg, "-") { - errors = append(errors, fmt.Sprintf("npx package name '%s' is invalid: names must not start with '-'", pkg)) - continue - } - // Use npm view to check if package exists cmd := exec.Command("npm", "view", pkg, "name") output, err := cmd.CombinedOutput() diff --git a/pkg/workflow/pip_validation.go b/pkg/workflow/pip_validation.go index e8c42c07d82..e364ac47f46 100644 --- a/pkg/workflow/pip_validation.go +++ b/pkg/workflow/pip_validation.go @@ -129,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 { @@ -167,12 +174,6 @@ func (c *Compiler) validateUvPackages(workflowData *WorkflowData) error { pkgName = pkg[:eqIndex] } - // Reject names starting with '-' to prevent argument injection - if strings.HasPrefix(pkgName, "-") { - errors = append(errors, fmt.Sprintf("uv package name '%s' is invalid: names must not start with '-'", pkg)) - continue - } - // Use uv pip show to check if package exists on PyPI cmd := exec.Command("uv", "pip", "show", pkgName, "--no-cache") _, err := cmd.CombinedOutput()