From 8bb2470c8a0119689dc91469c42706e05e7f6cc6 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 01/12] 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 df10175a..aae2bbd2 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 6255ef24..b6fa8381 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 4e327c42..f5e747af 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 5617b419..1635fb60 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -746,8 +746,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) @@ -765,6 +826,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", @@ -799,6 +861,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 @@ -878,6 +941,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 430c0600..4fcbf375 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -786,6 +786,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] From 52dae20b78e72ae680fb47350ab2855ed17cd132 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:00:39 -0500 Subject: [PATCH 02/12] fix(node): delegate branch ref validation to git check-ref-format Rebase onto current main and tighten validate_git_ref to call git check-ref-format --branch so symbolic names like HEAD and trailing-dot components are rejected at storage boundaries and git sinks. Add boundary and legacy-row tests for HEAD, feature., and feature/x. --- crates/gitlawb-node/src/git/store.rs | 60 +++---- crates/gitlawb-node/src/test_support.rs | 216 ++++++++++++++++++++++++ 2 files changed, 236 insertions(+), 40 deletions(-) diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 1635fb60..7acafc6a 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -746,12 +746,13 @@ 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(), - ); - } + let output = Command::new("git") + .args(["check-ref-format", "--branch", name]) + .output() + .map_err(|e| format!("failed to run git check-ref-format: {e}"))?; + + if output.status.success() { + Ok(()) + } else { + let stderr = String::from_utf8_lossy(&output.stderr); + Err(stderr.trim().to_string()) } - Ok(()) } /// Reject both refs at the sink, so an option-shaped ref can never reach a git @@ -961,9 +940,11 @@ mod tests { #[test] fn validate_git_ref_rejects_option_injection_and_malformed_refs() { - let long = "a".repeat(256); for bad in [ "", + "HEAD", + "feature.", + "feature/x.", "--output=/tmp/x", "-rf", "a b", @@ -974,7 +955,6 @@ mod tests { "/leading", "trailing/", "a//b", - long.as_str(), ] { assert!(validate_git_ref(bad).is_err(), "{bad:?} should be rejected"); } diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 4fcbf375..41a6b6b9 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -1015,6 +1015,222 @@ mod tests { ); } + /// Branch-name contract (jatmn #379): create_pr must reject symbolic revision + /// names and trailing-dot components that `git check-ref-format --branch` refuses. + #[sqlx::test] + async fn create_pr_rejects_non_branch_ref_names(pool: PgPool) { + let owner = "did:key:zPRBRANCHREFAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let state = test_state(pool).await; + let repo = seed_repo(owner, "branch-ref-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}/branch-ref-repo/pulls"); + + for (field, bad) in [ + ("source_branch", "HEAD"), + ("source_branch", "feature."), + ("source_branch", "feature/x."), + ("target_branch", "HEAD"), + ("target_branch", "feature."), + ("target_branch", "feature/x."), + ] { + let body_json = if field == "source_branch" { + format!(r#"{{"title":"x","source_branch":"{bad}","target_branch":"main"}}"#) + } else { + format!(r#"{{"title":"x","source_branch":"main","target_branch":"{bad}"}}"#) + }; + let body = Body::from(body_json); + let resp = router() + .oneshot(signed_request_as(owner, Method::POST, &uri, body)) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "{field}={bad:?} 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 {field}={bad:?} is rejected" + ); + } + } + + /// Branch-name contract (jatmn #379): create_repo must reject a default_branch + /// that is not a real branch name. + #[sqlx::test] + async fn create_repo_rejects_non_branch_default_branch(pool: PgPool) { + let owner = "did:key:zREPOBRANCHREFAAAAAAAAAAAAAAAAAAAAAAAAA"; + 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()) + }; + + for (name_suffix, bad) in [ + ("head", "HEAD"), + ("trail-dot", "feature."), + ("trail-dot-comp", "feature/x."), + ] { + let repo_name = format!("bad-def-{name_suffix}"); + let body = Body::from(format!( + r#"{{"name":"{repo_name}","default_branch":"{bad}"}}"# + )); + let resp = router() + .oneshot(signed_request_as( + owner, + Method::POST, + "/api/v1/repos", + body, + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "default_branch={bad:?} must be rejected, got {}", + resp.status() + ); + assert!( + state + .db + .get_repo(owner, &repo_name) + .await + .unwrap() + .is_none(), + "no repo row when default_branch={bad:?} is rejected" + ); + } + } + + /// SINK GUARD: a legacy row with source_branch=HEAD must be rejected before + /// merge_branch runs git. Without the sink guard, git merge HEAD succeeds on + /// the checked-out target and the PR is recorded as merged without merging + /// the submitted source branch. + #[sqlx::test] + async fn legacy_pr_row_with_head_source_is_rejected_at_merge_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-head-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); + run(&["branch", "feature"], &src); + std::fs::write(src.join("f.txt"), b"changed").unwrap(); + run(&["add", "f.txt"], &src); + run(&["commit", "-q", "-m", "on feature"], &src); + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join("head-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, "head-probe"); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + 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: "HEAD".into(), + target_branch: "main".into(), + 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 legacy row"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/merge", + axum::routing::post(crate::api::pulls::merge_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner_did}/head-probe/pulls/1/merge"); + let resp = router + .oneshot(signed_request_as(&owner_did, Method::POST, &uri, Body::empty())) + .await + .unwrap(); + assert!( + !resp.status().is_success(), + "merge must not succeed for source_branch=HEAD, got {}", + resp.status() + ); + + let stored = state + .db + .get_pr(&repo.id, 1) + .await + .expect("get_pr") + .expect("pr row"); + assert_eq!( + stored.status, "open", + "a rejected merge must not record pull_request.merged" + ); + } + /// 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] From bdaae4712386a0a219246b3e6a4599c87293d2ed Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:52:51 -0500 Subject: [PATCH 03/12] fix(node): reject fully qualified refs and close review test gaps Reject refs/ prefixes after check-ref-format so tags and qualified head names cannot be stored as PR branch fields. Add sink tests for option-shaped merge source, HEAD target, and poisoned default_branch resolution; assert get_pr_diff fails on poisoned diff rows. --- crates/gitlawb-node/src/git/store.rs | 16 +- crates/gitlawb-node/src/test_support.rs | 260 ++++++++++++++++++++++++ 2 files changed, 273 insertions(+), 3 deletions(-) diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 7acafc6a..7aac03e6 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -747,9 +747,13 @@ 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()); } + if name.starts_with("refs/") { + return Err("branch ref must be a local branch name, not a fully qualified ref".into()); + } let output = Command::new("git") .args(["check-ref-format", "--branch", name]) .output() @@ -945,6 +952,9 @@ mod tests { "HEAD", "feature.", "feature/x.", + "refs/heads/main", + "refs/tags/v1", + "refs/heads/--output=/tmp/x", "--output=/tmp/x", "-rf", "a b", diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 41a6b6b9..fe7bdf0c 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -883,6 +883,10 @@ mod tests { 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(); + assert!( + !st.is_success(), + "get_pr_diff must not succeed for a poisoned row (got {st})" + ); let written = pwn.exists() || pwn_glued.exists(); assert!( @@ -1038,9 +1042,11 @@ mod tests { ("source_branch", "HEAD"), ("source_branch", "feature."), ("source_branch", "feature/x."), + ("source_branch", "refs/tags/v1"), ("target_branch", "HEAD"), ("target_branch", "feature."), ("target_branch", "feature/x."), + ("target_branch", "refs/heads/main"), ] { let body_json = if field == "source_branch" { format!(r#"{{"title":"x","source_branch":"{bad}","target_branch":"main"}}"#) @@ -1086,6 +1092,7 @@ mod tests { ("head", "HEAD"), ("trail-dot", "feature."), ("trail-dot-comp", "feature/x."), + ("tag-ref", "refs/tags/v1"), ] { let repo_name = format!("bad-def-{name_suffix}"); let body = Body::from(format!( @@ -1231,6 +1238,259 @@ mod tests { ); } + /// SINK GUARD: legacy target_branch=HEAD must be rejected before worktree add. + #[sqlx::test] + async fn legacy_pr_row_with_head_target_is_rejected_at_merge_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-head-tgt-{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); + run(&["branch", "feature"], &src); + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join("head-tgt-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, "head-tgt-probe"); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + 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: "feature".into(), + target_branch: "HEAD".into(), + 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 legacy row"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/merge", + axum::routing::post(crate::api::pulls::merge_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner_did}/head-tgt-probe/pulls/1/merge"); + let resp = router + .oneshot(signed_request_as(&owner_did, Method::POST, &uri, Body::empty())) + .await + .unwrap(); + assert!( + !resp.status().is_success(), + "merge must not succeed for target_branch=HEAD, got {}", + resp.status() + ); + let stored = state + .db + .get_pr(&repo.id, 1) + .await + .expect("get_pr") + .expect("pr row"); + assert_eq!(stored.status, "open"); + } + + /// SINK GUARD: a poisoned row with option-shaped source_branch must not reach + /// git merge argv (distinct from the diff revspec injection shape). + #[sqlx::test] + async fn poisoned_pr_row_option_source_is_rejected_at_merge_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-merge-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("merge-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, "merge-probe"); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + let pwn = std::env::temp_dir().join(format!("gl-MERGE-{short}")); + let _ = std::fs::remove_file(&pwn); + + 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: format!("--output={}", pwn.to_str().unwrap()), + target_branch: "main".into(), + 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}/merge", + axum::routing::post(crate::api::pulls::merge_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner_did}/merge-probe/pulls/1/merge"); + let resp = router + .oneshot(signed_request_as(&owner_did, Method::POST, &uri, Body::empty())) + .await + .unwrap(); + assert!( + !resp.status().is_success(), + "merge must not succeed for option-shaped source_branch, got {}", + resp.status() + ); + assert!( + !pwn.exists(), + "merge sink must not write via option-shaped source_branch" + ); + let stored = state + .db + .get_pr(&repo.id, 1) + .await + .expect("get_pr") + .expect("pr row"); + assert_eq!(stored.status, "open"); + } + + /// create_pr must validate the resolved target_branch even when the caller + /// omits target_branch and the repo row carries a poisoned default_branch. + #[sqlx::test] + async fn create_pr_rejects_resolved_target_from_poisoned_default_branch(pool: PgPool) { + let owner = "did:key:zPRDEFPOISONAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let state = test_state(pool).await; + let repo = seed_repo(owner, "def-poison-repo"); + state.db.create_repo(&repo).await.expect("seed repo"); + sqlx::query("UPDATE repos SET default_branch = $1 WHERE id = $2") + .bind("HEAD") + .bind(&repo.id) + .execute(state.db.pool()) + .await + .expect("poison default_branch"); + + 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}/def-poison-repo/pulls"); + let body = Body::from(r#"{"title":"x","source_branch":"feature"}"#); + let resp = router + .oneshot(signed_request_as(owner, Method::POST, &uri, body)) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::BAD_REQUEST, + "resolved target from poisoned default_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 resolved target_branch is invalid" + ); + } + /// 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] From 71c8f8090a2ab37c792260005f28e56e1b027ceb Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:54:37 -0500 Subject: [PATCH 04/12] fix(node): allow-unbounded-git for check-ref-format validation Stateless ref-name validation holds no repo concurrency permit. --- crates/gitlawb-node/src/git/store.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 7aac03e6..35feee53 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -769,6 +769,7 @@ pub fn validate_git_ref(name: &str) -> std::result::Result<(), String> { if name.starts_with("refs/") { return Err("branch ref must be a local branch name, not a fully qualified ref".into()); } + // allow-unbounded-git: stateless check-ref-format on a user-supplied name; no repo acquire or concurrency permit let output = Command::new("git") .args(["check-ref-format", "--branch", name]) .output() From 799b73a92d622fadf084948e414232e78401eb7b Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:56:02 -0500 Subject: [PATCH 05/12] style: cargo fmt --- crates/gitlawb-node/src/test_support.rs | 26 ++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index fe7bdf0c..244ce171 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -1065,10 +1065,7 @@ mod tests { resp.status() ); let prs = state.db.list_prs(&repo.id).await.expect("list_prs"); - assert!( - prs.is_empty(), - "no PR row when {field}={bad:?} is rejected" - ); + assert!(prs.is_empty(), "no PR row when {field}={bad:?} is rejected"); } } @@ -1217,7 +1214,12 @@ mod tests { .with_state(state.clone()); let uri = format!("/api/v1/repos/{owner_did}/head-probe/pulls/1/merge"); let resp = router - .oneshot(signed_request_as(&owner_did, Method::POST, &uri, Body::empty())) + .oneshot(signed_request_as( + &owner_did, + Method::POST, + &uri, + Body::empty(), + )) .await .unwrap(); assert!( @@ -1324,7 +1326,12 @@ mod tests { .with_state(state.clone()); let uri = format!("/api/v1/repos/{owner_did}/head-tgt-probe/pulls/1/merge"); let resp = router - .oneshot(signed_request_as(&owner_did, Method::POST, &uri, Body::empty())) + .oneshot(signed_request_as( + &owner_did, + Method::POST, + &uri, + Body::empty(), + )) .await .unwrap(); assert!( @@ -1430,7 +1437,12 @@ mod tests { .with_state(state.clone()); let uri = format!("/api/v1/repos/{owner_did}/merge-probe/pulls/1/merge"); let resp = router - .oneshot(signed_request_as(&owner_did, Method::POST, &uri, Body::empty())) + .oneshot(signed_request_as( + &owner_did, + Method::POST, + &uri, + Body::empty(), + )) .await .unwrap(); assert!( From 259ef673175a802f406d0aa2befc8c08800d27af Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 23:30:11 -0500 Subject: [PATCH 06/12] fix(node): reject revision shorthands and split ref-validation errors git check-ref-format accepts @ and pseudoref names that git later treats as symbolic revisions, so merge could succeed without merging the source branch. Reject those explicitly at the shared validator and add sink/boundary tests. Spawn failures from check-ref-format now surface as git_error (500) instead of bad_request (400) at create_pr/create_repo boundaries. --- crates/gitlawb-node/src/api/mod.rs | 11 +++ crates/gitlawb-node/src/api/pulls.rs | 6 +- crates/gitlawb-node/src/api/repos.rs | 3 +- crates/gitlawb-node/src/git/store.rs | 112 ++++++++++++++++++--- crates/gitlawb-node/src/test_support.rs | 126 ++++++++++++++++++++++++ 5 files changed, 243 insertions(+), 15 deletions(-) diff --git a/crates/gitlawb-node/src/api/mod.rs b/crates/gitlawb-node/src/api/mod.rs index aae2bbd2..40ed8c81 100644 --- a/crates/gitlawb-node/src/api/mod.rs +++ b/crates/gitlawb-node/src/api/mod.rs @@ -98,6 +98,17 @@ pub(crate) fn require_repo_owner(record: &RepoRecord, caller: &str) -> Result<() /// caller. Its unit tests live beside the definition in `git/store.rs`. pub(crate) use crate::git::store::validate_git_ref; +/// Map ref-validation failures to the correct HTTP surface: malformed caller +/// input stays 400; an inability to run git stays on the git/server-error path. +pub(crate) fn map_git_ref_validation_error( + err: crate::git::store::GitRefValidationError, +) -> AppError { + match err { + crate::git::store::GitRefValidationError::Invalid(msg) => AppError::BadRequest(msg), + crate::git::store::GitRefValidationError::GitUnavailable(msg) => AppError::Git(msg), + } +} + #[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 b6fa8381..c665447a 100644 --- a/crates/gitlawb-node/src/api/pulls.rs +++ b/crates/gitlawb-node/src/api/pulls.rs @@ -57,8 +57,10 @@ pub async fn create_pr( // 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)?; + crate::api::validate_git_ref(&req.source_branch) + .map_err(crate::api::map_git_ref_validation_error)?; + crate::api::validate_git_ref(&target_branch) + .map_err(crate::api::map_git_ref_validation_error)?; 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 f5e747af..a4edffc8 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -245,7 +245,8 @@ 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)?; + crate::api::validate_git_ref(&req.default_branch) + .map_err(crate::api::map_git_ref_validation_error)?; // 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 35feee53..24689972 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -746,6 +746,46 @@ pub fn read_object(repo_path: &Path, sha256_hex: &str) -> Result) -> std::fmt::Result { + match self { + Self::Invalid(msg) | Self::GitUnavailable(msg) => f.write_str(msg), + } + } +} + +/// Reject revision shorthands and pseudoref names that `git check-ref-format +/// --branch` accepts but that git later interprets as symbolic revisions rather +/// than literal local branch names. +fn reject_revision_shorthand(name: &str) -> Option<&'static str> { + if name == "@" { + return Some("branch ref must not be a revision shorthand"); + } + if name.starts_with("@{") { + return Some("branch ref must not be a reflog revision expression"); + } + const PSEUDOREFS: &[&str] = &[ + "FETCH_HEAD", + "ORIG_HEAD", + "MERGE_HEAD", + "CHERRY_PICK_HEAD", + "RERERE_MERGE_HEAD", + ]; + if PSEUDOREFS.contains(&name) { + return Some("branch ref must not be a git pseudoref name"); + } + None +} + /// Validate a git *branch name* before it is stored or interpolated into a git /// argv element. Delegates to `git check-ref-format --branch`, then rejects /// fully qualified ref paths (`refs/heads/...`, `refs/tags/...`) that git would @@ -754,6 +794,9 @@ pub fn read_object(repo_path: &Path, sha256_hex: &str) -> Result Result std::result::Result<(), String> { +pub fn validate_git_ref(name: &str) -> std::result::Result<(), GitRefValidationError> { + validate_git_ref_with_git("git", name) +} + +fn validate_git_ref_with_git( + git_bin: &str, + name: &str, +) -> std::result::Result<(), GitRefValidationError> { if name.is_empty() { - return Err("branch ref must not be empty".into()); + return Err(GitRefValidationError::Invalid( + "branch ref must not be empty".into(), + )); } if name.starts_with("refs/") { - return Err("branch ref must be a local branch name, not a fully qualified ref".into()); + return Err(GitRefValidationError::Invalid( + "branch ref must be a local branch name, not a fully qualified ref".into(), + )); + } + if let Some(reason) = reject_revision_shorthand(name) { + return Err(GitRefValidationError::Invalid(reason.into())); } // allow-unbounded-git: stateless check-ref-format on a user-supplied name; no repo acquire or concurrency permit - let output = Command::new("git") + let output = Command::new(git_bin) .args(["check-ref-format", "--branch", name]) .output() - .map_err(|e| format!("failed to run git check-ref-format: {e}"))?; + .map_err(|e| { + GitRefValidationError::GitUnavailable(format!( + "failed to run git check-ref-format: {e}" + )) + })?; if output.status.success() { Ok(()) } else { let stderr = String::from_utf8_lossy(&output.stderr); - Err(stderr.trim().to_string()) + Err(GitRefValidationError::Invalid(stderr.trim().to_string())) } } /// 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 map_ref_validation_to_anyhow(which: &str, err: GitRefValidationError) -> anyhow::Error { + match err { + GitRefValidationError::Invalid(msg) => { + anyhow::anyhow!("invalid {which} branch ref: {msg}") + } + GitRefValidationError::GitUnavailable(msg) => anyhow::anyhow!("{msg}"), + } +} + 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}"))?; + validate_git_ref(target_branch).map_err(|e| map_ref_validation_to_anyhow("target", e))?; + validate_git_ref(source_branch).map_err(|e| map_ref_validation_to_anyhow("source", e))?; Ok(()) } @@ -928,7 +996,7 @@ pub fn repo_disk_path(repos_dir: &Path, owner_did: &str, repo_name: &str) -> Pat #[cfg(test)] mod tests { - use super::validate_git_ref; + use super::{validate_git_ref, validate_git_ref_with_git, GitRefValidationError}; #[test] fn validate_git_ref_accepts_normal_branch_names() { @@ -938,6 +1006,8 @@ mod tests { "release-1.2", "v1.0.0", "user/fix-bug", + "feature@", + "user@host", ] { assert!( validate_git_ref(good).is_ok(), @@ -951,6 +1021,10 @@ mod tests { for bad in [ "", "HEAD", + "@", + "@{-1}", + "FETCH_HEAD", + "MERGE_HEAD", "feature.", "feature/x.", "refs/heads/main", @@ -967,10 +1041,24 @@ mod tests { "trailing/", "a//b", ] { - assert!(validate_git_ref(bad).is_err(), "{bad:?} should be rejected"); + let err = validate_git_ref(bad).expect_err("{bad:?} should be rejected"); + assert!( + matches!(err, GitRefValidationError::Invalid(_)), + "{bad:?} should be an invalid-name rejection, got {err:?}" + ); } } + #[test] + fn validate_git_ref_spawn_failure_is_git_unavailable_not_invalid() { + let err = validate_git_ref_with_git("/nonexistent/gitlawb-git", "main") + .expect_err("missing git binary should fail"); + assert!( + matches!(err, GitRefValidationError::GitUnavailable(_)), + "spawn failure must not be classified as invalid input, got {err:?}" + ); + } + 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 244ce171..cd7cfac2 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -1040,10 +1040,15 @@ mod tests { for (field, bad) in [ ("source_branch", "HEAD"), + ("source_branch", "@"), + ("source_branch", "@{-1}"), + ("source_branch", "FETCH_HEAD"), ("source_branch", "feature."), ("source_branch", "feature/x."), ("source_branch", "refs/tags/v1"), ("target_branch", "HEAD"), + ("target_branch", "@"), + ("target_branch", "@{-1}"), ("target_branch", "feature."), ("target_branch", "feature/x."), ("target_branch", "refs/heads/main"), @@ -1087,6 +1092,9 @@ mod tests { for (name_suffix, bad) in [ ("head", "HEAD"), + ("at-shorthand", "@"), + ("reflog-shorthand", "@{-1}"), + ("fetch-head", "FETCH_HEAD"), ("trail-dot", "feature."), ("trail-dot-comp", "feature/x."), ("tag-ref", "refs/tags/v1"), @@ -1240,6 +1248,124 @@ mod tests { ); } + /// SINK GUARD: a legacy row with source_branch=@ must be rejected before + /// merge_branch runs git. Without the sink guard, git merge @ is a no-op on + /// the checked-out target and the PR is recorded as merged without merging + /// the submitted source branch. + #[sqlx::test] + async fn legacy_pr_row_with_at_shorthand_source_is_rejected_at_merge_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-at-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); + run(&["branch", "feature"], &src); + std::fs::write(src.join("f.txt"), b"changed").unwrap(); + run(&["add", "f.txt"], &src); + run(&["commit", "-q", "-m", "on feature"], &src); + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join("at-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, "at-probe"); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + 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: "@".into(), + target_branch: "main".into(), + 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 legacy row"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/merge", + axum::routing::post(crate::api::pulls::merge_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner_did}/at-probe/pulls/1/merge"); + let resp = router + .oneshot(signed_request_as( + &owner_did, + Method::POST, + &uri, + Body::empty(), + )) + .await + .unwrap(); + assert!( + !resp.status().is_success(), + "merge must not succeed for source_branch=@, got {}", + resp.status() + ); + + let stored = state + .db + .get_pr(&repo.id, 1) + .await + .expect("get_pr") + .expect("pr row"); + assert_eq!( + stored.status, "open", + "a rejected merge must not record pull_request.merged" + ); + } + /// SINK GUARD: legacy target_branch=HEAD must be rejected before worktree add. #[sqlx::test] async fn legacy_pr_row_with_head_target_is_rejected_at_merge_sink(pool: PgPool) { From acb3214dad5bcde23bee8751e6a250d1105f2297 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 23:58:35 -0500 Subject: [PATCH 07/12] test(node): close ce-review gaps on git ref validation Add sink tests for @{-1} and pseudoref poisoned rows at diff/merge, HTTP tests that spawn failure returns git_error (500) not bad_request, and expand API boundary cases for remaining pseudorefs and qualified refs. Introduce GITLAWB_TEST_VALIDATE_GIT_BIN test seam with a mutex so parallel sqlx tests do not race on the override. --- crates/gitlawb-node/src/git/store.rs | 11 +- crates/gitlawb-node/src/test_support.rs | 355 ++++++++++++++++++++++++ 2 files changed, 365 insertions(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 24689972..ad55a390 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -805,8 +805,17 @@ fn reject_revision_shorthand(name: &str) -> Option<&'static str> { /// call it too via the `crate::api` re-export to fail fast with a 400, but the /// guard here is what makes the property hold for every caller and every row, /// including legacy rows and any writer that skipped the boundary check. +fn validate_git_ref_binary() -> std::borrow::Cow<'static, str> { + #[cfg(test)] + if let Ok(bin) = std::env::var("GITLAWB_TEST_VALIDATE_GIT_BIN") { + return std::borrow::Cow::Owned(bin); + } + std::borrow::Cow::Borrowed("git") +} + pub fn validate_git_ref(name: &str) -> std::result::Result<(), GitRefValidationError> { - validate_git_ref_with_git("git", name) + let git_bin = validate_git_ref_binary(); + validate_git_ref_with_git(git_bin.as_ref(), name) } fn validate_git_ref_with_git( diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index cd7cfac2..6597a389 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -187,8 +187,40 @@ mod tests { use crate::db::{AgentTask, RepoRecord}; use axum::http::StatusCode; use chrono::Utc; + use std::sync::Mutex; use tower::ServiceExt; + static VALIDATE_GIT_BIN_TEST_LOCK: Mutex<()> = Mutex::new(()); + + /// Serialize tests that override `GITLAWB_TEST_VALIDATE_GIT_BIN` (process-global). + struct ValidateGitBinTestOverride { + _lock: std::sync::MutexGuard<'static, ()>, + prev: Option, + } + + impl ValidateGitBinTestOverride { + fn missing_git() -> Self { + let lock = VALIDATE_GIT_BIN_TEST_LOCK + .lock() + .expect("validate git bin test lock"); + let prev = std::env::var("GITLAWB_TEST_VALIDATE_GIT_BIN").ok(); + std::env::set_var( + "GITLAWB_TEST_VALIDATE_GIT_BIN", + "/nonexistent/gitlawb-validate-git", + ); + Self { _lock: lock, prev } + } + } + + impl Drop for ValidateGitBinTestOverride { + fn drop(&mut self) { + match self.prev.take() { + Some(v) => std::env::set_var("GITLAWB_TEST_VALIDATE_GIT_BIN", v), + None => std::env::remove_var("GITLAWB_TEST_VALIDATE_GIT_BIN"), + } + } + } + fn seed_repo(owner_did: &str, name: &str) -> RepoRecord { let now = Utc::now(); RepoRecord { @@ -1043,12 +1075,18 @@ mod tests { ("source_branch", "@"), ("source_branch", "@{-1}"), ("source_branch", "FETCH_HEAD"), + ("source_branch", "ORIG_HEAD"), + ("source_branch", "MERGE_HEAD"), + ("source_branch", "CHERRY_PICK_HEAD"), + ("source_branch", "RERERE_MERGE_HEAD"), ("source_branch", "feature."), ("source_branch", "feature/x."), ("source_branch", "refs/tags/v1"), ("target_branch", "HEAD"), ("target_branch", "@"), ("target_branch", "@{-1}"), + ("target_branch", "FETCH_HEAD"), + ("target_branch", "ORIG_HEAD"), ("target_branch", "feature."), ("target_branch", "feature/x."), ("target_branch", "refs/heads/main"), @@ -1095,9 +1133,14 @@ mod tests { ("at-shorthand", "@"), ("reflog-shorthand", "@{-1}"), ("fetch-head", "FETCH_HEAD"), + ("orig-head", "ORIG_HEAD"), + ("merge-head", "MERGE_HEAD"), + ("cherry-head", "CHERRY_PICK_HEAD"), + ("rerere-head", "RERERE_MERGE_HEAD"), ("trail-dot", "feature."), ("trail-dot-comp", "feature/x."), ("tag-ref", "refs/tags/v1"), + ("qualified-head", "refs/heads/main"), ] { let repo_name = format!("bad-def-{name_suffix}"); let body = Body::from(format!( @@ -1366,6 +1409,318 @@ mod tests { ); } + /// SINK GUARD: legacy rows with reflog shorthands or pseudoref source branches + /// must be rejected at merge before git runs. + #[sqlx::test] + async fn legacy_pr_rows_with_shorthand_or_pseudoref_source_rejected_at_merge_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); + } + } + + for (label, bad_source) in [ + ("reflog", "@{-1}"), + ("fetch-head", "FETCH_HEAD"), + ("merge-head", "MERGE_HEAD"), + ] { + 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.clone()).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-{label}-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); + run(&["branch", "feature"], &src); + std::fs::write(src.join("f.txt"), b"changed").unwrap(); + run(&["add", "f.txt"], &src); + run(&["commit", "-q", "-m", "on feature"], &src); + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join(format!("{label}-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 repo_name = format!("{label}-probe"); + let mut repo = seed_repo(&owner_did, &repo_name); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + 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: bad_source.into(), + target_branch: "main".into(), + 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 legacy row"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/merge", + axum::routing::post(crate::api::pulls::merge_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner_did}/{repo_name}/pulls/1/merge"); + let resp = router + .oneshot(signed_request_as( + &owner_did, + Method::POST, + &uri, + Body::empty(), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::INTERNAL_SERVER_ERROR, + "merge must return git_error for source_branch={bad_source:?}, got {}", + resp.status() + ); + + let stored = state + .db + .get_pr(&repo.id, 1) + .await + .expect("get_pr") + .expect("pr row"); + assert_eq!( + stored.status, "open", + "merge must not record pull_request.merged for source_branch={bad_source:?}" + ); + } + } + + /// SINK GUARD: legacy rows with reflog/pseudoref refs must be rejected at diff + /// before git argv is built. + #[sqlx::test] + async fn legacy_pr_rows_with_shorthand_or_pseudoref_rejected_at_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); + } + } + + for (label, bad_source) in [("reflog", "@{-1}"), ("fetch-head", "FETCH_HEAD")] { + 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.clone()).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-diff-{label}-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(format!("diff-{label}.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 repo_name = format!("diff-{label}"); + let mut repo = seed_repo(&owner_did, &repo_name); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + 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: bad_source.into(), + target_branch: "main".into(), + 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 legacy 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}/{repo_name}/pulls/1/diff"); + let resp = router.oneshot(anon_get(&uri)).await.unwrap(); + assert_eq!( + resp.status(), + StatusCode::INTERNAL_SERVER_ERROR, + "get_pr_diff must return git_error for source_branch={bad_source:?}, got {}", + resp.status() + ); + } + } + + /// When git cannot be spawned for check-ref-format, storage boundaries must + /// return git_error (500), not bad_request (400). + #[sqlx::test] + async fn create_pr_returns_git_error_when_validate_git_ref_cannot_spawn_git(pool: PgPool) { + let _git_override = ValidateGitBinTestOverride::missing_git(); + let owner = "did:key:zPRGITSPAWNAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let state = test_state(pool).await; + let repo = seed_repo(owner, "git-spawn-pr"); + 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}/git-spawn-pr/pulls"); + let body = Body::from(r#"{"title":"x","source_branch":"main","target_branch":"main"}"#); + let resp = router + .oneshot(signed_request_as(owner, Method::POST, &uri, body)) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::INTERNAL_SERVER_ERROR, + "spawn failure must not surface as 400, got {}", + resp.status() + ); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("read body"); + let v: serde_json::Value = serde_json::from_slice(&bytes).expect("json body"); + assert_eq!( + v.get("error").and_then(|e| e.as_str()), + Some("git_error"), + "spawn failure must use git_error code, got {v}" + ); + let prs = state.db.list_prs(&repo.id).await.expect("list_prs"); + assert!(prs.is_empty(), "no PR row when git is unavailable"); + } + + #[sqlx::test] + async fn create_repo_returns_git_error_when_validate_git_ref_cannot_spawn_git(pool: PgPool) { + let _git_override = ValidateGitBinTestOverride::missing_git(); + let owner = "did:key:zREPOGITSPAWNAAAAAAAAAAAAAAAAAAAAAAAAA"; + 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":"git-spawn-repo","default_branch":"main"}"#); + let resp = router + .oneshot(signed_request_as( + owner, + Method::POST, + "/api/v1/repos", + body, + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::INTERNAL_SERVER_ERROR, + "spawn failure must not surface as 400, got {}", + resp.status() + ); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("read body"); + let v: serde_json::Value = serde_json::from_slice(&bytes).expect("json body"); + assert_eq!( + v.get("error").and_then(|e| e.as_str()), + Some("git_error"), + "spawn failure must use git_error code, got {v}" + ); + assert!( + state + .db + .get_repo(owner, "git-spawn-repo") + .await + .unwrap() + .is_none(), + "no repo row when git is unavailable" + ); + } + /// SINK GUARD: legacy target_branch=HEAD must be rejected before worktree add. #[sqlx::test] async fn legacy_pr_row_with_head_target_is_rejected_at_merge_sink(pool: PgPool) { From d31042e773bc05b3fd840878393c91270e41341d Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sat, 29 Aug 2026 12:38:21 -0500 Subject: [PATCH 08/12] fix(node): prefix sink git argv with refs/heads and close shorthand gap Pass stored branch short names to diff/merge/worktree as refs/heads/{name} so revision shorthands cannot retarget another ref namespace at the sink. Reject heads/, tags/, and remotes/ prefixes at storage boundaries, extend the pseudoref denylist, and route boundary validation through AppState::git_bin instead of a process-global test env override that raced parallel tests. --- crates/gitlawb-node/src/api/mod.rs | 12 ++-- crates/gitlawb-node/src/api/pulls.rs | 4 +- crates/gitlawb-node/src/api/repos.rs | 2 +- crates/gitlawb-node/src/git/store.rs | 84 ++++++++++++++----------- crates/gitlawb-node/src/test_support.rs | 48 ++++---------- 5 files changed, 69 insertions(+), 81 deletions(-) diff --git a/crates/gitlawb-node/src/api/mod.rs b/crates/gitlawb-node/src/api/mod.rs index 40ed8c81..cecdcfb2 100644 --- a/crates/gitlawb-node/src/api/mod.rs +++ b/crates/gitlawb-node/src/api/mod.rs @@ -92,11 +92,13 @@ 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; +/// Validate a branch name using the configured git binary (storage boundaries). +pub(crate) fn validate_git_ref_with_git( + git_bin: &str, + name: &str, +) -> std::result::Result<(), crate::git::store::GitRefValidationError> { + crate::git::store::validate_git_ref_with_git(git_bin, name) +} /// Map ref-validation failures to the correct HTTP surface: malformed caller /// input stays 400; an inability to run git stays on the git/server-error path. diff --git a/crates/gitlawb-node/src/api/pulls.rs b/crates/gitlawb-node/src/api/pulls.rs index c665447a..d1e0185a 100644 --- a/crates/gitlawb-node/src/api/pulls.rs +++ b/crates/gitlawb-node/src/api/pulls.rs @@ -57,9 +57,9 @@ pub async fn create_pr( // 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) + crate::api::validate_git_ref_with_git(&state.git_bin, &req.source_branch) .map_err(crate::api::map_git_ref_validation_error)?; - crate::api::validate_git_ref(&target_branch) + crate::api::validate_git_ref_with_git(&state.git_bin, &target_branch) .map_err(crate::api::map_git_ref_validation_error)?; 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 a4edffc8..1fb21c51 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -245,7 +245,7 @@ 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) + crate::api::validate_git_ref_with_git(&state.git_bin, &req.default_branch) .map_err(crate::api::map_git_ref_validation_error)?; // Owner is the authenticated agent's DID diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index ad55a390..5279decf 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -779,6 +779,10 @@ fn reject_revision_shorthand(name: &str) -> Option<&'static str> { "MERGE_HEAD", "CHERRY_PICK_HEAD", "RERERE_MERGE_HEAD", + "REBASE_HEAD", + "REVERT_HEAD", + "BISECT_HEAD", + "AUTO_MERGE", ]; if PSEUDOREFS.contains(&name) { return Some("branch ref must not be a git pseudoref name"); @@ -786,39 +790,27 @@ fn reject_revision_shorthand(name: &str) -> Option<&'static str> { None } -/// Validate a git *branch name* before it is stored or interpolated into a git -/// argv element. Delegates to `git check-ref-format --branch`, then rejects -/// fully qualified ref paths (`refs/heads/...`, `refs/tags/...`) that git would -/// accept as branch-name syntax but that resolve as tags or other refs when -/// passed back to git as bare revision arguments. +/// Validate a git *branch name* before it is stored or passed to git subprocesses. +/// Delegates to `git check-ref-format --branch`, then rejects fully qualified +/// ref paths, revision-namespace shorthands (`heads/`, `tags/`, `remotes/`), and +/// symbolic names that git would reinterpret at the sink. /// -/// Symbolic revision names (`HEAD`), option-shaped names (leading `-`), trailing -/// dots, and other invalid forms are rejected by check-ref-format itself. -/// Revision shorthands that pass the grammar check (`@`, `@{-1}`, pseudorefs) -/// are rejected explicitly because git treats them as symbolic revisions at the -/// sink, not as literal branch names. +/// At diff/merge/worktree sinks, validated short names are passed as +/// `refs/heads/{name}` so grammar-valid revision shorthands cannot retarget another +/// ref namespace. Symbolic revision names (`HEAD`), option-shaped names (leading `-`), +/// trailing dots, and other invalid forms are rejected by check-ref-format itself. +/// Revision shorthands that pass the grammar check (`@`, `@{-1}`, pseudorefs) are +/// rejected explicitly because git treats them as symbolic revisions when bare. /// -/// A `--` delimiter is not the fix at the sink: these arguments are revisions, -/// and `--` there reinterprets them as pathspecs. -/// -/// This is the sink-level guard. Storage boundaries (`create_pr`, `create_repo`) -/// call it too via the `crate::api` re-export to fail fast with a 400, but the -/// guard here is what makes the property hold for every caller and every row, +/// Storage boundaries call this via [`validate_git_ref_with_git`] with the configured +/// git binary. Sink functions call [`validate_git_ref`] (system `git`) then prefix +/// before building argv, so the property holds for every caller and every row, /// including legacy rows and any writer that skipped the boundary check. -fn validate_git_ref_binary() -> std::borrow::Cow<'static, str> { - #[cfg(test)] - if let Ok(bin) = std::env::var("GITLAWB_TEST_VALIDATE_GIT_BIN") { - return std::borrow::Cow::Owned(bin); - } - std::borrow::Cow::Borrowed("git") -} - pub fn validate_git_ref(name: &str) -> std::result::Result<(), GitRefValidationError> { - let git_bin = validate_git_ref_binary(); - validate_git_ref_with_git(git_bin.as_ref(), name) + validate_git_ref_with_git("git", name) } -fn validate_git_ref_with_git( +pub(crate) fn validate_git_ref_with_git( git_bin: &str, name: &str, ) -> std::result::Result<(), GitRefValidationError> { @@ -832,6 +824,11 @@ fn validate_git_ref_with_git( "branch ref must be a local branch name, not a fully qualified ref".into(), )); } + if name.starts_with("heads/") || name.starts_with("tags/") || name.starts_with("remotes/") { + return Err(GitRefValidationError::Invalid( + "branch ref must be a local branch name, not a revision shorthand".into(), + )); + } if let Some(reason) = reject_revision_shorthand(name) { return Err(GitRefValidationError::Invalid(reason.into())); } @@ -840,6 +837,7 @@ fn validate_git_ref_with_git( .args(["check-ref-format", "--branch", name]) .output() .map_err(|e| { + // allow-unbounded-git: stateless check-ref-format on user-supplied name; no repo acquire GitRefValidationError::GitUnavailable(format!( "failed to run git check-ref-format: {e}" )) @@ -870,14 +868,26 @@ fn guard_refs(target_branch: &str, source_branch: &str) -> Result<()> { Ok(()) } +/// Stored short branch names are passed to git as explicit local-branch refs so a +/// syntax-valid name cannot be reinterpreted as a revision shorthand at the sink. +fn local_branch_ref(short_name: &str) -> String { + format!("refs/heads/{short_name}") +} + /// 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 target = local_branch_ref(target_branch); + let source = local_branch_ref(source_branch); let output = Command::new("git") - .args(["diff", &format!("{target_branch}...{source_branch}")]) + .args(["diff", &format!("{target}...{source}")]) .current_dir(repo_path) .output() .context("failed to run git diff")?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("git diff failed: {stderr}"); + } Ok(String::from_utf8_lossy(&output.stdout).to_string()) } @@ -891,13 +901,10 @@ pub fn branch_diff_names( source_branch: &str, ) -> Result> { guard_refs(target_branch, source_branch)?; + let target = local_branch_ref(target_branch); + let source = local_branch_ref(source_branch); let output = Command::new("git") - .args([ - "diff", - "--name-only", - "-z", - &format!("{target_branch}...{source_branch}"), - ]) + .args(["diff", "--name-only", "-z", &format!("{target}...{source}")]) .current_dir(repo_path) .output() .context("failed to run git diff --name-only")?; @@ -926,6 +933,8 @@ pub fn merge_branch( pr_title: &str, ) -> Result { guard_refs(target_branch, source_branch)?; + let target_ref = local_branch_ref(target_branch); + let source_ref = local_branch_ref(source_branch); let worktree_path = repo_path.join("_merge_worktree"); // Clean up any leftover worktree @@ -939,7 +948,7 @@ pub fn merge_branch( // Create worktree on target branch let wt = Command::new("git") - .args(["worktree", "add", "_merge_worktree", target_branch]) + .args(["worktree", "add", "_merge_worktree", &target_ref]) .current_dir(repo_path) .output() .context("failed to create worktree")?; @@ -955,7 +964,7 @@ pub fn merge_branch( .args([ "merge", "--no-ff", - source_branch, + &source_ref, "-m", &format!( "Merge branch '{}' into {} ({})", @@ -1039,6 +1048,9 @@ mod tests { "refs/heads/main", "refs/tags/v1", "refs/heads/--output=/tmp/x", + "heads/main", + "tags/v1", + "remotes/origin/main", "--output=/tmp/x", "-rf", "a b", diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 6597a389..a31ec554 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -187,40 +187,7 @@ mod tests { use crate::db::{AgentTask, RepoRecord}; use axum::http::StatusCode; use chrono::Utc; - use std::sync::Mutex; use tower::ServiceExt; - - static VALIDATE_GIT_BIN_TEST_LOCK: Mutex<()> = Mutex::new(()); - - /// Serialize tests that override `GITLAWB_TEST_VALIDATE_GIT_BIN` (process-global). - struct ValidateGitBinTestOverride { - _lock: std::sync::MutexGuard<'static, ()>, - prev: Option, - } - - impl ValidateGitBinTestOverride { - fn missing_git() -> Self { - let lock = VALIDATE_GIT_BIN_TEST_LOCK - .lock() - .expect("validate git bin test lock"); - let prev = std::env::var("GITLAWB_TEST_VALIDATE_GIT_BIN").ok(); - std::env::set_var( - "GITLAWB_TEST_VALIDATE_GIT_BIN", - "/nonexistent/gitlawb-validate-git", - ); - Self { _lock: lock, prev } - } - } - - impl Drop for ValidateGitBinTestOverride { - fn drop(&mut self) { - match self.prev.take() { - Some(v) => std::env::set_var("GITLAWB_TEST_VALIDATE_GIT_BIN", v), - None => std::env::remove_var("GITLAWB_TEST_VALIDATE_GIT_BIN"), - } - } - } - fn seed_repo(owner_did: &str, name: &str) -> RepoRecord { let now = Utc::now(); RepoRecord { @@ -1082,6 +1049,8 @@ mod tests { ("source_branch", "feature."), ("source_branch", "feature/x."), ("source_branch", "refs/tags/v1"), + ("source_branch", "heads/main"), + ("source_branch", "tags/v1"), ("target_branch", "HEAD"), ("target_branch", "@"), ("target_branch", "@{-1}"), @@ -1090,6 +1059,8 @@ mod tests { ("target_branch", "feature."), ("target_branch", "feature/x."), ("target_branch", "refs/heads/main"), + ("target_branch", "heads/main"), + ("target_branch", "tags/v1"), ] { let body_json = if field == "source_branch" { format!(r#"{{"title":"x","source_branch":"{bad}","target_branch":"main"}}"#) @@ -1141,6 +1112,9 @@ mod tests { ("trail-dot-comp", "feature/x."), ("tag-ref", "refs/tags/v1"), ("qualified-head", "refs/heads/main"), + ("heads-shorthand", "heads/main"), + ("tags-shorthand", "tags/v1"), + ("remotes-shorthand", "remotes/origin/main"), ] { let repo_name = format!("bad-def-{name_suffix}"); let body = Body::from(format!( @@ -1636,9 +1610,9 @@ mod tests { /// return git_error (500), not bad_request (400). #[sqlx::test] async fn create_pr_returns_git_error_when_validate_git_ref_cannot_spawn_git(pool: PgPool) { - let _git_override = ValidateGitBinTestOverride::missing_git(); let owner = "did:key:zPRGITSPAWNAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; - let state = test_state(pool).await; + let mut state = test_state(pool).await; + state.git_bin = "/nonexistent/gitlawb-validate-git".into(); let repo = seed_repo(owner, "git-spawn-pr"); state.db.create_repo(&repo).await.expect("seed repo"); @@ -1675,9 +1649,9 @@ mod tests { #[sqlx::test] async fn create_repo_returns_git_error_when_validate_git_ref_cannot_spawn_git(pool: PgPool) { - let _git_override = ValidateGitBinTestOverride::missing_git(); + let mut state = test_state(pool).await; + state.git_bin = "/nonexistent/gitlawb-validate-git".into(); let owner = "did:key:zREPOGITSPAWNAAAAAAAAAAAAAAAAAAAAAAAAA"; - let state = test_state(pool).await; let router = Router::new() .route( From 0c89c6b54bfe6df89ef93fc42d323a797254a18d Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 30 Aug 2026 17:51:00 -0500 Subject: [PATCH 09/12] fix(node): attach merge worktree to the target branch git worktree add with refs/heads/{name} checks out detached HEAD, so a successful merge did not advance refs/heads/{target}. Use the branch name for the worktree and keep refs/heads/ qualification on merge revisions. Add an end-to-end regression for diverged branches. --- crates/gitlawb-node/src/git/store.rs | 13 +- crates/gitlawb-node/src/test_support.rs | 161 ++++++++++++++++++++++++ 2 files changed, 168 insertions(+), 6 deletions(-) diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 5279decf..3b8740d4 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -795,9 +795,10 @@ fn reject_revision_shorthand(name: &str) -> Option<&'static str> { /// ref paths, revision-namespace shorthands (`heads/`, `tags/`, `remotes/`), and /// symbolic names that git would reinterpret at the sink. /// -/// At diff/merge/worktree sinks, validated short names are passed as -/// `refs/heads/{name}` so grammar-valid revision shorthands cannot retarget another -/// ref namespace. Symbolic revision names (`HEAD`), option-shaped names (leading `-`), +/// At diff/merge sinks, validated short names are passed as `refs/heads/{name}` so +/// grammar-valid revision shorthands cannot retarget another ref namespace. The +/// merge worktree checks out the local branch name instead, so merge commits +/// advance refs/heads/{target}. Symbolic revision names (`HEAD`), option-shaped /// trailing dots, and other invalid forms are rejected by check-ref-format itself. /// Revision shorthands that pass the grammar check (`@`, `@{-1}`, pseudorefs) are /// rejected explicitly because git treats them as symbolic revisions when bare. @@ -933,7 +934,6 @@ pub fn merge_branch( pr_title: &str, ) -> Result { guard_refs(target_branch, source_branch)?; - let target_ref = local_branch_ref(target_branch); let source_ref = local_branch_ref(source_branch); let worktree_path = repo_path.join("_merge_worktree"); @@ -946,9 +946,10 @@ pub fn merge_branch( let _ = std::fs::remove_dir_all(&worktree_path); } - // Create worktree on target branch + // Check out the local target branch in the worktree. Passing refs/heads/{name} + // would detach HEAD, so a successful merge would not advance refs/heads/{target}. let wt = Command::new("git") - .args(["worktree", "add", "_merge_worktree", &target_ref]) + .args(["worktree", "add", "_merge_worktree", target_branch]) .current_dir(repo_path) .output() .context("failed to create worktree")?; diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index a31ec554..021c573f 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -1803,6 +1803,167 @@ mod tests { assert_eq!(stored.status, "open"); } + /// Successful merge path (jatmn #379): diverged branches must produce a merge + /// commit on refs/heads/{target}, not only on a detached worktree HEAD. + #[sqlx::test] + async fn merge_pr_advances_target_ref_with_diverged_branches(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) + ); + out + }; + let rev = |args: &[&str], cwd: &std::path::Path| { + String::from_utf8_lossy(&run(args, cwd).stdout) + .trim() + .to_string() + }; + + let src = std::env::temp_dir().join(format!("gl-merge-ok-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("base.txt"), b"base").unwrap(); + run(&["add", "base.txt"], &src); + run(&["commit", "-q", "-m", "seed"], &src); + run(&["checkout", "-b", "feature"], &src); + std::fs::write(src.join("feature.txt"), b"feature-only").unwrap(); + run(&["add", "feature.txt"], &src); + run(&["commit", "-q", "-m", "on feature"], &src); + run(&["checkout", "main"], &src); + std::fs::write(src.join("main.txt"), b"main-only").unwrap(); + run(&["add", "main.txt"], &src); + run(&["commit", "-q", "-m", "on main"], &src); + let feature_tip = rev(&["rev-parse", "feature"], &src); + + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join("merge-success.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 main_before = rev(&["rev-parse", "refs/heads/main"], &bare); + + let mut repo = seed_repo(&owner_did, "merge-success"); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + let pr = crate::db::PullRequest { + id: uuid::Uuid::new_v4().to_string(), + repo_id: repo.id.clone(), + number: 1, + title: "merge feature".into(), + body: None, + author_did: owner_did.clone(), + source_branch: "feature".into(), + target_branch: "main".into(), + 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 pr row"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/merge", + axum::routing::post(crate::api::pulls::merge_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner_did}/merge-success/pulls/1/merge"); + let resp = router + .oneshot(signed_request_as( + &owner_did, + Method::POST, + &uri, + Body::empty(), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::OK, + "merge must succeed for diverged branches, got {}", + resp.status() + ); + + let body = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let json: serde_json::Value = serde_json::from_slice(&body).expect("json body"); + let merge_sha = json["merge_sha"] + .as_str() + .expect("merge_sha in response"); + assert_eq!(json["status"].as_str(), Some("merged")); + + let stored = state + .db + .get_pr(&repo.id, 1) + .await + .expect("get_pr") + .expect("pr row"); + assert_eq!(stored.status, "merged"); + assert!(stored.merged_by_did.is_some()); + + let main_after = rev(&["rev-parse", "refs/heads/main"], &bare); + assert_ne!( + main_after, main_before, + "refs/heads/main must advance after a successful merge" + ); + assert_eq!( + main_after, merge_sha, + "reported merge_sha must match refs/heads/main" + ); + run( + &[ + "merge-base", + "--is-ancestor", + &feature_tip, + &main_after, + ], + &bare, + ); + run( + &["cat-file", "-e", &format!("{main_after}:feature.txt")], + &bare, + ); + } + /// SINK GUARD: a poisoned row with option-shaped source_branch must not reach /// git merge argv (distinct from the diff revspec injection shape). #[sqlx::test] From 7001cd960a6c4aa5feb10a9479de4d7ff7b66012 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 30 Aug 2026 17:55:10 -0500 Subject: [PATCH 10/12] chore: cargo fmt on merge ref-advance test --- crates/gitlawb-node/src/test_support.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 021c573f..96121650 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -1926,9 +1926,7 @@ mod tests { .await .unwrap(); let json: serde_json::Value = serde_json::from_slice(&body).expect("json body"); - let merge_sha = json["merge_sha"] - .as_str() - .expect("merge_sha in response"); + let merge_sha = json["merge_sha"].as_str().expect("merge_sha in response"); assert_eq!(json["status"].as_str(), Some("merged")); let stored = state @@ -1950,12 +1948,7 @@ mod tests { "reported merge_sha must match refs/heads/main" ); run( - &[ - "merge-base", - "--is-ancestor", - &feature_tip, - &main_after, - ], + &["merge-base", "--is-ancestor", &feature_tip, &main_after], &bare, ); run( From c2025209f54c4df159cf6ef07e6cb1a00d8f9820 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:12:59 +0800 Subject: [PATCH 11/12] fix(node): bind merge worktree to the exact local target branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validate_git_ref proves branch-name grammar only; git's revision DWIM can resolve the same short name in another namespace (refs/tags/{name}), so a PR targeting a tag-shadowed name checked out a detached HEAD, merged onto it, discarded the commit at cleanup, and the unchecked plain rev-parse echoed refs/heads/{target} back as a bogus merge SHA — marking the PR merged and firing pull_request.merged with no branch advanced. Close the resolution class instead of denylisting names: (1) require the exact refs/heads/{target} via show-ref --verify before creating the worktree, (2) verify the worktree's symbolic HEAD is attached to exactly that ref before merging, (3) accept the merge only if rev-parse --verify {target}^{commit} exits zero. Add end-to-end regressions for a same-named tag with no local branch (merge fails, PR stays open, no merged event) and for a tag/branch coexistence (the local branch advances, the tag stays). --- crates/gitlawb-node/src/git/store.rs | 76 +++++- crates/gitlawb-node/src/test_support.rs | 334 ++++++++++++++++++++++++ 2 files changed, 399 insertions(+), 11 deletions(-) diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 3b8740d4..9a05573a 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -935,19 +935,49 @@ pub fn merge_branch( ) -> Result { guard_refs(target_branch, source_branch)?; let source_ref = local_branch_ref(source_branch); + let target_ref = local_branch_ref(target_branch); let worktree_path = repo_path.join("_merge_worktree"); - // Clean up any leftover worktree - if worktree_path.exists() { + let remove_worktree = || { let _ = Command::new("git") .args(["worktree", "remove", "--force", "_merge_worktree"]) .current_dir(repo_path) .output(); let _ = std::fs::remove_dir_all(&worktree_path); + }; + + // Clean up any leftover worktree + if worktree_path.exists() { + remove_worktree(); + } + + // The merge must land on the LOCAL target branch, so the worktree has to end + // up attached to exactly refs/heads/{target}. validate_git_ref proves + // branch-name grammar only: it does not prove refs/heads/{target} exists, + // and it cannot stop git's revision DWIM rules from resolving the same + // short name in another namespace (refs/tags/{name}, refs/remotes/...). If + // DWIM won, `worktree add` would check out a detached HEAD at the foreign + // ref, the merge would succeed on that disposable HEAD, and cleanup would + // discard the commit without ever advancing refs/heads/{target}. + // + // (1) Require the exact local ref before creating the worktree. show-ref + // --verify matches the full refname literally — no DWIM, no abbreviation — + // so a same-named tag with no local branch fails here instead of being + // silently resolved. + let target_exists = Command::new("git") + .args(["show-ref", "--verify", "--quiet", &target_ref]) + .current_dir(repo_path) + .output() + .context("failed to run git show-ref")?; + if !target_exists.status.success() { + bail!("target branch {target_ref} does not exist as a local branch"); } - // Check out the local target branch in the worktree. Passing refs/heads/{name} - // would detach HEAD, so a successful merge would not advance refs/heads/{target}. + // (2) Check out the local target branch in the worktree. The bare short + // name is what makes git attach HEAD to the branch (passing + // refs/heads/{name} would detach, so a successful merge would not advance + // refs/heads/{target}); step (1) guarantees the branch exists, so an + // attached checkout is the only acceptable outcome — verified right below. let wt = Command::new("git") .args(["worktree", "add", "_merge_worktree", target_branch]) .current_dir(repo_path) @@ -960,6 +990,22 @@ pub fn merge_branch( ); } + // The worktree's HEAD must be a symbolic ref to exactly refs/heads/{target}. + // A detached or differently-attached HEAD means git resolved the name as + // something other than the local target branch; a merge committed on it + // would be thrown away by cleanup, so refuse before merging rather than + // relying on any denylist of symbolic shapes. + let head_ref = Command::new("git") + .args(["symbolic-ref", "--quiet", "HEAD"]) + .current_dir(&worktree_path) + .output() + .context("failed to run git symbolic-ref")?; + let head_ref_name = String::from_utf8_lossy(&head_ref.stdout).trim().to_string(); + if !head_ref.status.success() || head_ref_name != target_ref { + remove_worktree(); + bail!("merge worktree is not attached to {target_ref} (HEAD is {head_ref_name:?})"); + } + // Run merge in worktree let merge = Command::new("git") .args([ @@ -983,11 +1029,7 @@ pub fn merge_branch( let success = merge.status.success(); // Always remove worktree - let _ = Command::new("git") - .args(["worktree", "remove", "--force", "_merge_worktree"]) - .current_dir(repo_path) - .output(); - let _ = std::fs::remove_dir_all(&worktree_path); + remove_worktree(); if !success { bail!( @@ -996,12 +1038,24 @@ pub fn merge_branch( ); } - // Get new HEAD of target branch + // (3) The merge only counts if the exact target ref now points at a commit. + // Plain `rev-parse` echoes an unresolvable token back on stdout with a + // nonzero exit status, so an unchecked call here would hand the literal + // string "refs/heads/{target}" upward as an apparently valid merge SHA and + // the caller would mark the PR merged and fire the webhook. --verify plus + // ^{commit} plus an explicit status check turn "target ref missing or not a + // commit" into a hard failure before any of that happens. let head = Command::new("git") - .args(["rev-parse", &format!("refs/heads/{target_branch}")]) + .args(["rev-parse", "--verify", &format!("{target_ref}^{{commit}}")]) .current_dir(repo_path) .output() .context("failed to get merge commit")?; + if !head.status.success() { + bail!( + "merge completed but {target_ref} does not resolve to a commit: {}", + String::from_utf8_lossy(&head.stderr) + ); + } Ok(String::from_utf8_lossy(&head.stdout).trim().to_string()) } diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 96121650..93d4744b 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -1957,6 +1957,340 @@ mod tests { ); } + /// SINK GUARD (jatmn #379): a PR targeting a name that exists only as a tag + /// (refs/tags/{name}, no refs/heads/{name}) must fail closed. Without the + /// exact-ref checks, git's revision DWIM resolves the bare name to the tag, + /// `worktree add` checks out a detached HEAD there, the merge succeeds on + /// that disposable HEAD, cleanup discards the commit, and the unchecked + /// final rev-parse echoes "refs/heads/{name}" back as a bogus merge SHA — + /// so the PR is marked merged and pull_request.merged fires even though no + /// branch ever moved. + #[sqlx::test] + async fn merge_pr_fails_closed_when_target_is_only_a_tag(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) + ); + out + }; + let rev = |args: &[&str], cwd: &std::path::Path| { + String::from_utf8_lossy(&run(args, cwd).stdout) + .trim() + .to_string() + }; + + let src = std::env::temp_dir().join(format!("gl-tag-only-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("base.txt"), b"base").unwrap(); + run(&["add", "base.txt"], &src); + run(&["commit", "-q", "-m", "seed"], &src); + // The tag that shadows the missing target branch: "release" resolves via + // DWIM but refs/heads/release never exists. + run(&["tag", "release"], &src); + run(&["checkout", "-q", "-b", "feature"], &src); + std::fs::write(src.join("feature.txt"), b"feature-only").unwrap(); + run(&["add", "feature.txt"], &src); + run(&["commit", "-q", "-m", "on feature"], &src); + + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join("tag-only.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 tag_before = rev(&["rev-parse", "refs/tags/release"], &bare); + // Precondition of the whole scenario: the tag exists, the branch does not. + let no_branch = Command::new("git") + .args(["show-ref", "--verify", "--quiet", "refs/heads/release"]) + .current_dir(&bare) + .output() + .expect("git show-ref"); + assert!( + !no_branch.status.success(), + "fixture must not have a local release branch" + ); + + let mut repo = seed_repo(&owner_did, "tag-only"); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + let pr = crate::db::PullRequest { + id: uuid::Uuid::new_v4().to_string(), + repo_id: repo.id.clone(), + number: 1, + title: "merge into tag-shadowed name".into(), + body: None, + author_did: owner_did.clone(), + source_branch: "feature".into(), + target_branch: "release".into(), + 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 pr row"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/merge", + axum::routing::post(crate::api::pulls::merge_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner_did}/tag-only/pulls/1/merge"); + let resp = router + .oneshot(signed_request_as( + &owner_did, + Method::POST, + &uri, + Body::empty(), + )) + .await + .unwrap(); + assert!( + !resp.status().is_success(), + "merge must fail when the target exists only as a tag, got {}", + resp.status() + ); + + // The handler updates the PR row and fires pull_request.merged only + // after merge_branch returns Ok, so an open row proves no merged event + // was emitted. + let stored = state + .db + .get_pr(&repo.id, 1) + .await + .expect("get_pr") + .expect("pr row"); + assert_eq!( + stored.status, "open", + "a failed merge must leave the PR open and emit no pull_request.merged" + ); + assert!(stored.merged_by_did.is_none()); + + // The repo itself must be untouched: still no local release branch, and + // the tag did not move. + let still_no_branch = Command::new("git") + .args(["show-ref", "--verify", "--quiet", "refs/heads/release"]) + .current_dir(&bare) + .output() + .expect("git show-ref"); + assert!( + !still_no_branch.status.success(), + "a failed merge must not create refs/heads/release" + ); + let tag_after = rev(&["rev-parse", "refs/tags/release"], &bare); + assert_eq!(tag_after, tag_before, "the tag must not move"); + } + + /// Companion to the tag-only case (jatmn #379): when a same-named tag AND a + /// local branch coexist, the merge must attach to and advance + /// refs/heads/{target} while refs/tags/{target} stays where it was — the + /// exact-ref binding must pick the branch, not merely fail less. + #[sqlx::test] + async fn merge_pr_advances_local_branch_shadowed_by_same_named_tag(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) + ); + out + }; + let rev = |args: &[&str], cwd: &std::path::Path| { + String::from_utf8_lossy(&run(args, cwd).stdout) + .trim() + .to_string() + }; + + let src = std::env::temp_dir().join(format!("gl-tag-shadow-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("base.txt"), b"base").unwrap(); + run(&["add", "base.txt"], &src); + run(&["commit", "-q", "-m", "seed"], &src); + // Tag "release" pinned at the seed commit; the branch of the same name + // moves past it. Revision DWIM prefers refs/tags/ over refs/heads/ for + // an ambiguous short name, so this fixture is the strongest shadowing + // shape: a naive resolution would merge onto the stale tag commit. + run(&["tag", "release"], &src); + run(&["checkout", "-q", "-b", "release"], &src); + std::fs::write(src.join("release.txt"), b"release-only").unwrap(); + run(&["add", "release.txt"], &src); + run(&["commit", "-q", "-m", "on release"], &src); + run(&["checkout", "-q", "-b", "feature"], &src); + std::fs::write(src.join("feature.txt"), b"feature-only").unwrap(); + run(&["add", "feature.txt"], &src); + run(&["commit", "-q", "-m", "on feature"], &src); + run(&["checkout", "-q", "release"], &src); + std::fs::write(src.join("release2.txt"), b"release-again").unwrap(); + run(&["add", "release2.txt"], &src); + run(&["commit", "-q", "-m", "diverge release"], &src); + let feature_tip = rev(&["rev-parse", "refs/heads/feature"], &src); + + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join("tag-shadow.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 tag_before = rev(&["rev-parse", "refs/tags/release"], &bare); + let branch_before = rev(&["rev-parse", "refs/heads/release"], &bare); + assert_ne!( + tag_before, branch_before, + "fixture must diverge the tag from the branch" + ); + + let mut repo = seed_repo(&owner_did, "tag-shadow"); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + let pr = crate::db::PullRequest { + id: uuid::Uuid::new_v4().to_string(), + repo_id: repo.id.clone(), + number: 1, + title: "merge feature into shadowed release".into(), + body: None, + author_did: owner_did.clone(), + source_branch: "feature".into(), + target_branch: "release".into(), + 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 pr row"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/merge", + axum::routing::post(crate::api::pulls::merge_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner_did}/tag-shadow/pulls/1/merge"); + let resp = router + .oneshot(signed_request_as( + &owner_did, + Method::POST, + &uri, + Body::empty(), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::OK, + "merge must succeed when the local branch exists alongside the tag, got {}", + resp.status() + ); + + let body = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let json: serde_json::Value = serde_json::from_slice(&body).expect("json body"); + let merge_sha = json["merge_sha"].as_str().expect("merge_sha in response"); + assert_eq!(json["status"].as_str(), Some("merged")); + + let stored = state + .db + .get_pr(&repo.id, 1) + .await + .expect("get_pr") + .expect("pr row"); + assert_eq!(stored.status, "merged"); + + // The LOCAL BRANCH advanced to the reported merge SHA; the tag stayed put. + let branch_after = rev(&["rev-parse", "refs/heads/release"], &bare); + assert_ne!( + branch_after, branch_before, + "refs/heads/release must advance past the merge" + ); + assert_eq!( + branch_after, merge_sha, + "reported merge_sha must match refs/heads/release" + ); + let tag_after = rev(&["rev-parse", "refs/tags/release"], &bare); + assert_eq!(tag_after, tag_before, "the tag must not move"); + run( + &["merge-base", "--is-ancestor", &feature_tip, &branch_after], + &bare, + ); + run( + &["cat-file", "-e", &format!("{branch_after}:feature.txt")], + &bare, + ); + } + /// SINK GUARD: a poisoned row with option-shaped source_branch must not reach /// git merge argv (distinct from the diff revspec injection shape). #[sqlx::test] From 1d08ae9cac03657fe4c29ace8c5a1f0b5f7839e6 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 6 Sep 2026 15:48:04 -0500 Subject: [PATCH 12/12] fix(node): recover a leftover merge worktree instead of wedging the endpoint merge_branch cleaned up a leftover _merge_worktree only when the directory still existed. The state git worktree add actually refuses is the opposite: a registration under $GIT_DIR/worktrees whose directory is gone. It fails with "missing but already registered" (exit 128), and once a repo is in that state every later merge on it fails, permanently. The state is self-inflicted. remove_worktree runs git worktree remove and then removes the directory unconditionally, so one failed removal leaves the registration behind. Two changes. Run the cleanup unconditionally at entry, which also makes the bail-without-cleanup paths self-healing on the next merge. And pass --force twice: a single --force refuses a LOCKED worktree ("cannot remove a locked working tree"), and git worktree prune skips locked registrations too, so with one flag a locked leftover survives the whole cleanup and git worktree add then fails with "missing but locked worktree". Enumerated the leftover states that can reach this code (registered or not, x directory present / gone / locked / corrupt admin dir, x gc.worktreePruneExpire default and never). remove -f -f plus the directory removal clears all of them, and adding a separate git worktree prune changes no outcome in any of them, so there is no prune call here. Four tests, one per leftover shape that reaches this code. Against merge_branch: the plain stale registration, the locked variant, and an unregistered leftover DIRECTORY (git worktree remove refuses that one with "is not a working tree", so the directory removal is what clears it). Plus an end-to-end pass through the merge endpoint on the locked state, asserting the PR row reaches "merged", the reported merge_sha IS the new refs/heads/{target}, and the merge leaves no worktree behind for the next one. The e2e fixture asserts git really refuses the state before asserting recovery, so a future git cannot make it vacuous. --- crates/gitlawb-node/src/git/store.rs | 297 +++++++++++++++++++++++- crates/gitlawb-node/src/test_support.rs | 201 ++++++++++++++++ 2 files changed, 492 insertions(+), 6 deletions(-) diff --git a/crates/gitlawb-node/src/git/store.rs b/crates/gitlawb-node/src/git/store.rs index 9a05573a..866fa315 100644 --- a/crates/gitlawb-node/src/git/store.rs +++ b/crates/gitlawb-node/src/git/store.rs @@ -939,17 +939,32 @@ pub fn merge_branch( let worktree_path = repo_path.join("_merge_worktree"); let remove_worktree = || { + // Two --force flags, not one. A single --force refuses a LOCKED worktree + // ("cannot remove a locked working tree") and `git worktree prune` skips + // locked registrations too, so with one flag a locked leftover survives + // the whole cleanup and wedges this endpoint for good. With both flags a + // separate prune adds nothing: over every leftover state that can reach + // here (registered/unregistered x directory present/gone/locked/corrupt) + // `remove -f -f` plus the removal below already clears the registration. let _ = Command::new("git") - .args(["worktree", "remove", "--force", "_merge_worktree"]) + .args([ + "worktree", + "remove", + "--force", + "--force", + "_merge_worktree", + ]) .current_dir(repo_path) .output(); let _ = std::fs::remove_dir_all(&worktree_path); }; - // Clean up any leftover worktree - if worktree_path.exists() { - remove_worktree(); - } + // Clean up any leftover worktree. This cannot be gated on the directory + // existing: a registration whose directory is gone is the one state + // `worktree add` refuses outright ("missing but already registered"), and + // it is reachable from the cleanup above, so gating on .exists() would let + // one failed removal wedge every later merge on this repo. + remove_worktree(); // The merge must land on the LOCAL target branch, so the worktree has to end // up attached to exactly refs/heads/{target}. validate_git_ref proves @@ -964,6 +979,13 @@ pub fn merge_branch( // --verify matches the full refname literally — no DWIM, no abbreviation — // so a same-named tag with no local branch fails here instead of being // silently resolved. + // + // allow-unbounded-git: read-only `show-ref --verify` on one literal refname, + // no network and no object walk. It also sits alongside the pre-existing bare + // `git diff`, `git merge` and `git rev-parse` spawns in this same function, + // all under the same acquire_write lock, so bounding only this one would not + // change the function's exposure. Bounding merge_branch as a whole is a + // separate change against already-reviewed code, tracked as follow-up. let target_exists = Command::new("git") .args(["show-ref", "--verify", "--quiet", &target_ref]) .current_dir(repo_path) @@ -978,6 +1000,11 @@ pub fn merge_branch( // refs/heads/{name} would detach, so a successful merge would not advance // refs/heads/{target}); step (1) guarantees the branch exists, so an // attached checkout is the only acceptable outcome — verified right below. + // + // allow-unbounded-git: local `worktree add`, no network. Same reasoning as the + // show-ref spawn above: the surrounding function's other git children are + // already bare under this lock, so this one is not the thing that would need + // bounding first. let wt = Command::new("git") .args(["worktree", "add", "_merge_worktree", target_branch]) .current_dir(repo_path) @@ -995,6 +1022,9 @@ pub fn merge_branch( // something other than the local target branch; a merge committed on it // would be thrown away by cleanup, so refuse before merging rather than // relying on any denylist of symbolic shapes. + // + // allow-unbounded-git: read-only `symbolic-ref` resolving one local HEAD, no + // network. Same reasoning as the two spawns above. let head_ref = Command::new("git") .args(["symbolic-ref", "--quiet", "HEAD"]) .current_dir(&worktree_path) @@ -1135,7 +1165,7 @@ mod tests { ); } - use super::branch_diff_names; + use super::{branch_diff_names, merge_branch}; use std::path::Path; use std::process::Command; @@ -2282,4 +2312,259 @@ mod tests { "a clean `missing` twice on a readable store is a genuine absence; got {res:?}" ); } + + /// CodeRabbit #379: `merge_branch`'s leftover-worktree cleanup ran only when the + /// `_merge_worktree` DIRECTORY still existed. A directory that is gone while + /// `$GIT_DIR/worktrees/_merge_worktree` still holds its registration is precisely + /// the state `git worktree add` refuses ("is a missing but already registered + /// worktree", exit 128), and it is self-inflicted: `remove_worktree` deletes the + /// directory even when the `git worktree remove` before it failed. Once a repo is + /// in that state every later merge on it fails permanently. REVERT PROOF (RED): + /// put the cleanup call back behind `if worktree_path.exists()` and this test + /// fails at the merge below with "missing but already registered". + #[test] + fn merge_branch_recovers_from_a_stale_worktree_registration() { + let td = tempfile::TempDir::new().unwrap(); + let work = td.path().join("work"); + std::fs::create_dir_all(&work).unwrap(); + let g = |args: &[&str], dir: &Path| { + assert!( + Command::new("git") + .args(args) + .current_dir(dir) + .status() + .unwrap() + .success(), + "git {args:?} failed" + ); + }; + g(&["init", "-q"], &work); + g(&["config", "user.email", "t@t"], &work); + g(&["config", "user.name", "t"], &work); + std::fs::write(work.join("base.txt"), b"base\n").unwrap(); + g(&["add", "."], &work); + g(&["commit", "-qm", "base"], &work); + let main = { + let o = Command::new("git") + .args(["symbolic-ref", "--short", "HEAD"]) + .current_dir(&work) + .output() + .unwrap(); + String::from_utf8_lossy(&o.stdout).trim().to_string() + }; + g(&["checkout", "-q", "-b", "feature"], &work); + std::fs::write(work.join("feat.txt"), b"feat\n").unwrap(); + g(&["add", "."], &work); + g(&["commit", "-qm", "feat"], &work); + g(&["checkout", "-q", &main], &work); + + let bare = td.path().join("bare.git"); + g( + &[ + "clone", + "--bare", + "-q", + work.to_str().unwrap(), + bare.to_str().unwrap(), + ], + td.path(), + ); + + // Reproduce the stuck state exactly: register the worktree, then delete its + // directory without unregistering it. This is what a failed `worktree remove` + // followed by the unconditional `remove_dir_all` leaves behind. + // + // `gc.worktreePruneExpire=never` is set on purpose. Recovery must not depend on + // git's prune expiry policy: a registration this fresh would sit inside any + // sane expiry window, so a cleanup that only pruned on expiry would leave the + // repo wedged. Both `worktree remove --force` and `worktree prune` unregister a + // worktree whose gitdir target is gone regardless of this setting, and pinning + // it here keeps that true if either call is ever changed. + g(&["config", "gc.worktreePruneExpire", "never"], &bare); + g(&["worktree", "add", "_merge_worktree", &main], &bare); + std::fs::remove_dir_all(bare.join("_merge_worktree")).unwrap(); + assert!( + bare.join("worktrees/_merge_worktree").exists(), + "fixture must leave the registration behind" + ); + + let before = Command::new("git") + .args(["rev-parse", &format!("refs/heads/{main}")]) + .current_dir(&bare) + .output() + .unwrap(); + let before = String::from_utf8_lossy(&before.stdout).trim().to_string(); + + let merged = merge_branch(&bare, &main, "feature", "did:key:zTest", "t") + .expect("a stale worktree registration must not wedge the merge endpoint"); + + let after = Command::new("git") + .args(["rev-parse", &format!("refs/heads/{main}")]) + .current_dir(&bare) + .output() + .unwrap(); + let after = String::from_utf8_lossy(&after.stdout).trim().to_string(); + assert_ne!(before, after, "the merge must advance refs/heads/{main}"); + assert_eq!(after, merged, "the returned sha must be the new branch tip"); + } + + /// Companion to the stale-registration case: the leftover worktree is also LOCKED. + /// `git worktree remove --force` (ONE flag) refuses a locked worktree outright, and + /// `git worktree prune` skips locked registrations too, so with a single --force the + /// cleanup makes no progress and `worktree add` keeps failing. REVERT PROOF (RED): + /// drop either --force from remove_worktree and this test fails. + #[test] + fn merge_branch_recovers_from_a_locked_stale_worktree() { + let td = tempfile::TempDir::new().unwrap(); + let work = td.path().join("work"); + std::fs::create_dir_all(&work).unwrap(); + let g = |args: &[&str], dir: &Path| { + assert!( + Command::new("git") + .args(args) + .current_dir(dir) + .status() + .unwrap() + .success(), + "git {args:?} failed" + ); + }; + g(&["init", "-q"], &work); + g(&["config", "user.email", "t@t"], &work); + g(&["config", "user.name", "t"], &work); + std::fs::write(work.join("base.txt"), b"base\n").unwrap(); + g(&["add", "."], &work); + g(&["commit", "-qm", "base"], &work); + let main = { + let o = Command::new("git") + .args(["symbolic-ref", "--short", "HEAD"]) + .current_dir(&work) + .output() + .unwrap(); + String::from_utf8_lossy(&o.stdout).trim().to_string() + }; + g(&["checkout", "-q", "-b", "feature"], &work); + std::fs::write(work.join("feat.txt"), b"feat\n").unwrap(); + g(&["add", "."], &work); + g(&["commit", "-qm", "feat"], &work); + g(&["checkout", "-q", &main], &work); + + let bare = td.path().join("bare.git"); + g( + &[ + "clone", + "--bare", + "-q", + work.to_str().unwrap(), + bare.to_str().unwrap(), + ], + td.path(), + ); + + g(&["worktree", "add", "_merge_worktree", &main], &bare); + g(&["worktree", "lock", "_merge_worktree"], &bare); + std::fs::remove_dir_all(bare.join("_merge_worktree")).unwrap(); + + let before = Command::new("git") + .args(["rev-parse", &format!("refs/heads/{main}")]) + .current_dir(&bare) + .output() + .unwrap(); + let before = String::from_utf8_lossy(&before.stdout).trim().to_string(); + + let merged = merge_branch(&bare, &main, "feature", "did:key:zTest", "t") + .expect("a locked leftover worktree must not wedge the merge endpoint"); + + let after = Command::new("git") + .args(["rev-parse", &format!("refs/heads/{main}")]) + .current_dir(&bare) + .output() + .unwrap(); + let after = String::from_utf8_lossy(&after.stdout).trim().to_string(); + assert_ne!(before, after, "the merge must advance refs/heads/{main}"); + assert_eq!(after, merged, "the returned sha must be the new branch tip"); + } + + /// The third leftover shape: a `_merge_worktree` DIRECTORY with no registration + /// behind it, which is what a crash between `worktree add` creating the directory + /// and git recording it leaves. `git worktree remove` refuses this one outright + /// ("is not a working tree"), so the `remove_dir_all` in remove_worktree is the + /// only thing that clears it and `worktree add` otherwise fails on the existing + /// path. REVERT PROOF (RED): drop the remove_dir_all from remove_worktree and this + /// test fails. + #[test] + fn merge_branch_recovers_from_an_unregistered_leftover_directory() { + let td = tempfile::TempDir::new().unwrap(); + let work = td.path().join("work"); + std::fs::create_dir_all(&work).unwrap(); + let g = |args: &[&str], dir: &Path| { + assert!( + Command::new("git") + .args(args) + .current_dir(dir) + .status() + .unwrap() + .success(), + "git {args:?} failed" + ); + }; + g(&["init", "-q"], &work); + g(&["config", "user.email", "t@t"], &work); + g(&["config", "user.name", "t"], &work); + std::fs::write(work.join("base.txt"), b"base\n").unwrap(); + g(&["add", "."], &work); + g(&["commit", "-qm", "base"], &work); + let main = { + let o = Command::new("git") + .args(["symbolic-ref", "--short", "HEAD"]) + .current_dir(&work) + .output() + .unwrap(); + String::from_utf8_lossy(&o.stdout).trim().to_string() + }; + g(&["checkout", "-q", "-b", "feature"], &work); + std::fs::write(work.join("feat.txt"), b"feat\n").unwrap(); + g(&["add", "."], &work); + g(&["commit", "-qm", "feat"], &work); + g(&["checkout", "-q", &main], &work); + + let bare = td.path().join("bare.git"); + g( + &[ + "clone", + "--bare", + "-q", + work.to_str().unwrap(), + bare.to_str().unwrap(), + ], + td.path(), + ); + + // A directory in the way, with nothing registered behind it. + std::fs::create_dir_all(bare.join("_merge_worktree")).unwrap(); + std::fs::write(bare.join("_merge_worktree/leftover.txt"), b"junk\n").unwrap(); + assert!( + !bare.join("worktrees/_merge_worktree").exists(), + "fixture must leave no registration, only the directory" + ); + + let before = Command::new("git") + .args(["rev-parse", &format!("refs/heads/{main}")]) + .current_dir(&bare) + .output() + .unwrap(); + let before = String::from_utf8_lossy(&before.stdout).trim().to_string(); + + let merged = merge_branch(&bare, &main, "feature", "did:key:zTest", "t") + .expect("an unregistered leftover directory must not wedge the merge endpoint"); + + let after = Command::new("git") + .args(["rev-parse", &format!("refs/heads/{main}")]) + .current_dir(&bare) + .output() + .unwrap(); + let after = String::from_utf8_lossy(&after.stdout).trim().to_string(); + assert_ne!(before, after, "the merge must advance refs/heads/{main}"); + assert_eq!(after, merged, "the returned sha must be the new branch tip"); + } } diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 93d4744b..6bff5d16 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -2291,6 +2291,207 @@ mod tests { ); } + /// E2E for the leftover-worktree cleanup (CodeRabbit #379), driven through the + /// real merge endpoint rather than merge_branch directly, because the defect's + /// consequence is an endpoint that wedges: every later merge on the repo fails. + /// + /// The fixture leaves the repo in the worst reachable leftover state -- the + /// `_merge_worktree` registration present, its directory gone, AND the worktree + /// LOCKED. `git worktree add` refuses that with "missing but locked worktree", + /// a single `--force` refuses to remove it, and `git worktree prune` skips it. + /// + /// Both response classes are asserted, not just the 200: the PR row must end + /// "merged" with a merge_sha that IS the new refs/heads/{target}, and the + /// endpoint must not report success without advancing the branch. REVERT PROOF + /// (RED): drop either --force from remove_worktree in store.rs and this test + /// fails with the endpoint returning non-200 and the PR row still open. + #[sqlx::test] + async fn merge_pr_recovers_from_a_locked_stale_worktree(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) + ); + out + }; + let rev = |args: &[&str], cwd: &std::path::Path| { + String::from_utf8_lossy(&run(args, cwd).stdout) + .trim() + .to_string() + }; + + let src = std::env::temp_dir().join(format!("gl-locked-wt-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("base.txt"), b"base").unwrap(); + run(&["add", "base.txt"], &src); + run(&["commit", "-q", "-m", "seed"], &src); + run(&["checkout", "-q", "-b", "feature"], &src); + std::fs::write(src.join("feature.txt"), b"feature-only").unwrap(); + run(&["add", "feature.txt"], &src); + run(&["commit", "-q", "-m", "on feature"], &src); + run(&["checkout", "-q", "main"], &src); + let feature_tip = rev(&["rev-parse", "refs/heads/feature"], &src); + + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join("locked-wt.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 main_before = rev(&["rev-parse", "refs/heads/main"], &bare); + + // Wedge the repo exactly the way a failed cleanup does: register the + // worktree, lock it, then delete its directory. remove_worktree's + // unconditional remove_dir_all is what produces the directory half of + // this in production; the lock is what makes a single --force useless. + run(&["worktree", "add", "_merge_worktree", "main"], &bare); + run(&["worktree", "lock", "_merge_worktree"], &bare); + std::fs::remove_dir_all(bare.join("_merge_worktree")).unwrap(); + assert!( + bare.join("worktrees/_merge_worktree").exists(), + "fixture must leave a locked registration behind" + ); + // Prove the fixture really is the refusing state before asserting recovery, + // so a future git that stopped refusing cannot make this test vacuous. + let blocked = Command::new("git") + .args(["worktree", "add", "_merge_worktree", "main"]) + .current_dir(&bare) + .output() + .expect("git"); + assert!( + !blocked.status.success() + && String::from_utf8_lossy(&blocked.stderr).contains("missing but locked"), + "fixture must be the state git refuses, got: {}", + String::from_utf8_lossy(&blocked.stderr) + ); + + let mut repo = seed_repo(&owner_did, "locked-wt"); + repo.is_public = true; + state.db.create_repo(&repo).await.expect("seed repo"); + + let pr = crate::db::PullRequest { + id: uuid::Uuid::new_v4().to_string(), + repo_id: repo.id.clone(), + number: 1, + title: "merge over a locked leftover worktree".into(), + body: None, + author_did: owner_did.clone(), + source_branch: "feature".into(), + target_branch: "main".into(), + 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 pr row"); + + let router = Router::new() + .route( + "/api/v1/repos/{owner}/{repo}/pulls/{number}/merge", + axum::routing::post(crate::api::pulls::merge_pr), + ) + .with_state(state.clone()); + let uri = format!("/api/v1/repos/{owner_did}/locked-wt/pulls/1/merge"); + let resp = router + .oneshot(signed_request_as( + &owner_did, + Method::POST, + &uri, + Body::empty(), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::OK, + "a locked leftover worktree must not wedge the merge endpoint, got {}", + resp.status() + ); + + let body = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let json: serde_json::Value = serde_json::from_slice(&body).expect("json body"); + let merge_sha = json["merge_sha"].as_str().expect("merge_sha in response"); + assert_eq!(json["status"].as_str(), Some("merged")); + + let stored = state + .db + .get_pr(&repo.id, 1) + .await + .expect("get_pr") + .expect("pr row"); + assert_eq!(stored.status, "merged"); + assert_eq!(stored.merged_by_did.as_deref(), Some(owner_did.as_str())); + + // A reported merge is only real if the branch moved to exactly that sha + // and carries the source commit. This is the half that catches a wedge + // rendered as success rather than as an error. + let main_after = rev(&["rev-parse", "refs/heads/main"], &bare); + assert_ne!( + main_after, main_before, + "refs/heads/main must advance past the merge" + ); + assert_eq!( + main_after, merge_sha, + "reported merge_sha must match refs/heads/main" + ); + run( + &["merge-base", "--is-ancestor", &feature_tip, &main_after], + &bare, + ); + run( + &["cat-file", "-e", &format!("{main_after}:feature.txt")], + &bare, + ); + + // The cleanup must also leave nothing behind for the NEXT merge. + assert!( + !bare.join("worktrees/_merge_worktree").exists(), + "the merge must not leave its own worktree registered" + ); + assert!( + !bare.join("_merge_worktree").exists(), + "the merge must not leave its own worktree directory" + ); + } + /// SINK GUARD: a poisoned row with option-shaped source_branch must not reach /// git merge argv (distinct from the diff revspec injection shape). #[sqlx::test]