fix: address Sighthound CWE-78 command injection findings (top 5) - #47242
Conversation
- gitutil.go: add #nosec G204 annotation; relPath validated by filepath.Rel and checked against '..' prefix - dependabot.go: add filepath.IsAbs validation for npmPath + #nosec G204 - pip_validation.go: add #nosec G204; pkgName pre-validated by validatePipPackageName - upgrade_command.go: add filepath.IsAbs validation for exe + #nosec G204 - runner_guard.go: apply filepath.Clean to scanPath; improve #nosec G204 comment Closes #47208 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Addresses #47208 by documenting validated exec.Command usage and adding path safeguards.
Changes:
- Adds G204 suppression rationales.
- Validates dynamic executable paths.
- Normalizes Runner Guard scan paths.
Show a summary per file
| File | Description |
|---|---|
pkg/gitutil/gitutil.go |
Documents safe Git argument handling. |
pkg/workflow/dependabot.go |
Requires an absolute npm path. |
pkg/workflow/pip_validation.go |
Documents package validation. |
pkg/cli/upgrade_command.go |
Validates the relaunch executable path. |
pkg/cli/runner_guard.go |
Cleans the scanner path. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Medium
| relDir, relErr := filepath.Rel(gitRoot, workflowDir) | ||
| if relErr == nil && relDir != ".." && !strings.HasPrefix(relDir, ".."+string(filepath.Separator)) { | ||
| scanPath = relDir | ||
| scanPath = filepath.Clean(relDir) |
| // #nosec G204 -- exe is validated as an absolute path above and originates from | ||
| // os.Executable() or the known install path; newArgs is a copy of the current process | ||
| // argv (os.Args[1:]) plus a single hardcoded flag, containing no untrusted user input. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. This PR (#47242) addresses Sighthound CWE-78 command injection findings in 5 production files but contains no test additions or modifications. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47242 does not have the 'implementation' label and has only 27 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: CWE-78 command injection fixes
Overall the changes are well-structured. The defense-in-depth guards (filepath.IsAbs, filepath.Clean) are sensible additions, and most #nosec G204 suppressions are accurately documented.
Two issues are flagged in existing inline comments:
-
runner_guard.goline 60 —filepath.Cleandoes not prevent option/flag injection. A user-controlledworkflowDirlike--helpproduces ascanPathstarting with-, interpretable as a Docker CLI flag. Consider prefixing with./or adding--before the path in the Docker command. -
upgrade_command.goline 491 — The#nosecrationale claimsnewArgscontains no untrusted input, butnewArgs = os.Args[1:]is user-controlled. Shell injection is not possible here (exec.Command uses argv directly), but the comment is misleading and should be corrected.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 21.8 AIC · ⌖ 4.53 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting one targeted fix before merge.
📋 Key Themes & Highlights
Key Themes
- Defense-in-depth inconsistency:
pip_validation.gouses a bare"pip"/"pip3"command name without priorexec.LookPath+filepath.IsAbsvalidation, unlikedependabot.gowhich correctly resolves and validates the path. The#nosecrationale is therefore weaker than claimed. - Option injection (pre-existing): The existing Copilot comments on
runner_guard.go(line 60) andupgrade_command.go(line 491) correctly note thatfilepath.IsAbs/filepath.Cleando not guard against flag-injection via values starting with-. These are real gaps worth addressing.
Positive Highlights
- ✅
dependabot.go: theexec.LookPath+filepath.IsAbspattern is the right approach and well-documented. - ✅
upgrade_command.go:filepath.IsAbsguard onexeis a good defense-in-depth addition. - ✅
gitutil.go: the#nosecrationale is accurate and well-scoped. - ✅ All suppression comments now explain why the call is safe, not just that it is.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 26 AIC · ⌖ 4.67 AIC · ⊞ 6.7K
Comment /matt to run again
| // Include --pre flag to check for pre-release versions (alpha, beta, rc) | ||
| // #nosec G204 -- pipCmd is one of the hardcoded values "pip" or "pip3"; pkgName is | ||
| // validated above by validatePipPackageName against the strict PyPI PEP 508 allowlist. | ||
| cmd := exec.Command(pipCmd, "index", "versions", pkgName, "--pre") |
There was a problem hiding this comment.
[/diagnosing-bugs] pipCmd is used as a bare command name ("pip" / "pip3"), relying on PATH resolution at exec time — unlike dependabot.go, which calls exec.LookPath and validates filepath.IsAbs before the #nosec. The #nosec rationale here says the values are "hardcoded" but omits that they are resolved via PATH, making the defense-in-depth story inconsistent across files.
💡 Suggested fix
Apply the same pattern used in dependabot.go: resolve with exec.LookPath, validate with filepath.IsAbs, then pass the resolved absolute path to exec.Command.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — Two medium-severity gaps in the new hardening logic need fixes before this merges.
Blocking findings
1. pip_validation.go: pipCmd not resolved via exec.LookPath
This PR correctly adds exec.LookPath + filepath.IsAbs for npmPath in dependabot.go, but omits the same treatment for pip/pip3 in pip_validation.go. The nosec comment argues pipCmd is hardcoded, but the resolved binary is still PATH-dependent at exec time — the scanner will likely flag this too, and it is inconsistent with the pattern applied elsewhere in this PR.
2. upgrade_command.go: exeOverride branch skips filepath.EvalSymlinks
The new filepath.IsAbs guard is only a partial fix. When exeOverride is non-empty it bypasses the EvalSymlinks call that the os.Executable() branch uses. An absolute-path symlink still resolves to an unintended binary. EvalSymlinks should be applied to exe regardless of source.
The two pre-existing comments (option injection in runner_guard.go, nosec rationale accuracy in upgrade_command.go) remain valid and unaddressed.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 38.6 AIC · ⌖ 4.83 AIC · ⊞ 5.6K
Comment /review to run again
| // Include --pre flag to check for pre-release versions (alpha, beta, rc) | ||
| // #nosec G204 -- pipCmd is one of the hardcoded values "pip" or "pip3"; pkgName is | ||
| // validated above by validatePipPackageName against the strict PyPI PEP 508 allowlist. | ||
| cmd := exec.Command(pipCmd, "index", "versions", pkgName, "--pre") |
There was a problem hiding this comment.
pipCmd resolved via PATH without exec.LookPath, inconsistent with the npmPath pattern added in this same PR: a tampered PATH could cause an attacker-controlled binary to be executed.
💡 Suggested fix
Mirror the dependabot.go treatment from this PR:
resolvedPip, err := exec.LookPath(pipCmd)
if err != nil {
return fmt.Errorf("pip command %q not found: %w", pipCmd, err)
}
if !filepath.IsAbs(resolvedPip) {
return fmt.Errorf("pip path is not absolute: %s", resolvedPip)
}
cmd := exec.Command(resolvedPip, "index", "versions", pkgName, "--pre")The nosec comment correctly notes pipCmd is a hardcoded string, but the resolved binary is still PATH-dependent. The defense-in-depth absolute-path validation applied to npmPath in dependabot.go in this same PR should be applied here consistently to satisfy the scanner and reduce risk from a compromised environment.
|
|
||
| // #nosec G204 -- exe is validated as an absolute path above and originates from | ||
| // os.Executable() or the known install path; newArgs is a copy of the current process | ||
| // argv (os.Args[1:]) plus a single hardcoded flag, containing no untrusted user input. |
There was a problem hiding this comment.
exeOverride branch skips filepath.EvalSymlinks, leaving a symlink attack surface that the os.Executable() branch explicitly guards against: filepath.IsAbs only confirms the path is absolute, not that the final resolved binary is the intended one.
💡 Suggested fix
Apply filepath.EvalSymlinks before the IsAbs check regardless of which branch provides exe:
if exeOverride != "" {
exe = exeOverride
} else {
exe, err = os.Executable()
// ...existing error handling...
}
// Resolve symlinks for both branches before validation
exe, err = filepath.EvalSymlinks(exe)
if err != nil {
return fmt.Errorf("failed to resolve executable symlinks: %w", err)
}
if !filepath.IsAbs(exe) {
return fmt.Errorf("executable path is not absolute: %s", exe)
}The nosec comment states exe "originates from os.Executable() or the known install path" but exeOverride is a caller-controlled string; symlink resolution is a separate concern from path absoluteness.
|
@copilot run pr-finisher skill |
- runner_guard.go: prefix scanPath with './' and convert to forward slashes for the Linux container, preventing option injection when workflowDir begins with '-' (e.g. '--help') - upgrade_command.go: fix #nosec G204 rationale to accurately state that exec.Command bypasses the shell entirely (execve), so shell-injection is impossible regardless of argv content Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Both review comments have been addressed in commit
|
|
🎉 This pull request is included in a new release. Release: |
Five Critical-severity CWE-78 findings from the Sighthound scan flagged
exec.Commandcalls where dynamic values (executable paths, package names, relative paths) were passed without explicit validation or suppression annotations.Changes
pkg/gitutil/gitutil.go— add#nosec G204with rationale:relPathis produced byfilepath.Reland already guarded by a..-prefix check; no shell execution.pkg/workflow/dependabot.go— addfilepath.IsAbs(npmPath)guard before invoking npm (defense-in-depth;exec.LookPathnormally returns an absolute path, but this makes it explicit) +#nosec G204.pkg/workflow/pip_validation.go— add#nosec G204;pkgNameis already validated against the PEP 508 allowlist viavalidatePipPackageNamebefore reaching theexec.Commandcall;pipCmdis one of the hardcoded strings"pip"/"pip3".pkg/cli/upgrade_command.go— addfilepath.IsAbs(exe)guard before re-launching the process;exealways comes fromos.Executable()or a rename of it, but the check makes the invariant enforceable +#nosec G204.pkg/cli/runner_guard.go— applyfilepath.CleantoscanPathafter the existing..-escape check; tighten#nosec G204comment to document bothgitRoot(absolute, fromgit rev-parse) andscanPath(cleaned, escape-validated relative path) guarantees.