diff --git a/crates/gitlawb-node/src/api/issues.rs b/crates/gitlawb-node/src/api/issues.rs index 0eacfa72..879a9ba8 100644 --- a/crates/gitlawb-node/src/api/issues.rs +++ b/crates/gitlawb-node/src/api/issues.rs @@ -449,3 +449,198 @@ mod lock_pool_shed_tests { ); } } + +/// #426: end-to-end at the handler seam. A corrupt object store behind a +/// resolving issue ref used to look identical to an absent issue: get returned +/// a fake 404, close returned a fake 404 after releasing the write guard, and +/// list silently under-reported. All three must now surface a git error. +#[cfg(test)] +mod corrupt_store_tests { + use super::*; + use axum::response::IntoResponse; + use sqlx::PgPool; + use tempfile::TempDir; + + fn seed_repo(owner_did: &str, name: &str) -> crate::db::RepoRecord { + let now = Utc::now(); + crate::db::RepoRecord { + id: Uuid::new_v4().to_string(), + name: name.to_string(), + owner_did: owner_did.to_string(), + description: None, + is_public: true, + default_branch: "main".to_string(), + created_at: now, + updated_at: now, + disk_path: format!("/tmp/{name}"), + forked_from: None, + machine_id: None, + } + } + + /// A state whose repo store roots in `repos_dir`, so the test can lay down + /// a real repo and then corrupt it without touching /tmp. + async fn state_with_repos_dir(pool: &PgPool, repos_dir: &std::path::Path) -> AppState { + let mut state = crate::test_support::test_state(pool.clone()).await; + state.repo_store = + crate::git::repo_store::RepoStore::for_testing(repos_dir.to_path_buf(), pool.clone()); + state + } + + /// Bare repo at the store's expected path, holding one issue ref. + fn bare_repo_with_issue( + repos_dir: &std::path::Path, + owner: &str, + name: &str, + ) -> (std::path::PathBuf, String) { + let repo_path = repos_dir + .join(owner.replace([':', '/'], "_")) + .join(format!("{name}.git")); + std::fs::create_dir_all(&repo_path).unwrap(); + let out = std::process::Command::new("git") + .args(["init", "--bare"]) + .arg(&repo_path) + .output() + .unwrap(); + assert!(out.status.success()); + let issue_id = "aaaabbbb-0000-0000-0000-000000000000".to_string(); + git_issues::create_issue( + &repo_path, + &issue_id, + &serde_json::json!({ + "id": issue_id, + "title": "t", + "status": "open", + }) + .to_string(), + ) + .unwrap(); + (repo_path, issue_id) + } + + /// Delete the blob object behind the issue ref and put garbage in its + /// place: `cat-file` fails while `rev-parse` still resolves the ref. + fn corrupt_issue_object(repo_path: &std::path::Path, issue_id: &str) { + let out = std::process::Command::new("git") + .args(["rev-parse", &format!("refs/gitlawb/issues/{issue_id}")]) + .current_dir(repo_path) + .output() + .unwrap(); + let oid = String::from_utf8(out.stdout).unwrap().trim().to_string(); + let obj = repo_path.join("objects").join(&oid[..2]).join(&oid[2..]); + std::fs::remove_file(&obj).unwrap(); + std::fs::write(&obj, b"garbage").unwrap(); + } + + /// The regression: closing over a corrupt store used to return a fake 404 + /// (or panic deeper in). It must reach the caller as a git 500, and the + /// handler must return rather than unwind. + #[sqlx::test] + async fn close_issue_on_corrupt_store_returns_git_error_not_panic(pool: PgPool) { + let owner = "did:key:zISSUECORRUPTCCCCCCCCCCCCCCCCCCCCCCCCC"; + let dir = TempDir::new().unwrap(); + let state = state_with_repos_dir(&pool, dir.path()).await; + state + .db + .create_repo(&seed_repo(owner, "corrupt-close")) + .await + .expect("seed repo"); + let (repo_path, issue_id) = bare_repo_with_issue(dir.path(), owner, "corrupt-close"); + corrupt_issue_object(&repo_path, &issue_id); + + let err = close_issue( + State(state), + Extension(AuthenticatedDid(owner.to_string())), + Path((owner.to_string(), "corrupt-close".to_string(), issue_id)), + ) + .await + .expect_err("a corrupt store must error, not panic or 404"); + assert_eq!( + err.into_response().status(), + StatusCode::INTERNAL_SERVER_ERROR, + "a read failure is a git 500, not a fake not-found" + ); + } + + /// get on a corrupt store: 500, not the 404 the fold used to produce. + #[sqlx::test] + async fn get_issue_on_corrupt_store_returns_500_not_404(pool: PgPool) { + let owner = "did:key:zISSUECORRUPTGETDDDDDDDDDDDDDDDDDDDDDDD"; + let dir = TempDir::new().unwrap(); + let state = state_with_repos_dir(&pool, dir.path()).await; + state + .db + .create_repo(&seed_repo(owner, "corrupt-get")) + .await + .expect("seed repo"); + let (repo_path, issue_id) = bare_repo_with_issue(dir.path(), owner, "corrupt-get"); + corrupt_issue_object(&repo_path, &issue_id); + + let err = get_issue( + State(state), + Path((owner.to_string(), "corrupt-get".to_string(), issue_id)), + None, + ) + .await + .expect_err("a corrupt store must error, not 404"); + assert_eq!( + err.into_response().status(), + StatusCode::INTERNAL_SERVER_ERROR, + ); + } + + /// list on a corrupt store must fail outright; a 200 that silently drops + /// the unreadable issue is the other half of the fold. + #[sqlx::test] + async fn list_issues_on_corrupt_store_returns_git_error(pool: PgPool) { + let owner = "did:key:zISSUECORRUPTLISTEEEEEEEEEEEEEEEEEEEEEE"; + let dir = TempDir::new().unwrap(); + let state = state_with_repos_dir(&pool, dir.path()).await; + state + .db + .create_repo(&seed_repo(owner, "corrupt-list")) + .await + .expect("seed repo"); + let (repo_path, issue_id) = bare_repo_with_issue(dir.path(), owner, "corrupt-list"); + corrupt_issue_object(&repo_path, &issue_id); + + let err = list_issues( + State(state), + Path((owner.to_string(), "corrupt-list".to_string())), + None, + ) + .await + .expect_err("a corrupt store must error, not under-report"); + assert_eq!( + err.into_response().status(), + StatusCode::INTERNAL_SERVER_ERROR, + ); + } + + /// Must-not direction: a genuinely absent issue still gets the 404. + #[sqlx::test] + async fn get_issue_on_absent_ref_still_returns_404(pool: PgPool) { + let owner = "did:key:zISSUEABSENTFFFFFFFFFFFFFFFFFFFFFFFFFFF"; + let dir = TempDir::new().unwrap(); + let state = state_with_repos_dir(&pool, dir.path()).await; + state + .db + .create_repo(&seed_repo(owner, "absent-get")) + .await + .expect("seed repo"); + bare_repo_with_issue(dir.path(), owner, "absent-get"); + + let err = get_issue( + State(state), + Path(( + owner.to_string(), + "absent-get".to_string(), + "ffffffff-0000-0000-0000-000000000000".to_string(), + )), + None, + ) + .await + .expect_err("a missing issue is still not-found"); + assert_eq!(err.into_response().status(), StatusCode::NOT_FOUND); + } +} diff --git a/crates/gitlawb-node/src/git/issues.rs b/crates/gitlawb-node/src/git/issues.rs index 73098302..b204e572 100644 --- a/crates/gitlawb-node/src/git/issues.rs +++ b/crates/gitlawb-node/src/git/issues.rs @@ -61,6 +61,119 @@ pub fn create_issue(repo_path: &Path, issue_id: &str, json: &str) -> Result<()> Ok(()) } +/// Issue ids are UUIDs; refs under `refs/gitlawb/issues/` only ever carry +/// `[0-9a-zA-Z_-]` names. Anything else is not a value we pass to git. +fn issue_ref_name_is_safe(ref_name: &str) -> bool { + let Some(id) = ref_name.strip_prefix("refs/gitlawb/issues/") else { + return false; + }; + !id.is_empty() + && id + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') +} + +/// Resolve a ref name without putting it on a command line: enumerate refs +/// (constant argv) and match in memory. A nonzero exit propagates, so "cannot +/// determine" is an error rather than a quiet `false`. +fn ref_name_resolves(repo_path: &Path, ref_name: &str) -> Result { + // allow-unbounded-git: failure-path existence recheck, module convention. + let out = Command::new("git") + .args(["for-each-ref", "--format=%(refname)"]) + .current_dir(repo_path) + .output() + .context("failed to run git for-each-ref")?; + if !out.status.success() { + let stderr = String::from_utf8_lossy(&out.stderr); + anyhow::bail!("git for-each-ref failed: {}", stderr.trim()); + } + let stdout = String::from_utf8_lossy(&out.stdout); + Ok(stdout.lines().any(|line| line.trim() == ref_name)) +} + +/// Read the blob behind an issue ref. +/// +/// `Ok(None)` means the ref itself is absent. A ref that resolves but whose +/// object cannot be read (corrupt object, unreadable store, a mid-read gc) is +/// an error, not an absence (#426). +/// +/// The ref name reaches git on stdin, never argv: `cat-file --batch` answers +/// ` \n\n` for a readable object and +/// ` missing\n` for an absent ref or an unreadable object alike, so the +/// `missing` case is re-resolved by name enumeration to tell them apart. +fn read_issue_blob(repo_path: &Path, ref_name: &str) -> Result> { + if !issue_ref_name_is_safe(ref_name) { + anyhow::bail!("refusing issue ref outside the issues namespace: {ref_name}"); + } + + // allow-unbounded-git: this module's spawns predate the bounded runner and + // its callers run inside issue handlers that already hold the repo guard. + let mut child = Command::new("git") + .args(["cat-file", "--batch"]) + .current_dir(repo_path) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .context("failed to spawn git cat-file --batch")?; + + use std::io::Write; + // Reap the child even when the write fails so a stdin error cannot drop an + // unwaited Child and leak a zombie (#53). + let write_result = match child.stdin.take() { + Some(mut stdin) => stdin + .write_all(ref_name.as_bytes()) + .and_then(|()| stdin.write_all(b"\n")), + None => Err(std::io::Error::new( + std::io::ErrorKind::BrokenPipe, + "git cat-file --batch stdin unavailable", + )), + }; + let output = child + .wait_with_output() + .context("git cat-file --batch failed")?; + write_result.context("failed to write ref name to git cat-file stdin")?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!("git cat-file --batch failed: {}", stderr.trim()); + } + + // " missing\n" covers an absent ref and an unreadable object; only a + // ref that still resolves is the error case. + let missing = format!("{ref_name} missing\n"); + if output.stdout.as_slice() == missing.as_bytes() { + return match ref_name_resolves(repo_path, ref_name)? { + true => anyhow::bail!("issue ref resolves but its object is unreadable: {ref_name}"), + false => Ok(None), + }; + } + + // Header: " \n" then exactly bytes plus a newline. + let header_end = output + .stdout + .iter() + .position(|b| *b == b'\n') + .context("unexpected git cat-file --batch output")?; + let header = String::from_utf8_lossy(&output.stdout[..header_end]); + let mut parts = header.split(' '); + parts.next(); + let obj_type = parts.next().unwrap_or(""); + if obj_type != "blob" { + anyhow::bail!("issue ref {ref_name} resolves to a non-blob object"); + } + let size: usize = parts + .next() + .and_then(|s| s.parse().ok()) + .context("unexpected git cat-file --batch header")?; + let body = &output.stdout[header_end + 1..]; + anyhow::ensure!( + body.len() >= size, + "truncated git cat-file --batch body for {ref_name}" + ); + Ok(Some(String::from_utf8_lossy(&body[..size]).to_string())) +} + /// List all issue refs and return their JSON content. pub fn list_issues(repo_path: &Path) -> Result> { // List all refs under refs/gitlawb/issues/ @@ -74,9 +187,11 @@ pub fn list_issues(repo_path: &Path) -> Result> { .output() .context("failed to run git for-each-ref")?; + // for-each-ref exits 0 with empty output when no refs match, so a nonzero + // exit is a real enumeration failure, not "no issues yet". if !list_output.status.success() { - // No issues yet - return Ok(vec![]); + let stderr = String::from_utf8_lossy(&list_output.stderr); + anyhow::bail!("git for-each-ref failed: {}", stderr.trim()); } let refs_str = String::from_utf8_lossy(&list_output.stdout); @@ -88,16 +203,12 @@ pub fn list_issues(repo_path: &Path) -> Result> { continue; } - // Read the blob content - let cat_output = Command::new("git") - .args(["cat-file", "blob", ref_name]) - .current_dir(repo_path) - .output() - .context("failed to run git cat-file")?; - - if cat_output.status.success() { - let content = String::from_utf8_lossy(&cat_output.stdout).to_string(); - issues.push(content); + // Read the blob content. A ref that vanished between for-each-ref and + // here is skipped; a ref that resolves but fails to read propagates so + // a degraded object store errors the listing instead of under-reporting. + match read_issue_blob(repo_path, ref_name)? { + Some(content) => issues.push(content), + None => continue, } } @@ -127,8 +238,12 @@ pub fn resolve_issue_id(repo_path: &Path, id_or_prefix: &str) -> Result Result> { None => return Ok(None), }; - let raw = get_issue(repo_path, &full_id)? - .expect("ref existed in resolve but not in get — should be impossible"); + // A ref that resolved and is now absent was deleted between the two calls; + // any other read failure propagates as an error rather than panic (#426). + let raw = match get_issue(repo_path, &full_id)? { + Some(raw) => raw, + None => return Ok(None), + }; let mut issue: serde_json::Value = serde_json::from_str(&raw).context("invalid issue JSON in git ref")?; @@ -181,18 +300,7 @@ pub fn get_issue(repo_path: &Path, issue_id: &str) -> Result> { }; let ref_name = format!("refs/gitlawb/issues/{full_id}"); - let cat_output = Command::new("git") - .args(["cat-file", "blob", &ref_name]) - .current_dir(repo_path) - .output() - .context("failed to run git cat-file")?; - - if !cat_output.status.success() { - return Ok(None); - } - - let content = String::from_utf8_lossy(&cat_output.stdout).to_string(); - Ok(Some(content)) + read_issue_blob(repo_path, &ref_name) } #[cfg(test)] @@ -294,4 +402,109 @@ mod tests { let v: serde_json::Value = serde_json::from_str(&updated).unwrap(); assert_eq!(v["status"], "closed"); } + + /// Overwrite the loose object behind an issue ref with garbage, so + /// `cat-file` fails while `rev-parse` still resolves the ref (#426). + fn corrupt_issue_object(dir: &TempDir, full_id: &str) { + let oid_out = Command::new("git") + .args(["rev-parse", &format!("refs/gitlawb/issues/{full_id}")]) + .current_dir(dir.path()) + .output() + .unwrap(); + let oid = String::from_utf8(oid_out.stdout) + .unwrap() + .trim() + .to_string(); + let obj = dir + .path() + .join(".git/objects") + .join(&oid[..2]) + .join(&oid[2..]); + std::fs::remove_file(&obj).unwrap(); + std::fs::write(&obj, b"garbage").unwrap(); + } + + #[test] + fn test_get_and_close_missing_issue_return_none() { + let dir = TempDir::new().unwrap(); + init_repo(&dir); + assert_eq!(get_issue(dir.path(), "nosuch00").unwrap(), None); + assert_eq!(close_issue(dir.path(), "nosuch00").unwrap(), None); + } + + // #426: a directory that is not a repo makes `cat-file --batch` exit + // nonzero; that failure is an error, not a quiet None. + #[test] + fn test_issue_read_on_non_repo_errors_instead_of_none() { + let dir = TempDir::new().unwrap(); + let not_a_repo = dir.path().join("not-a-repo"); + std::fs::create_dir_all(¬_a_repo).unwrap(); + assert!(read_issue_blob(¬_a_repo, "refs/gitlawb/issues/deadbeef").is_err()); + } + + // #426: the ref name is constrained to the issues namespace and a safe + // charset before it is fed to git, even on stdin (a newline would smuggle + // a second `--batch` spec). + #[test] + fn test_issue_read_rejects_ref_outside_issues_namespace() { + let dir = TempDir::new().unwrap(); + init_repo(&dir); + assert!(read_issue_blob(dir.path(), "refs/heads/main").is_err()); + assert!(read_issue_blob(dir.path(), "refs/gitlawb/issues/../x").is_err()); + } + + // #426: a failed for-each-ref enumeration is an error, not an empty list. + #[test] + fn test_list_issues_on_non_repo_errors_instead_of_empty() { + let dir = TempDir::new().unwrap(); + let not_a_repo = dir.path().join("not-a-repo"); + std::fs::create_dir_all(¬_a_repo).unwrap(); + assert!(list_issues(¬_a_repo).is_err()); + } + + // #426: the same enumeration failure inside resolve_issue_id must reach + // get_issue/close_issue as an error, not a "not found" None. + #[test] + fn test_get_and_close_on_non_repo_error_instead_of_none() { + let dir = TempDir::new().unwrap(); + let not_a_repo = dir.path().join("not-a-repo"); + std::fs::create_dir_all(¬_a_repo).unwrap(); + assert!(get_issue(¬_a_repo, "nosuch00").is_err()); + assert!(close_issue(¬_a_repo, "nosuch00").is_err()); + } + + // #426: a ref that resolves but whose object is corrupt is a read error, + // not an absent issue. `close_issue` used to `expect()` on that None and + // panic inside a request handler holding the write guard. + #[test] + fn test_corrupt_issue_object_errors_instead_of_panicking() { + let dir = TempDir::new().unwrap(); + init_repo(&dir); + let full_id = "bad00000-0000-0000-0000-000000000000"; + create_issue(dir.path(), full_id, r#"{"status":"open"}"#).unwrap(); + corrupt_issue_object(&dir, full_id); + + assert!(get_issue(dir.path(), full_id).is_err()); + assert!(close_issue(dir.path(), full_id).is_err()); + assert!(list_issues(dir.path()).is_err()); + } + + #[test] + fn test_list_issues_returns_all() { + let dir = TempDir::new().unwrap(); + init_repo(&dir); + create_issue( + dir.path(), + "aaa00000-0000-0000-0000-000000000000", + r#"{"status":"open"}"#, + ) + .unwrap(); + create_issue( + dir.path(), + "bbb00000-0000-0000-0000-000000000000", + r#"{"status":"open"}"#, + ) + .unwrap(); + assert_eq!(list_issues(dir.path()).unwrap().len(), 2); + } }