Harden Git revision argument handling - #58412
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Category: bug | Risk: medium | Score: 53 (impact 35 + urgency 10 + quality 8) | Action: batch_review | Batch: none
|
1 similar comment
|
Category: bug | Risk: medium | Score: 53 (impact 35 + urgency 10 + quality 8) | Action: batch_review | Batch: none
|
There was a problem hiding this comment.
🟡 Changes recommended
The revised Git command path lacks regression coverage with a configured upstream.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens Git revision handling against argument injection.
Changes:
- Rejects control characters in revision arguments.
- Passes upstream refs separately via
git log --not. - Adds control-character validation tests.
File summaries
| File | Description |
|---|---|
pkg/cli/git.go |
Hardens revision validation and Git invocation. |
pkg/cli/git_test.go |
Covers unsafe control characters. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| // #nosec G204 -- upstream is validated above by isSafeGitRevisionArg and relPath was | ||
| // validated by validateRelPathForGit; "--" separates revision args from the path. | ||
| cmd = exec.Command("git", "-C", gitRoot, "log", upstream+"..HEAD", "--oneline", "--", relPath) | ||
| cmd = exec.Command("git", "-C", gitRoot, "log", "--oneline", "HEAD", "--not", upstream, "--", relPath) |
There was a problem hiding this comment.
Addressed in 4bb01c8: I added the configured-upstream regression to TestCheckWorkflowFileStatus, asserting a workflow-specific local commit reports HasUnpushedCommits == true while an unrelated change remains false. I also split the long git helpers to satisfy the repo lint gate without changing behavior.
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review for #58412; submitted overall review with no additional line comments because the only actionable issue was already present in existing review comments.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
@copilot Please take the next forward-progress pass on this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed via correctness/security fallback (backend Go change, no UI applicable). The revision-argument hardening is correct: swapping the composed upstream..HEAD string for the standalone --not upstream argument avoids injection via revision-expression concatenation, and isSafeGitRevisionArg now correctly rejects control characters (including newline and Unicode ``). New tests cover both cases. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 21.1 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
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.
Verdict
No new blocking issues in the current patch.
Why I'm not requesting changes
The only substantive risk I found is the existing test coverage gap already called out on this PR: the changed git log --not path still is not exercised by TestCheckWorkflowFileStatus, because those tests never configure an upstream branch. Beyond that, the revision hardening itself is appropriately narrow and the added control-character coverage is aligned with existing validation helpers.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 17.9 AIC · ⌖ 7.32 AIC · ⊞ 23.5K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — the security hardening logic itself is sound, but the changed git log code path is still not exercised by a passing test.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap:
checkWorkflowFileStatus's--not upstreaminvocation is never exercised becauseTestCheckWorkflowFileStatusnever configures a real upstream branch (rev-parse @{u}fails first). A prior reviewer comment already flagged this; it remains open. - The new
isSafeGitRevisionArgcontrol-character tests (git_test.go) are correct and directly cover the newcontainsControlCharacterscheck with good edge cases (newline, U+202E RTL override).
Positive Highlights
- ✅ Passing
upstreamas a standalone--notargument togit loginstead of composing a revision expression (upstream+"..HEAD") is a real hardening improvement — it removes a string-concatenation surface that could be misinterpreted ifupstreamever contained shell-adjacent characters. - ✅ Rejecting control characters (including Unicode format chars and line/paragraph separators) in
isSafeGitRevisionArgcloses a real gap: a ref containing\nor\rcould previously be used to smuggle additional arguments/lines into git's argument parsing or terminal output. - ✅ Good, targeted new unit tests for the validation function itself.
Skill selection: change type is a bug-fix/security-hardening PR, so /tdd and /diagnosing-bugs were applied.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 21.8 AIC · ⌖ 14.7 AIC · ⊞ 10.3K
Comment /matt to run again
| // #nosec G204 -- upstream is validated above by isSafeGitRevisionArg and relPath was | ||
| // validated by validateRelPathForGit; "--" separates revision args from the path. | ||
| cmd = exec.Command("git", "-C", gitRoot, "log", upstream+"..HEAD", "--oneline", "--", relPath) | ||
| cmd = exec.Command("git", "-C", gitRoot, "log", "--oneline", "HEAD", "--not", upstream, "--", relPath) |
There was a problem hiding this comment.
[/tdd] The new git log --oneline HEAD --not upstream -- relPath invocation has no regression test exercising HasUnpushedCommits == true. TestCheckWorkflowFileStatus never configures an upstream/remote-tracking branch, so rev-parse @{u} always fails and this whole code path (including the argument-order change) is untested.
💡 Suggested test addition
Add a subtest that creates a fake remote-tracking ref (e.g. git update-ref refs/remotes/origin/main HEAD~1 plus git branch --set-upstream-to=origin/main), commits a new change on HEAD, and asserts status.HasUnpushedCommits == true. This would have caught any accidental swap between --not upstream and range syntax, and locks in the fix's actual behavior rather than just the string it produces.
t.Run("unpushed_commits", func(t *testing.T) {
exec.Command("git", "update-ref", "refs/remotes/origin/main", "HEAD").Run()
exec.Command("git", "branch", "--set-upstream-to=origin/main").Run()
// ... commit a change to workflowFile on HEAD ...
status, err := checkWorkflowFileStatus(workflowFile)
require.NoError(t, err)
assert.True(t, status.HasUnpushedCommits)
})@copilot please address this.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot This PR looks ready for a forward-progress pass.
Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "registry.npmjs.org"See Network Configuration for more information.
|
|
Category: bug | Risk: medium | Score: 60 (impact 28 urgency 18 quality 14) | Action: batch_review | Batch: workflow-reliability Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"See Network Configuration for more information.
|
|
@copilot The latest review thread already has an author follow-up but is still unresolved: Please resolve the thread if the reviewer concern is fully addressed, then run the
|
@copilot The latest review thread already has an author follow-up but is still unresolved.
Thread to close out:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The security scan flagged dynamic process arguments as potential command injection. Existing call sites already avoid shell execution; this change further hardens Git revision handling.
git log --notargument instead of composing a revision expression.Run: https://github.com/github/gh-aw/actions/runs/33907360580