Uh oh!
There was an error while loading. Please reload this page.
Harden git argument construction against option injection and path traversal - #53601
Conversation
…raversal Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…dation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
TriageCategory: bug | Risk: medium | Priority: high | Score: 68/100 (impact:30, urgency:20, quality:18)
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53601 does not have the 'implementation' label and has 62 new lines of code in business logic directories (below the 100-line threshold).
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review for #53601. I found no actionable issues in the changed lines, so no PR review comments or review submission were needed.
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed for over-engineering only (per ponytail-review scope): validateRelPathForGit is a minimal, single-purpose 22-line security check with a table-driven test covering all branches (empty, leading-dash, traversal variants, absolute path, valid paths). No speculative abstractions, reinvented stdlib, or dead flexibility to cut.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
The hardening looks correct and complete.
validateRelPathForGitcorrectly rejects all injection vectors: leading-/--, absolute paths, and..traversal afterfilepath.Cleannormalization.- The
"--"separator already present in thegit logcommand properly isolates the path from revision arguments. #nosec G204annotations are correctly scoped and justified with clear comments.- Tests cover all meaningful cases, including the tricky
sub/../../secret.txtnested traversal.
No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.6 AIC · ⌖ 8.77 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Pull request overview
Hardens Git path handling in workflow status checks.
Changes:
- Adds relative-path validation before Git commands.
- Documents static-analysis suppressions.
- Adds validation unit tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/git.go | Validates Git path arguments. |
pkg/cli/git_test.go | Tests accepted and rejected paths. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| clean := filepath.Clean(relPath) | ||
| if filepath.IsAbs(clean) { | ||
| return fmt.Errorf("path %q must not escape the repository root", relPath) | ||
| } | ||
| cleanedSlash := filepath.ToSlash(clean) | ||
| if cleanedSlash == ".." || strings.HasPrefix(cleanedSlash, "../") { | ||
| return fmt.Errorf("path %q must not escape the repository root", relPath) | ||
| } |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Key Themes
#nosec G204placement: The annotations are on comment lines before theexec.Commandstatements, not on the statements themselves. gosec requires them inline on the flagged line, so the scanner will continue to report these as findings.- Unspecified behaviour for
".":validateRelPathForGit(".")returnsnil(passes). Whether a bare"."should be accepted by this function is worth an explicit test case and comment. - Security argument readability: The dual code path (absolute vs relative
workflowPath) is correct, but a brief inline comment explaining why the two-step logic closes the traversal gap would help future reviewers.
Positive Highlights
- ✅ The validation function is well-scoped and reusable — it does one thing clearly.
- ✅
filepath.Clean+filepath.ToSlashis the right cross-platform approach. - ✅ Test table covers the key attack vectors (leading dash,
..,../, nested traversal, absolute path). - ✅
isSafeGitRevisionArgalready existed for theupstreamarg;validateRelPathForGitfollows that same pattern consistently.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.5 AIC · ⌖ 11.6 AIC · ⊞ 7.8K
Comment /matt to run again
| gitLog.Printf("Checking git status for: %s", relPath) | ||
| // Check for modified or staged changes using git status --porcelain | ||
| // #nosec G204 -- relPath is validated above by validateRelPathForGit to reject |
There was a problem hiding this comment.
[/diagnosing-bugs]#nosec G204 must appear on the same line as the exec.Command call to suppress the gosec finding. Placing it on a preceding comment line has no effect — the scanner will still flag lines 686 and 738.
💡 Fix
Move the annotation onto the statement line itself:
cmd:=exec.Command("git", "-C", gitRoot, "status", "--porcelain", relPath) // #nosec G204 -- relPath validated by validateRelPathForGitSee the gosec #nosec annotation docs: the directive must be on the statement line, not a preceding comment.
@copilot please address this.
| {"nested relative path", ".github/workflows/workflow.md", false}, | ||
| {"dot prefixed but within repo", "./workflow.md", false}, | ||
| } | ||
| for _, tt := range tests { |
There was a problem hiding this comment.
[/tdd] The test covers "./workflow.md" (dot-prefixed) as valid, but filepath.Clean("./workflow.md") → "workflow.md" which passes. Consider also testing "." (current directory) — it cleans to "." and does not start with ../, so validateRelPathForGit(".") currently returns nil. Is passing a bare "." to git status --porcelain . the intended behaviour?
💡 Suggested test case
{"bare dot (current dir)", ".", false}, // or true, depending on intentIf the caller should never pass ".", add a check and document the expectation explicitly in the test.
@copilot please address this.
| relPath = workflowPath | ||
| } | ||
| // Reject paths that escape the repository root (path traversal) or that |
There was a problem hiding this comment.
[/diagnosing-bugs] The relPath validation happens after computing gitRoot and relPath, but gitRoot is obtained from gitutil.FindGitRoot() earlier in the function. If workflowPath is already absolute and the filepath.Rel succeeds, the traversal guard relies on filepath.Clean not producing a leading ... This is correct for absolute paths resolved under gitRoot, but it's worth adding a comment explaining why the filepath.Rel call (if present) makes additional traversal via absolute input impossible — currently a reader must mentally trace two code paths to verify safety.
💡 Suggestion
Add a short comment at the validation call site explaining the invariant:
// relPath was either passed directly (relative) or derived via filepath.Rel(gitRoot, workflowPath).// validateRelPathForGit rejects any path that would escape gitRoot.iferr:=validateRelPathForGit(relPath); err!=nil {This makes the security argument self-documenting for future reviewers.
@copilot please address this.
gh-aw-bot
commented
Aug 18, 2026
@copilot Please investigate the failed checks on this PR, refresh the branch if needed, then run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Investigated: |
PR TriageCategory: bug | Risk: high (security) | Priority: high (score 85)
Recommended action: fast_track
|
🎉 This pull request is included in a new release. Release: |
Sighthound flagged
exec.Commandcall sites (CWE-78) inpip_validation.go,dependabot_manifests.go,grype.go,upgrade_command.go, andgit.goas Critical findings, citing risk of argument injection or path traversal via unsanitized values passed as argv elements.Four of the five flagged sites already had proper allowlist/validation logic from a prior fix (package name regex validation, docker image ref validation, executable path resolution, safe git-ref checks). Investigation surfaced one remaining gap in
git.go.Changes
pkg/cli/git.go: addedvalidateRelPathForGitto validaterelPathbefore it's used inexec.Commandcalls incheckWorkflowFileStatus:-— closes a real gap wheregit status --porcelain relPath(unlike the adjacentgit log ... -- relPathcall) has no--separator, so a leading-dash path could be parsed as a git flag...traversal or absolute paths, checked on the OS-native cleaned path for cross-platform correctness.#nosec G204annotations documenting the validation at each call site.TestValidateRelPathForGitcovering leading-dash, traversal, and absolute-path rejection alongside normal relative-path acceptance.