Guard git command arguments against flag injection (Sighthound findings)#52401
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52401 does not have the 'implementation' label and has only 42 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I don't see a blocking issue in the changed lines.
Review notes
The new guard is narrow but correctly placed on the call sites that build git revision arguments, and the tests cover the intended leading-dash cases. I did not find a changed-line regression or an obviously missing error path that would justify blocking this PR.
I also attempted to collect the advisory sub-agent output, but no result was available at review time, so this verdict is based on my own pass over the diff.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 3.56 AIC · ⌖ 4.55 AIC · ⊞ 6.5K
Comment /review to run again
There was a problem hiding this comment.
Review: Guard git command arguments against flag injection
This is a clean, well-scoped security fix. The isSafeGitRevisionArg helper correctly guards against flag injection (leading - turning refs into CLI flags). Guards are applied consistently at all three callsites, with appropriate logging and graceful fallbacks. Test coverage is solid.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.4 AIC · ⌖ 6.27 AIC · ⊞ 5.4K
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel analysis complete: Score 100/100. Single table-driven test comprehensively validates security boundary (isSafeGitRevisionArg). Test covers 8 scenarios (empty, leading dashes, valid refs). No violations. Ready for approval.
|
There was a problem hiding this comment.
Pull request overview
Adds validation to prevent Git revision arguments from being interpreted as command-line flags.
Changes:
- Adds a reusable Git revision safety check.
- Validates upstream and experiment refs before Git execution.
- Adds table-driven validation tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/git.go |
Adds revision validation and guards upstream refs. |
pkg/cli/experiments_command.go |
Guards refs used by git show and git rev-parse. |
pkg/cli/git_test.go |
Tests safe and unsafe revision inputs. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting only; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Doc/name precision:
isSafeGitRevisionArgimplies broader validation than it delivers (flag-injection guard only). The comment should be tightened to avoid false confidence in future callers. - Silent failure path:
checkWorkflowFileStatusreturns the current status without an error when the upstream ref is unsafe, which may silently suppress the diff check from the caller's perspective. Returning an error keeps the caller in control. - Missing rejection tests for callers:
TestIsSafeGitRevisionArgcovers the helper well, but the rejection paths inloadLocalMetricEvalResultsandgitRefExistshave no dedicated tests.
Positive Highlights
- ✅ Minimal, surgical fix — only adds guards, no churn
- ✅ Good test coverage for the helper itself (
TestIsSafeGitRevisionArg), including leading-dash and double-dash cases - ✅ Rejection is logged for observability in
experiments_command.go - ✅ Accurate triage of false positives in the PR description
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 22.1 AIC · ⌖ 6.86 AIC · ⊞ 7K
Comment /matt to run again
| // argument (e.g. to "git log <ref>..HEAD" or "git show <ref>:<path>"). | ||
| // It rejects empty values and values starting with "-", which could | ||
| // otherwise be misinterpreted as command-line options (flag injection). | ||
| func isSafeGitRevisionArg(ref string) bool { |
There was a problem hiding this comment.
[/diagnosing-bugs] The function name and comment say "safe git revision arg" but the implementation only blocks leading-dash flag injection — it does not validate rev syntax. This over-broad name may invite callers to rely on a guarantee it doesn't provide.
💡 Suggestion
Narrow the doc comment to the actual invariant:
// isSafeGitRevisionArg reports whether ref cannot be misinterpreted as a
// git CLI flag (non-empty, no leading dash). It does NOT validate that
// ref is a well-formed git revision.This prevents future callers from treating it as a full validation gate.
@copilot please address this.
There was a problem hiding this comment.
Updated the helper comment in 8562188 to state the exact invariant: it rejects empty and dash-prefixed values to prevent flag interpretation, but does not validate Git revision syntax.
| if !isSafeGitRevisionArg(upstream) { | ||
| gitLog.Printf("Rejecting unsafe upstream ref: %q", upstream) | ||
| return status, nil // Ignore unexpected/unsafe upstream ref, return current status | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] On an unsafe upstream ref the function silently returns the current status with no error. Callers that rely on the returned status to decide whether to alert the user will silently skip the diff check — potentially hiding real workflow drift.
💡 Suggestion
Return a wrapped error so the caller can decide how to surface it:
if !isSafeGitRevisionArg(upstream) {
return status, fmt.Errorf("unexpected upstream ref %q", upstream)
}The caller can then log a warning and fall through gracefully, rather than silently losing the check.
@copilot please address this.
There was a problem hiding this comment.
Updated checkWorkflowFileStatus in 8562188 to return an explicit unexpected upstream ref error instead of silently returning the partial status.
| } | ||
| ref = branchName | ||
| } | ||
| if !isSafeGitRevisionArg(ref) { |
There was a problem hiding this comment.
[/tdd] loadLocalMetricEvalResults returns nil on an unsafe ref, but no test covers this path. A regression could re-open the injection vector silently.
💡 Suggested test skeleton
func TestLoadLocalMetricEvalResultsRejectsUnsafeRef(t *testing.T) {
// A leading-dash ref must not reach exec.Command
result := loadLocalMetricEvalResults("--evil")
assert.Nil(t, result, "expected nil for unsafe ref")
}Combine with a spy/mock on exec.Command to assert the command is never invoked.
@copilot please address this.
There was a problem hiding this comment.
No caller-path test added: workflowID is always prefixed by WorkflowStateBranchName and then origin/, so loadLocalMetricEvalResults("--evil") cannot produce a dash-leading Git argument or exercise this guard. The security invariant itself remains directly covered by TestIsSafeGitRevisionArg.
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100 Excellent
Summary
PR adds a focused, high-quality security test for the isSafeGitRevisionArg() function that guards against flag-injection vulnerabilities in git command execution.
Test Analysis
TestIsSafeGitRevisionArg: Table-driven test with 8 scenarios
- ✓ Design contract test (security boundary)
- ✓ Edge cases: empty, leading
-, leading--, valid refs - ✓ 8 parameterized assertions via subtests
- ✓ Build tag:
(go/redacted):build !integration - ✓ No mock libraries or inflation
Metrics
- Implementation tests: 0% (threshold: ≤30%) ✓
- Test inflation: 1.69:1 (threshold: ≤2:1) ✓
- No coding violations ✓
Approval: Comprehensive security test with excellent behavioral coverage.
|
@copilot Please inspect the latest branch state, refresh the branch if needed, and run the No unresolved review threads were found. Run context: https://github.com/github/gh-aw/actions/runs/31671853647
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Ran |
PR TriageCategory: bug | Risk: medium | Priority: high | Score: 83/100 Score breakdown: impact 40/50, urgency 28/30, quality 15/20 CI: green Recommended action: Security hardening (flag injection); multiple review comments from copilot-swe-agent self-review to address. Automated triage — run 52401
|
Sighthound flagged 5 critical CWE-78/89 findings across
pkg/cli. Twoexec.Commandcall sites concatenate git-derived values (ref,upstream) directly into revision-range/object arguments without validating they can't be interpreted as CLI flags.Changes
isSafeGitRevisionArg(ref string) boolinpkg/cli/git.go— rejects empty strings and values starting with-, preventing them from being misread as options bygit.checkWorkflowFileStatus(pkg/cli/git.go) now validatesupstreambefore buildingupstream+"..HEAD"forgit log.gitRefExistsandloadLocalMetricEvalResults(pkg/cli/experiments_command.go) now validaterefbefore buildingref+":"+fileforgit show; rejection is logged for observability.TestIsSafeGitRevisionArgcovering empty, leading-dash, branch, remote-branch, SHA, and fully-qualified ref inputs.Triaged as false positives / already mitigated (no change)
pkg/cli/forecast.go:78— flagged line is afmt.Fprintlnlog statement, not an exec sink.pkg/cli/bootstrap_profile_github_app.go:286— OAuthcodeis already validated viaisBootstrapGitHubAppManifestCode; no SQL sink exists.pkg/cli/grant.go:184—imageRef,grantImageRef, and mount paths are already validated viavalidateDockerImageRef/validateContainerMountPath/buildDockerReadonlyFileMount, with an existing#nosecjustification.