From e29cf9e869e938a2061ca7b39946ffbf6afd4b57 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 24 Aug 2026 22:27:15 -0500 Subject: [PATCH] fix(node): validate git branch refs to close option injection PR branch refs and a repo's default_branch were stored from request bodies with no ref validation, then interpolated into single git argv elements: git diff {target}...{source} (branch_diff / branch_diff_names), and git worktree add ... {target} / git merge {source} (merge_branch). A value beginning with '-', e.g. --output=/tmp/x, is parsed by git as an option rather than a revision, so it becomes an arbitrary file write. get_pr_diff takes an optional identity, so on a public repo the trigger is unauthenticated; planting the PR needs only read access, and the write happens at the withhold check before the visibility gate. Defense is applied at two layers: - Storage boundaries: create_pr validates source_branch and the resolved target_branch; create_repo validates default_branch (which becomes a PR's target when the PR omits one). These fail fast with 400 and keep junk out of the DB. - The sink: branch_diff, branch_diff_names, and merge_branch reject an option-shaped ref before building the git argv, so the property holds for every caller and every row, including legacy rows and any future writer that skips the boundary check. The shared validator is crate::git::store::validate_git_ref (git check-ref-format rules, leading-dash rejection as the core), re-exported as crate::api::validate_git_ref for the boundary handlers. No -- delimiter is used: the arguments are revisions, and -- there reinterprets them as pathspecs. Both boundary guards and the sink guard are mutation-proven load-bearing. resolve_head is unaffected (it prefixes refs/heads/); fork_repo takes no branch from the request. --- crates/gitlawb-node/src/api/mod.rs | 6 + crates/gitlawb-node/src/api/pulls.rs | 9 + crates/gitlawb-node/src/api/repos.rs | 5 + crates/gitlawb-node/src/git/store.rs | 102 +++++++++++ crates/gitlawb-node/src/test_support.rs | 229 ++++++++++++++++++++++++ 5 files changed, 351 insertions(+) diff --git a/crates/gitlawb-node/src/api/mod.rs b/crates/gitlawb-node/src/api/mod.rs index 71bfa43c5..0435a5284 100644 --- a/crates/gitlawb-node/src/api/mod.rs +++ b/crates/gitlawb-node/src/api/mod.rs @@ -92,6 +92,12 @@ pub(crate) fn require_repo_owner(record: &RepoRecord, caller: &str) -> Result<() } } +/// Re-export of the sink-level git ref validator (canonical home: +/// `crate::git::store::validate_git_ref`). Storage boundaries call it here to +/// fail fast with a 400; the sink guards enforce the same property for every +/// caller. Its unit tests live beside the definition in `git/store.rs`. +pub(crate) use crate::git::store::validate_git_ref; + #[cfg(test)] mod did_tests { use super::did_matches; diff --git a/crates/gitlawb-node/src/api/pulls.rs b/crates/gitlawb-node/src/api/pulls.rs index 26be6109c..b1e683144 100644 --- a/crates/gitlawb-node/src/api/pulls.rs +++ b/crates/gitlawb-node/src/api/pulls.rs @@ -50,6 +50,15 @@ pub async fn create_pr( let target_branch = req .target_branch .unwrap_or_else(|| record.default_branch.clone()); + + // Validate both refs before they are stored, since both are later + // interpolated into git argv (git diff / worktree add / merge). Validate the + // RESOLVED target, not just a caller-supplied one: create_repo also gates + // default_branch, but validating here as well means a PR can never feed the + // git sink an unchecked ref even if a default was poisoned by an older row + // or a future path that skips create_repo's gate. + crate::api::validate_git_ref(&req.source_branch).map_err(AppError::BadRequest)?; + crate::api::validate_git_ref(&target_branch).map_err(AppError::BadRequest)?; let number = state.db.next_pr_number(&record.id).await?; let now = Utc::now().to_rfc3339(); diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index b09cb6da5..5aa3a01db 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -242,6 +242,11 @@ pub async fn create_repo( )); } + // default_branch is caller-supplied and becomes a PR's target_branch when the + // PR omits one, which is interpolated into a git revision argument. Validate + // it as a ref so it cannot begin with '-' and inject a git option downstream. + crate::api::validate_git_ref(&req.default_branch).map_err(AppError::BadRequest)?; + // Owner is the authenticated agent's DID let owner_did = auth.0; diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 80b632300..1a50cde15 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -687,8 +687,69 @@ pub fn read_object(repo_path: &Path, sha256_hex: &str) -> Result std::result::Result<(), String> { + if name.is_empty() { + return Err("branch ref must not be empty".into()); + } + // Option-injection core: a leading '-' makes git read the value as a flag. + if name.starts_with('-') { + return Err("branch ref must not begin with '-'".into()); + } + if name.len() > 255 { + return Err("branch ref must be at most 255 bytes".into()); + } + if name.chars().any(|c| c.is_ascii_control() || c == ' ') { + return Err("branch ref must not contain control characters or spaces".into()); + } + if name.contains(['~', '^', ':', '?', '*', '[', '\\']) { + return Err("branch ref must not contain any of ~ ^ : ? * [ \\".into()); + } + if name.contains("..") || name.contains("@{") { + return Err("branch ref must not contain '..' or '@{'".into()); + } + if name == "@" { + return Err("branch ref must not be '@'".into()); + } + if name.starts_with('/') || name.ends_with('/') || name.contains("//") { + return Err("branch ref must not have empty path components".into()); + } + if name.ends_with(".lock") { + return Err("branch ref must not end with '.lock'".into()); + } + for component in name.split('/') { + if component.starts_with('.') || component.ends_with(".lock") { + return Err( + "no branch ref path component may start with '.' or end with '.lock'".into(), + ); + } + } + Ok(()) +} + +/// Reject both refs at the sink, so an option-shaped ref can never reach a git +/// argv element regardless of how it was stored. +fn guard_refs(target_branch: &str, source_branch: &str) -> Result<()> { + validate_git_ref(target_branch) + .map_err(|e| anyhow::anyhow!("invalid target branch ref: {e}"))?; + validate_git_ref(source_branch) + .map_err(|e| anyhow::anyhow!("invalid source branch ref: {e}"))?; + Ok(()) +} + /// Get the diff between two branches: changes on source_branch not in target_branch. pub fn branch_diff(repo_path: &Path, target_branch: &str, source_branch: &str) -> Result { + guard_refs(target_branch, source_branch)?; let output = Command::new("git") .args(["diff", &format!("{target_branch}...{source_branch}")]) .current_dir(repo_path) @@ -706,6 +767,7 @@ pub fn branch_diff_names( target_branch: &str, source_branch: &str, ) -> Result> { + guard_refs(target_branch, source_branch)?; let output = Command::new("git") .args([ "diff", @@ -740,6 +802,7 @@ pub fn merge_branch( author_did: &str, pr_title: &str, ) -> Result { + guard_refs(target_branch, source_branch)?; let worktree_path = repo_path.join("_merge_worktree"); // Clean up any leftover worktree @@ -819,6 +882,45 @@ pub fn repo_disk_path(repos_dir: &Path, owner_did: &str, repo_name: &str) -> Pat #[cfg(test)] mod tests { + use super::validate_git_ref; + + #[test] + fn validate_git_ref_accepts_normal_branch_names() { + for good in [ + "main", + "feature/foo", + "release-1.2", + "v1.0.0", + "user/fix-bug", + ] { + assert!( + validate_git_ref(good).is_ok(), + "{good:?} should be accepted" + ); + } + } + + #[test] + fn validate_git_ref_rejects_option_injection_and_malformed_refs() { + let long = "a".repeat(256); + for bad in [ + "", + "--output=/tmp/x", + "-rf", + "a b", + "a..b", + "a~b", + "refs/heads/@{x}", + "foo.lock", + "/leading", + "trailing/", + "a//b", + long.as_str(), + ] { + assert!(validate_git_ref(bad).is_err(), "{bad:?} should be rejected"); + } + } + use super::branch_diff_names; use std::path::Path; use std::process::Command; diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 2b5aef951..e05b253e2 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -753,6 +753,235 @@ mod tests { ); } + /// SINK GUARD (defense in depth): a poisoned PR row that bypassed the create_pr + /// (a pre-fix row, or any writer create_pr does not gate) still reach the git + /// sink? Inserts the row directly via db.create_pr and drives get_pr_diff + /// anonymously. If the attacker-named file appears, the storage-boundary fix + /// does not cover legacy/other-writer rows and the sink itself needs a guard. + #[sqlx::test] + async fn poisoned_pr_row_cannot_write_a_file_through_the_diff_sink(pool: PgPool) { + use gitlawb_core::identity::Keypair; + use std::process::Command; + struct DirGuard(std::path::PathBuf); + impl Drop for DirGuard { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + let kp = Keypair::generate(); + let owner_did = kp.did().to_string(); + let short = owner_did.split(':').next_back().unwrap().to_string(); + let slug = owner_did.replace([':', '/'], "_"); + let state = test_state(pool).await; + let run = |args: &[&str], cwd: &std::path::Path| { + let out = Command::new("git") + .args(args) + .current_dir(cwd) + .output() + .expect("git"); + assert!( + out.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&out.stderr) + ); + }; + let src = std::env::temp_dir().join(format!("gl-probe-src-{short}")); + let _ = std::fs::remove_dir_all(&src); + std::fs::create_dir_all(&src).unwrap(); + let _sg = DirGuard(src.clone()); + run(&["init", "-q", "-b", "main"], &src); + run(&["config", "user.email", "t@t"], &src); + run(&["config", "user.name", "t"], &src); + std::fs::write(src.join("f.txt"), b"hi").unwrap(); + run(&["add", "f.txt"], &src); + run(&["commit", "-q", "-m", "seed"], &src); + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join("probe.git"); + let _ = std::fs::remove_dir_all(&bare); + std::fs::create_dir_all(bare.parent().unwrap()).unwrap(); + let _bg = DirGuard(bare.clone()); + run( + &[ + "clone", + "--bare", + "-q", + src.to_str().unwrap(), + bare.to_str().unwrap(), + ], + &std::env::temp_dir(), + ); + + let mut repo = seed_repo(&owner_did, "probe"); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + let pwn = std::env::temp_dir().join(format!("gl-PROBE-{short}")); + let _ = std::fs::remove_file(&pwn); + let pwn_glued: std::path::PathBuf = format!("{}...main", pwn.to_str().unwrap()).into(); + let _pg1 = DirGuard(pwn.clone()); + let _pg2 = DirGuard(pwn_glued.clone()); + + // Insert the poisoned row DIRECTLY, as a pre-fix row or an ungated writer would. + let pr = crate::db::PullRequest { + id: uuid::Uuid::new_v4().to_string(), + repo_id: repo.id.clone(), + number: 1, + title: "x".into(), + body: None, + author_did: owner_did.clone(), + source_branch: "main".into(), + target_branch: format!("--output={}", pwn.to_str().unwrap()), + status: "open".into(), + merged_by_did: None, + merged_at: None, + created_at: chrono::Utc::now().to_rfc3339(), + updated_at: chrono::Utc::now().to_rfc3339(), + }; + state.db.create_pr(&pr).await.expect("insert poisoned row"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/diff", + axum::routing::get(crate::api::pulls::get_pr_diff), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner_did}/probe/pulls/1/diff"); + let resp = router.oneshot(anon_get(&uri)).await.unwrap(); + let st = resp.status(); + + let written = pwn.exists() || pwn_glued.exists(); + assert!( + !written, + "the sink must not write a file even for a poisoned row that bypassed the \ + create_pr boundary (get_pr_diff status {st})" + ); + } + + /// SECURITY (option injection, source arm): create_pr must reject an + /// option-shaped source_branch too. merge_branch interpolates source as its + /// own argv element (git merge {source}), so this is the merge-shaped twin of + /// the target test above. + #[sqlx::test] + async fn create_pr_rejects_option_injecting_source_branch(pool: PgPool) { + let owner = "did:key:zPRSRCINJAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let state = test_state(pool).await; + let repo = seed_repo(owner, "pub-src-inj-repo"); + state.db.create_repo(&repo).await.expect("seed repo"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls", + axum::routing::post(crate::api::pulls::create_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner}/pub-src-inj-repo/pulls"); + let body = Body::from( + r#"{"title":"x","source_branch":"--output=/tmp/gl-should-not-exist-src","target_branch":"main"}"#, + ); + let resp = router + .oneshot(signed_request_as(owner, Method::POST, &uri, body)) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "an option-shaped source_branch must be rejected, got {}", + resp.status() + ); + let prs = state.db.list_prs(&repo.id).await.expect("list_prs"); + assert!(prs.is_empty(), "no PR row when source_branch is rejected"); + } + + /// SECURITY (option injection, second entry point): create_repo must reject a + /// default_branch that git would parse as an option. Otherwise an owner sets + /// default_branch = "--output=...", opens a PR omitting target_branch so the + /// stored target falls back to that default, and the diff/merge sink injects. + #[sqlx::test] + async fn create_repo_rejects_option_injecting_default_branch(pool: PgPool) { + let owner = "did:key:zRepoDEFINJAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let state = test_state(pool).await; + + let router = Router::new() + .route( + "/api/v1/repos", + axum::routing::post(crate::api::repos::create_repo), + ) + .with_state(state.clone()); + let body = Body::from( + r#"{"name":"inj-default","default_branch":"--output=/tmp/gl-should-not-exist-def"}"#, + ); + let resp = router + .oneshot(signed_request_as( + owner, + Method::POST, + "/api/v1/repos", + body, + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "an option-shaped default_branch must be rejected, got {}", + resp.status() + ); + + // Nothing stored: the boundary rejects before init/create_repo. + assert!( + state + .db + .get_repo(owner, "inj-default") + .await + .unwrap() + .is_none(), + "no repo row must be created when default_branch is rejected" + ); + } + + /// SECURITY (option injection): create_pr must reject a branch ref that git + /// would parse as an option, so a stored `--output=...` target cannot later + /// turn get_pr_diff / merge into an arbitrary file write. The caller here is + /// the owner (a reader) of a PUBLIC repo, i.e. the minimum access needed to + /// plant a PR, and the request must be refused before any row is written. + #[sqlx::test] + async fn create_pr_rejects_option_injecting_branch_ref(pool: PgPool) { + let owner = "did:key:zPRINJOWNERAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let state = test_state(pool).await; + let repo = seed_repo(owner, "pub-inj-repo"); // is_public = true + state.db.create_repo(&repo).await.expect("seed repo"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls", + axum::routing::post(crate::api::pulls::create_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner}/pub-inj-repo/pulls"); + let body = Body::from( + r#"{"title":"x","source_branch":"main","target_branch":"--output=/tmp/gl-should-not-exist-inj"}"#, + ); + + let resp = router + .oneshot(signed_request_as(owner, Method::POST, &uri, body)) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "an option-shaped branch ref must be rejected, got {}", + resp.status() + ); + + // And nothing was stored: the boundary rejects before the write. + let prs = state.db.list_prs(&repo.id).await.expect("list_prs"); + assert!( + prs.is_empty(), + "no PR row must be created when the branch ref is rejected" + ); + } + /// Adversarial-review GATE-2 (create_issue): filing an issue requires read /// access. A non-reader is denied on a private repo before any git work. #[sqlx::test]