Compare commits

..
Author SHA1 Message Date
beardthelion 923d47d491 chore: refresh PR head 2026-11-05 10:53:18 -06:00
beardthelion f9942ee710 fix(node): validate ref-update SHAs are hex, not just length 40
parse_ref_updates accepted any 40-byte string as an object id, so a
peer-supplied pkt-line with non-hex content was forwarded into
RefUpdateEvents on gossip, branch_cid rows, and sync-notify JSON as a
canonical-looking identifier. Require [0-9a-fA-F] on both fields.

Closes #398
2026-09-10 17:17:49 -05:00
3 changed files with 73 additions and 436 deletions

No files matched your search

-195
View File
@@ -449,198 +449,3 @@ 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);
}
}
+46 -1
View File
@@ -3153,6 +3153,12 @@ struct RefUpdate {
ref_name: String,
}
/// A git object id is 40 hex chars; length alone is not enough, since a peer
/// can send 40 arbitrary bytes (#398).
fn is_hex_sha(s: &str) -> bool {
s.len() == 40 && s.bytes().all(|b| b.is_ascii_hexdigit())
}
/// Parse git receive-pack pkt-line ref updates from the request body.
/// Format per line: `<40-hex-old> <40-hex-new> <refname>[NUL capabilities]\n`
fn parse_ref_updates(body: &[u8]) -> Vec<RefUpdate> {
@@ -3194,7 +3200,7 @@ fn parse_ref_updates(body: &[u8]) -> Vec<RefUpdate> {
.trim_end_matches('\n');
let parts: Vec<&str> = line.splitn(3, ' ').collect();
if parts.len() == 3 && parts[0].len() == 40 && parts[1].len() == 40 {
if parts.len() == 3 && is_hex_sha(parts[0]) && is_hex_sha(parts[1]) {
updates.push(RefUpdate {
old_sha: parts[0].to_string(),
new_sha: parts[1].to_string(),
@@ -3488,6 +3494,45 @@ mod tests {
);
}
// #398: a peer-supplied pkt-line whose 40-byte "SHA" fields carry non-hex
// content must be dropped, not forwarded into RefUpdate events, branch_cid
// rows, and sync-notify JSON as a canonical-looking identifier.
#[test]
fn parse_ref_updates_requires_hex_shas() {
let sha_a = "a".repeat(40);
let sha_b = "b".repeat(40);
let pkt = |old: &str, new: &str, refname: &str| {
let line = format!("{old} {new} {refname}");
format!("{:04x}{}0000", line.len() + 4, line).into_bytes()
};
// Valid 40-hex old + new -> accepted.
let updates = parse_ref_updates(&pkt(&sha_a, &sha_b, "refs/heads/main"));
assert_eq!(updates.len(), 1);
assert_eq!(updates[0].old_sha, sha_a);
assert_eq!(updates[0].new_sha, sha_b);
assert_eq!(updates[0].ref_name, "refs/heads/main");
// 40-byte non-hex fields -> dropped.
let junk = "Z".repeat(40);
assert!(parse_ref_updates(&pkt(&junk, &sha_b, "refs/heads/main")).is_empty());
assert!(parse_ref_updates(&pkt(&sha_a, &junk, "refs/heads/main")).is_empty());
// Wrong lengths -> dropped.
assert!(parse_ref_updates(&pkt(&"a".repeat(39), &sha_b, "refs/heads/main")).is_empty());
assert!(parse_ref_updates(&pkt(&sha_a, &"b".repeat(41), "refs/heads/main")).is_empty());
// A bad line among good ones drops only the bad line.
let sha_c = "c".repeat(40);
let bad = pkt(&junk, &sha_b, "refs/heads/bad");
let good = pkt(&sha_a, &sha_c, "refs/heads/good");
let mut mixed = bad[..bad.len() - 4].to_vec(); // strip trailing flush
mixed.extend_from_slice(&good);
let updates = parse_ref_updates(&mixed);
assert_eq!(updates.len(), 1);
assert_eq!(updates[0].ref_name, "refs/heads/good");
}
#[test]
fn git_service_app_error_classifies_timeout_bad_request_and_git() {
// GitServiceTimeout carried through anyhow -> 504 Timeout.
+27 -240
View File
@@ -61,119 +61,6 @@ 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<bool> {
// 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
/// `<oid> <type> <size>\n<body>\n` for a readable object and
/// `<spec> 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<Option<String>> {
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());
}
// "<spec> 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: "<oid> <type> <size>\n" then exactly <size> 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<Vec<String>> {
// List all refs under refs/gitlawb/issues/
@@ -187,11 +74,9 @@ pub fn list_issues(repo_path: &Path) -> Result<Vec<String>> {
.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() {
let stderr = String::from_utf8_lossy(&list_output.stderr);
anyhow::bail!("git for-each-ref failed: {}", stderr.trim());
// No issues yet
return Ok(vec![]);
}
let refs_str = String::from_utf8_lossy(&list_output.stdout);
@@ -203,12 +88,16 @@ pub fn list_issues(repo_path: &Path) -> Result<Vec<String>> {
continue;
}
// 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,
// 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);
}
}
@@ -238,12 +127,8 @@ pub fn resolve_issue_id(repo_path: &Path, id_or_prefix: &str) -> Result<Option<S
.output()
.context("failed to run git for-each-ref")?;
// for-each-ref exits 0 with empty output when nothing matches, so a
// nonzero exit is a real enumeration failure (e.g. not a repo), not
// "issue not found".
if !list.status.success() {
let stderr = String::from_utf8_lossy(&list.stderr);
anyhow::bail!("git for-each-ref failed: {}", stderr.trim());
return Ok(None);
}
let output = String::from_utf8_lossy(&list.stdout);
@@ -276,12 +161,8 @@ pub fn close_issue(repo_path: &Path, issue_id: &str) -> Result<Option<String>> {
None => return Ok(None),
};
// 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 raw = get_issue(repo_path, &full_id)?
.expect("ref existed in resolve but not in get — should be impossible");
let mut issue: serde_json::Value =
serde_json::from_str(&raw).context("invalid issue JSON in git ref")?;
@@ -300,7 +181,18 @@ pub fn get_issue(repo_path: &Path, issue_id: &str) -> Result<Option<String>> {
};
let ref_name = format!("refs/gitlawb/issues/{full_id}");
read_issue_blob(repo_path, &ref_name)
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))
}
#[cfg(test)]
@@ -402,109 +294,4 @@ 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(&not_a_repo).unwrap();
assert!(read_issue_blob(&not_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(&not_a_repo).unwrap();
assert!(list_issues(&not_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(&not_a_repo).unwrap();
assert!(get_issue(&not_a_repo, "nosuch00").is_err());
assert!(close_issue(&not_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);
}
}