🛡️ Sentinel: [CRITICAL] Fix Git argument injection via branch name#71
🛡️ Sentinel: [CRITICAL] Fix Git argument injection via branch name#71xbmc4lyfe wants to merge 1 commit into
Conversation
Added validation checks in `_validate_pr_metadata` to ensure that `expected_base`, `pr_base`, and `headRefName` do not start with a hyphen. This prevents potential Git argument injection vulnerabilities when these values are passed to `git` commands (e.g., `git worktree add`). Co-authored-by: xbmc4lyfe <273732874+xbmc4lyfe@users.noreply.github.com>
|
👋 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. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds defensive Git argument injection guards to the PR metadata validation function. The ChangesGit Argument Injection Validation
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ralph_loop/cli.py (1)
336-337: 💤 Low valueConsider adding
isinstancecheck for consistency and defensive coding.Lines 356 and 368 use
isinstance(value, str)before calling.startswith(), but line 336 calls.startswith()directly onexpected_base. Whileexpected_basecomes fromargs.base(argparse) and should always be a string, adding theisinstancecheck would make the validation pattern consistent and more defensive.🛡️ Proposed fix for consistency
- if expected_base.startswith("-"): + if isinstance(expected_base, str) and expected_base.startswith("-"): raise CommandError("Git argument injection detected: base branch starts with a hyphen.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ralph_loop/cli.py` around lines 336 - 337, Add the same defensive isinstance check used elsewhere before calling .startswith() on expected_base: verify that expected_base (derived from args.base) is an instance of str before calling expected_base.startswith("-"); if it's not a str, either skip the hyphen check or raise the same CommandError. Update the validation around expected_base to mirror the pattern used at the other checks (isinstance(value, str) then .startswith()) so the code in ralph_loop/cli.py remains consistent and robust against non-string inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ralph_loop/cli.py`:
- Around line 336-337: Add the same defensive isinstance check used elsewhere
before calling .startswith() on expected_base: verify that expected_base
(derived from args.base) is an instance of str before calling
expected_base.startswith("-"); if it's not a str, either skip the hyphen check
or raise the same CommandError. Update the validation around expected_base to
mirror the pattern used at the other checks (isinstance(value, str) then
.startswith()) so the code in ralph_loop/cli.py remains consistent and robust
against non-string inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 663d1789-3189-4f19-a83c-457dfe5bee73
📒 Files selected for processing (2)
.jules/sentinel.mdralph_loop/cli.py
📜 Review details
🔇 Additional comments (3)
ralph_loop/cli.py (1)
336-337: LGTM!Also applies to: 355-369
.jules/sentinel.md (2)
2-4: LGTM!
1-1: ⚡ Quick winVerify
.jules/sentinel.mdentry date convention.
.jules/sentinel.mdcontains a single entry dated2024-05-24(“Fix Git argument injection via branch name”), but there’s no in-repo convention/docs explaining whether sentinel dates are intentionally backdated or expected to match the PR timeline. Confirm the intended dating rule for sentinel entries.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea0f9d63f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(branch, str) and branch.startswith("-"): | ||
| raise CommandError("Git argument injection detected: PR head branch starts with a hyphen.") |
There was a problem hiding this comment.
When --pr is omitted, _resolve_pr_data() uses the current branch name as the gh pr view selector before this new headRefName guard ever runs. The gh pr view usage is gh pr view [<number> | <url> | <branch>] [flags] and includes inherited flags like --repo, so a current branch whose ref name starts with - can still be parsed as a gh option instead of being rejected here. Add the same hyphen check to the locally-derived pr_ref before _pr_view(pr_ref) is invoked.
Useful? React with 👍 / 👎.
| if expected_base.startswith("-"): | ||
| raise CommandError("Git argument injection detected: base branch starts with a hyphen.") |
There was a problem hiding this comment.
This expected_base guard only runs after a specific PR has been resolved, but the --all-prs/implicit fan-out path calls _list_open_prs(args.base) first, which builds gh pr list --base <base> .... In that fan-out context, a --base=-... value still reaches the GitHub CLI before validation, so the new protection does not cover one of the CLI inputs it says it protects. Validate args.base before deciding/listing fan-out PRs, not only inside per-PR metadata validation.
Useful? React with 👍 / 👎.
| if isinstance(branch, str) and branch.startswith("-"): | ||
| raise CommandError("Git argument injection detected: PR head branch starts with a hyphen.") |
There was a problem hiding this comment.
For a same-repository PR whose valid head branch starts with +, this guard still allows the value into _fetch_pr_branch_or_head, where git fetch origin +foo is parsed as a force refspec for foo rather than as the branch +foo. In a repo that already has origin/+foo, that leaves the PR tracking ref stale while Ralph proceeds from origin/+foo, so repairs/rebases can run against an old PR head. Treat leading + as unsafe here as well, or fetch by an explicit full refspec that cannot reinterpret the branch name.
Useful? React with 👍 / 👎.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Branch and base names fetched from remote sources or user input were being passed directly to
gitcommands without validation. If a branch name started with a hyphen (e.g.,-b), it could be interpreted as an argument bygitcommands likegit worktree add, leading to Git argument injection.🎯 Impact: An attacker could potentially execute arbitrary Git options, potentially altering the repository state maliciously or bypassing security controls.
🔧 Fix: Added checks in
_validate_pr_metadatato raise aCommandErrorifbranch,pr_base, orexpected_basestart with a hyphen.✅ Verification: Ran syntax checks and the test suite. Ensure
PYTHONPATH=. pytest tests/ -k "not test_main_signal_handler"passes.PR created automatically by Jules for task 13106249361622343253 started by @xbmc4lyfe