From 3a7fa1d1288c28eae0ee58b4c448c672ec3b69c2 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 27 Jan 2026 19:43:41 +0000 Subject: fix: return HTTP 200 with ERR pkt-line for git protocol errors Previously, all git upload-pack/receive-pack failures returned HTTP 500, but the git smart HTTP protocol requires protocol-level errors (like "not our ref") to be returned as HTTP 200 OK with an ERR pkt-line in the response body. Changes: - Add build_git_protocol_error_response() to create HTTP 200 responses with properly formatted ERR pkt-line ("ERR \n") - Add is_git_protocol_error() to detect protocol errors (exit code 128 with stderr content) vs transport errors - Update handle_upload_pack() and handle_receive_pack() to return protocol errors as HTTP 200 with ERR pkt-line - Keep HTTP 500 for actual transport errors (spawn failures, I/O errors, signals) This allows git clients to properly parse and display protocol error messages instead of seeing generic HTTP 500 errors. --- src/git/handlers.rs | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) (limited to 'src/git') diff --git a/src/git/handlers.rs b/src/git/handlers.rs index 017eee4..e3a6ad4 100644 --- a/src/git/handlers.rs +++ b/src/git/handlers.rs @@ -99,6 +99,42 @@ pub async fn handle_info_refs( .unwrap()) } +/// Build an HTTP 200 OK response with an ERR pkt-line for git protocol errors. +/// +/// Per the git smart HTTP protocol spec, protocol-level errors (like "not our ref") +/// should be returned as HTTP 200 OK with the error message in pkt-line format: +/// `PKT-LINE("ERR" SP explanation-text)` +/// +/// This allows git clients to properly parse and display the error message. +fn build_git_protocol_error_response( + service: GitService, + error_message: &str, +) -> Response> { + // Format: "ERR \n" + let err_content = format!("ERR {}\n", error_message.trim()); + let err_pktline = PktLine::data(err_content.as_bytes()).encode(); + + Response::builder() + .status(StatusCode::OK) + .header("content-type", service.result_content_type()) + .header("cache-control", "no-cache") + .body(Full::new(Bytes::from(err_pktline))) + .unwrap() +} + +/// Check if a git process failure is a protocol error (vs transport error). +/// +/// Protocol errors are communicated via stderr when git exits with code 128. +/// These should be returned to the client as HTTP 200 with ERR pkt-line. +/// +/// Transport errors (process spawn failures, I/O errors, signals) should +/// remain as HTTP 500 errors. +fn is_git_protocol_error(exit_code: Option, stderr: &[u8]) -> bool { + // Git uses exit code 128 for protocol/usage errors + // If there's stderr content, it's a protocol error message + exit_code == Some(128) && !stderr.is_empty() +} + /// Handle POST /git-upload-pack (clone/fetch) pub async fn handle_upload_pack( repo_path: PathBuf, @@ -150,6 +186,21 @@ pub async fn handle_upload_pack( if !status.success() { let stderr_str = String::from_utf8_lossy(&stderr_output); + + // Check if this is a git protocol error (exit code 128 with stderr) + // Protocol errors should be returned as HTTP 200 with ERR pkt-line + if is_git_protocol_error(status.code(), &stderr_output) { + warn!( + "Git upload-pack protocol error (returning ERR pkt-line): {}", + stderr_str + ); + return Ok(build_git_protocol_error_response( + GitService::UploadPack, + &stderr_str, + )); + } + + // Transport errors (spawn failures, signals, etc.) remain as HTTP 500 error!("Git upload-pack failed: {}", stderr_str); return Err(GitError::GitFailed(status.code())); } @@ -277,6 +328,21 @@ pub async fn handle_receive_pack( if !status.success() { let stderr_str = String::from_utf8_lossy(&stderr_output); + + // Check if this is a git protocol error (exit code 128 with stderr) + // Protocol errors should be returned as HTTP 200 with ERR pkt-line + if is_git_protocol_error(status.code(), &stderr_output) { + warn!( + "Git receive-pack protocol error (returning ERR pkt-line): {}", + stderr_str + ); + return Ok(build_git_protocol_error_response( + GitService::ReceivePack, + &stderr_str, + )); + } + + // Transport errors (spawn failures, signals, etc.) remain as HTTP 500 error!("Git receive-pack failed: {}", stderr_str); return Err(GitError::GitFailed(status.code())); } -- cgit v1.2.3 From c163d717147b92b16d89da2fbccef775647b5a07 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 3 Feb 2026 16:13:59 +0000 Subject: fix: accept no-op pushes where old_oid == new_oid Fixes race condition where user's push becomes no-op after state event is applied between fetch and push. Now accepts these as successful no-ops, matching Git's 'Everything up-to-date' behavior. - Add early detection in get_state_authorization_for_specific_owner_repo - Return success for all-noop pushes without requiring purgatory event - Document behavior in inline-authorization.md --- docs/explanation/inline-authorization.md | 4 ++++ src/git/authorization.rs | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+) (limited to 'src/git') diff --git a/docs/explanation/inline-authorization.md b/docs/explanation/inline-authorization.md index 7081f63..80bd98f 100644 --- a/docs/explanation/inline-authorization.md +++ b/docs/explanation/inline-authorization.md @@ -352,6 +352,10 @@ pub async fn authorize_push( - If no event found, create placeholder (git-data-first scenario) - Collect PR events from purgatory for post-push processing +**No-Op Push Acceptance:** Pushes where all refs have `old_oid == new_oid` are accepted without requiring a purgatory state event, matching Git's "Everything up-to-date" behavior and avoiding race condition rejections. + +--- + ## State Event Authorization State events (kind 30618) undergo authorization checks at three points (defense-in-depth): diff --git a/src/git/authorization.rs b/src/git/authorization.rs index e174b51..db2b992 100644 --- a/src/git/authorization.rs +++ b/src/git/authorization.rs @@ -575,6 +575,28 @@ pub async fn get_state_authorization_for_specific_owner_repo( owner_pubkey ); + // Accept pushes where all refs are already at the desired state (old_oid == new_oid) + // This handles race conditions where state events are applied between fetch and push + if !pushed_refs.is_empty() { + let all_refs_unchanged = pushed_refs + .iter() + .all(|(old_oid, new_oid, _)| old_oid == new_oid); + + if all_refs_unchanged { + debug!( + "All pushed refs unchanged (old_oid == new_oid) for {} owned by {}, accepting without purgatory check", + identifier, owner_pubkey + ); + return Ok(AuthorizationResult { + authorized: true, + reason: "Push accepted: all refs already at desired state (no-op)".to_string(), + state: None, + maintainers: authorized.into_iter().collect(), + purgatory_events: vec![], + }); + } + } + // Check purgatory for matching state events // Convert pushed refs to RefUpdate (filter out refs/nostr/* refs) let pushed_updates: Vec = pushed_refs -- cgit v1.2.3 From d392f0bc14bcd687e918d4653ae016226496b4c4 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 3 Feb 2026 21:21:33 +0000 Subject: feat: add diagnostic logging for partial state event matches Improves observability when pushes are rejected due to state events that only partially match the pushed refs. Previously, logs only showed 'No state event found' even when state events existed but didn't match. Changes: - Add diagnose_state_mismatch() to explain why state events don't match - Log specific reasons: missing refs, wrong SHAs, or extra refs - Update rejection message to 'No matching state event found' (more accurate) - Add 4 unit tests for diagnostic function Example diagnostic output: WARN State event abc123 from authorized author doesn't match push: refs/heads/main missing (state declares 9cc3d93b) This addresses the issue where a push with only refs/heads/test was rejected because the state event also declared refs/heads/main, but logs didn't explain why the match failed. --- src/git/authorization.rs | 80 ++++++++++++++++++- src/git/handlers.rs | 2 +- src/purgatory/helpers.rs | 204 +++++++++++++++++++++++++++++++++++++++++++++++ src/purgatory/mod.rs | 2 +- 4 files changed, 284 insertions(+), 4 deletions(-) (limited to 'src/git') diff --git a/src/git/authorization.rs b/src/git/authorization.rs index db2b992..27107db 100644 --- a/src/git/authorization.rs +++ b/src/git/authorization.rs @@ -666,12 +666,88 @@ pub async fn get_state_authorization_for_specific_owner_repo( debug!("Purgatory events found but none from authorized authors"); } } else { - debug!("No matching state events found in purgatory"); + // Check if there are ANY state events in purgatory for this identifier + let all_purgatory_states = purgatory.find_state(identifier); + + if !all_purgatory_states.is_empty() { + // There are state events but none match the push - diagnose why + debug!( + "Found {} state event(s) in purgatory for {} but none match the push", + all_purgatory_states.len(), + identifier + ); + + // Count authorized state events and collect diagnostic info + let mut authorized_count = 0; + let mut diagnostic_reasons = Vec::new(); + + // Diagnose why each authorized state event doesn't match + for entry in all_purgatory_states.iter() { + let author_hex = entry.event.pubkey.to_hex(); + if authorized.contains(&author_hex) { + authorized_count += 1; + if let Some(reason) = crate::purgatory::diagnose_state_mismatch( + &entry.event, + &pushed_updates, + &local_refs, + ) { + debug!( + "State event {} from authorized author {} doesn't match push: {}", + entry.event.id, + entry + .event + .pubkey + .to_bech32() + .unwrap_or_else(|_| author_hex.clone()), + reason + ); + diagnostic_reasons.push(reason); + } + } + } + + // Create concise WARN message summarizing the rejection + let summary = if authorized_count > 0 { + let reason_summary = if !diagnostic_reasons.is_empty() { + // Take the first diagnostic reason as representative + format!(" ({})", diagnostic_reasons[0]) + } else { + String::new() + }; + format!( + "{} state event{} in purgatory from authorized publisher{} but doesn't match push{}", + authorized_count, + if authorized_count == 1 { "" } else { "s" }, + if authorized_count == 1 { "" } else { "s" }, + reason_summary + ) + } else { + format!( + "{} state event{} in purgatory but none from authorized publishers", + all_purgatory_states.len(), + if all_purgatory_states.len() == 1 { + "" + } else { + "s" + } + ) + }; + + warn!("Push rejected for {}: {}", identifier, summary); + return Ok(AuthorizationResult::denied(summary)); + } else { + debug!("No state events found in purgatory for {}", identifier); + warn!( + "Push rejected for {}: No state events in purgatory", + identifier + ); + return Ok(AuthorizationResult::denied("No state events in purgatory")); + } } // No matching state found in purgatory Ok(AuthorizationResult::denied( - "No state event found in purgatory from authorized publishers", + "No matching state event found in purgatory from authorized publishers", )) } diff --git a/src/git/handlers.rs b/src/git/handlers.rs index e3a6ad4..7244abb 100644 --- a/src/git/handlers.rs +++ b/src/git/handlers.rs @@ -254,7 +254,7 @@ pub async fn handle_receive_pack( } // GRASP Authorization Check - info!( + debug!( "Authorizing push for {} owned by {} via database query", identifier, owner_pubkey ); diff --git a/src/purgatory/helpers.rs b/src/purgatory/helpers.rs index 193ef99..a9f6e66 100644 --- a/src/purgatory/helpers.rs +++ b/src/purgatory/helpers.rs @@ -225,6 +225,117 @@ pub fn get_unpushed_refs(event: &Event, pushed_refs: &[RefPair]) -> Vec .collect() } +/// Diagnose why a state event doesn't match the push. +/// +/// Returns a human-readable explanation of the mismatch between the state event +/// and what would result from applying the push to local refs. +/// +/// # Arguments +/// * `event` - The state event to check +/// * `pushed_updates` - Ref updates in the current push operation +/// * `local_refs` - Refs already existing locally (ref_name -> SHA) +/// +/// # Returns +/// String explaining why the state doesn't match, or None if it matches +pub fn diagnose_state_mismatch( + event: &Event, + pushed_updates: &[RefUpdate], + local_refs: &HashMap, +) -> Option { + let state_refs = extract_refs_from_state(event); + + // Filter local_refs to only branches and tags + let mut would_be_state: HashMap = local_refs + .iter() + .filter(|(ref_name, _)| { + ref_name.starts_with("refs/heads/") || ref_name.starts_with("refs/tags/") + }) + .map(|(k, v)| (k.clone(), v.clone())) + .collect(); + + // Apply all pushed updates to create the would-be state + for update in pushed_updates { + // Only process branches and tags + if !update.ref_name.starts_with("refs/heads/") && !update.ref_name.starts_with("refs/tags/") + { + continue; + } + + if update.is_deletion() { + would_be_state.remove(&update.ref_name); + } else { + would_be_state.insert(update.ref_name.clone(), update.new_oid.clone()); + } + } + + // Convert event's state refs to a HashMap for comparison + let declared_state: HashMap = state_refs + .into_iter() + .map(|r| (r.ref_name, r.object_sha)) + .collect(); + + // Check if they match + if would_be_state == declared_state { + return None; // No mismatch + } + + // Build diagnostic message + let mut reasons = Vec::new(); + + // Check for refs in declared state but not in would-be state + for (ref_name, declared_sha) in &declared_state { + if let Some(would_be_sha) = would_be_state.get(ref_name) { + if would_be_sha != declared_sha { + let would_be_short = if would_be_sha.len() >= 8 { + &would_be_sha[..8] + } else { + would_be_sha.as_str() + }; + let declared_short = if declared_sha.len() >= 8 { + &declared_sha[..8] + } else { + declared_sha.as_str() + }; + reasons.push(format!( + "{} would be at {} but state declares {}", + ref_name, would_be_short, declared_short + )); + } + } else { + let declared_short = if declared_sha.len() >= 8 { + &declared_sha[..8] + } else { + declared_sha.as_str() + }; + reasons.push(format!( + "{} missing (state declares {})", + ref_name, declared_short + )); + } + } + + // Check for refs in would-be state but not in declared state + for (ref_name, would_be_sha) in &would_be_state { + if !declared_state.contains_key(ref_name) { + let would_be_short = if would_be_sha.len() >= 8 { + &would_be_sha[..8] + } else { + would_be_sha.as_str() + }; + reasons.push(format!( + "{} would exist at {} but state doesn't declare it", + ref_name, would_be_short + )); + } + } + + if reasons.is_empty() { + Some("Unknown mismatch".to_string()) + } else { + Some(reasons.join("; ")) + } +} + #[cfg(test)] mod tests { use super::*; @@ -695,4 +806,97 @@ mod tests { // Should return true - real OID exists, symbolic ref skipped assert!(can_apply_state(&event, repo_path)); } + + #[test] + fn test_diagnose_state_mismatch_missing_ref() { + // State declares both main and test branches + let event = create_test_state_event( + "test-repo", + vec![("refs/heads/main", "abc123"), ("refs/heads/test", "def456")], + ); + + // Push only creates test branch + let pushed_updates = vec![RefUpdate { + old_oid: "0000000000000000000000000000000000000000".to_string(), + new_oid: "def456".to_string(), + ref_name: "refs/heads/test".to_string(), + }]; + + // No local refs + let local_refs = HashMap::new(); + + let diagnosis = diagnose_state_mismatch(&event, &pushed_updates, &local_refs); + assert!(diagnosis.is_some()); + let msg = diagnosis.unwrap(); + assert!(msg.contains("refs/heads/main")); + assert!(msg.contains("missing")); + } + + #[test] + fn test_diagnose_state_mismatch_wrong_sha() { + // State declares main at abc123 + let event = create_test_state_event("test-repo", vec![("refs/heads/main", "abc123")]); + + // Push updates main to different SHA + let pushed_updates = vec![RefUpdate { + old_oid: "0000000000000000000000000000000000000000".to_string(), + new_oid: "wrong123".to_string(), + ref_name: "refs/heads/main".to_string(), + }]; + + let local_refs = HashMap::new(); + + let diagnosis = diagnose_state_mismatch(&event, &pushed_updates, &local_refs); + assert!(diagnosis.is_some()); + let msg = diagnosis.unwrap(); + assert!(msg.contains("refs/heads/main")); + assert!(msg.contains("would be at")); + assert!(msg.contains("state declares")); + } + + #[test] + fn test_diagnose_state_mismatch_extra_ref() { + // State declares only main + let event = create_test_state_event("test-repo", vec![("refs/heads/main", "abc123")]); + + // Push creates both main and test + let pushed_updates = vec![ + RefUpdate { + old_oid: "0000000000000000000000000000000000000000".to_string(), + new_oid: "abc123".to_string(), + ref_name: "refs/heads/main".to_string(), + }, + RefUpdate { + old_oid: "0000000000000000000000000000000000000000".to_string(), + new_oid: "def456".to_string(), + ref_name: "refs/heads/test".to_string(), + }, + ]; + + let local_refs = HashMap::new(); + + let diagnosis = diagnose_state_mismatch(&event, &pushed_updates, &local_refs); + assert!(diagnosis.is_some()); + let msg = diagnosis.unwrap(); + assert!(msg.contains("refs/heads/test")); + assert!(msg.contains("doesn't declare")); + } + + #[test] + fn test_diagnose_state_mismatch_no_mismatch() { + // State declares main + let event = create_test_state_event("test-repo", vec![("refs/heads/main", "abc123")]); + + // Push creates main at correct SHA + let pushed_updates = vec![RefUpdate { + old_oid: "0000000000000000000000000000000000000000".to_string(), + new_oid: "abc123".to_string(), + ref_name: "refs/heads/main".to_string(), + }]; + + let local_refs = HashMap::new(); + + let diagnosis = diagnose_state_mismatch(&event, &pushed_updates, &local_refs); + assert!(diagnosis.is_none()); // No mismatch + } } diff --git a/src/purgatory/mod.rs b/src/purgatory/mod.rs index 8094450..2c278f6 100644 --- a/src/purgatory/mod.rs +++ b/src/purgatory/mod.rs @@ -16,7 +16,7 @@ pub mod persistence; pub mod sync; mod types; -pub use helpers::{can_apply_state, can_satisfy_state, extract_refs_from_state, get_unpushed_refs}; +pub use helpers::{can_apply_state, can_satisfy_state, diagnose_state_mismatch, extract_refs_from_state, get_unpushed_refs}; pub use types::{EventSource, PrPurgatoryEntry, RefPair, RefUpdate, StatePurgatoryEntry}; use dashmap::DashMap; -- cgit v1.2.3 From f200bd1bde04ff79a40c8d73df0edde2cf28493c Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 3 Feb 2026 21:49:38 +0000 Subject: Reduce log noise: change per-ref updates to DEBUG level Only the final summary 'Aligned repository with state' remains at INFO level, showing the total count of refs_created/refs_updated/refs_deleted. --- src/git/mod.rs | 2 +- src/git/sync.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'src/git') diff --git a/src/git/mod.rs b/src/git/mod.rs index b3fee69..1255b6f 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -253,7 +253,7 @@ pub fn update_ref(repo_path: &Path, ref_name: &str, commit_hash: &str) -> Result return Err(format!("git update-ref failed: {}", stderr)); } - info!( + debug!( "Updated ref {} to {} in {}", ref_name, commit_hash, diff --git a/src/git/sync.rs b/src/git/sync.rs index e8e9655..b1a9b49 100644 --- a/src/git/sync.rs +++ b/src/git/sync.rs @@ -667,7 +667,7 @@ pub fn align_repository_with_state(repo_path: &Path, state: &RepositoryState) -> match git::update_ref(repo_path, ref_name, expected_commit) { Ok(()) => { if current_commit.is_some() { - info!( + debug!( "Updated {} to {} in {}", ref_name, expected_commit, @@ -675,7 +675,7 @@ pub fn align_repository_with_state(repo_path: &Path, state: &RepositoryState) -> ); result.refs_updated += 1; } else { - info!( + debug!( "Created {} at {} in {}", ref_name, expected_commit, @@ -701,7 +701,7 @@ pub fn align_repository_with_state(repo_path: &Path, state: &RepositoryState) -> if let Some(head_commit) = state.get_branch_commit(branch_name) { match git::try_set_head_if_available(repo_path, head_ref, head_commit) { Ok(true) => { - info!("Set HEAD to {} in {}", head_ref, repo_path.display()); + debug!("Set HEAD to {} in {}", head_ref, repo_path.display()); result.head_set = true; } Ok(false) => { -- cgit v1.2.3 From 22d93ea5e707e99d67ab367d60c7a9d867b7665c Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 3 Feb 2026 22:00:31 +0000 Subject: Add error logging to all git handler IO operations Previously, some IO errors in git handlers were logged while others were not, leading to inconsistent observability. Additionally, the HTTP layer logged all git errors redundantly, adding no useful context beyond what was already logged at the source. Changes: - Add error logging to all previously unlogged IO operations in handle_upload_pack and handle_receive_pack (stdin writes, stdout/stderr reads, process waits) - Remove redundant error logging at HTTP layer since all errors are now logged at their source with full context - Ensures consistent error-level logging for all git subprocess failures This provides complete observability of git operations while eliminating duplicate log entries that don't add value. --- src/git/handlers.rs | 40 ++++++++++++++++++++++++++++++++-------- src/http/mod.rs | 2 +- 2 files changed, 33 insertions(+), 9 deletions(-) (limited to 'src/git') diff --git a/src/git/handlers.rs b/src/git/handlers.rs index 7244abb..28cb47f 100644 --- a/src/git/handlers.rs +++ b/src/git/handlers.rs @@ -156,7 +156,10 @@ pub async fn handle_upload_pack( stdin .write_all(&request_body) .await - .map_err(GitError::IoError)?; + .map_err(|e| { + error!("Failed to write to git upload-pack stdin: {}", e); + GitError::IoError(e) + })?; // Close stdin to signal end of input drop(stdin); } @@ -170,7 +173,10 @@ pub async fn handle_upload_pack( stdout .read_to_end(&mut output) .await - .map_err(GitError::IoError)?; + .map_err(|e| { + error!("Failed to read git upload-pack stdout: {}", e); + GitError::IoError(e) + })?; } if let Some(stderr) = git.take_stderr() { @@ -178,11 +184,17 @@ pub async fn handle_upload_pack( stderr .read_to_end(&mut stderr_output) .await - .map_err(GitError::IoError)?; + .map_err(|e| { + error!("Failed to read git upload-pack stderr: {}", e); + GitError::IoError(e) + })?; } // Wait for process - let status = git.wait().await.map_err(GitError::IoError)?; + let status = git.wait().await.map_err(|e| { + error!("Failed to wait for git upload-pack process: {}", e); + GitError::IoError(e) + })?; if !status.success() { let stderr_str = String::from_utf8_lossy(&stderr_output); @@ -299,7 +311,10 @@ pub async fn handle_receive_pack( stdin .write_all(&request_body) .await - .map_err(GitError::IoError)?; + .map_err(|e| { + error!("Failed to write to git receive-pack stdin: {}", e); + GitError::IoError(e) + })?; drop(stdin); } @@ -312,7 +327,10 @@ pub async fn handle_receive_pack( stdout .read_to_end(&mut output) .await - .map_err(GitError::IoError)?; + .map_err(|e| { + error!("Failed to read git receive-pack stdout: {}", e); + GitError::IoError(e) + })?; } if let Some(stderr) = git.take_stderr() { @@ -320,11 +338,17 @@ pub async fn handle_receive_pack( stderr .read_to_end(&mut stderr_output) .await - .map_err(GitError::IoError)?; + .map_err(|e| { + error!("Failed to read git receive-pack stderr: {}", e); + GitError::IoError(e) + })?; } // Wait for process - let status = git.wait().await.map_err(GitError::IoError)?; + let status = git.wait().await.map_err(|e| { + error!("Failed to wait for git receive-pack process: {}", e); + GitError::IoError(e) + })?; if !status.success() { let stderr_str = String::from_utf8_lossy(&stderr_output); diff --git a/src/http/mod.rs b/src/http/mod.rs index ffb1562..edc28a3 100644 --- a/src/http/mod.rs +++ b/src/http/mod.rs @@ -332,7 +332,7 @@ impl Service> for HttpService { .unwrap()) } Err(e) => { - tracing::error!("Git handler error: {}", e); + // Errors are already logged at their source with full context let error_msg = format!("Git error: {}", e); Ok(add_cors_headers(Response::builder()) .status(e.status_code()) -- cgit v1.2.3