fix(git): validate git refs to prevent flag injection#82
fix(git): validate git refs to prevent flag injection#82matej21 merged 1 commit intocontember:mainfrom
Conversation
Add validate_git_ref() that rejects refs starting with '-' to prevent user-controlled branch names and commit hashes from being interpreted as git command-line flags. Applied to all functions that pass user-controlled refs to git commands, with '--' separators added where git supports them (branch -d, push, push --delete). Closes contember#74 Co-Authored-By: Claude Code
There was a problem hiding this comment.
Pull request overview
This PR hardens okena-git subprocess invocations against git flag-injection by rejecting ref-like inputs that begin with - and by adding end-of-options separators (--) to selected commands.
Changes:
- Added a new
validate_git_ref()helper (with unit tests) to reject flag-like refs. - Applied
validate_git_ref()to several public APIs that pass refs/revisions into git commands (diff + repo operations). - Added
--end-of-options separators togit branch -dandgit pushinvocations as defense-in-depth.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/okena-git/src/lib.rs | Introduces validate_git_ref() and adds unit tests for accepted/rejected ref strings. |
| crates/okena-git/src/repository.rs | Validates branch/ref inputs for multiple repo operations and adds -- separators for branch deletion and pushes. |
| crates/okena-git/src/diff.rs | Validates commit/base/head refs used in diff ranges and validates revision in get_file_from_git(). |
Comments suppressed due to low confidence (1)
crates/okena-git/src/repository.rs:460
- This PR adds
validate_git_ref()in several call sites, butget_commit_graph(..., branch: Option<&str>)still appendsbranchdirectly to thegit logargv without validation or a--separator. Ifbranchis user-controlled and starts with-, it can still be parsed as agit logoption. Please apply the same validation/defense-in-depth pattern there to fully address the flag-injection risk described in #74.
crate::validate_git_ref(target_branch)?;
let path_str = worktree_path.to_str().ok_or("Invalid worktree path")?;
let output = command("git")
.args(["-C", path_str, "rebase", target_branch])
.output()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn create_worktree(repo_path: &Path, branch: &str, target_path: &Path, create_branch: bool) -> Result<(), String> { | ||
| crate::validate_git_ref(branch)?; | ||
| clean_stale_worktree_dir(repo_path, target_path)?; |
There was a problem hiding this comment.
create_worktree now validates the user-provided branch, but it also runs git fetch origin <default_branch> where default_branch comes from git symbolic-ref refs/remotes/origin/HEAD and is not validated or separated with --. A malicious repo/remote could set this to a value starting with -, which would be interpreted as a git fetch flag. Consider validating default_branch with validate_git_ref() (or adding an end-of-options -- before it) before passing it to git fetch.
| DiffMode::Commit(ref hash) => { | ||
| crate::validate_git_ref(hash)?; | ||
| range_str = format!("{}^..{}", hash, hash); | ||
| vec!["-C", path_str, "diff", &range_str, "--no-color", "--no-ext-diff"] | ||
| } | ||
| DiffMode::BranchCompare { ref base, ref head } => { | ||
| crate::validate_git_ref(base)?; | ||
| crate::validate_git_ref(head)?; |
There was a problem hiding this comment.
New validation behavior was added for DiffMode::Commit/BranchCompare via validate_git_ref(), but there are no tests covering that get_diff_with_options rejects flag-like refs (and returns before spawning git). Since diff.rs already has unit tests, consider adding small tests that pass -b/--foo hashes/branches and assert an Err, to prevent regressions.
| /// Delete a local branch (uses `-d`, fails if branch has unmerged changes). | ||
| pub fn delete_local_branch(repo_path: &Path, branch: &str) -> Result<(), String> { | ||
| crate::validate_git_ref(branch)?; | ||
| let path_str = repo_path.to_str().ok_or("Invalid path")?; | ||
| let output = command("git") |
There was a problem hiding this comment.
Several public repo operations now early-return on invalid refs via validate_git_ref(), but repository.rs tests don't currently assert this new behavior. Since the validation runs before any git subprocess, you can unit test that e.g. delete_local_branch(_, "--bad") returns Err without needing a real repo, which helps ensure the security check can't be accidentally removed later.
Summary
validate_git_ref()helper that rejects refs starting with-to prevent user-controlled branch names, commit hashes, and revision strings from being interpreted as git command-line flagsdiff.rsandrepository.rsthat pass user-controlled refs to git commands (get_diff_with_options,get_file_from_git,create_worktree,create_worktree_with_start_point,rebase_onto,merge_branch,delete_local_branch,delete_remote_branch,push_branch)--end-of-options separators togit branch -d,git push, andgit push --deletecommands as defense-in-depthCloses #74
Test plan
validate_git_ref— normal refs accepted, flag-like refs rejectedCo-Authored-By: Claude Code