diff --git a/crates/gitlawb-node/src/api/mod.rs b/crates/gitlawb-node/src/api/mod.rs index 71bfa43c..0435a528 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 26be6109..b1e68314 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 b09cb6da..5aa3a01d 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 80b63230..1a50cde1 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 2b5aef95..e05b253e 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]