diff --git a/docs/adr/49063-harden-docker-argument-validation-against-injection.md b/docs/adr/49063-harden-docker-argument-validation-against-injection.md new file mode 100644 index 00000000000..160ca07cc5f --- /dev/null +++ b/docs/adr/49063-harden-docker-argument-validation-against-injection.md @@ -0,0 +1,52 @@ +# ADR-49063: Harden Docker Argument Validation Against Command Injection + +**Date**: 2026-07-30 +**Status**: Accepted +**Deciders**: pelikhan, Copilot + +--- + +### Context + +Sighthound's static security scan flagged 33 Critical CWE-78 (OS Command Injection) findings in the `pkg/cli` package. Five high-severity findings were in the Docker exec paths of `runner_guard.go`, `grant.go`, `poutine.go`, and the self-relaunch path of `upgrade_command.go`. In these paths, Docker volume mount strings were assembled by direct string concatenation (for example, `gitRoot + ":/workdir"`), and some callers still relied on `exec.Command("docker", ...)` PATH resolution at launch time. While `exec.Command` avoids shell interpretation, unsanitized mount arguments can still be re-parsed by Docker's colon-delimited `-v` syntax, and copyable verbose commands can become unsafe when user-controlled values are interpolated without shell quoting. The fix needed to be applied consistently across multiple scanners with minimal duplication. + +### Decision + +We will centralize Docker `-v` argument construction into shared helpers (`buildDockerVolumeMount`, `buildDockerReadonlyFileMount`) in `pkg/cli/docker_args_validation.go` that enforce absolute-path validation, container-path normalization, rejection of control characters and colon-delimiter injection, and file-type checks. On the host side, callers will reject mount paths containing unsupported `:` characters while preserving Windows drive-letter paths; on the container side, callers will reject any colon because Docker would reinterpret it as an extra mount segment. All Docker exec sites in `grant.go`, `poutine.go`, and `runner_guard.go` will use these helpers instead of string concatenation, and every scanner will resolve the Docker executable through `fileutil.ResolveExecutablePath("docker")` before launch. The self-relaunch path in `upgrade_command.go` will reject any argument containing a NUL byte before passing it to `exec.Command`. Verbose "run this directly" hints for the Grant and Poutine scanners will be rendered with shell-escaped arguments so they remain copyable without reintroducing injection risk in documentation output. + +### Alternatives Considered + +#### Alternative 1: Per-function inline validation (status quo + extensions) + +Each scanner function continues to perform its own validation inline, as `runner_guard.go` already did for absolute-path checks. Validation logic would be duplicated across `grant.go`, `poutine.go`, and `runner_guard.go`. Not chosen because it produces inconsistent coverage (each site may miss different edge cases), makes auditing harder, and does not consolidate error handling in a single testable unit. + +#### Alternative 2: Switch all scanners to Docker `--mount` syntax + +Replace every `-v host:container[:ro]` mount with `--mount type=bind,src=...,dst=...[,readonly]` so colons in host paths do not need special-case handling. This was rejected for this fix because the existing scanners already share `-v`-style call sites and test expectations, and the smallest safe patch was to harden those exact paths by rejecting ambiguous colon-delimited inputs. A broader `--mount` migration remains possible later if we want to permit more host-path shapes. + +#### Alternative 3: Shell-escape / shlex sanitization before concatenation + +Apply a shlex-style escaping function to volume mount components before string concatenation, similar to how some container runtimes sanitize arguments. Not chosen because `exec.Command` already avoids shell interpretation — the risk is not shell metacharacters but rather structural injection into Docker's `-v` argument parsing and NUL bytes in argument vectors. Escaping addresses the wrong threat model and masks invalid input rather than rejecting it. + +### Consequences + +#### Positive +- Eliminates the CWE-78 Critical findings in Docker exec paths by ensuring all volume mount strings are constructed from validated, normalized, absolute paths. +- Centralizes validation logic into a single package-internal file, making future audits and policy changes a one-location edit. +- Prevents ambiguous Docker `-v` parsing by rejecting colon-bearing container paths and unsupported host-path colons before launch, instead of relying on Docker's own mount parser to fail safely. +- Aligns Poutine with Grant and Runner Guard by resolving the Docker executable path explicitly before `exec.Command`, closing the remaining PATH-hijack gap in this hardening pass. +- Adds explicit NUL byte rejection in the self-relaunch path, preventing argument injection in process re-exec scenarios. +- Adds unit tests for the shared helpers, colon/control-character rejection paths, verbose command quoting, and the image reference / NUL byte validations, increasing coverage of security-critical paths. + +#### Negative +- Each `buildDockerReadonlyFileMount` call now performs an `os.Stat` syscall to verify the host file is a regular file; this adds a small overhead on every Docker scanner invocation. +- The new helpers return errors that all callers must propagate, increasing the error-path surface in functions that previously could not fail at mount-construction time. +- Callers must now satisfy stricter pre-conditions (normalized absolute paths and unambiguous `-v` mount components); any future refactor that produces relative paths or colon-bearing mount targets will fail at runtime rather than silently passing an invalid mount string. + +#### Neutral +- The `pkg/cli` package gains a new internal dependency on `pkg/fileutil` (already a transitive dependency via other files in the package). +- `grant.go` and `poutine.go` now resolve the `docker` binary via `fileutil.ResolveExecutablePath` rather than relying on the ambient PATH; this is a behavioral change that could surface errors on systems where `docker` is not on PATH. + +--- + +*Accepted after implementation and conformance validation.* diff --git a/pkg/cli/audit_test.go b/pkg/cli/audit_test.go index 1d140cd28c7..d5ff9d6ec9f 100644 --- a/pkg/cli/audit_test.go +++ b/pkg/cli/audit_test.go @@ -527,6 +527,9 @@ func TestAuditCachingBehavior(t *testing.T) { if _, err := os.Stat(summaryPath); os.IsNotExist(err) { t.Fatalf("Run summary file should exist after saveRunSummary") } + if err := markArtifactDownloaded(runOutputDir, string(ArtifactSetAll)); err != nil { + t.Fatalf("markArtifactDownloaded: %v", err) + } // Load the summary back loadedSummary, ok := loadRunSummary(runOutputDir, false) diff --git a/pkg/cli/docker_args_validation.go b/pkg/cli/docker_args_validation.go new file mode 100644 index 00000000000..396b58ba548 --- /dev/null +++ b/pkg/cli/docker_args_validation.go @@ -0,0 +1,80 @@ +package cli + +import ( + "errors" + "fmt" + "os" + "path" + "strings" + + "github.com/github/gh-aw/pkg/fileutil" +) + +func validateContainerMountPath(containerPath string) (string, error) { + if containerPath == "" { + return "", errors.New("container path cannot be empty. Example: /workdir") + } + if strings.ContainsAny(containerPath, "\x00\r\n:") { + return "", errors.New("container path contains invalid control characters or reserved characters. Example: /workdir") + } + if !path.IsAbs(containerPath) { + return "", fmt.Errorf("container path must be absolute. Example: /workdir. Got: %s", containerPath) + } + cleanPath := path.Clean(containerPath) + if cleanPath != containerPath { + return "", fmt.Errorf("container path must be normalized. Example: /workdir/config. Got: %s", containerPath) + } + return cleanPath, nil +} + +func validateHostMountPath(hostPath string) (string, error) { + cleanHostPath, err := fileutil.ValidateAbsolutePath(hostPath) + if err != nil { + return "", fmt.Errorf("invalid host path %q: %w", hostPath, err) + } + if strings.Contains(cleanHostPath[2:], ":") || (!isWindowsDrivePath(cleanHostPath) && strings.Contains(cleanHostPath, ":")) { + return "", fmt.Errorf("host path contains unsupported ':' for docker -v mount syntax. Example: /tmp/repo or C:/repo. Got: %s", cleanHostPath) + } + return cleanHostPath, nil +} + +func isWindowsDrivePath(hostPath string) bool { + if len(hostPath) < 3 { + return false + } + driveLetter := hostPath[0] + return ((driveLetter >= 'a' && driveLetter <= 'z') || (driveLetter >= 'A' && driveLetter <= 'Z')) && + hostPath[1] == ':' && + (hostPath[2] == '\\' || hostPath[2] == '/') +} + +func buildDockerVolumeMount(hostPath, containerPath string) (string, error) { + cleanHostPath, err := validateHostMountPath(hostPath) + if err != nil { + return "", err + } + cleanContainerPath, err := validateContainerMountPath(containerPath) + if err != nil { + return "", err + } + return cleanHostPath + ":" + cleanContainerPath, nil +} + +func buildDockerReadonlyFileMount(hostFile, containerPath string) (string, error) { + cleanHostFile, err := validateHostMountPath(hostFile) + if err != nil { + return "", fmt.Errorf("invalid host file %q: %w", hostFile, err) + } + info, err := os.Stat(cleanHostFile) + if err != nil { + return "", fmt.Errorf("failed to stat host file %q: %w", cleanHostFile, err) + } + if !info.Mode().IsRegular() { + return "", fmt.Errorf("host file is not a regular file: %s", cleanHostFile) + } + cleanContainerPath, err := validateContainerMountPath(containerPath) + if err != nil { + return "", err + } + return cleanHostFile + ":" + cleanContainerPath + ":ro", nil +} diff --git a/pkg/cli/docker_args_validation_test.go b/pkg/cli/docker_args_validation_test.go new file mode 100644 index 00000000000..45e2bf3f2f9 --- /dev/null +++ b/pkg/cli/docker_args_validation_test.go @@ -0,0 +1,64 @@ +//go:build !integration + +package cli + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestBuildDockerVolumeMount(t *testing.T) { + tmpDir := t.TempDir() + + mount, err := buildDockerVolumeMount(tmpDir, "/workdir") + require.NoError(t, err) + require.Equal(t, tmpDir+":/workdir", mount) + + _, err = buildDockerVolumeMount("relative/path", "/workdir") + require.Error(t, err) + require.ErrorContains(t, err, "invalid host path") + + _, err = buildDockerVolumeMount(tmpDir, "workdir") + require.Error(t, err) + require.ErrorContains(t, err, "container path must be absolute") + + _, err = buildDockerVolumeMount(tmpDir, "/work:dir") + require.Error(t, err) + require.ErrorContains(t, err, "reserved characters") + + _, err = buildDockerVolumeMount(tmpDir, "/work\x00dir") + require.Error(t, err) + require.ErrorContains(t, err, "invalid control characters") + + if runtime.GOOS != "windows" { + _, err = buildDockerVolumeMount("/tmp/repo:fixture", "/workdir") + require.Error(t, err) + require.ErrorContains(t, err, "unsupported ':'") + } +} + +func TestBuildDockerReadonlyFileMount(t *testing.T) { + tmpDir := t.TempDir() + policyFile := filepath.Join(tmpDir, ".grant.yaml") + require.NoError(t, os.WriteFile(policyFile, []byte("policy: true\n"), 0o644)) + + mount, err := buildDockerReadonlyFileMount(policyFile, "/tmp/policy.yaml") + require.NoError(t, err) + require.Equal(t, policyFile+":/tmp/policy.yaml:ro", mount) + + _, err = buildDockerReadonlyFileMount(tmpDir, "/tmp/policy.yaml") + require.Error(t, err) + require.ErrorContains(t, err, "not a regular file") + + _, err = buildDockerReadonlyFileMount(policyFile, "/tmp/policy:yaml") + require.Error(t, err) + require.ErrorContains(t, err, "reserved characters") + + _, err = buildDockerReadonlyFileMount(policyFile, "/tmp/policy\x00yaml") + require.Error(t, err) + require.ErrorContains(t, err, "invalid control characters") +} diff --git a/pkg/cli/grant.go b/pkg/cli/grant.go index 4f919e8005a..4ace357860a 100644 --- a/pkg/cli/grant.go +++ b/pkg/cli/grant.go @@ -11,6 +11,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/fileutil" "github.com/github/gh-aw/pkg/gitutil" "github.com/github/gh-aw/pkg/logger" ) @@ -154,13 +155,31 @@ func grantPolicyFile() (string, error) { func grantRunOnImage(imageRef, policyFile string, verbose bool) (*grantOutput, error) { containerPolicyPath := grantContainerPolicyPath + imageRef = strings.TrimSpace(imageRef) + if imageRef == "" { + return nil, errors.New("grant image reference cannot be empty. Example: ghcr.io/example/image:tag") + } + if strings.ContainsAny(imageRef, " \t\r\n\x00") { + return nil, fmt.Errorf("grant image reference contains invalid whitespace/control characters. Example: ghcr.io/example/image:tag. Got: %q", imageRef) + } + + dockerPath, err := fileutil.ResolveExecutablePath("docker") + if err != nil { + return nil, fmt.Errorf("docker command not found: %w", err) + } + + volumeMount, err := buildDockerReadonlyFileMount(policyFile, containerPolicyPath) + if err != nil { + return nil, fmt.Errorf("invalid grant policy mount: %w", err) + } + // #nosec G204 -- imageRef and policyFile are derived from compiled lock files and the // current repository checkout. exec.Command passes arguments directly without a shell. cmd := exec.Command( - "docker", + dockerPath, "run", "--rm", - "-v", policyFile+":"+containerPolicyPath+":ro", + "-v", volumeMount, GrantImage, "--config", containerPolicyPath, "--output", "json", @@ -169,8 +188,17 @@ func grantRunOnImage(imageRef, policyFile string, verbose bool) (*grantOutput, e ) if verbose { - dockerCmd := fmt.Sprintf("docker run --rm -v %s:%s:ro %s --config %s --output json check %s", - policyFile, containerPolicyPath, GrantImage, containerPolicyPath, imageRef) + dockerCmd := shellJoinArgs([]string{ + "docker", + "run", + "--rm", + "-v", volumeMount, + GrantImage, + "--config", containerPolicyPath, + "--output", "json", + "check", + imageRef, + }) fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Run grant directly: "+dockerCmd)) } diff --git a/pkg/cli/grant_test.go b/pkg/cli/grant_test.go index 052a89529c0..3e31c9f2195 100644 --- a/pkg/cli/grant_test.go +++ b/pkg/cli/grant_test.go @@ -3,8 +3,12 @@ package cli import ( + "os" "path/filepath" + "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestGrantDisplayFindings_NilOutput(t *testing.T) { @@ -73,3 +77,52 @@ func TestGrantPolicyFile(t *testing.T) { t.Fatalf("Expected policy file basename %q, got %q", grantPolicyFilename, filepath.Base(policyFile)) } } + +func TestGrantRunOnImageRejectsInvalidImageRef(t *testing.T) { + tmpDir := t.TempDir() + policyFile := filepath.Join(tmpDir, "policy.yaml") + require.NoError(t, os.WriteFile(policyFile, []byte("policy: true\n"), 0o644)) + + testCases := []struct { + name string + imageRef string + want string + }{ + {name: "whitespace", imageRef: "bad image", want: "invalid whitespace/control characters"}, + {name: "empty", imageRef: "", want: "cannot be empty"}, + {name: "null byte", imageRef: "img\x00ref", want: "invalid whitespace/control characters"}, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + _, err := grantRunOnImage(tt.imageRef, policyFile, false) + require.Error(t, err) + require.ErrorContains(t, err, tt.want) + }) + } +} + +func TestGrantRunOnImageVerboseCommandEscapesImageRef(t *testing.T) { + tmpDir := t.TempDir() + policyFile := filepath.Join(tmpDir, "policy file.yaml") + require.NoError(t, os.WriteFile(policyFile, []byte("policy: true\n"), 0o644)) + + volumeMount, err := buildDockerReadonlyFileMount(policyFile, grantContainerPolicyPath) + require.NoError(t, err) + + command := shellJoinArgs([]string{ + "docker", + "run", + "--rm", + "-v", volumeMount, + GrantImage, + "--config", grantContainerPolicyPath, + "--output", "json", + "check", + "alpine;id", + }) + + if !strings.Contains(command, "'alpine;id'") { + t.Fatalf("Expected image ref to be shell-escaped in verbose command, got: %s", command) + } +} diff --git a/pkg/cli/poutine.go b/pkg/cli/poutine.go index 5226b52c80a..21beb2d1842 100644 --- a/pkg/cli/poutine.go +++ b/pkg/cli/poutine.go @@ -84,8 +84,9 @@ func runPoutineOnDirectory(workflowDir string, verbose bool, strict bool) error } // Validate gitRoot is an absolute path (security: ensure trusted path from git) - if !filepath.IsAbs(gitRoot) { - return fmt.Errorf("git root is not an absolute path: %s", gitRoot) + gitRoot, err = fileutil.ValidateAbsolutePath(gitRoot) + if err != nil { + return fmt.Errorf("invalid git root %q: %w", gitRoot, err) } // Ensure poutine config exists with custom runner configuration @@ -97,11 +98,19 @@ func runPoutineOnDirectory(workflowDir string, verbose bool, strict bool) error // docker run --rm -v "$(pwd)":/workdir -w /workdir ghcr.io/boostsecurityio/poutine:latest analyze_local . --format json // #nosec G204 -- gitRoot comes from git rev-parse (trusted source) and is validated as absolute path // exec.Command with separate args (not shell execution) prevents command injection + volumeMount, err := buildDockerVolumeMount(gitRoot, "/workdir") + if err != nil { + return fmt.Errorf("invalid docker mount path: %w", err) + } + dockerPath, err := fileutil.ResolveExecutablePath("docker") + if err != nil { + return fmt.Errorf("docker command not found: %w", err) + } cmd := exec.Command( - "docker", + dockerPath, "run", "--rm", - "-v", gitRoot+":/workdir", + "-v", volumeMount, "-w", "/workdir", "ghcr.io/boostsecurityio/poutine:latest", "analyze_local", @@ -115,8 +124,18 @@ func runPoutineOnDirectory(workflowDir string, verbose bool, strict bool) error // In verbose mode, also show the command that users can run directly if verbose { - dockerCmd := fmt.Sprintf("docker run --rm -v \"%s:/workdir\" -w /workdir ghcr.io/boostsecurityio/poutine:latest analyze_local . --format json --quiet", - gitRoot) + dockerCmd := shellJoinArgs([]string{ + "docker", + "run", + "--rm", + "-v", volumeMount, + "-w", "/workdir", + "ghcr.io/boostsecurityio/poutine:latest", + "analyze_local", + ".", + "--format", "json", + "--quiet", + }) fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage("Run poutine directly: "+dockerCmd)) } @@ -152,7 +171,7 @@ func runPoutineOnDirectory(workflowDir string, verbose bool, strict bool) error if exitCode == 1 { // In strict mode, any findings in the scan are treated as errors if strict && totalWarnings > 0 { - return fmt.Errorf("strict mode: poutine found %d security warnings/errors - workflows must have no poutine findings in strict mode", totalWarnings) + return fmt.Errorf("strict mode: poutine found %d security warnings/errors - workflows must have no poutine findings in strict mode. Example: rerun after resolving all reported findings", totalWarnings) } // In non-strict mode, findings are logged but not treated as errors return nil @@ -179,8 +198,9 @@ func runPoutineOnFile(lockFile string, verbose bool, strict bool) error { } // Validate gitRoot is an absolute path (security: ensure trusted path from git) - if !filepath.IsAbs(gitRoot) { - return fmt.Errorf("git root is not an absolute path: %s", gitRoot) + gitRoot, err = fileutil.ValidateAbsolutePath(gitRoot) + if err != nil { + return fmt.Errorf("invalid git root %q: %w", gitRoot, err) } // Ensure poutine config exists with custom runner configuration @@ -198,11 +218,19 @@ func runPoutineOnFile(lockFile string, verbose bool, strict bool) error { // docker run --rm -v "$(pwd)":/workdir -w /workdir ghcr.io/boostsecurityio/poutine:latest analyze_local . --format json // #nosec G204 -- gitRoot comes from git rev-parse (trusted source) and is validated as absolute path // exec.Command with separate args (not shell execution) prevents command injection + volumeMount, err := buildDockerVolumeMount(gitRoot, "/workdir") + if err != nil { + return fmt.Errorf("invalid docker mount path: %w", err) + } + dockerPath, err := fileutil.ResolveExecutablePath("docker") + if err != nil { + return fmt.Errorf("docker command not found: %w", err) + } cmd := exec.Command( - "docker", + dockerPath, "run", "--rm", - "-v", gitRoot+":/workdir", + "-v", volumeMount, "-w", "/workdir", "ghcr.io/boostsecurityio/poutine:latest", "analyze_local", @@ -216,8 +244,18 @@ func runPoutineOnFile(lockFile string, verbose bool, strict bool) error { // In verbose mode, also show the command that users can run directly if verbose { - dockerCmd := fmt.Sprintf("docker run --rm -v \"%s:/workdir\" -w /workdir ghcr.io/boostsecurityio/poutine:latest analyze_local . --format json --quiet", - gitRoot) + dockerCmd := shellJoinArgs([]string{ + "docker", + "run", + "--rm", + "-v", volumeMount, + "-w", "/workdir", + "ghcr.io/boostsecurityio/poutine:latest", + "analyze_local", + ".", + "--format", "json", + "--quiet", + }) fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage("Run poutine directly: "+dockerCmd)) } @@ -255,7 +293,7 @@ func runPoutineOnFile(lockFile string, verbose bool, strict bool) error { if exitCode == 1 { // In strict mode, any findings in the scan are treated as errors if strict && totalWarnings > 0 { - return fmt.Errorf("strict mode: poutine found %d security warnings/errors in %s - workflows must have no poutine findings in strict mode", totalWarnings, filepath.Base(lockFile)) + return fmt.Errorf("strict mode: poutine found %d security warnings/errors in %s - workflows must have no poutine findings in strict mode. Example: rerun after resolving all reported findings in %s", totalWarnings, filepath.Base(lockFile), filepath.Base(lockFile)) } // In non-strict mode, findings are logged but not treated as errors return nil @@ -283,7 +321,7 @@ func parseAndDisplayPoutineOutput(stdout, targetFile string, verbose bool) (int, if !strings.HasPrefix(trimmed, "{") { // Non-JSON output, likely an error if trimmed != "" { - return 0, fmt.Errorf("unexpected poutine output format: %s", trimmed) + return 0, fmt.Errorf("unexpected poutine output format (expected JSON object). Example: {\"findings\":[]}. Got: %s", trimmed) } return 0, nil } @@ -393,7 +431,7 @@ func parseAndDisplayPoutineOutputForDirectory(stdout string, verbose bool, gitRo if !strings.HasPrefix(trimmed, "{") { // Non-JSON output, likely an error if trimmed != "" { - return 0, fmt.Errorf("unexpected poutine output format: %s", trimmed) + return 0, fmt.Errorf("unexpected poutine output format (expected JSON object). Example: {\"findings\":[]}. Got: %s", trimmed) } return 0, nil } diff --git a/pkg/cli/runner_guard.go b/pkg/cli/runner_guard.go index b4f828e0166..d9667eecee8 100644 --- a/pkg/cli/runner_guard.go +++ b/pkg/cli/runner_guard.go @@ -84,7 +84,10 @@ func runRunnerGuardOnDirectory(workflowDir string, verbose bool, strict bool) er if err != nil { return fmt.Errorf("docker command not found: %w", err) } - volumeMount := gitRoot + ":/workdir" + volumeMount, err := buildDockerVolumeMount(gitRoot, "/workdir") + if err != nil { + return fmt.Errorf("invalid docker mount path: %w", err) + } // #nosec G204 -- gitRoot is validated as an absolute path above (from git rev-parse, a trusted // source). containerScanPath is derived from filepath.Rel(gitRoot, workflowDir), cleaned with // filepath.Clean, validated to not escape the repository root (no ".." prefix), and prefixed @@ -148,7 +151,7 @@ func runRunnerGuardOnDirectory(workflowDir string, verbose bool, strict bool) er return fmt.Errorf("strict mode: runner-guard exited with code 1 (findings present) and output could not be parsed: %w", parseErr) } if totalFindings > 0 { - return fmt.Errorf("strict mode: runner-guard found %d security findings - workflows must have no runner-guard findings in strict mode", totalFindings) + return fmt.Errorf("strict mode: runner-guard found %d security findings - workflows must have no runner-guard findings in strict mode. Example: rerun after resolving all reported findings", totalFindings) } // Exit code 1 with no parseable findings is still a failure in strict mode return errors.New("strict mode: runner-guard exited with code 1 indicating findings are present") @@ -176,7 +179,7 @@ func parseAndDisplayRunnerGuardOutput(stdout string, verbose bool, gitRoot strin trimmed := strings.TrimSpace(stdout) if !strings.HasPrefix(trimmed, "{") && !strings.HasPrefix(trimmed, "[") { if trimmed != "" { - return 0, fmt.Errorf("unexpected runner-guard output format: %s", trimmed) + return 0, fmt.Errorf("unexpected runner-guard output format (expected JSON object or array). Example: {\"findings\":[]}. Got: %s", trimmed) } return 0, nil } diff --git a/pkg/cli/shell_escape.go b/pkg/cli/shell_escape.go new file mode 100644 index 00000000000..41dc8d6604f --- /dev/null +++ b/pkg/cli/shell_escape.go @@ -0,0 +1,21 @@ +package cli + +import "strings" + +func shellJoinArgs(args []string) string { + escapedArgs := make([]string, 0, len(args)) + for _, arg := range args { + escapedArgs = append(escapedArgs, shellEscapeArg(arg)) + } + return strings.Join(escapedArgs, " ") +} + +func shellEscapeArg(arg string) string { + if arg == "" { + return "''" + } + if !strings.ContainsAny(arg, "()[]{}*?$`\"'\\|&;<> \t\r\n") { + return arg + } + return "'" + strings.ReplaceAll(arg, "'", "'\\''") + "'" +} diff --git a/pkg/cli/upgrade_command.go b/pkg/cli/upgrade_command.go index e3093cfc9b7..20ffe0801a2 100644 --- a/pkg/cli/upgrade_command.go +++ b/pkg/cli/upgrade_command.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" @@ -103,7 +104,7 @@ This command always upgrades all Markdown files in .github/workflows.`, } if targetRepo != "" && targetOrg != "" { - return errors.New("cannot specify both --repo and --org flags; use --repo for a single repository or --org for organization-wide upgrades") + return errors.New("cannot specify both --repo and --org flags; use --repo for a single repository or --org for organization-wide upgrades. Example: gh aw upgrade --repo owner/repo") } if len(repoGlobs) > 0 && targetOrg == "" { @@ -115,7 +116,7 @@ This command always upgrades all Markdown files in .github/workflows.`, } if createPR && createIssue { - return errors.New("cannot specify both --create-pull-request and --create-issue") + return errors.New("cannot specify both --create-pull-request and --create-issue. Example: gh aw upgrade --org my-org --create-pull-request") } // Handle audit mode @@ -478,6 +479,11 @@ func relaunchWithSameArgs(extraFlag string, exeOverride string) error { // Explicitly copy os.Args[1:] so appending the extra flag does not modify // the original slice backing array. newArgs := append(append([]string(nil), os.Args[1:]...), extraFlag) + for _, arg := range newArgs { + if strings.ContainsRune(arg, '\x00') { + return errors.New("invalid relaunch arguments: argument contains NUL byte. Example: compile .github/workflows/example.md") + } + } upgradeLog.Printf("Re-launching with new binary: %s %v", exe, newArgs) // Validate the executable path before re-launching it (defense-in-depth). diff --git a/pkg/cli/upgrade_command_test.go b/pkg/cli/upgrade_command_test.go index 15b46f550f1..dbd121d5f1c 100644 --- a/pkg/cli/upgrade_command_test.go +++ b/pkg/cli/upgrade_command_test.go @@ -4,6 +4,7 @@ package cli import ( "context" + "os" "testing" "github.com/stretchr/testify/assert" @@ -144,3 +145,13 @@ func TestRelaunchWithSameArgsRejectsRelativeExecutableOverride(t *testing.T) { require.Error(t, err) require.ErrorContains(t, err, "invalid executable path") } + +func TestRelaunchWithSameArgsRejectsNullByteArgument(t *testing.T) { + origArgs := os.Args + t.Cleanup(func() { os.Args = origArgs }) + os.Args = []string{"gh-aw", "compile", "bad\x00arg"} + + err := relaunchWithSameArgs("--post-upgrade", "/bin/echo") + require.Error(t, err) + require.ErrorContains(t, err, "argument contains NUL byte") +}