🛡️ Sentinel: [MEDIUM] Hardening Git and HTTP operations in remote_fetch#19
🛡️ Sentinel: [MEDIUM] Hardening Git and HTTP operations in remote_fetch#19T-ahamed2 wants to merge 3 commits into
Conversation
- Fix Git argument injection by adding '--' separator to all git commands involving user-controlled inputs (refs, paths, repo URLs). - Prevent resource exhaustion by replacing http.DefaultClient with a timed client (DefaultHTTPClientTimeout) in public API fallback paths. - Improve process management by using exec.CommandContext in downloadFileViaGit and downloadFileViaGitClone to properly propagate cancellation. - Add security learning to .jules/sentinel.md.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances security and resource management by introducing the -- separator in Git commands to prevent argument injection, replacing http.DefaultClient with a custom client that enforces timeouts, and utilizing exec.CommandContext to respect context lifecycles. The reviewer identified two critical issues where the -- separator was placed incorrectly: in git archive, placing it before the tree-ish ref instead of the path will cause the command to fail, and in git checkout, placing it before the commit SHA ref incorrectly forces Git to treat the commit as a pathspec.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // #nosec G204 -- repoURL, ref, and path are from workflow import configuration authored by the | ||
| // developer; exec.Command with separate args (not shell execution) prevents shell injection. | ||
| cmd := exec.Command("git", "archive", "--remote="+repoURL, ref, path) | ||
| cmd := exec.CommandContext(ctx, "git", "archive", "--remote="+repoURL, "--", ref, path) |
There was a problem hiding this comment.
In git archive, the -- separator marks the end of options and the start of paths/files, not the tree-ish (ref). Placing -- before ref causes git to treat ref as a pathspec rather than the target tree-ish, which will cause the command to fail. The correct syntax is git archive [options] <tree-ish> -- <path>.
| cmd := exec.CommandContext(ctx, "git", "archive", "--remote="+repoURL, "--", ref, path) | |
| cmd := exec.CommandContext(ctx, "git", "archive", "--remote="+repoURL, ref, "--", path) |
|
|
||
| // Now checkout the specific commit | ||
| checkoutCmd := exec.Command("git", "-C", tmpDir, "checkout", ref) | ||
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--", ref) |
There was a problem hiding this comment.
In git checkout, the -- separator is used to distinguish between branches/commits and file paths. Placing -- before ref forces git to treat ref as a pathspec (file/directory path) to restore, rather than a commit/branch to checkout. Since ref is a 40-character commit SHA (as isSHA is true), it cannot start with a hyphen and does not need protection from option injection. Removing -- ensures the commit is correctly checked out.
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--", ref) | |
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", ref) |
- Fix Git argument injection by adding '--' separator to git commands. - Prevent resource exhaustion by enforcing HTTP timeouts and context propagation. - Address CI failures in link-check by ensuring reports/ directory existence. - Resolve error-message-lint and other linting issues (defer in loops, string checks). - Add security learning to .jules/sentinel.md.
This commit hardens the application against Git argument injection by adding the `--` separator to all Git subprocess calls. It also mitigates resource exhaustion risks by implementing HTTP timeouts and ensuring subprocesses respect context cancellation. - Prevent Git flag injection in clone, ls-remote, archive, and checkout. - Implement DefaultHTTPClientTimeout (30s) for remote fetches. - Use exec.CommandContext for improved process management. - Refactor error messages to include constructive guidance and examples while preserving operational context. - Fix CI link-check failure by maintaining reports/ directory.
1 participant
🚨 Severity: MEDIUM
💡 Vulnerability: Potential argument injection in Git commands and resource exhaustion via indefinite HTTP hangs.
🎯 Impact: An attacker providing a malicious Git ref starting with a hyphen could potentially inject CLI flags. Lack of HTTP timeouts could lead to DoS through resource exhaustion.
🔧 Fix: Added '--' to Git commands, implemented HTTP timeouts, and ensured context propagation for child processes.
✅ Verification: Ran 'go test ./pkg/parser/...' and 'make agent-report-progress-lint'. Core tests passed.
PR created automatically by Jules for task 11274516767056671896 started by @T-ahamed2