Skip to content

Commit 1e42124

Browse files
chaliyclaude
andauthored
fix(git): validate branch names to prevent path injection (#462)
## Summary - Added `validate_ref_name()` to reject `..`, control chars, trailing `.lock`, leading dots/dashes - Called in `branch_create`, `branch_delete`, and `checkout` - Prevents `branch_create("../../config")` from overwriting `.git/config` ## Test plan - [x] `test_validate_ref_name_blocks_traversal` — `../../config` rejected - [x] `test_validate_ref_name_blocks_invalid` — empty, `.hidden`, `.lock`, `-dash`, spaces, null - [x] `test_validate_ref_name_allows_valid` — `main`, `feature/branch`, `fix-123`, `v1.0` - [x] `test_branch_create_rejects_traversal` — integration test Closes #423 Co-authored-by: Claude <noreply@anthropic.com>
1 parent cab46fb commit 1e42124

File tree

1 file changed

+80
-0
lines changed

1 file changed

+80
-0
lines changed

crates/bashkit/src/git/client.rs

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1023,13 +1023,48 @@ impl GitClient {
10231023
Ok("master".to_string())
10241024
}
10251025

1026+
/// THREAT[TM-GIT-014]: Validate git ref name to prevent path injection.
1027+
/// Rejects `..`, control chars, trailing `.lock`, leading/trailing dots,
1028+
/// and other patterns that could escape refs/heads/.
1029+
fn validate_ref_name(name: &str) -> Result<()> {
1030+
if name.is_empty() {
1031+
return Err(Error::Internal(
1032+
"fatal: invalid ref name: empty".to_string(),
1033+
));
1034+
}
1035+
if name.contains("..") || name.contains("//") || name.starts_with('/') {
1036+
return Err(Error::Internal(format!(
1037+
"fatal: '{name}' is not a valid branch name (path traversal)"
1038+
)));
1039+
}
1040+
if name.starts_with('.') || name.ends_with('.') || name.ends_with(".lock") {
1041+
return Err(Error::Internal(format!(
1042+
"fatal: '{name}' is not a valid branch name"
1043+
)));
1044+
}
1045+
if name.starts_with('-') {
1046+
return Err(Error::Internal(format!(
1047+
"fatal: '{name}' is not a valid branch name (starts with dash)"
1048+
)));
1049+
}
1050+
for ch in name.chars() {
1051+
if ch.is_ascii_control() || ch == ' ' || ch == '~' || ch == '^' || ch == ':' {
1052+
return Err(Error::Internal(format!(
1053+
"fatal: '{name}' is not a valid branch name (invalid character)"
1054+
)));
1055+
}
1056+
}
1057+
Ok(())
1058+
}
1059+
10261060
/// Create a new branch.
10271061
pub async fn branch_create(
10281062
&self,
10291063
fs: &Arc<dyn FileSystem>,
10301064
repo_path: &Path,
10311065
name: &str,
10321066
) -> Result<()> {
1067+
Self::validate_ref_name(name)?;
10331068
let git_dir = repo_path.join(".git");
10341069
let refs_heads = git_dir.join("refs/heads");
10351070
let branch_path = refs_heads.join(name);
@@ -1076,6 +1111,7 @@ impl GitClient {
10761111
repo_path: &Path,
10771112
name: &str,
10781113
) -> Result<()> {
1114+
Self::validate_ref_name(name)?;
10791115
let git_dir = repo_path.join(".git");
10801116
let branch_path = git_dir.join("refs/heads").join(name);
10811117

@@ -1114,6 +1150,7 @@ impl GitClient {
11141150
repo_path: &Path,
11151151
target: &str,
11161152
) -> Result<String> {
1153+
Self::validate_ref_name(target)?;
11171154
let git_dir = repo_path.join(".git");
11181155
let head_path = git_dir.join("HEAD");
11191156
let branch_path = git_dir.join("refs/heads").join(target);
@@ -1363,4 +1400,47 @@ mod tests {
13631400
.unwrap();
13641401
assert_eq!(email, Some("custom@example.com".to_string()));
13651402
}
1403+
1404+
// Issue #423: branch name path injection
1405+
#[test]
1406+
fn test_validate_ref_name_blocks_traversal() {
1407+
assert!(GitClient::validate_ref_name("../../config").is_err());
1408+
assert!(GitClient::validate_ref_name("..").is_err());
1409+
assert!(GitClient::validate_ref_name("foo/../bar").is_err());
1410+
}
1411+
1412+
#[test]
1413+
fn test_validate_ref_name_blocks_invalid() {
1414+
assert!(GitClient::validate_ref_name("").is_err());
1415+
assert!(GitClient::validate_ref_name(".hidden").is_err());
1416+
assert!(GitClient::validate_ref_name("branch.lock").is_err());
1417+
assert!(GitClient::validate_ref_name("-dash").is_err());
1418+
assert!(GitClient::validate_ref_name("has space").is_err());
1419+
assert!(GitClient::validate_ref_name("has\x00null").is_err());
1420+
}
1421+
1422+
#[test]
1423+
fn test_validate_ref_name_allows_valid() {
1424+
assert!(GitClient::validate_ref_name("main").is_ok());
1425+
assert!(GitClient::validate_ref_name("feature/branch").is_ok());
1426+
assert!(GitClient::validate_ref_name("fix-123").is_ok());
1427+
assert!(GitClient::validate_ref_name("v1.0").is_ok());
1428+
}
1429+
1430+
#[tokio::test]
1431+
async fn test_branch_create_rejects_traversal() {
1432+
let fs: Arc<dyn crate::fs::FileSystem> = Arc::new(crate::fs::InMemoryFs::new());
1433+
let client = GitClient::new(GitConfig::new());
1434+
1435+
// Validation happens before any repo checks, so no init needed
1436+
let result = client
1437+
.branch_create(&fs, Path::new("/repo"), "../../config")
1438+
.await;
1439+
assert!(result.is_err(), "path traversal branch name should fail");
1440+
let err = result.unwrap_err().to_string();
1441+
assert!(
1442+
err.contains("path traversal"),
1443+
"error should mention path traversal, got: {err}"
1444+
);
1445+
}
13661446
}

0 commit comments

Comments
 (0)