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/11] 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 df10175a9..aae2bbd2b 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 6255ef246..b6fa8381e 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 4e327c429..f5e747af4 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 5617b4198..1635fb60c 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 430c06002..4fcbf375d 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/11] 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 1635fb60c..7acafc6ab 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 4fcbf375d..41a6b6b94 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/11] 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 7acafc6ab..7aac03e6f 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 41a6b6b94..fe7bdf0ca 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/11] 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 7aac03e6f..35feee535 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/11] 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 fe7bdf0ca..244ce171d 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/11] 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 aae2bbd2b..40ed8c815 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 b6fa8381e..c665447a4 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 f5e747af4..a4edffc8c 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 35feee535..24689972f 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 244ce171d..cd7cfac2e 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/11] 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 24689972f..ad55a3903 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 cd7cfac2e..6597a3898 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/11] 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 40ed8c815..cecdcfb22 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 c665447a4..d1e0185ad 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 a4edffc8c..1fb21c51a 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 ad55a3903..5279decfc 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 6597a3898..a31ec5549 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/11] 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 5279decfc..3b8740d4d 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 a31ec5549..021c573fd 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/11] 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 021c573fd..961216504 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/11] 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 3b8740d4d..9a05573af 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 961216504..93d4744b7 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]