From fa36794d7ebbaba006a94da914efff26ca87a6e4 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Fri, 25 Sep 2026 13:32:38 +0000 Subject: [PATCH] fix(git): verify already-completed deletion pushes without writes A stale deletion still required a pending state after background promotion had already removed the ref. Rewriting its old OID to zero is unsafe: receive-pack can interpret that command as an unconditional deletion of a recreated ref. For fully satisfied unsigned pushes containing ordinary-ref deletions, retain normal repository and PR authorization, then verify all requested targets in one Git ref transaction. Verification checks absent refs and existing targets without issuing updates or deletes. Report success at that transaction point, respecting report-status/v2 and sideband framing, without unpacking redundant objects. Recreated or changed refs fail verification and remain untouched. Only fully satisfied requests take this path. Pushes making changes still use receive-pack; certificates, malformed and duplicate commands do not take the shortcut. This does not authorize new targets from stored state or objects. Validation: the deletion reproduction previously failed with missing purgatory authorization and now passes in ordinary and sideband status-v2 modes. The 949-test library suite and 202 selected Git integration tests passed; all four final completed-push tests pass, including ref recreation and companion-ref changes between inspection and verification. Strict all-target Clippy passed. Assisted-by: GPT-6 --- CHANGELOG.md | 3 +- docs/explanation/architecture.md | 8 + src/git/authorization.rs | 2 +- src/git/completed_push.rs | 272 +++++++++++++++++++++++++++++++ src/git/handlers.rs | 57 ++++++- src/git/mod.rs | 1 + tests/git_push_promotion_race.rs | 34 +++- 7 files changed, 367 insertions(+), 10 deletions(-) create mode 100644 src/git/completed_push.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d64abf6..5604bc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,7 +57,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 promotion after ref advertisement, while retaining authorization for changed targets and Git's protection against concurrent ref updates. Validate PR refs independently so mixed pushes do not require state authorization for unchanged - branches. + branches. Complete fully satisfied pushes containing deletions with a + verify-only ref transaction, without risking deletion of a recreated ref. - Reject PR and PR-update events missing commit metadata as terminal invalid events, avoiding repeated recovery attempts for immutable malformed data. - Omit raw rejected relay payloads from SDK diagnostics while retaining URLs and diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index b69d3f2..c7b36d0 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -448,6 +448,14 @@ pub struct Purgatory { and signed commands and `refs/nostr/` commands are not rewritten. - State no-op checks consider only ordinary refs. PR refs retain their own validation and cannot make an unchanged branch require a pending state. + - Fully satisfied unsigned pushes containing ordinary-ref deletions pass + normal authorization and then verify every target in one Git ref + transaction. This checks both existing targets and absent refs without + writing refs or unpacking the redundant upload. A changed or recreated + ref rejects verification. The response honors report-status (including v2) + and sideband negotiation. Commands making changes still use receive-pack; + zero-to-zero deletions are never synthesized for that process because Git + can interpret them as unconditional deletes. - Helper functions in [`helpers.rs`](../../src/purgatory/helpers.rs) handle ref extraction 4. **Bidirectional Waiting**: Either side can arrive first diff --git a/src/git/authorization.rs b/src/git/authorization.rs index 97deaf6..1411292 100644 --- a/src/git/authorization.rs +++ b/src/git/authorization.rs @@ -1102,7 +1102,7 @@ fn parse_text_refs(data: &[u8]) -> Vec<(String, String, String)> { } /// Parse a single ref update line: "old_oid new_oid ref_name\0capabilities" -fn parse_ref_line(payload: &[u8]) -> Option<(String, String, String)> { +pub(super) fn parse_ref_line(payload: &[u8]) -> Option<(String, String, String)> { // Convert to string, handling potential invalid UTF-8 let line = String::from_utf8_lossy(payload); diff --git a/src/git/completed_push.rs b/src/git/completed_push.rs new file mode 100644 index 0000000..0d9b91a --- /dev/null +++ b/src/git/completed_push.rs @@ -0,0 +1,272 @@ +//! Complete fully satisfied pushes containing deletions without issuing writes. +//! A zero-to-zero receive-pack deletion is not a compare-and-swap: Git can +//! delete a concurrently recreated ref. Verify all targets in one transaction +//! instead, then report the push as satisfied at that point in time. + +use super::{authorization::parse_ref_line, protocol::PktLine}; +use hyper::body::Bytes; +use std::{ + collections::{HashMap, HashSet}, + io::{self, Write}, + path::Path, + process::{Command, Stdio}, +}; + +const ZERO: &str = "0000000000000000000000000000000000000000"; + +pub(super) struct CompletedPush { + /// Only for authorization. Never pass these zero-to-zero commands to Git. + pub authorization_body: Bytes, + targets: Vec<(String, String)>, + report_status: bool, + sideband: bool, +} + +impl CompletedPush { + /// Only unsigned, well-formed commands with every target already satisfied + /// qualify. Pushes making any change continue through receive-pack. + pub fn prepare(request: &[u8], local_refs: &HashMap) -> Option { + let mut input = request; + let mut targets = Vec::new(); + let mut seen = HashSet::new(); + let mut authorization = Vec::new(); + let mut has_deletion = false; + let mut report_status = false; + let mut sideband = false; + loop { + let (packet, remaining) = PktLine::parse(input).ok()?; + input = remaining; + let PktLine::Data(payload) = packet else { + break; + }; + // Certificates and malformed commands cannot parse as a ref line. + let (old, new, name) = parse_ref_line(&payload)?; + let line = std::str::from_utf8(&payload).ok()?.trim_end_matches('\n'); + let (command, caps) = line.split_once('\0').unwrap_or((line, "")); + if command != format!("{old} {new} {name}") + || !name.starts_with("refs/") + || !seen.insert(name.clone()) + || (!targets.is_empty() && line.contains('\0')) + { + return None; + } + if targets.is_empty() { + report_status = caps + .split_whitespace() + .any(|c| matches!(c, "report-status" | "report-status-v2")); + sideband = caps.split_whitespace().any(|c| c == "side-band-64k"); + } + let satisfied = if new == ZERO { + !local_refs.contains_key(&name) + } else { + local_refs.get(&name) == Some(&new) + }; + if !satisfied { + return None; + } + let ordinary = !name.starts_with("refs/nostr/"); + has_deletion |= ordinary && new == ZERO; + let auth_old = if ordinary { &new } else { &old }; + authorization + .extend(PktLine::data(format!("{auth_old} {new} {name}\n").into_bytes()).encode()); + targets.push((name, new)); + } + if !has_deletion { + return None; + } + authorization.extend(PktLine::flush().encode()); + Some(Self { + authorization_body: authorization.into(), + targets, + report_status, + sideband, + }) + } + + /// Git locks every ref during prepare. A ref recreated or changed since + /// inspection rejects the whole verification; no update or delete is sent. + pub fn verify(&self, repo: &Path) -> io::Result<()> { + let mut commands = b"start\0option no-deref\0".to_vec(); + for (name, oid) in &self.targets { + commands.extend(format!("verify {name}\0{oid}\0").as_bytes()); + } + commands.extend(b"prepare\0commit\0"); + let mut child = Command::new("git") + .current_dir(repo) + .args(["update-ref", "--stdin", "-z"]) + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::piped()) + .spawn()?; + let write_result = child + .stdin + .take() + .expect("piped stdin") + .write_all(&commands); + let output = child.wait_with_output()?; + write_result?; + if !output.status.success() { + return Err(io::Error::other( + String::from_utf8_lossy(&output.stderr).trim().to_owned(), + )); + } + Ok(()) + } + + pub fn response(&self) -> Vec { + let mut report = Vec::new(); + if self.report_status { + report.extend(PktLine::data(b"unpack ok\n".to_vec()).encode()); + for (name, _) in &self.targets { + report.extend(PktLine::data(format!("ok {name}\n").into_bytes()).encode()); + } + report.extend(PktLine::flush().encode()); + } + if !self.sideband { + return report; + } + let mut response = Vec::new(); + for chunk in report.chunks(65515) { + let mut payload = vec![1]; + payload.extend(chunk); + response.extend(PktLine::data(payload).encode()); + } + response.extend(PktLine::flush().encode()); + response + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn git(repo: &Path, args: &[&str]) -> String { + let output = Command::new("git") + .current_dir(repo) + .args(["-c", "user.name=Test", "-c", "user.email=test@example.com"]) + .args(args) + .output() + .unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8(output.stdout).unwrap().trim().to_owned() + } + + fn request(oid: &str, caps: &str) -> Vec { + let mut bytes = + PktLine::data(format!("{oid} {ZERO} refs/heads/gone\0{caps}\n").into_bytes()).encode(); + bytes.extend(PktLine::flush().encode()); + bytes + } + + #[test] + fn verification_rejects_recreated_ref_without_deleting_it() { + let temp = tempfile::tempdir().unwrap(); + git(temp.path(), &["init", "--bare"]); + let tree = git(temp.path(), &["mktree"]); + let oid = git( + temp.path(), + &["commit-tree", &tree, "-m", "existing object"], + ); + let push = + CompletedPush::prepare(&request(&oid, "report-status"), &HashMap::new()).unwrap(); + push.verify(temp.path()).unwrap(); + assert!(git(temp.path(), &["for-each-ref"]).is_empty()); + // Recreate exactly the stale client's old target between inspection + // and verification. It must survive, not become an authorized delete. + git(temp.path(), &["update-ref", "refs/heads/gone", &oid]); + assert!(push.verify(temp.path()).is_err()); + assert_eq!(git(temp.path(), &["rev-parse", "refs/heads/gone"]), oid); + assert!(CompletedPush::prepare( + &request(&oid, "report-status"), + &HashMap::from([("refs/heads/gone".into(), oid)]) + ) + .is_none()); + } + + #[test] + fn verification_checks_companion_refs_without_reverting_them() { + let temp = tempfile::tempdir().unwrap(); + git(temp.path(), &["init", "--bare"]); + let tree = git(temp.path(), &["mktree"]); + let first = git(temp.path(), &["commit-tree", &tree, "-m", "first"]); + let second = git(temp.path(), &["commit-tree", &tree, "-m", "second"]); + git(temp.path(), &["update-ref", "refs/heads/main", &first]); + let mut bytes = request(&first, "report-status"); + bytes.truncate(bytes.len() - 4); + bytes.extend( + PktLine::data(format!("{ZERO} {first} refs/heads/main\n").into_bytes()).encode(), + ); + bytes.extend(PktLine::flush().encode()); + let push = + CompletedPush::prepare(&bytes, &HashMap::from([("refs/heads/main".into(), first)])) + .unwrap(); + push.verify(temp.path()).unwrap(); + git(temp.path(), &["update-ref", "refs/heads/main", &second]); + assert!(push.verify(temp.path()).is_err()); + assert_eq!(git(temp.path(), &["rev-parse", "refs/heads/main"]), second); + assert!(git(temp.path(), &["for-each-ref", "refs/heads/gone"]).is_empty()); + } + + #[test] + fn response_respects_status_capabilities_and_sideband_framing() { + for caps in [ + "", + "report-status", + "report-status-v2", + "report-status side-band-64k", + "report-status-v2 side-band-64k", + "side-band-64k", + ] { + let push = + CompletedPush::prepare(&request(&"1".repeat(40), caps), &HashMap::new()).unwrap(); + let wire = push.response(); + let mut report = Vec::new(); + if caps.contains("side-band-64k") { + let mut remaining = wire.as_slice(); + loop { + let (packet, tail) = PktLine::parse(remaining).unwrap(); + remaining = tail; + match packet { + PktLine::Data(payload) => { + assert_eq!(payload[0], 1); + report.extend(&payload[1..]); + } + PktLine::Flush => { + assert!(remaining.is_empty()); + break; + } + } + } + } else { + report = wire; + } + let expected = if caps.contains("report-status") { + [ + PktLine::data(b"unpack ok\n".to_vec()).encode(), + PktLine::data(b"ok refs/heads/gone\n".to_vec()).encode(), + PktLine::flush().encode(), + ] + .concat() + } else { + Vec::new() + }; + assert_eq!(report, expected, "{caps}"); + } + } + + #[test] + fn signed_malformed_and_duplicate_commands_do_not_take_shortcut() { + let line = request(&"1".repeat(40), "report-status"); + let mut signed = PktLine::data(b"push-cert\0report-status\n".to_vec()).encode(); + signed.extend(&line); + assert!(CompletedPush::prepare(&signed, &HashMap::new()).is_none()); + assert!(CompletedPush::prepare(&line[..line.len() - 4], &HashMap::new()).is_none()); + let mut duplicate = line[..line.len() - 4].to_vec(); + duplicate.extend(&line); + assert!(CompletedPush::prepare(&duplicate, &HashMap::new()).is_none()); + } +} diff --git a/src/git/handlers.rs b/src/git/handlers.rs index baf37da..79d215f 100644 --- a/src/git/handlers.rs +++ b/src/git/handlers.rs @@ -16,6 +16,7 @@ use tokio::sync::mpsc; use tokio::time::MissedTickBehavior; use tracing::{debug, error, info, warn, Instrument}; +use super::completed_push::CompletedPush; use super::protocol::{GitService, PktLine}; use super::storage::{FamilyKey, FamilyWriteLease, LocalGitStorage}; use super::subprocess::GitSubprocess; @@ -688,10 +689,21 @@ pub async fn handle_receive_pack( // advertised and removed its state from purgatory. Normalize only commands // already satisfied by this view; keep ordinary authorization for changes // and Git's compare-and-swap protection against a later writer. - let request_body = match crate::git::list_refs(&repo_path) { - Ok(refs) => normalize_applied_ref_updates(&request_body, &refs.into_iter().collect()), - Err(_) => request_body, + let (request_body, completed) = match crate::git::list_refs(&repo_path) { + Ok(refs) => { + let refs = refs.into_iter().collect(); + let completed = CompletedPush::prepare(&request_body, &refs); + ( + normalize_applied_ref_updates(&request_body, &refs), + completed, + ) + } + Err(_) => (request_body, None), }; + let authorization_body = completed + .as_ref() + .map(|push| &push.authorization_body) + .unwrap_or(&request_body); // GRASP Authorization Check debug!( @@ -704,7 +716,7 @@ pub async fn handle_receive_pack( &database, identifier, owner_pubkey, - &request_body, + authorization_body, &purgatory, &repo_path, ) @@ -740,6 +752,43 @@ pub async fn handle_receive_pack( } }; + if let Some(completed) = completed { + let verify_path = repo_path.clone(); + let result = tokio::task::spawn_blocking(move || { + completed.verify(&verify_path)?; + Ok::<_, io::Error>(completed.response()) + }) + .await + .map_err(|error| GitError::Storage(error.to_string()))?; + return match result { + Ok(body) => { + info!( + identifier, + "Completed already-applied push with a verify-only ref transaction" + ); + record_git_operation(&metrics, "push", "success"); + Ok(Response::builder() + .status(StatusCode::OK) + .header( + "content-type", + GitService::ReceivePack.result_content_type(), + ) + .header("cache-control", "no-cache") + .body(full_body(body)) + .unwrap()) + } + Err(error) => { + warn!(identifier, %error, "Already-applied push failed ref verification"); + record_git_operation(&metrics, "push", "error"); + Ok(build_git_protocol_error_response( + GitService::ReceivePack, + "already-applied push no longer matches local refs", + Some(&request_body), + )) + } + }; + } + // Ref updates remain in the selected view; receive-pack's quarantine and // final objects are installed directly in the shared family inventory. let mut git = GitSubprocess::spawn_with_object_directory( diff --git a/src/git/mod.rs b/src/git/mod.rs index d93a684..7aa3279 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -19,6 +19,7 @@ pub mod authorization; pub mod authorization_integrity; +mod completed_push; pub mod handlers; pub mod integrity; pub mod migration; diff --git a/tests/git_push_promotion_race.rs b/tests/git_push_promotion_race.rs index a687296..1959b58 100644 --- a/tests/git_push_promotion_race.rs +++ b/tests/git_push_promotion_race.rs @@ -35,15 +35,25 @@ fn push(repo: &Path, old: &str, new: &str) -> Bytes { #[tokio::test] async fn state_promoted_after_advertisement_does_not_reject_an_already_applied_push() { - promoted_push(false).await; + promoted_push(false, None).await; } #[tokio::test] async fn already_applied_branch_does_not_require_state_for_a_pr_ref() { - promoted_push(true).await; + promoted_push(true, None).await; } -async fn promoted_push(with_pr: bool) { +#[tokio::test] +async fn already_applied_deletion_is_a_verified_noop() { + promoted_push(false, Some("report-status")).await; +} + +#[tokio::test] +async fn already_applied_deletion_supports_sideband_status_v2() { + promoted_push(false, Some("report-status-v2 side-band-64k")).await; +} + +async fn promoted_push(with_pr: bool, deletion_caps: Option<&str>) { let dir = tempfile::tempdir().unwrap(); let owner = Keys::generate(); let identifier = "promotion-race"; @@ -92,7 +102,15 @@ async fn promoted_push(with_pr: bool) { // The client saw an absent branch; background promotion installed it // before its upload arrived and removed the state from purgatory. - let request = if with_pr { + let request = if let Some(caps) = deletion_caps { + let branch = format!("{} {commit} refs/heads/main\0{caps}\n", "0".repeat(40)); + let deletion = format!("{commit} {} refs/heads/gone\n", "0".repeat(40)); + Bytes::from(format!( + "{:04x}{branch}{:04x}{deletion}0000", + branch.len() + 4, + deletion.len() + 4 + )) + } else if with_pr { let command = format!( "{} {commit} refs/heads/main\0report-status\n", "0".repeat(40) @@ -144,6 +162,14 @@ async fn promoted_push(with_pr: bool) { "{result}" ); + if deletion_caps.is_some() { + assert!(result.contains("ok refs/heads/gone"), "{result}"); + assert!( + !String::from_utf8(git(&repo, &["for-each-ref", "refs/heads/gone"])) + .unwrap() + .contains("refs/heads/gone") + ); + } if with_pr { assert!( result.contains(&format!("ok refs/nostr/{}", "a".repeat(64))),